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.
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"")
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.