Skip to content

Conversation

@chlowell
Copy link
Member

Mypy is warning about AadClientCertificate.sign() because it uses a private key returned by cryptography's load_pem_private_key() for signing, and that method returns one of several possible types all of whose sign methods take different arguments. So those are legitimate warnings, thank you mypy, that call does indeed raise e.g. TypeError: sign() takes 3 positional arguments but 4 were given when a user passes in a non-RSA private key.

However, no user should do that because Azure AD expects clients to sign JWT assertions with RS256, which is to say the client's certificate must have an RSA private key and load_pem_private_key() must have returned RSAPrivateKey, or the client can't authenticate anyway, even if it could sign an assertion. The cryptic TypeError won't lead a user with the wrong cert back onto the right path, so this PR has CertificateCredential instead raise ValueError with a clear message, documents its requirement for an RSA private key, and makes the situation clear to mypy with a cast.

@chlowell chlowell added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels Feb 10, 2021
private_key = serialization.load_pem_private_key(pem_bytes, password=password, backend=default_backend())
if not isinstance(private_key, RSAPrivateKey):
raise ValueError("CertificateCredential requires an RSA private key because it uses RS256 for signing")
self._private_key = cast(RSAPrivateKey, private_key)
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming this cast is to appease mypy?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't actually need the cast, it's a vestige of my first attempt, which mypy couldn't figure out:

self._private_key = serialization.load_pem_private_key(pem_bytes, password=password, backend=default_backend())
if not isinstance(self._private_key, RSAPrivateKey):
    raise ValueError("CertificateCredential requires an RSA private key because it uses RS256 for signing")

I suppose because the assignment came before the isinstance check?

@chlowell chlowell enabled auto-merge (squash) February 11, 2021 01:20
@chlowell chlowell merged commit 62d6317 into Azure:master Feb 11, 2021
@chlowell chlowell deleted the fix-mypy branch February 11, 2021 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Identity Client This issue points to a problem in the data-plane of the library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants