Address PR feedback

AzureAD · Jul 16, 2024 · cb7c22b · cb7c22b
1 parent 30a9eee
commit cb7c22b
Show file tree

Hide file tree

Showing 3 changed files with 9 additions and 9 deletions.
diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ClientCertificate.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ClientCertificate.java
@@ -8,8 +8,6 @@
 
 import java.io.IOException;
 import java.io.InputStream;
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
 import java.security.KeyStore;
 import java.security.KeyStoreException;
 import java.security.MessageDigest;
@@ -21,7 +19,6 @@
 import java.security.cert.CertificateEncodingException;
 import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
-import java.security.interfaces.RSAPrivateKey;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Base64;
@@ -53,7 +50,7 @@ public String publicCertificateHash()
             throws CertificateEncodingException, NoSuchAlgorithmException {
 
         return Base64.getEncoder().encodeToString(ClientCertificate
-                .getHash(publicKeyCertificateChain.get(0).getEncoded()));
+                .getHashSha256(publicKeyCertificateChain.get(0).getEncoded()));
     }
 
     public String publicCertificateHashSha1()
@@ -132,7 +129,7 @@ private static byte[] getHashSha1(final byte[] inputBytes) throws NoSuchAlgorith
         return md.digest();
     }
 
-    private static byte[] getHash(final byte[] inputBytes) throws NoSuchAlgorithmException {
+    private static byte[] getHashSha256(final byte[] inputBytes) throws NoSuchAlgorithmException {
         final MessageDigest md = MessageDigest.getInstance("SHA-256");
         md.update(inputBytes);
         return md.digest();

diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ConfidentialClientApplication.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ConfidentialClientApplication.java
@@ -102,7 +102,7 @@ private void initClientAuthentication(IClientCredential clientCredential) {
             this.clientCertAuthentication = true;
             this.clientCertificate = (ClientCertificate) clientCredential;
             if (Authority.detectAuthorityType(this.authenticationAuthority.canonicalAuthorityUrl()) == AuthorityType.ADFS) {
-                clientAuthentication = buildValidClientCertificateAuthorityLegacySha1();
+                clientAuthentication = buildValidClientCertificateAuthoritySha1();
             } else  {
                 clientAuthentication = buildValidClientCertificateAuthority();
             }
@@ -136,7 +136,9 @@ private ClientAuthentication buildValidClientCertificateAuthority() {
         return createClientAuthFromClientAssertion(clientAssertion);
     }
 
-    private ClientAuthentication buildValidClientCertificateAuthorityLegacySha1() {
+    //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
+    private ClientAuthentication buildValidClientCertificateAuthoritySha1() {
         ClientAssertion clientAssertion = JwtHelper.buildJwt(
                 clientId(),
                 clientCertificate,

diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/JwtHelper.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/JwtHelper.java
@@ -22,7 +22,7 @@ final class JwtHelper {
 
     static ClientAssertion buildJwt(String clientId, final ClientCertificate credential,
                                     final String jwtAudience, boolean sendX5c,
-                                    boolean useLegacySha1) throws MsalClientException {
+                                    boolean useSha1) throws MsalClientException {
         if (StringHelper.isBlank(clientId)) {
             throw new IllegalArgumentException("clientId is null or empty");
         }
@@ -56,7 +56,8 @@ static ClientAssertion buildJwt(String clientId, final ClientCertificate credent
                 builder.x509CertChain(certs);
             }
 
-            if (useLegacySha1) {
+            //SHA-256 is preferred, however certain flows still require SHA-1 due to what is supported server-side
+            if (useSha1) {
                 builder.x509CertThumbprint(new Base64URL(credential.publicCertificateHashSha1()));
             } else {
                 builder.x509CertSHA256Thumbprint(new Base64URL(credential.publicCertificateHash()));