CertificateCredential supports PKCS12 certs

[chlowell]

Closes #13540

[mccoyp]

Is there a reason (like an MSAL requirement) for using PrivateFormat.TraditionalOpenSSL, rather than PrivateFormat.PKCS8?

[chlowell]

No. So far as I can tell we have no reason to prefer either, so I'll follow cryptography's preference for PKCS8.

[chlowell]

I split the cert-as-bytes part of this into #16410, will refocus this PR on PKCS12 support.

[xiangyan99]

Seems like certificate_path is not required. Why not give it a default value?

[chlowell]

Either certificate_path or certificate_data is required. CertificateCredential.__init__ will always have a value for certificate_path because it was originally a required parameter.

[xiangyan99]

Then if user wants to use certificate_data, they have to explicitly set certificate_path to None?

My concern (though not very strong) is if we imply user must set the value of certificate_path and if certificate_path is not None, setting certificate_data raises ValueError. It will cause confusion if user wants to use certificate_data to get_client_credential?

[chlowell]

This (get_client_credential) is an internal helper method called only by CertificateCredential.__init__, so I doubt it implies anything to users. This is the public API:

# load a cert from disk
CertificateCredential(tenant_id, client_id, "/cert.pem")

# load a cert from bytes
CertificateCredential(tenant_id, client_id, certificate_data=some_bytes)

[xiangyan99]

Get it. Then never mind.

Thanks for the clarification.

[mccoyp]

Just so that I know for future reference, how do you know when a nosec suppression is necessary? Is there a command to locally run security checks?

[chlowell]

The tool is bandit. Its warnings are rare enough that I've never run it locally. It runs as a stage of the Analyze job, so I expect a bandit warning to fail a CI run. You suppress the warning when you really need to do something bandit considers insecure and have confirmed there is no danger. For example, the issue here is that a collision attack against SHA1 is feasible. In this context that means an attacker could create a forged cert having the same fingerprint as a legitimate one. That's okay because AAD has a copy of the legitimate cert and uses the fingerprint only to identify that copy. An attacker can't authenticate with such a forged cert because its signatures won't verify against the legitimate cert.

[mccoyp]

What's request here?

[chlowell]

One of pytest's built-in fixtures: https://docs.pytest.org/en/6.2.x/reference.html?highlight=fixture#request

[DhavalShewale]

@chlowell By when can we expect support for PKCS12 certs to be available in the azure-identity library

[chlowell]

This change will be in 1.7.0b4, which we plan to release September 8

[DhavalShewale]

@chlowell Tks for the quick revert. I presume 1.7.0b4 will be a beta release.

Do we have clarity on when a stable release for version 1.7.0 will be available?

[chlowell]

October at the earliest. If you need something in the meantime, it's not too difficult to convert PKCS12 to PEM with cryptography (for example).