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.

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

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

Thumbprint for certificate made optional #835

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
915a51d
certificate made optionall
Jun 24, 2025
b7bfcff
update as per discussion
Jun 26, 2025
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
31 changes: 21 additions & 10 deletions msal/application.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,25 +66,27 @@ def _str2bytes(raw):
except:
return raw

def _extract_cert_and_thumbprints(private_key, cert):
# Cert concepts https://security.stackexchange.com/a/226758/125264
from cryptography.hazmat.primitives import hashes, serialization
cert_pem = cert.public_bytes(encoding=serialization.Encoding.PEM).decode()
x5c = [
'\n'.join(cert_pem.splitlines()[1:-1])
]
sha256_thumbprint = cert.fingerprint(hashes.SHA256()).hex()
sha1_thumbprint = cert.fingerprint(hashes.SHA1()).hex()
return private_key, sha256_thumbprint, sha1_thumbprint, x5c

def _parse_pfx(pfx_path, passphrase_bytes):
# Cert concepts https://security.stackexchange.com/a/226758/125264
from cryptography.hazmat.primitives import hashes, serialization
from cryptography.hazmat.primitives.serialization import pkcs12
with open(pfx_path, 'rb') as f:
private_key, cert, _ = pkcs12.load_key_and_certificates( # cryptography 2.5+
# https://cryptography.io/en/latest/hazmat/primitives/asymmetric/serialization/#cryptography.hazmat.primitives.serialization.pkcs12.load_key_and_certificates
f.read(), passphrase_bytes)
if not (private_key and cert):
raise ValueError("Your PFX file shall contain both private key and cert")
cert_pem = cert.public_bytes(encoding=serialization.Encoding.PEM).decode() # cryptography 1.0+
x5c = [
'\n'.join(cert_pem.splitlines()[1:-1]) # Strip the "--- header ---" and "--- footer ---"
]
sha256_thumbprint = cert.fingerprint(hashes.SHA256()).hex() # cryptography 0.7+
sha1_thumbprint = cert.fingerprint(hashes.SHA1()).hex() # cryptography 0.7+
# https://cryptography.io/en/latest/x509/reference/#x-509-certificate-object
return private_key, sha256_thumbprint, sha1_thumbprint, x5c
return _extract_cert_and_thumbprints(private_key, cert)


def _load_private_key_from_pem_str(private_key_pem_str, passphrase_bytes):
Expand Down Expand Up @@ -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 •
edited
Loading

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.

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?

"public_certificate": "...-----BEGIN CERTIFICATE-----...",
"passphrase": "Passphrase if the private_key is encrypted (Optional. Added in version 1.6.0)",
}
Expand Down Expand Up @@ -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 •
edited
Loading

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.

private_key, sha256_thumbprint, sha1_thumbprint, x5c =(
_extract_cert_and_thumbprints(
client_credential['private_key'],
client_credential['public_certificate']))
if x5c:
headers["x5c"] = x5c
elif (
client_credential.get("private_key") # PEM blob
and client_credential.get("thumbprint")):
Expand Down
2 changes: 1 addition & 1 deletion msal/sku.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

SKU = "MSAL.Python"