Fix regression issue in IClientCertificate caused by SHA256 changes #898

Avery-Dunn · 2025-01-10T21:21:43Z

Fix for the issue described in #863

As part of #840, the internal implementation of the IClientCertificate interface's publicCertificateHash() method was changed to use SHA-256, and a new publicCertificateHashSha1() method was added to the internal implementation to handle scenarios that still needed SHA-1.

However, this meant that anyone who implemented their own IClientCredential instead of using our ClientCredentialFactory would have a publicCertificateHash() method that still used SHA-1, causing issues because our library expected that method to produce a SHA-256 hash.

This PR solves that issue by adding a new publicCertificateHash256 method to the public interface to handle SHA-256 scenarios, and once again has publicCertificateHash handling the SHA-1 scenarios.

The new interface method is given a default implementation so that it doesn't break any customer's implementations, and if it's not implemented then the library falls back to publicCertificateHash and SHA-1. Anyone who was always using our ClientCredentialFactory should not encounter any issues from either the original changes or the ones in this PR.

bgavrilMS · 2025-01-13T15:22:01Z

msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ClientCertificateTest.java

@@ -36,4 +41,35 @@ void testGetClient() {
        final ClientCertificate kc = ClientCertificate.create(key, null);
        assertNotNull(kc);
    }
+
+    @Test
+    void testIClientCertificateInterface_Sha256AndSha1() throws NoSuchAlgorithmException, CertificateException {


Please create a better E2E (not necessarily integration) test that uses MSAL public API. To assert, you can intercept the HTTP call to the /token endpoint, get the client_assertion parameter and observe that it has x5t#s256 which is correctly computed.

In the latest commit I added a more thorough test: everything up until the call to the token endpoint is real, and then the mocked response will only be successful if 'x5t#s256' is one of the headers in the assertion.

bgavrilMS · 2025-01-13T15:23:05Z

msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/TestHelper.java

@@ -97,4 +105,34 @@ static HttpResponse expectedResponse(int statusCode, String response) {

        return httpResponse;
    }
+
+    static void setPrivateKeyAndCert() {


Can you use a static certificate instead? Like in MISE? That way you can assert on the the well known hash too.

The variables are already static, or are you saying to just create one locally and add it to the repo?

bgavrilMS

Insufficient testing.

…tion-library-for-java into avdunn/sha256-fix # Conflicts: # msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/TestHelper.java

Robbie-Microsoft · 2025-01-22T19:14:35Z

msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ConfidentialClientApplication.java

            if (Authority.detectAuthorityType(this.authenticationAuthority.canonicalAuthorityUrl()) == AuthorityType.ADFS) {
-                clientAuthentication = buildValidClientCertificateAuthoritySha1();
+                //When this was added, ADFS did not support SHA256 hashes for client certificates
+                clientAuthentication = buildValidClientCertificateAuthority(true);
            } else  {
-                clientAuthentication = buildValidClientCertificateAuthority();
+                clientAuthentication = buildValidClientCertificateAuthority(false);


You could eliminate the if statement and call buildValidClientCertificateAuthority only once:

const useSha1: boolean = (Authority.detectAuthorityType(this.authenticationAuthority.canonicalAuthorityUrl()) == AuthorityType.ADFS); clientAuthentication = buildValidClientCertificateAuthority(useSha1)

Good point, in the latest commit I moved this logic down into the buildValidClientCertificateAuthority method. Now that method does the ADFS authority check itself, with a comment explaining why it's done.

msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ConfidentialClientApplication.java

Robbie-Microsoft · 2025-01-22T19:18:03Z

msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ConfidentialClientApplication.java

-                false);
-        return createClientAuthFromClientAssertion(clientAssertion);
-    }
-
    //The library originally used SHA-1 for thumbprints as other algorithms were not supported server-side,
    //  and while support for SHA-256 has been added certain flows still only allow SHA-1


super duper nit: add a , after added. In my mind, it makes sense to indicate a pause to break up the sentence - please feel free to ignore this if you want

In the latest commit I fixed the grammar and adjusted the wording a bit.

Robbie-Microsoft · 2025-01-22T19:46:25Z

msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/JwtHelper.java

-                builder.x509CertThumbprint(new Base64URL(credential.publicCertificateHashSha1()));
+            //SHA-256 is preferred, however certain flows still require SHA-1 due to what is supported server-side. If SHA-256
+            // is not supported or the IClientCredential.publicCertificateHash256() method is not implemented, the library will default to SHA-1.
+            if (useSha1 || credential.publicCertificateHash256() == null) {


You're using credential.publicCertificateHash256() twice - once in the if check and once in the else. Can you set it to a const before the if check?

(and add a type, below is typescript)

const publicCertificateHash256: hash | null = credential.publicCertificateHash256();

In the latest commit it's now only set once.

Robbie-Microsoft · 2025-01-22T19:57:12Z

What is the "default" hash algorithm that you're implementing in this PR? Sha1 or Sha256? Your description states (as I interpret it, based on the last paragraph) that Sha256 should be the default, but your code revolves around checks for if Sha1, then do this, else Sha256 instead of if Sha256, then do this, else Sha1.

Avery-Dunn · 2025-01-24T22:52:59Z

What is the "default" hash algorithm that you're implementing in this PR? Sha1 or Sha256? Your description states (as I interpret it, based on the last paragraph) that Sha256 should be the default, but your code revolves around checks for if Sha1, then do this, else Sha256 instead of if Sha256, then do this, else Sha1.

To clarify, 'default' here refers a default method in an interface. If you add a new method to a public interface, it needs a default implementation or else anything that implements the interface would also need to change to implement the new method (which would be a breaking change): https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html

So technically the publicCertificateHash() method that returns a SHA-1 hash can be considered the 'default' hashing method, because the library will work fine even if the new publicCertificateHash256() API is not given a 'real' implementation and just returns null. However, the vast majority of customers will use the library's implementation of the IClientCertificate interface, which uses SHA-256 for everything except ADFS.

(as a bit of background, we found this issue because a customer was using the library's implementation of IClientCertificate.publicCertificateHash() in their own token calls. The library's implementation is package-private but the interface is public, so when that method started producing a SHA-256 hash instead of SHA-1 it was a breaking change for their very niche use case)

…tion-library-for-java into avdunn/sha256-fix

Add new default method to IClientCertificate to handle SHA256

Loading
Loading status checks…

538e1a5

Avery-Dunn requested a review from a team as a code owner January 10, 2025 21:21

bgavrilMS reviewed Jan 13, 2025

View reviewed changes

bgavrilMS requested changes Jan 13, 2025

View reviewed changes

Avery-Dunn added 2 commits January 21, 2025 09:20

Merge branch 'dev' of https://github.com/AzureAD/microsoft-authentica…

Loading
Loading status checks…

edd4114

…tion-library-for-java into avdunn/sha256-fix # Conflicts: # msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/TestHelper.java

Add test to look for x5t#s256 parameter

Loading
Loading status checks…

d6acf80

Avery-Dunn requested a review from bgavrilMS January 22, 2025 18:30

Robbie-Microsoft reviewed Jan 22, 2025

View reviewed changes

Address PR feedback

Loading
Loading status checks…

35faa26

Avery-Dunn changed the title ~~Fix regression issue in IClientCredential caused by SHA256 changes~~ Fix regression issue in IClientCertificate caused by SHA256 changes Jan 24, 2025

Avery-Dunn added 2 commits January 28, 2025 14:38

Merge branch 'dev' of https://github.com/AzureAD/microsoft-authentica…

a71b7eb

…tion-library-for-java into avdunn/sha256-fix

Remove use of package that is not available in Java 17

Loading
Loading status checks…

1b90d43

bgavrilMS approved these changes Feb 4, 2025

View reviewed changes

Avery-Dunn merged commit 175a4eb into dev Feb 4, 2025
5 checks passed

This was referenced Feb 7, 2025

Version updates for release 1.19.0 #906

Merged

[Bug] IClientCredential public interface is broken by the SHA256 changes #863

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regression issue in IClientCertificate caused by SHA256 changes #898

Fix regression issue in IClientCertificate caused by SHA256 changes #898

Avery-Dunn commented Jan 10, 2025

bgavrilMS Jan 13, 2025

Avery-Dunn Jan 24, 2025

bgavrilMS Jan 13, 2025

Avery-Dunn Jan 22, 2025

bgavrilMS left a comment

Robbie-Microsoft Jan 22, 2025 •

edited

Loading

Avery-Dunn Jan 24, 2025

Robbie-Microsoft Jan 22, 2025 •

edited

Loading

Avery-Dunn Jan 24, 2025

Robbie-Microsoft Jan 22, 2025

Avery-Dunn Jan 24, 2025

Robbie-Microsoft commented Jan 22, 2025

Avery-Dunn commented Jan 24, 2025

Fix regression issue in IClientCertificate caused by SHA256 changes #898

Fix regression issue in IClientCertificate caused by SHA256 changes #898

Conversation

Avery-Dunn commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS left a comment

Choose a reason for hiding this comment

Robbie-Microsoft Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robbie-Microsoft Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robbie-Microsoft commented Jan 22, 2025

Avery-Dunn commented Jan 24, 2025

Robbie-Microsoft Jan 22, 2025 •

edited

Loading

Robbie-Microsoft Jan 22, 2025 •

edited

Loading