feat: add ecdsa p-384 support #1872
Conversation
GDC (Google Distributed Cloud) needs to support ECDSA-P384 keys for compliance. This change creates an EsSigner and EsVerifier class that is capable of supporting both ECDSA-P256 and ECDSA-P384 keys for backwards compatibility. The EsSigner and EsVerifier classes are plumbed through to the GDC service accounts and are used to both sign and verify JWTs. This implementation was successfully tested against a GDC instance using both ECDSA-P256 and ECDSA-P384 keys.
daniel-sanche
added
the
kokoro:run
yoshi-kokoro
removed
the
kokoro:run
google/auth/crypt/__init__.py
Outdated
| ES256Verifier = es256.ES256Verifier | ||
| if es is not None: # pragma: NO COVER | ||
| ES256Signer = es.EsSigner | ||
| ES256Verifier = es.EsVerifier |
There was a problem hiding this comment.
We have ES256Verifier/ES256Signer as separate classes in the the es256 module, but the import file is shadowing them with a EsSigner/EsVerifier aliases
I understand they are both set up as aliases, so this is essentially equivalent for now. But I could see it getting very confusing in the future, if we try to make changes to the classes
There was a problem hiding this comment.
I kept these as aliases in case there are users that import ES256Signer/ES256Verifier to not break them. What is the deprecation policy for this library?
I agree that it is confusing though. Thoughts?
There was a problem hiding this comment.
Yes, we need to keep them around to avoid breaking existing code. Can we replace these with imports from the es256.py file, so we are defining the alias in one place?
| try: | ||
| from google.auth.crypt import es256 | ||
| from google.auth.crypt import es | ||
| except ImportError: # pragma: NO COVER |
There was a problem hiding this comment.
- In what case would the import fail (I assume importing cryptography?)? That should probably be documented as a comment
- this is marked as NO COVER. But it seems like we should have some coverage for this scenerio. Thoughts?
There was a problem hiding this comment.
Added a comment.
I tried to add a unit test to test this, but found that it is not trivial to simulate an import error. The logic here is straight forward enough that it might be ok not to have code coverage for this part. What do you think?
There was a problem hiding this comment.
IIRC, you can unimport a package using something like del cryptography. That should let you define a test method that doesn't have cryptography active. Did you try doing something like that?
There was a problem hiding this comment.
Yeah, I tried using monkey patch to remove the library and then to reload imports but it wasn't working. I think spending additional time to figure out how to simulate an import error just to check that the ES* types are not defined has low ROI.
| @@ -0,0 +1,208 @@ | |||
| # Copyright 2017 Google Inc. | |||
There was a problem hiding this comment.
Note to self (and other reviewers): most of this file is copied over from es256.py, so I only focused on diff for this review
- Changed ESAttributes to _ESAttributes. - Merged from_private_key and from_public_key to from_key.
daniel-sanche
added
the
kokoro:run
yoshi-kokoro
removed
the
kokoro:run
Head branch was pushed to by a user without write access
shengjiang3
dismissed stale reviews from Linchin and daniel-sanche
via
1090a46
daniel-sanche
added
the
kokoro:run
yoshi-kokoro
removed
the
kokoro:run
daniel-sanche
added
the
kokoro:run
yoshi-kokoro
removed
the
kokoro:run
daniel-sanche
added
the
kokoro:run
yoshi-kokoro
removed
the
kokoro:run
4 participants
GDC (Google Distributed Cloud) needs to support ECDSA-P384 keys for compliance. This change creates an EsSigner and EsVerifier class that is capable of supporting both ECDSA-P256 and ECDSA-P384 keys for backwards compatibility. The EsSigner and EsVerifier classes are plumbed through to the GDC service accounts and are used to both sign and verify JWTs.
This implementation was successfully tested against a GDC instance using both ECDSA-P256 and ECDSA-P384 keys.