Thumbprint for certificate made optional #835
Conversation
msal/application.py
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
| "thumbprint": "A1B2C3D4E5F6...", (Optinal, if not provided, MSAL will calculate it. Added in version 1.34.0) | |
| "thumbprint": "A1B2C3D4E5F6...", (Deprecated. Provide the public certificate instead.) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
msal/application.py
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
Can you update the doc at line 288 as well?
There was a problem hiding this comment.
@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?
msal/application.py
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
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.
msal/application.py
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
- 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"")
There was a problem hiding this comment.
@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 nextelseblock, to reduce code duplication on the passphrase handling. -
Forget about my
private_key_pfx_bytesidea, for now. Representing binary data inclient_credentialparameter has other implications (so far theclient_credentialvalue could bejson.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" | |||
There was a problem hiding this comment.
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.
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.