-
Notifications
You must be signed in to change notification settings - Fork 206
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
base: dev
Are you sure you want to change the base?
Conversation
msal/application.py
Outdated
@@ -280,6 +280,9 @@ def __init__( | |||
|
|||
.. admonition:: Support using a certificate in X.509 (.pem) format | |||
|
|||
Deprecated because it uses SHA-1 thumbprint. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 ( 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. |
There was a problem hiding this 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.
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? |
|
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 .