Skip to content

Commit 175a4eb

Browse files
Avery-DunnAvery-Dunn
authored
Merge pull request #898 from AzureAD/avdunn/sha256-fix
Fix regression issue in IClientCertificate caused by SHA256 changes
2 parents 15da510 + 1b90d43 commit 175a4eb

File tree

6 files changed

+147
-29
lines changed

6 files changed

+147
-29
lines changed

‎msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ClientCertificate.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,14 +46,15 @@ final class ClientCertificate implements IClientCertificate {
4646
this.publicKeyCertificateChain = publicKeyCertificateChain;
4747
}
4848

49-
public String publicCertificateHash()
49+
@Override
50+
public String publicCertificateHash256()
5051
throws CertificateEncodingException, NoSuchAlgorithmException {
5152

5253
return Base64.getEncoder().encodeToString(ClientCertificate
5354
.getHashSha256(publicKeyCertificateChain.get(0).getEncoded()));
5455
}
5556

56-
public String publicCertificateHashSha1()
57+
public String publicCertificateHash()
5758
throws CertificateEncodingException, NoSuchAlgorithmException {
5859

5960
return Base64.getEncoder().encodeToString(ClientCertificate

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

Lines changed: 5 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -101,11 +101,7 @@ private void initClientAuthentication(IClientCredential clientCredential) {
101101
} else if (clientCredential instanceof ClientCertificate) {
102102
this.clientCertAuthentication = true;
103103
this.clientCertificate = (ClientCertificate) clientCredential;
104-
if (Authority.detectAuthorityType(this.authenticationAuthority.canonicalAuthorityUrl()) == AuthorityType.ADFS) {
105-
clientAuthentication = buildValidClientCertificateAuthoritySha1();
106-
} else {
107-
clientAuthentication = buildValidClientCertificateAuthority();
108-
}
104+
clientAuthentication = buildValidClientCertificateAuthority();
109105
} else if (clientCredential instanceof ClientAssertion) {
110106
clientAuthentication = createClientAuthFromClientAssertion((ClientAssertion) clientCredential);
111107
} else {
@@ -119,32 +115,23 @@ protected ClientAuthentication clientAuthentication() {
119115
final Date currentDateTime = new Date(System.currentTimeMillis());
120116
final Date expirationTime = ((PrivateKeyJWT) clientAuthentication).getJWTAuthenticationClaimsSet().getExpirationTime();
121117
if (expirationTime.before(currentDateTime)) {
122-
//The asserted private jwt with the client certificate can expire so rebuild it when the
123118
clientAuthentication = buildValidClientCertificateAuthority();
124119
}
125120
}
126121
return clientAuthentication;
127122
}
128123

129124
private ClientAuthentication buildValidClientCertificateAuthority() {
130-
ClientAssertion clientAssertion = JwtHelper.buildJwt(
131-
clientId(),
132-
clientCertificate,
133-
this.authenticationAuthority.selfSignedJwtAudience(),
134-
sendX5c,
135-
false);
136-
return createClientAuthFromClientAssertion(clientAssertion);
137-
}
125+
//The library originally used SHA-1 for thumbprints as other algorithms were not supported server-side.
126+
//When this was written support for SHA-256 had been added, however ADFS scenarios still only allowed SHA-1.
127+
boolean useSha1 = Authority.detectAuthorityType(this.authenticationAuthority.canonicalAuthorityUrl()) == AuthorityType.ADFS;
138128

139-
//The library originally used SHA-1 for thumbprints as other algorithms were not supported server-side,
140-
// and while support for SHA-256 has been added certain flows still only allow SHA-1
141-
private ClientAuthentication buildValidClientCertificateAuthoritySha1() {
142129
ClientAssertion clientAssertion = JwtHelper.buildJwt(
143130
clientId(),
144131
clientCertificate,
145132
this.authenticationAuthority.selfSignedJwtAudience(),
146133
sendX5c,
147-
true);
134+
useSha1);
148135
return createClientAuthFromClientAssertion(clientAssertion);
149136
}
150137

‎msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IClientCertificate.java‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,20 @@ public interface IClientCertificate extends IClientCredential {
2323
PrivateKey privateKey();
2424

2525
/**
26-
* Base64 encoded hash of the the public certificate.
26+
* Base64 encoded SHA-256 hash of the public certificate.
27+
*
28+
* @return base64 encoded string
29+
* @throws CertificateEncodingException if an encoding error occurs
30+
* @throws NoSuchAlgorithmException if requested algorithm is not available in the environment
31+
*/
32+
default String publicCertificateHash256() throws CertificateEncodingException, NoSuchAlgorithmException {
33+
//Default implementation that returns null, to add backwards compatibility for those implementing this public interface.
34+
//If left as null, the library will default to the older publicCertificateHash() method and SHA-1 hashing.
35+
return null;
36+
}
37+
38+
/**
39+
* Base64 encoded SHA-1 hash of the public certificate.
2740
*
2841
* @return base64 encoded string
2942
* @throws CertificateEncodingException if an encoding error occurs

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,13 @@ static ClientAssertion buildJwt(String clientId, final ClientCertificate credent
5656
builder.x509CertChain(certs);
5757
}
5858

59-
//SHA-256 is preferred, however certain flows still require SHA-1 due to what is supported server-side
60-
if (useSha1) {
61-
builder.x509CertThumbprint(new Base64URL(credential.publicCertificateHashSha1()));
59+
//SHA-256 is preferred, however certain flows still require SHA-1 due to what is supported server-side. If SHA-256
60+
// is not supported or the IClientCredential.publicCertificateHash256() method is not implemented, the library will default to SHA-1.
61+
String hash256 = credential.publicCertificateHash256();
62+
if (useSha1 || hash256 == null) {
63+
builder.x509CertThumbprint(new Base64URL(credential.publicCertificateHash()));
6264
} else {
63-
builder.x509CertSHA256Thumbprint(new Base64URL(credential.publicCertificateHash()));
65+
builder.x509CertSHA256Thumbprint(new Base64URL(hash256));
6466
}
6567

6668
jwt = new SignedJWT(builder.build(), claimsSet);

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

Lines changed: 72 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,22 @@
33

44
package com.microsoft.aad.msal4j;
55

6+
import com.nimbusds.oauth2.sdk.auth.PrivateKeyJWT;
67
import org.junit.jupiter.api.Test;
78
import org.junit.jupiter.api.TestInstance;
89

910
import static org.junit.jupiter.api.Assertions.assertEquals;
1011
import static org.junit.jupiter.api.Assertions.assertNotNull;
12+
import static org.junit.jupiter.api.Assertions.assertNull;
1113
import static org.junit.jupiter.api.Assertions.assertThrows;
12-
import static org.mockito.Mockito.doReturn;
13-
import static org.mockito.Mockito.mock;
14+
import static org.mockito.ArgumentMatchers.any;
15+
import static org.mockito.Mockito.*;
1416

1517
import java.math.BigInteger;
16-
import java.security.PrivateKey;
18+
import java.security.*;
19+
import java.security.cert.CertificateException;
1720
import java.security.interfaces.RSAPrivateKey;
21+
import java.util.*;
1822

1923
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
2024
class ClientCertificateTest {
@@ -36,4 +40,69 @@ void testGetClient() {
3640
final ClientCertificate kc = ClientCertificate.create(key, null);
3741
assertNotNull(kc);
3842
}
43+
44+
@Test
45+
void testIClientCertificateInterface_Sha1andSha256() throws NoSuchAlgorithmException, CertificateException {
46+
//See https://github.com/AzureAD/microsoft-authentication-library-for-java/issues/863 for context on this test.
47+
//Essentially, it aims to test compatibility for customers that implemented IClientCertificate in older versions of the library.
48+
49+
//IClientCertificate.publicCertificateHash256() returns null by default if not implemented...
50+
IClientCertificate certificate = new TestClientCredential();
51+
assertNull(certificate.publicCertificateHash256());
52+
53+
//... but ClientCredentialFactory has an implemented version, so it should not be null.
54+
certificate = ClientCredentialFactory.createFromCertificate(TestHelper.getPrivateKey(), TestHelper.getX509Cert());
55+
assertNotNull(certificate.publicCertificateHash256());
56+
}
57+
58+
@Test
59+
void testIClientCertificateInterface_CredentialFactoryUsesSha256() throws Exception {
60+
DefaultHttpClient httpClientMock = mock(DefaultHttpClient.class);
61+
62+
ConfidentialClientApplication cca =
63+
ConfidentialClientApplication.builder("clientId", ClientCredentialFactory.createFromCertificate(TestHelper.getPrivateKey(), TestHelper.getX509Cert()))
64+
.authority("https://login.microsoftonline.com/tenant")
65+
.instanceDiscovery(false)
66+
.validateAuthority(false)
67+
.httpClient(httpClientMock)
68+
.build();
69+
70+
HashMap<String, String> tokenResponseValues = new HashMap<>();
71+
tokenResponseValues.put("access_token", "accessTokenSha256");
72+
73+
when(httpClientMock.send(any(HttpRequest.class))).thenAnswer( parameters -> {
74+
HttpRequest request = parameters.getArgument(0);
75+
Set<String> headerParams = ((PrivateKeyJWT) cca.clientAuthentication()).getClientAssertion().getHeader().getIncludedParams();
76+
if (request.body().contains(((PrivateKeyJWT) cca.clientAuthentication()).getClientAssertion().serialize())
77+
&& headerParams.contains("x5t#S256")) {
78+
79+
return TestHelper.expectedResponse(200, TestHelper.getSuccessfulTokenResponse(tokenResponseValues));
80+
}
81+
return null;
82+
});
83+
84+
ClientCredentialParameters parameters = ClientCredentialParameters.builder(Collections.singleton("scopes")).build();
85+
86+
IAuthenticationResult result = cca.acquireToken(parameters).get();
87+
88+
assertNotNull(result);
89+
assertEquals("accessTokenSha256", result.accessToken());
90+
}
91+
92+
class TestClientCredential implements IClientCertificate {
93+
@Override
94+
public PrivateKey privateKey() {
95+
return null;
96+
}
97+
98+
@Override
99+
public String publicCertificateHash() {
100+
return "";
101+
}
102+
103+
@Override
104+
public List<String> getEncodedPublicKeyCertificateChain() {
105+
return Collections.emptyList();
106+
}
107+
}
39108
}

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

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,15 @@
77
import com.nimbusds.jose.crypto.RSASSASigner;
88
import com.nimbusds.jose.jwk.RSAKey;
99
import com.nimbusds.jose.jwk.gen.RSAKeyGenerator;
10+
1011
import java.io.File;
1112
import java.io.FileWriter;
1213
import java.io.IOException;
1314
import java.net.URISyntaxException;
1415
import java.nio.file.Files;
1516
import java.nio.file.Paths;
17+
import java.security.*;
18+
import java.security.cert.X509Certificate;
1619
import java.util.Base64;
1720
import java.util.Collections;
1821
import java.util.HashMap;
@@ -40,6 +43,14 @@ class TestHelper {
4043
"\"tid\": \"%s\"," +
4144
"\"ver\": \"2.0\"}";
4245

46+
static X509Certificate x509Cert = getX509Cert();
47+
static PrivateKey privateKey = getPrivateKey();
48+
49+
public static String CERTIFICATE_ALIAS = "LabAuth.MSIDLab.com";
50+
private static final String WIN_KEYSTORE = "Windows-MY";
51+
private static final String KEYSTORE_PROVIDER = "SunMSCAPI";
52+
private static final String MAC_KEYSTORE = "KeychainStore";
53+
4354
static String readResource(Class<?> classInstance, String resource) {
4455
try {
4556
return new String(Files.readAllBytes(Paths.get(classInstance.getResource(resource).toURI())));
@@ -125,4 +136,39 @@ static String createIdToken(HashMap<String, String> idTokenValues) {
125136

126137
return String.format("someheader.%s.somesignature", encodedTokenValues);
127138
}
128-
}
139+
140+
static void setPrivateKeyAndCert() {
141+
try {
142+
String os = System.getProperty("os.name");
143+
KeyStore keystore;
144+
if (os.toLowerCase().contains("windows")) {
145+
keystore = KeyStore.getInstance(WIN_KEYSTORE, KEYSTORE_PROVIDER);
146+
} else {
147+
keystore = KeyStore.getInstance(MAC_KEYSTORE);
148+
}
149+
150+
keystore.load(null, null);
151+
privateKey = (PrivateKey) keystore.getKey(CERTIFICATE_ALIAS, null);
152+
x509Cert = (X509Certificate) keystore.getCertificate(
153+
CERTIFICATE_ALIAS);
154+
} catch (Exception e) {
155+
throw new RuntimeException("Error getting certificate from keystore: " + e.getMessage());
156+
}
157+
}
158+
159+
static X509Certificate getX509Cert() {
160+
if (x509Cert == null) {
161+
setPrivateKeyAndCert();
162+
}
163+
164+
return x509Cert;
165+
}
166+
167+
static PrivateKey getPrivateKey() {
168+
if (privateKey == null) {
169+
setPrivateKeyAndCert();
170+
}
171+
172+
return privateKey;
173+
}
174+
}

0 commit comments

Comments
 (0)