Skip to content

Thumbprint for certificate made optional #835

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

vi7us
Copy link

@vi7us vi7us commented Jun 24, 2025

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.

@vi7us vi7us requested a review from a team as a code owner June 24, 2025 11:27
@@ -306,7 +308,7 @@ def __init__(

{
"private_key": "...-----BEGIN PRIVATE KEY-----... in PEM format",
"thumbprint": "A1B2C3D4E5F6...",
"thumbprint": "A1B2C3D4E5F6...", (Optinal, if not provided, MSAL will calculate it. Added in version 1.34.0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"thumbprint": "A1B2C3D4E5F6...", (Optinal, if not provided, MSAL will calculate it. Added in version 1.34.0)
"thumbprint": "A1B2C3D4E5F6...", (Deprecated. Provide the public certificate instead.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought you said (in PR 833) that it is not fully deprecated. We will need to settle on the wording, one way or another.

Copy link
Author

Choose a reason for hiding this comment

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

Updated as per request

Copy link
Member

@bgavrilMS bgavrilMS Jun 27, 2025

Choose a reason for hiding this comment

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

@rayluo - if the CX provides the public certificate, then thumbprint is not needed? MSAL can always compute it? In that case the property becomes indeed deprecated. Internally, MSAL will still continue to use SHA1 for ADFS.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I am not mistaken, the thumbprint is a hash of the private key, not the public certificate. So, whether the customer provides a public certificate (to use SNI feature), is irrelevant.

But you are right that MSAL could treat the absence of the thumbprint parameter as "compute the hash automatically". We may still want to keep the old thumbprint="an sha-1 hash" usage for ADFS, because MSAL historically just uses a magic string "https://contoso.com /adfs" to detect ADFS mode, and that might not always be reliable. Better keep the old thumbprint="an sha-1 hash" usage, so that ADFS customer may still opt in to use SHA-1 when needed.

@@ -306,7 +308,7 @@ def __init__(

{
"private_key": "...-----BEGIN PRIVATE KEY-----... in PEM format",
"thumbprint": "A1B2C3D4E5F6...",
"thumbprint": "A1B2C3D4E5F6...", (Optinal, if not provided, MSAL will calculate it. Added in version 1.34.0)
Copy link
Member

Choose a reason for hiding this comment

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

Can you update the doc at line 288 as well?

Copy link
Author

Choose a reason for hiding this comment

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

@bgavrilMS i'm not sure if it will work without public cert passed as we use it to calculate x5c, maybe method _extract_cert_and_thumbprints can be updated to check if public cert is passed and if not x5c will be set to None?

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.

Approved with comments. @rayluo for final sign-off

@@ -306,7 +308,7 @@ def __init__(

{
"private_key": "...-----BEGIN PRIVATE KEY-----... in PEM format",
"thumbprint": "A1B2C3D4E5F6...",
"thumbprint": "A1B2C3D4E5F6...", (Optinal, if not provided, MSAL will calculate it. Added in version 1.34.0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought you said (in PR 833) that it is not fully deprecated. We will need to settle on the wording, one way or another.

@@ -815,6 +817,15 @@ 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")
and client_credential.get("public_certificate")
and not client_credential.get("thumbprint")): # in case user does not pass thumbprint but only certificate and private key
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vi7us , so this implementation only works for the Subject Name/Issuer (SNI) scenario. Why not also make it work for non-SNI?

Besides, this implementation does not seem to handle the passphrase in the {"private_key": "...", "public_certificate": "...", "passphrase": "..."} situation. That feels like a regression. Would you consider refactoring this into the next elif block starting at line 830?

Also, in case you are not aware of, currently MSAL Python's .pfx code path already supports the absence of thumbprint. We will probably consider adding a new "private_key_pfx_bytes" in the near future.

Copy link
Author

@vi7us vi7us Jun 26, 2025

Choose a reason for hiding this comment

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

@rayluo

  • updated to handle also passphrase option.
  • For the Non-SNI do you mean to include new parameter that will be used as a flag to add x5c to the headers?
       "private_key": "...-----BEGIN PRIVATE KEY-----... in PEM format",
       "thumbprint": "A1B2C3D4E5F6...",
       "public_certificate": "...-----BEGIN CERTIFICATE-----...",
       "passphrase": "Passphrase if the private_key is encrypted (Optional. Added in version 1.6.0)",

       "sni_auth": [True|False], # new param

I know it is possible to use .pfx file but that means it must be saved locally which is not what we were suggested (as per email thread).

For the private_key_pfx_bytes do you mean doing something like this?

        pkcs12Decoded = base64.b64decode(private_key_pfx_bytes)
        private_key, certificate, _ = pkcs12.load_key_and_certificates(pkcs12Decoded, B"")

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vi7us , thanks for continuing with this PR!

  • For non-SNI, we are already working on another PR to have non-SNI and SNI share the same doc "box" as we speak, the staged doc is already rendered here. Building on top of that concept and docs, what I have in mind in terms of the usage is this (CC @bgavrilMS ):

    {
      "private_key": "...-----BEGIN PRIVATE KEY-----... in PEM format",
    
      # If present, thumbprint shall be SHA-1 which is still needed when using ADFS.
      # If absent, MSAL Python will automatically use SHA256 thumbprint.
      "thumbprint": "An SHA-1 thumbprint such as A1B2C3D4E5F6...",  # Optional since version 1.34.0
    
      "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.
    }

    Implementation wise, I wonder whether the new elif ... block can be refactored into its next else block, to reduce code duplication on the passphrase handling.

  • Forget about my private_key_pfx_bytes idea, for now. Representing binary data in client_credential parameter has other implications (so far the client_credential value could be json.load(...) from a text string, via env var; introducing binary data would hinder that usage). Luckily, we don't immediately need it if we get this thumbprint-less PEM code path working.

@@ -2,5 +2,5 @@
"""

# The __init__.py will import this. Not the other way around.
__version__ = "1.33.0b1"
__version__ = "1.34.0b1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also revert this change.

We will only bump version number when we cut a release, and decide the actual number at that time.

Besides, bumping version number in each feature PR brings complications in case we would end up reverting the feature PR.

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.

3 participants