Thumbprint for certificate made optional #835
Conversation
update as per discussion
| 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: |
There was a problem hiding this comment.
@bgavrilMS , @vi7us ,
I was under the impression that this PR makes the usage of SHA-1 thumbprint parameter optional, by automatically calculating an SHA-1 thumbprint. But it ends up implicitly change the thumbprint-less code path to calculate an SHA256 instead.
-
Before this PR, the Subject Name / Issuer (SNI) code path sets only
sha1_thrumbprinttherefore @vi7us was NOT hitting thissha256_thumbprintcode path here. -
After @vi7us 's first commit with the new
_extract_cert_and_thumbprints()helper calculating both sha1 and sha256, I believe this sha256_thumbprint code path becomes activated. This is a subtle behavior change, but not necessarily a bad thing. Previously, I wasn't sure whether/when Entra ID fully supports sha256, now @vi7us made the change and started using the sha256 thumbprint code path successfully, we can infer that Entra ID indeed fully supports{"algorithm": "PS256", "sha256_thumbprint": sha256_thumbprint}now.
Therefore, I refactored this PR so that it becomes clear that (1) when thumbprint parameter is used, it expects an SHA1 thumbprint (for backward compatibility and for ADFS scenario); (2) when the thumbprint parameter is absent, MSAL Python will use an SHA256 (rather than SHA1) thumbprint.
@vi7us , if at all possible, please test this now-rebased feature branch (you can back up your original feature branch if desirable). If there is no issue reported from your side, I'll merge in this refactored PR and ship it in our next release.
There was a problem hiding this comment.
Previously, I wasn't sure whether/when Entra ID fully supports sha256... (1) when thumbprint parameter is used, it expects an SHA1 thumbprint (for backward compatibility and for ADFS scenario); (2) when the thumbprint parameter is absent, MSAL Python will use an SHA256 (rather than SHA1) thumbprint.
In case there were still any doubts, this is how Java and .NET currently work as well (SHA1 for ADFS, SHA256 for everything else):
AzureAD/microsoft-authentication-library-for-dotnet#4616
AzureAD/microsoft-authentication-library-for-java#840
rayluo
force-pushed
the
vitcurda/20250624/cert-thumbprint-made-optional
branch
from
b7bfcff to
6cd9ebd
Compare
rayluo
force-pushed
the
vitcurda/20250624/cert-thumbprint-made-optional
branch
from
6cd9ebd to
433ad4b
Compare
rayluo
force-pushed
the
vitcurda/20250624/cert-thumbprint-made-optional
branch
2 times, most recently
from
06f137d to
709fea1
Compare
| ) | ||
|
|
||
| # Determine thumbprints based on what's provided | ||
| if client_credential.get("thumbprint"): |
There was a problem hiding this comment.
Are there any tests covering this behavior?
There was a problem hiding this comment.
Good point! A new test file has been added in the modified commit. Please re-review.
rayluo
force-pushed
the
vitcurda/20250624/cert-thumbprint-made-optional
branch
from
709fea1 to
2c50ec4
Compare
4 participants
As per request not to use SHA-1 algorithm for calculation of SHA-1 certificate thumbprint, this parameter is now optional and if not passed, is calculated inside the library itself.