-
Notifications
You must be signed in to change notification settings - Fork 314
Adding support for certificate based credentials for app authentication #708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start! Could you also add an example / test for this crate like with other credentials? It would be expected that a customer provide their own certificate so that wouldn't need to be part of the sample, of course. But this also helps make sure it works as expected.
sdk/identity/src/token_credentials/client_certificate_credentials.rs
Outdated
Show resolved
Hide resolved
@heaths Thanks for reviewing the PR and providing feedback. I have added an example and fixed the comments. |
I have added an example. If there is some test infra i am happy to write a test aswell. |
sdk/identity/src/token_credentials/client_certificate_credentials.rs
Outdated
Show resolved
Hide resolved
Can this be an optional feature? There are occasions where linking against OpenSSL is a significant hurdle. |
@bmc-msft I agree. I realized this yesterday when i was building in a CI/CD pipeline 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, apart from a crate-specific feature name of client_certificate
as opposed to an implementation detail like openssl
.
sdk/identity/examples/client_certificate_credentials/src/main.rs
Outdated
Show resolved
Hide resolved
The error i see is in security_keyvault crate which i have not changed (but imported). Do i need to fix it? |
@msabansal , pr #719 will address the issues you're experiencing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The implementation is inspired by the implementation of client_secret_credential.