Skip to content

Using SHA256 and PSS padding #722

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Using SHA256 and PSS padding #722

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions msal/application.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,10 @@ def _parse_pfx(pfx_path, passphrase_bytes):
x5c = [
'\n'.join(cert_pem.splitlines()[1:-1]) # Strip the "--- header ---" and "--- footer ---"
]
sha256_thumbprint = cert.fingerprint(hashes.SHA256()).hex() # cryptography 0.7+
Copy link

@Robbie-Microsoft Robbie-Microsoft Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the below checks to this function so that you're only returning one thumbprint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean a test case? We do not currently have such a test case, but its utilization is in this part of the PR. You can see that we pass in only one of the thumbprints, not both.

sha1_thumbprint = cert.fingerprint(hashes.SHA1()).hex() # cryptography 0.7+
# https://cryptography.io/en/latest/x509/reference/#x-509-certificate-object
return private_key, sha1_thumbprint, x5c
return private_key, sha256_thumbprint, sha1_thumbprint, x5c


def _load_private_key_from_pem_str(private_key_pem_str, passphrase_bytes):
Expand Down Expand Up @@ -741,11 +742,12 @@ def _build_client(self, client_credential, authority, skip_regional_client=False
client_assertion = client_credential['client_assertion']
else:
headers = {}
sha1_thumbprint = sha256_thumbprint = None
passphrase_bytes = _str2bytes(
client_credential["passphrase"]
) if client_credential.get("passphrase") else None
if client_credential.get("private_key_pfx_path"):
private_key, sha1_thumbprint, x5c = _parse_pfx(
private_key, sha256_thumbprint, sha1_thumbprint, x5c = _parse_pfx(
client_credential["private_key_pfx_path"],
passphrase_bytes)
if client_credential.get("public_certificate") is True and x5c:
Expand All @@ -763,13 +765,22 @@ def _build_client(self, client_credential, authority, skip_regional_client=False
raise ValueError(
"client_credential needs to follow this format "
"https://msal-python.readthedocs.io/en/latest/#msal.ClientApplication.params.client_credential")
if ("x5c" not in headers # So we did not run the pfx code path
if ("x5c" not in headers # So the .pfx file contains no certificate
and isinstance(client_credential.get('public_certificate'), str)
): # Then we treat the public_certificate value as PEM content
headers["x5c"] = extract_certs(client_credential['public_certificate'])
if sha256_thumbprint and not authority.is_adfs:
assertion_params = {
"algorithm": "PS256", "sha256_thumbprint": sha256_thumbprint,
}
else: # Fall back
if not sha1_thumbprint:
raise ValueError("You shall provide a thumbprint in SHA1.")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the SHA thumbprints something that the developer is meant to provide, and would they see this error message if something went wrong? If so, you may want something more informative like "A SHA1 thumbprint is needed for ADFS" or similar to let them know which SHA version is needed for each flow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When an app developer encounters this error message, he or she is already in the ADFS code path, so the "you shall provide a thumbprint in SHA1" is good enough as an actionable item.

I incline to avoid mentioning too-specific info such as "ADFS" in this case, because that kind of wording tends to become outdated when/if we will add or remove some scenarios into this SHA1 group.

assertion_params = {
"algorithm": "RS256", "sha1_thumbprint": sha1_thumbprint,
}
assertion = JwtAssertionCreator(
private_key, algorithm="RS256",
sha1_thumbprint=sha1_thumbprint, headers=headers)
private_key, headers=headers, **assertion_params)
client_assertion = assertion.create_regenerative_assertion(
audience=authority.token_endpoint, issuer=self.client_id,
additional_claims=self.client_claims or {})
Expand Down