Conversation
Co-authored-by: heaths <1532486+heaths@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves azure_identity’s certificate credential ergonomics by letting callers pass raw PKCS#12 bytes directly, avoiding the previous base64 encode/decode roundtrip and making the API intent clearer.
Changes:
- Added a new public
SecretBytestype for securely holding secret byte buffers (no leakage viaDebug/Display, constant-timeEq). - Updated
ClientCertificateCredential::new()to acceptSecretBytesand removed the base64 decode step. - Updated
azure_identity’s changelog to document the new type and the breaking constructor signature change.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
sdk/identity/azure_identity/src/secret_bytes.rs |
Introduces SecretBytes with safe formatting, constant-time equality, conversions, and unit tests. |
sdk/identity/azure_identity/src/lib.rs |
Adds the module and re-exports SecretBytes from the crate root. |
sdk/identity/azure_identity/src/client_certificate_credential.rs |
Switches ClientCertificateCredential::new() from Secret (base64 string) to SecretBytes (raw bytes) and updates tests accordingly. |
sdk/identity/azure_identity/CHANGELOG.md |
Documents the new SecretBytes and the breaking API change in the unreleased section. |
heaths
left a comment
There was a problem hiding this comment.
Rename the function and make sure tests pass - specifically, spell check and mitigation issues appropriately.
Co-authored-by: heaths <1532486+heaths@users.noreply.github.com>
Renamed |
heaths
left a comment
There was a problem hiding this comment.
@copilot commit/pull request instructions make clear to summarize the changes in the PR description - not retain your task list. Do that, and please suggest improvements to make sure you always do that because you most often don't after feedback for changes is provided.
|
@copilot summarize the changes in the PR description. Do not retain your task list. |
Updated the PR description to a prose summary. I've also stored a memory to always replace task checklists with a summary before finalizing future PRs. |
|
@copilot move For context, @chlowell and I talked about this offline. I initially wrestled with this myself and ended up echoing @JeffreyRichter in these matters about how we can almost move things up the dependency stack; however, leaving behind something somewhat intrinsic and is likely to be needed elsehwere in the root of |
Moves `SecretBytes` from `azure_identity/src/secret_bytes.rs` into `azure_core/src/credentials.rs`, placing it alongside `Secret`. - `azure_identity` now re-exports it via `pub use azure_core::credentials::SecretBytes` - Updated `ClientCertificateCredential` and its tests to import `SecretBytes` from `azure_core::credentials` - Updated both `azure_core` and `azure_identity` CHANGELOGs Co-authored-by: heaths <1532486+heaths@users.noreply.github.com>
Done in 31374f3. |
- Remove bare `Debug` import; use `fmt::Debug` throughout - Update SecretBytes doc comment to use linked references - Fix azure_identity CHANGELOG to just say "Added SecretBytes" Co-authored-by: heaths <1532486+heaths@users.noreply.github.com>
|
Hi @copilot. We've noticed that new changes have been pushed to this pull request. Because it is set to automatically merge, we've reset the approvals to allow the opportunity to review the updates. |
Hi @heaths. We've noticed that new changes have been pushed to this pull request. Because it is set to automatically merge, we've reset the approvals to allow the opportunity to review the updates.
SecretBytes is defined in azure_core::credentials and should be imported from there directly. azure_identity uses it internally in client_certificate_credential.rs but does not re-export it. Co-authored-by: heaths <1532486+heaths@users.noreply.github.com>
|
Hi @copilot. We've noticed that new changes have been pushed to this pull request. Because it is set to automatically merge, we've reset the approvals to allow the opportunity to review the updates. |
Hi @heaths. We've noticed that new changes have been pushed to this pull request. Because it is set to automatically merge, we've reset the approvals to allow the opportunity to review the updates.
|
/check-enforcer evaluate |
ClientCertificateCredential::new()previously required callers to base64-encode their certificate bytes and pass them as aSecret, only for the credential to immediately decode them. This replaces that with a purpose-builtSecretBytestype that accepts raw bytes directly.New
SecretBytestype (inazure_core::credentials):Vec<u8>;DebugandDisplayemit"SecretBytes"— no data leakagePartialEq/Equse constant-time comparison (same approach asSecret)From<azure_core::Bytes>,From<&[u8]>,From<Vec<u8>>bytes()accessor returns&[u8]ClientCertificateCredential::new()—certificateparameter changed fromSecrettoSecretBytes; the base64 decode step is removed.Migration
Before:
After:
Breaking Change:
ClientCertificateCredential::new()—certificateis nowSecretBytesinstead ofSecret. SeeCHANGELOG.md.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.