-
Notifications
You must be signed in to change notification settings - Fork 643
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
Set SendCertificateChain
option in KeyVaultReader to enable SN+I authentication
#10179
Set SendCertificateChain
option in KeyVaultReader to enable SN+I authentication
#10179
Conversation
@@ -99,7 +99,19 @@ private SecretClient InitializeClient() | |||
} | |||
else | |||
{ | |||
credential = new ClientCertificateCredential(_configuration.TenantId, _configuration.ClientId, _configuration.Certificate); | |||
if (_configuration.SendX5c) |
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.
@adityapatwardhan
Do you have a use case for this scenario? Otherwise, we're moving to Managed identity. KV reader is less of a priority for us.
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.
PowerShell Gallery is planning to use SN+I. We cannot use Managed Identity for now.
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.
I see. Could you be able to add unit tests covering the new code path?
Good point on UT coverage from @erdembayar. This is especially important to assert this behavior is needed by PowerShell Gallery. |
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.
UTs, so we can protect regressing this code path your team needs.
Summary of the changes (in less than 80 characters):