Skip to content

Conversation

msabansal
Copy link
Contributor

The implementation is inspired by the implementation of client_secret_credential.

Copy link
Member

@heaths heaths left a 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.

@msabansal
Copy link
Contributor Author

@heaths Thanks for reviewing the PR and providing feedback. I have added an example and fixed the comments.

@msabansal
Copy link
Contributor Author

I have added an example. If there is some test infra i am happy to write a test aswell.

@msabansal msabansal requested a review from heaths April 5, 2022 18:44
@bmc-msft
Copy link
Contributor

bmc-msft commented Apr 6, 2022

Can this be an optional feature? There are occasions where linking against OpenSSL is a significant hurdle.

@msabansal
Copy link
Contributor Author

@bmc-msft I agree. I realized this yesterday when i was building in a CI/CD pipeline 👍

Copy link
Member

@heaths heaths left a 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.

@msabansal
Copy link
Contributor Author

The error i see is in security_keyvault crate which i have not changed (but imported). Do i need to fix it?

@bmc-msft
Copy link
Contributor

bmc-msft commented Apr 7, 2022

@msabansal , pr #719 will address the issues you're experiencing.

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@heaths heaths merged commit 1fb424d into Azure:main Apr 8, 2022
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.

4 participants