Skip to content

Document how to enable sha256 for client credential #833

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.

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

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented Jun 19, 2025

This PR updates the documentation for how to enable SHA-256 thumbprint for client credentials.

Note that the current interface does not take an explicit SHA-256 thumbprint. It will automatically use the SHA-256 thumbprint of the certificate when reading client certificates from PFX files.

The new doc can be read from this doc staging site. The new sentences are scattered in 4 different purple boxes.

This PR will resolve #832 .

@rayluo rayluo requested a review from a team as a code owner June 19, 2025 16:50
@@ -280,6 +280,9 @@ def __init__(

.. admonition:: Support using a certificate in X.509 (.pem) format

Deprecated because it uses SHA-1 thumbprint.

Choose a reason for hiding this comment

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

It's deprecated, but does it still work for backwards compatibility? Should you note that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My understanding of the term Deprecation is aligned with this answer:

Deprecated means that it is still in use, but only for historical purposes and it will be removed probably in the next big release. It is recommended that you do not use deprecated functions or features - even if they are present in the current library for example.

So, yes, it still works for backwards compatibility. If that is OK with you, I intend to not add a "it still works for backwards compatibility" statement to the docs here, otherwise we might need to retrofit the same sentence into maybe a dozen deprecated parameters here and there in our docs.

Copy link
Member

Choose a reason for hiding this comment

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

Robbie is right though. ADFS still uses this. So it's not fully deprecated.

The pfx solution is not yet fit for purpose, since it requires the cert to be on disk.

How about adding a thumbprintSha256 to the dictionary and adding a note that thumbprint is deprecated except for ADFS instead?

Internally, MSAL will choose thumbpritnSha256 if it is set, or fallback to thumbprint if it is not set.

@Robbie-Microsoft
Copy link

I'm confused as to how this all works... the changes in this PR are reflected on the document staging site? Is that correct?

There are 6 purple boxes there btw, not 4 like you've indicted above. And the changes in this PR don't seem to line up with the content of the purple boxes.

@rayluo
Copy link
Collaborator Author

rayluo commented Jun 23, 2025

The new doc can be read from this doc staging site. The new sentences are scattered in 4 different purple boxes.

I'm confused as to how this all works... the changes in this PR are reflected on the document staging site? Is that correct?

There are 6 purple boxes there btw, not 4 like you've indicted above. And the changes in this PR don't seem to line up with the content of the purple boxes.

Sorry about the confusion. There are 6 purple boxes for 6 scenarios. This PR deprecates 2 of them and added SHA-2 content into another 2, so the changes are "scattered in FOUR (not among six) different purple boxes".

Yes, the changes in this PR are reflected on the document staging site. If necessary, you can compare it with the current status-quo doc.
Before (i.e. the current latest): https://msal-python.readthedocs.io/en/latest/#msal.ClientApplication.params.client_credential
After (i.e. the staging): https://msal-python.readthedocs.io/en/docs-staging/#msal.ClientApplication.params.client_credential

Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

The x509 from disk is not a good solution, as per our advisors.

@bgavrilMS
Copy link
Member

The new doc can be read from this doc staging site. The new sentences are scattered in 4 different purple boxes.

I'm confused as to how this all works... the changes in this PR are reflected on the document staging site? Is that correct?
There are 6 purple boxes there btw, not 4 like you've indicted above. And the changes in this PR don't seem to line up with the content of the purple boxes.

Sorry about the confusion. There are 6 purple boxes for 6 scenarios. This PR deprecates 2 of them and added SHA-2 content into another 2, so the changes are "scattered in FOUR (not six) different purple boxes".

Yes, the changes in this PR are reflected on the document staging site. If necessary, you can compare it with the current status-quo doc. Before (i.e. the current latest): https://msal-python.readthedocs.io/en/latest/#msal.ClientApplication.params.client_credential After (i.e. the staging): https://msal-python.readthedocs.io/en/docs-staging/#msal.ClientApplication.params.client_credential

suggestion: I agree with @Robbie-Microsoft that there are too many boxes. Maybe you should combine the "normal cert" with the "sn/i cert" boxes?

@rayluo
Copy link
Collaborator Author

rayluo commented Jun 24, 2025

  • Good point on the deprecation is only for non-ADFS. With ADFS, customer still needs to use SHA-1. Docs has been modified accordingly.
  • Those 6 boxes were historically added in some 5 or 6 different versions, so they became that way. But you are both right that 6 sounds too much. The latest commit consolidated them into 4.
  • I have updated the PR accordingly, and the doc staging site is also updated.
  • Yes, we shall also support loading a cert without a file. Shall we create a dedicate github issue to track that enhancement? Meanwhile, this PR correctly represents the status quo of the current certificate usage of the MSAL Python versions already shipped. So, this PR in itself does not make situation any worse. Perhaps we can still review and merge this PR first. Shall we?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] The docs and the API enable ppl to use SHA256 for credentialas
3 participants