Skip to content

Profile name#233

Merged
jgowdy-godaddy merged 10 commits intogodaddy:mainfrom
chief-micco:profile-name
Apr 29, 2026
Merged

Profile name#233
jgowdy-godaddy merged 10 commits intogodaddy:mainfrom
chief-micco:profile-name

Conversation

@chief-micco
Copy link
Copy Markdown
Collaborator

Summary

Adds optional AwsProfileName to shared JSON config (serde) and threads it through Rust AWS SDK loading for KMS, DynamoDB, and Secrets Manager.

.NET

  • AsherahConfig / builder: WithAwsProfileName, serialized as AwsProfileName
  • Unit tests for config JSON (InternalsVisibleTo for internal ToJson)
  • XML docs on WithAwsProfileName; README updates merged from main

Testing

  • scripts/test.sh --unit / workspace tests as applicable
  • dotnet test on asherah-dotnet/GoDaddy.Asherah.Encryption.slnx

Expose WithAwsProfileName on the builder and serialize AwsProfileName for the
native JSON config. Document in README; add unit tests (InternalsVisibleTo for
ToJson assertions).

Made-with: Cursor
Add serde AwsProfileName to shared config; plumb profile through KMS,
Secrets Manager, and DynamoDB builders; shared aws_sdk_load for credential
profiles. Update Node bindings and READMEs; extend config_apply_env tests.

Made-with: Cursor
* main:
  fix(ffi,cobhan,session): zero decrypted plaintext before deallocation (godaddy#216)
  test(types): verify to_json_fast / from_json_fast produce serde-compatible JSON (godaddy#220)
Document the optional AWS credentials profile on the builder. Refresh
dotnet README tables after merge with main.

Made-with: Cursor
Match PreferredRegion and other ConfigOptions fields in this section.

Made-with: Cursor
Align wording length with neighboring optional fields.

Made-with: Cursor
Keep pub mod declarations contiguous; place non-exported helper before re-exports.

Made-with: Cursor
@jgowdy-godaddy
Copy link
Copy Markdown
Collaborator

Reviewed the PR end-to-end. The core change (AwsProfileName plumbing through ConfigOptionsResolvedConfig → KMS/DynamoDB/Secrets Manager loaders) is sound, the .NET tests are solid, and CI is green. There are a few things I'd like addressed before merge.

Blocking — markdown auto-formatter damage

A formatter (looks like Prettier) was run over CONTRIBUTING.md, asherah-dotnet/README.md, and asherah-py/README.md and broke several constructs:

Four hyperlinks are now wrapped in backticks, so they render as inline code instead of links:

File Line Result
CONTRIBUTING.md 78 `[asherah-dotnet/README.md](asherah-dotnet/README.md)`
asherah-py/README.md 42 `[samples/python/sample.py](../samples/python/sample.py)`
asherah-dotnet/README.md 34 `[docs/](./docs/)`
asherah-dotnet/README.md 76 `[samples/dotnet/Program.cs](../samples/dotnet/Program.cs)`

The PR Checklist in CONTRIBUTING.md (lines 82–86) lost its [ ] task-list markers- [ ] Code compiles… became - Code compiles…, so GitHub no longer renders interactive checkboxes.

List-item continuation indent was stripped — numbered list children went from 3-space (correct under 1. ) to 2-space, and bullet continuations lost their leading indent entirely. Under strict CommonMark those continuations break out of the list.

Trailing newlines were removed from all three files.

Please revert the unrelated reformatting (or split it into a separate PR with the formatter configured to leave inline-code-in-links and task lists alone).

Should fix

  • Python README config table not updated. asherah-py/README.md's setup() keys table doesn't list AwsProfileName. The Python binding picks it up automatically via serde, but it's invisible to anyone reading the docs.
  • Legacy AwsKms path silently drops the profile name. asherah/src/kms_aws.rs was migrated to aws_sdk_load::load_sdk_config(..., None) with None hard-coded. AwsKms::new is still called from kms_builders::aws_kms_from_env and AwsKmsBuilder. Either thread aws_profile_name through here too (consistent with the envelope path) or skip touching this file — partial refactor adds risk without benefit.
  • *_with_profile sibling-method explosion is unnecessary. Six new pub(crate) constructors in kms_aws_envelope.rs / kms_secrets_manager.rs. Since these are crate-private and only builders.rs calls them, just add the parameter to the existing methods and update the call sites.

Nits

  • Doc-comment wording: index.d.ts says ~/.aws/config, the .NET WithAwsProfileName XML doc says ~/.aws/credentials. Pick one (profiles can live in either) and reuse.
  • AsherahConfigTests.cs has an unused using GoDaddy.Asherah; — the test namespace already has it as an implicit ancestor.
  • Adding aws-types as a direct dep for SdkConfig works (it's already a transitive); alternatively, have load_sdk_config return ConfigLoader so the helper doesn't need to name the type. Minor.

Markdown formatter damage:
- Revert CONTRIBUTING.md, asherah-dotnet/README.md, asherah-py/README.md
  to main (the auto-formatter wrapped four hyperlinks in backticks
  breaking them, dropped the [ ] task-list markers from the PR
  Checklist, destroyed list-item continuation indent, and stripped
  trailing newlines).
- Re-add the legitimate doc additions on top: WithAwsProfileName row in
  the .NET builder table and AwsProfileName row in the Python config
  table.

Legacy AwsKms path:
- Thread aws_profile_name through AwsKms::new and AwsKms::new_async
  (was previously hard-coded to None in the shared loader call).
- AwsKmsBuilder gains a profile_name(...) builder method.
- aws_kms_from_env keeps the env-var contract by passing None — the
  SDK's default credential chain already honors AWS_PROFILE.

API duplication collapsed:
- Drop the *_with_profile sibling methods in kms_aws_envelope.rs
  (new_single_with_profile, new_multi_with_profile,
  new_single_async_with_profile, new_multi_async_with_profile) and
  kms_secrets_manager.rs (new_with_profile, new_async_with_profile).
- The original methods now take aws_profile_name: Option<&str>
  directly. Update all callers (builders.rs, integration tests,
  example, kms_aws_envelope unit test).

Wording consistency:
- index.d.ts said `~/.aws/config`; .NET XML doc said `~/.aws/credentials`.
  Unify on the .NET wording (typically credentials, with the SDK
  honoring both).

Test cleanup:
- Drop unused `using GoDaddy.Asherah;` in AsherahConfigTests.cs (the
  test namespace already has GoDaddy.Asherah as an implicit ancestor).
@jgowdy-godaddy
Copy link
Copy Markdown
Collaborator

jgowdy-godaddy commented Apr 29, 2026

Pushed commits to address the review feedback. Specifically:

Markdown formatter damage (blocking): reverted CONTRIBUTING.md, asherah-dotnet/README.md, and asherah-py/README.md to the versions on main, then re-applied the legitimate doc additions on top — WithAwsProfileName row in the .NET builder table and AwsProfileName row in the Python config table. This fixes:

  • Four hyperlinks that had been wrapped in backticks (rendering as inline code instead of links).
  • The PR Checklist task-list markers (- [ ]) that the formatter stripped.
  • List-item continuation indent that the formatter destroyed.
  • Trailing newlines that were stripped from all three files.

Legacy AwsKms path: kms_aws.rs::new and new_async now take aws_profile_name: Option<&str> (previously hard-coded None in the shared loader call). AwsKmsBuilder gains a profile_name(...) builder method. aws_kms_from_env keeps the existing env-var contract by passing None — the SDK's default credential chain already honors AWS_PROFILE.

API duplication collapsed: dropped the *_with_profile sibling methods in kms_aws_envelope.rs and kms_secrets_manager.rs. The original methods now take aws_profile_name: Option<&str> directly. All call sites updated (builders.rs, integration tests, example, the kms_aws_envelope unit test).

Python README config table: added an AwsProfileName row in the setup() keys table.

Wording consistency: index.d.ts and the .NET XML doc now both reference ~/.aws/credentials (the SDK honors both ~/.aws/credentials and ~/.aws/config; we standardize on credentials to match the .NET docstring and the more common case).

Test cleanup: removed the unused using GoDaddy.Asherah; in AsherahConfigTests.cs (the test namespace already has it as an implicit ancestor).

Local verification: full lint sweep green, .NET test suite green (124 + 30 each TFM), Python (50), Node (full roundtrip + 4 cache-bound), Go (full suite), Ruby (40). I also merged latest main into the branch — that brings in #234 and #235 (bounded LRU session caches across all bindings) so the cross-binding state is consistent.

@jgowdy-godaddy jgowdy-godaddy merged commit 82003f9 into godaddy:main Apr 29, 2026
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants