Skip to content

Commit

Permalink
Removing thumbprint check from PKeyAuth challenge (#2045)
Browse files Browse the repository at this point in the history
### Summary
In the PKeyAuth protocol, the non-interactive flow can send a
[thumbprint-based certificate
challenge](https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-pkap/31066cbf-2462-4cb6-b0b9-68af2f21a5d0).
This thumbprint is meant to serve as a hint to the client side to
cross-check the thumbprint of the current device certificate.
While we have logic for this check, it currently isn't being used due to
[this line always returning
true](https://github.com/AzureAD/microsoft-authentication-library-common-for-android/blob/478f706bf412b3bcc754ec90152568cb23826670/common4j/src/main/com/microsoft/identity/common/java/challengehandlers/PKeyAuthChallenge.java#L148)
(go into the isValidIssuer method to see why).
Additionally, the thumbprint is (supposedly, based on me asking some
folks) hashed with SHA-1, and we're currently working to remove most of
our use of SHA-1. [The iOS team has already removed their thumbprint
verification
logic](AzureAD/microsoft-authentication-library-for-objc#871),
so we're going to do the same.
This PR removes the logic related to the PKeyAuth thumbprint. No
additions are being made.

### Related PRs
- Broker: AzureAD/ad-accounts-for-android#2290
- MSAL:
AzureAD/microsoft-authentication-library-for-android#1828
- ADAL:
AzureAD/azure-activedirectory-library-for-android#1733
  • Loading branch information
melissaahn committed May 25, 2023
1 parent 1e0b106 commit 67efeab
Show file tree
Hide file tree
Showing 5 changed files with 2 additions and 30 deletions.
1 change: 1 addition & 0 deletions changelog.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ V.NEXT
- [MINOR] Add active broker cache (#2030)
- [MAJOR] Added support for DeviceCodeFlow with device id claims
- [MINOR] Replacing SHA-1 used in broker validation with SHA-512 (#2019)
- [MINOR] Removing thumbprint check from PKeyAuth challenge (#2045)

V.12.0.0
----------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,6 @@ public interface IDeviceCertificate {
@NonNull
X509Certificate getX509();

/**
* Gets thumbPrint for certificate.
*
* @return thumbPrint for certificate.
*/
String getThumbPrint();

/**
* Signs a piece of data with the (private key associated to the) certificate.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,10 @@

import com.microsoft.identity.common.java.AuthenticationSettings;
import com.microsoft.identity.common.java.exception.ClientException;
import com.microsoft.identity.common.java.exception.ErrorStrings;
import com.microsoft.identity.common.java.logging.Logger;
import com.microsoft.identity.common.java.util.JWSBuilder;
import com.microsoft.identity.common.java.util.StringUtil;

import java.security.PrivateKey;
import java.security.PublicKey;
import java.security.cert.X509Certificate;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -150,12 +146,7 @@ public Map<String, String> getChallengeHeader() throws ClientException {
"Found a certificate matching the provided authority.");
return getChallengeHeaderWithSignedJwt(deviceCertProxy);
}

if (StringUtil.equalsIgnoreCase(deviceCertProxy.getThumbPrint(), mThumbprint)){
Logger.info(TAG + methodName,
"Found a certificate matching the provided thumbprint.");
return getChallengeHeaderWithSignedJwt(deviceCertProxy);
}
//Note that we aren't validating thumbprint hints anymore, which is in line with the iOS team.

return getChallengeHeaderWithoutSignedJwt();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import com.microsoft.identity.common.java.util.JWSBuilder;

import java.security.PrivateKey;
import java.security.PublicKey;
import java.security.cert.X509Certificate;
import java.util.List;

Expand All @@ -42,8 +41,6 @@
@Accessors(prefix = "m")
public class MockCertLoader implements IDeviceCertificateLoader{

private final String MOCK_CERT_THUMBPRINT = "thumbprint1234";

@Getter
@Setter
private boolean mValidIssuer = true;
Expand All @@ -67,11 +64,6 @@ public boolean isValidIssuer(List<String> certAuthorities) {
return mX509;
}

@Override
public String getThumbPrint() {
return MOCK_CERT_THUMBPRINT;
}

@Override
public byte[] sign(@NonNull String algorithm, byte[] dataToBeSigned) throws ClientException {
throw new UnsupportedOperationException("Should be mocked!");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,11 +173,6 @@ public boolean isValidIssuer(List<String> certAuthorities) {
return mockX509;
}

@Override
public String getThumbPrint() {
return "mock_thumbprint";
}

@Override
public byte[] sign(@NonNull final String algorithm, final byte[] dataToBeSigned) throws ClientException {
// Do nothing.
Expand Down

0 comments on commit 67efeab

Please sign in to comment.