diff --git a/.gitignore b/.gitignore index 58868119..1af10eff 100644 --- a/.gitignore +++ b/.gitignore @@ -62,3 +62,6 @@ msal_cache.bin .env .perf.baseline + +*.pfx +.vscode/settings.json diff --git a/msal/application.py b/msal/application.py index ff1a4eec..57e40980 100644 --- a/msal/application.py +++ b/msal/application.py @@ -66,10 +66,24 @@ def _str2bytes(raw): except: return raw +def _extract_cert_and_thumbprints(cert): + # Cert concepts https://security.stackexchange.com/a/226758/125264 + from cryptography.hazmat.primitives import hashes, serialization + cert_pem = cert.public_bytes( # Requires cryptography 1.0+ + encoding=serialization.Encoding.PEM).decode() + x5c = [ + '\n'.join( + cert_pem.splitlines() + [1:-1] # Strip the "--- header ---" and "--- footer ---" + ) + ] + # https://cryptography.io/en/latest/x509/reference/#x-509-certificate-object + sha256_thumbprint = cert.fingerprint(hashes.SHA256()).hex() # Requires cryptography 0.7+ + sha1_thumbprint = cert.fingerprint(hashes.SHA1()).hex() # Requires cryptography 0.7+ + return sha256_thumbprint, sha1_thumbprint, x5c def _parse_pfx(pfx_path, passphrase_bytes): # Cert concepts https://security.stackexchange.com/a/226758/125264 - from cryptography.hazmat.primitives import hashes, serialization from cryptography.hazmat.primitives.serialization import pkcs12 with open(pfx_path, 'rb') as f: private_key, cert, _ = pkcs12.load_key_and_certificates( # cryptography 2.5+ @@ -77,13 +91,7 @@ def _parse_pfx(pfx_path, passphrase_bytes): f.read(), passphrase_bytes) if not (private_key and cert): raise ValueError("Your PFX file shall contain both private key and cert") - cert_pem = cert.public_bytes(encoding=serialization.Encoding.PEM).decode() # cryptography 1.0+ - x5c = [ - '\n'.join(cert_pem.splitlines()[1:-1]) # Strip the "--- header ---" and "--- footer ---" - ] - sha256_thumbprint = cert.fingerprint(hashes.SHA256()).hex() # cryptography 0.7+ - sha1_thumbprint = cert.fingerprint(hashes.SHA1()).hex() # cryptography 0.7+ - # https://cryptography.io/en/latest/x509/reference/#x-509-certificate-object + sha256_thumbprint, sha1_thumbprint, x5c = _extract_cert_and_thumbprints(cert) return private_key, sha256_thumbprint, sha1_thumbprint, x5c @@ -288,7 +296,10 @@ def __init__( { "private_key": "...-----BEGIN PRIVATE KEY-----... in PEM format", - "thumbprint": "An SHA-1 thumbprint such as A1B2C3D4E5F6...", + "thumbprint": "An SHA-1 thumbprint such as A1B2C3D4E5F6..." + "Changed in version 1.35.0, if thumbprint is absent" + "and a public_certificate is present, MSAL will" + "automatically calculate an SHA-256 thumbprint instead.", "passphrase": "Needed if the private_key is encrypted (Added in version 1.6.0)", "public_certificate": "...-----BEGIN CERTIFICATE-----...", # Needed if you use Subject Name/Issuer auth. Added in version 0.5.0. } @@ -803,15 +814,30 @@ def _build_client(self, client_credential, authority, skip_regional_client=False passphrase_bytes) if client_credential.get("public_certificate") is True and x5c: headers["x5c"] = x5c - elif ( - client_credential.get("private_key") # PEM blob - and client_credential.get("thumbprint")): - sha1_thumbprint = client_credential["thumbprint"] - if passphrase_bytes: - private_key = _load_private_key_from_pem_str( + elif client_credential.get("private_key"): # PEM blob + private_key = ( # handles both encrypted and unencrypted + _load_private_key_from_pem_str( client_credential['private_key'], passphrase_bytes) - else: # PEM without passphrase - private_key = client_credential['private_key'] + if passphrase_bytes + else client_credential['private_key'] + ) + + # Determine thumbprints based on what's provided + if client_credential.get("thumbprint"): + # User provided a thumbprint - use it as SHA-1 (legacy/manual approach) + sha1_thumbprint = client_credential["thumbprint"] + sha256_thumbprint = None + elif isinstance(client_credential.get('public_certificate'), str): + # No thumbprint provided, but we have a certificate to calculate thumbprints + from cryptography import x509 + cert = x509.load_pem_x509_certificate( + _str2bytes(client_credential['public_certificate'])) + sha256_thumbprint, sha1_thumbprint, headers["x5c"] = ( + _extract_cert_and_thumbprints(cert)) + else: + raise ValueError( + "You must provide either 'thumbprint' or 'public_certificate' " + "from which the thumbprint can be calculated.") else: raise ValueError( "client_credential needs to follow this format " @@ -1828,9 +1854,9 @@ def acquire_token_by_username_password( - A successful response would contain "access_token" key, - an error response would contain "error" and usually "error_description". - - [Deprecated] This API is deprecated for public client flows and will be - removed in a future release. Use a more secure flow instead. + + [Deprecated] This API is deprecated for public client flows and will be + removed in a future release. Use a more secure flow instead. Migration guide: https://aka.ms/msal-ropc-migration """ diff --git a/tests/test_optional_thumbprint.py b/tests/test_optional_thumbprint.py new file mode 100644 index 00000000..56ec4013 --- /dev/null +++ b/tests/test_optional_thumbprint.py @@ -0,0 +1,215 @@ +import unittest +from unittest.mock import Mock, patch +from msal.application import ConfidentialClientApplication + + +@patch('msal.application.Authority') +@patch('msal.application.JwtAssertionCreator', new_callable=lambda: Mock( + return_value=Mock(create_regenerative_assertion=Mock(return_value="mock_jwt_assertion")))) +class TestClientCredentialWithOptionalThumbprint(unittest.TestCase): + """Test that thumbprint is optional when public_certificate is provided""" + + # Sample test certificate and private key (PEM format) + # These are minimal valid PEM structures for testing + test_private_key = """-----BEGIN PRIVATE KEY----- +MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQC7VJTUt9Us8cKj +MzEfYyjiWA4R4/M2bS1+fWIcPm15j7uo6xKvRr4PNx5bKMDFqMdW6/xfqFWX0nZK +-----END PRIVATE KEY-----""" + + test_certificate = """-----BEGIN CERTIFICATE----- +MIIC5jCCAc6gAwIBAgIJALdYQVsVsNZHMA0GCSqGSIb3DQEBCwUAMBYxFDASBgNV +BAMMC0V4YW1wbGUgQ0EwHhcNMjQwMTAxMDAwMDAwWhcNMjUwMTAxMDAwMDAwWjAW +-----END CERTIFICATE-----""" + + def _setup_mocks(self, mock_authority_class, authority="https://login.microsoftonline.com/common"): + """Helper to setup Authority mock""" + # Setup Authority mock + mock_authority = Mock() + mock_authority.is_adfs = "adfs" in authority.lower() + + # Extract instance from authority URL + if mock_authority.is_adfs: + # For ADFS: https://adfs.contoso.com/adfs -> adfs.contoso.com + mock_authority.instance = authority.split("//")[1].split("/")[0] + mock_authority.token_endpoint = f"https://{mock_authority.instance}/adfs/oauth2/token" + mock_authority.authorization_endpoint = f"https://{mock_authority.instance}/adfs/oauth2/authorize" + else: + # For AAD: https://login.microsoftonline.com/common -> login.microsoftonline.com + mock_authority.instance = authority.split("//")[1].split("/")[0] + mock_authority.token_endpoint = f"https://{mock_authority.instance}/common/oauth2/v2.0/token" + mock_authority.authorization_endpoint = f"https://{mock_authority.instance}/common/oauth2/v2.0/authorize" + + mock_authority.device_authorization_endpoint = None + mock_authority_class.return_value = mock_authority + + return mock_authority + + def _setup_certificate_mocks(self, mock_extract, mock_load_cert): + """Helper to setup certificate parsing mocks""" + # Mock certificate loading + mock_cert = Mock() + mock_load_cert.return_value = mock_cert + + # Mock _extract_cert_and_thumbprints to return thumbprints + mock_extract.return_value = ( + "mock_sha256_thumbprint", # sha256_thumbprint + "mock_sha1_thumbprint", # sha1_thumbprint + ["mock_x5c_value"] # x5c + ) + + def _verify_assertion_params(self, mock_jwt_creator_class, expected_algorithm, + expected_thumbprint_type, expected_thumbprint_value=None, + has_x5c=False): + """Helper to verify JwtAssertionCreator was called with correct params""" + mock_jwt_creator_class.assert_called_once() + call_args = mock_jwt_creator_class.call_args + + # Verify algorithm + self.assertEqual(call_args[1]['algorithm'], expected_algorithm) + + # Verify thumbprint type + if expected_thumbprint_type == 'sha256': + self.assertIn('sha256_thumbprint', call_args[1]) + self.assertNotIn('sha1_thumbprint', call_args[1]) + elif expected_thumbprint_type == 'sha1': + self.assertIn('sha1_thumbprint', call_args[1]) + self.assertNotIn('sha256_thumbprint', call_args[1]) + if expected_thumbprint_value: + self.assertEqual(call_args[1]['sha1_thumbprint'], expected_thumbprint_value) + + # Verify x5c header if expected + if has_x5c: + self.assertIn('headers', call_args[1]) + self.assertIn('x5c', call_args[1]['headers']) + + return call_args + + @patch('cryptography.x509.load_pem_x509_certificate') + @patch('msal.application._extract_cert_and_thumbprints') + def test_pem_with_certificate_only_uses_sha256( + self, mock_extract, mock_load_cert, mock_jwt_creator_class, mock_authority_class): + """Test that providing only public_certificate (no thumbprint) uses SHA-256""" + authority = "https://login.microsoftonline.com/common" + self._setup_mocks(mock_authority_class, authority) + self._setup_certificate_mocks(mock_extract, mock_load_cert) + + # Create app with certificate credential WITHOUT thumbprint + app = ConfidentialClientApplication( + client_id="my_client_id", + client_credential={ + "private_key": self.test_private_key, + "public_certificate": self.test_certificate, + # Note: NO thumbprint provided + }, + authority=authority + ) + + # Verify SHA-256 with PS256 algorithm is used + self._verify_assertion_params( + mock_jwt_creator_class, + expected_algorithm='PS256', + expected_thumbprint_type='sha256', + has_x5c=True + ) + + def test_pem_with_manual_thumbprint_uses_sha1( + self, mock_jwt_creator_class, mock_authority_class): + """Test that providing manual thumbprint (no certificate) uses SHA-1""" + authority = "https://login.microsoftonline.com/common" + self._setup_mocks(mock_authority_class, authority) + + # Create app with manual thumbprint (legacy approach) + manual_thumbprint = "A1B2C3D4E5F6" + app = ConfidentialClientApplication( + client_id="my_client_id", + client_credential={ + "private_key": self.test_private_key, + "thumbprint": manual_thumbprint, + # Note: NO public_certificate provided + }, + authority=authority + ) + + # Verify SHA-1 with RS256 algorithm is used + self._verify_assertion_params( + mock_jwt_creator_class, + expected_algorithm='RS256', + expected_thumbprint_type='sha1', + expected_thumbprint_value=manual_thumbprint + ) + + def test_pem_with_both_uses_manual_thumbprint_as_sha1( + self, mock_jwt_creator_class, mock_authority_class): + """Test that providing both thumbprint and certificate prefers manual thumbprint (SHA-1)""" + authority = "https://login.microsoftonline.com/common" + self._setup_mocks(mock_authority_class, authority) + + # Create app with BOTH thumbprint and certificate + manual_thumbprint = "A1B2C3D4E5F6" + app = ConfidentialClientApplication( + client_id="my_client_id", + client_credential={ + "private_key": self.test_private_key, + "thumbprint": manual_thumbprint, + "public_certificate": self.test_certificate, + }, + authority=authority + ) + + # Verify manual thumbprint takes precedence (backward compatibility) + self._verify_assertion_params( + mock_jwt_creator_class, + expected_algorithm='RS256', + expected_thumbprint_type='sha1', + expected_thumbprint_value=manual_thumbprint, + has_x5c=True # x5c should still be present + ) + + @patch('cryptography.x509.load_pem_x509_certificate') + @patch('msal.application._extract_cert_and_thumbprints') + def test_pem_with_adfs_uses_sha1( + self, mock_extract, mock_load_cert, mock_jwt_creator_class, mock_authority_class): + """Test that ADFS authority uses SHA-1 even with SHA-256 thumbprint""" + authority = "https://adfs.contoso.com/adfs" + self._setup_mocks(mock_authority_class, authority) + self._setup_certificate_mocks(mock_extract, mock_load_cert) + + # Create app with certificate on ADFS + app = ConfidentialClientApplication( + client_id="my_client_id", + client_credential={ + "private_key": self.test_private_key, + "public_certificate": self.test_certificate, + }, + authority=authority + ) + + # ADFS should force SHA-1 with RS256 even though SHA-256 would be calculated + self._verify_assertion_params( + mock_jwt_creator_class, + expected_algorithm='RS256', + expected_thumbprint_type='sha1' + ) + + def test_pem_with_neither_raises_error(self, mock_jwt_creator_class, mock_authority_class): + """Test that providing neither thumbprint nor certificate raises ValueError""" + authority = "https://login.microsoftonline.com/common" + self._setup_mocks(mock_authority_class, authority) + + # Should raise ValueError when neither thumbprint nor certificate provided + with self.assertRaises(ValueError) as context: + app = ConfidentialClientApplication( + client_id="my_client_id", + client_credential={ + "private_key": self.test_private_key, + # Note: NO thumbprint and NO public_certificate + }, + authority=authority + ) + + self.assertIn("thumbprint", str(context.exception).lower()) + self.assertIn("public_certificate", str(context.exception).lower()) + + +if __name__ == "__main__": + unittest.main()