Skip to content
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

#6673 - Fix of SNI auth not working in Cert based authentication #6676

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

mikeus-hanzlik
Copy link
Contributor

Fixes #6673

Description

After upgrade from ADAL to MSAL auth library, sendX5C flag was not set and it breaks SN+I authentication with AAD app registrations.

Specific Changes

Added sendX5C flag to ConfidentialClientApplicationBuilder.WithCertificate() method.

Testing

Tested locally and also on our repo by implementing this fix on an inherited class.

…tion

After upgrade from ADAL to MSAL auth library, sendX5C flag was not set and it breaks SN+I authentication with AAD app registrations.
@mikeus-hanzlik mikeus-hanzlik requested a review from a team as a code owner July 28, 2023 11:56
@@ -151,11 +151,11 @@ protected override Lazy<IAuthenticator> BuildIAuthenticator()
LazyThreadSafetyMode.ExecutionAndPublication);
}

private Identity.Client.IConfidentialClientApplication CreateClientApplication(X509Certificate2 clientCertificate, string appId, HttpClient customHttpClient = null)
private Identity.Client.IConfidentialClientApplication CreateClientApplication(X509Certificate2 clientCertificate, string appId, bool sendX5c, HttpClient customHttpClient = null)

Choose a reason for hiding this comment

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

should we add default value (I assume false) not to make it a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a private method used only once in this class so we are fine like this.

@tracyboehrer tracyboehrer added the Automation: No parity PR does not need to be applied to other languages. label Jul 28, 2023
@tracyboehrer tracyboehrer merged commit 6baf0f8 into microsoft:main Jul 28, 2023
14 checks passed
@mikeus-hanzlik mikeus-hanzlik deleted the mihanzlk/auth-sni-fix branch October 4, 2023 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Automation: No parity PR does not need to be applied to other languages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SNI auth not working in Cert based both authentication
5 participants