Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: replace verify_interface with isinstance #467

Merged
merged 3 commits into from
Jun 3, 2022

Conversation

josecorella
Copy link
Contributor

@josecorella josecorella commented Jun 2, 2022

Issue #, if available:
#464

Description of changes:

  1. pyca/cryptography is planning to remove verify_interface. However; since they run the esdk as downstream in ci, they can't deprecate the function without breaking their CI. This PR is intended to unblock them by removing verify_interface from the ESDK. verify_interface takes an abstract class in this case ec.EllipticCurve and the subclass object you wish to verify, in our use case it's algorithm.signing_algorithm_info which is a subclass that implements the ec.EllipticCurve abstract class. The only allowed subclasses we use are ec.SECP256R1 and ec.SECP384R1 and verify_interface goes through the class and makes sure that the expected values are there. Since both of the allowed subclasses implement ec.EllipticCurve, using isinstance works as a replacement.

  2. I noticed that the test_vector_handlers static analysis action is failing because of linter dependency issues. I went ahead and updated it to use the pinned dependencies in dev_requirements/. If these changes don't make sense to include in the same PR I am happy to break them up into two :)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Check any applicable:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

@josecorella josecorella marked this pull request as ready for review June 2, 2022 19:27
@josecorella josecorella requested a review from a team as a code owner June 2, 2022 19:27
Copy link
Contributor

@texastony texastony left a comment

Choose a reason for hiding this comment

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

Looks good. Left a nit.
I am going to dig into what verify_interface actually does...
Is there a reason we originally used verify_interface instead of isinstance?
Did @josecorella look into this?
If so, can you update the PR's description to mention your findings?

UPDATE: I had not realized that pyca had already suggested to us that isinstance offers the same guarantees, at least in our use cases.

def test_signer_from_key_bytes(patch_default_backend, patch_serialization, patch_build_hasher, patch_ec):
mock_algorithm_info = MagicMock(return_value=sentinel.algorithm_info, spec=patch_ec.EllipticCurve)
_algorithm = MagicMock(signing_algorithm_info=mock_algorithm_info)
# _algorithm = MagicMock()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove left over commented out code

@josecorella josecorella merged commit d3c7638 into aws:master Jun 3, 2022
@josecorella josecorella deleted the refactor_verify_interface branch June 15, 2022 17:50
@nicolas-velazquez-koin
Copy link

Hi! Due to this refactor an error NotSupportedError("Unsupported signing algorithm info") is raised when decrypting with some Algorithms with version of cryptography=2.8. I upgrade the cryptography version to the latest (41.0.7 right now) and the error was gone. I dont know from what version the issue is resolved.

I suggest to increase the required version of cryptography in your guide.

Algorithm with issue: AlgorithmSuite.AES_256_GCM_HKDF_SHA512_COMMIT_KEY_ECDSA_P384

Error log:
Error on closing
Traceback (most recent call last):
File "/.local/lib/python3.8/site-packages/aws_encryption_sdk/init.py", line 196, in decrypt
plaintext = decryptor.read()
File "/.local/lib/python3.8/site-packages/aws_encryption_sdk/streaming_client.py", line 260, in read
self._prep_message()
File "/.local/lib/python3.8/site-packages/aws_encryption_sdk/streaming_client.py", line 792, in _prep_message
self._header, self.header_auth = self._read_header()
File "/.local/lib/python3.8/site-packages/aws_encryption_sdk/streaming_client.py", line 830, in _read_header
decryption_materials = self.config.materials_manager.decrypt_materials(request=decrypt_materials_request)
File "/.local/lib/python3.8/site-packages/aws_encryption_sdk/materials_managers/caching.py", line 251, in decrypt_materials
new_result = self.backing_materials_manager.decrypt_materials(request)
File "/.local/lib/python3.8/site-packages/aws_encryption_sdk/materials_managers/default.py", line 155, in decrypt_materials
verification_key = self._load_verification_key_from_encryption_context(
File "/.local/lib/python3.8/site-packages/aws_encryption_sdk/materials_managers/default.py", line 136, in _load_verification_key_from_encryption_context
verifier = Verifier.from_encoded_point(algorithm=algorithm, encoded_point=encoded_verification_key)
File "/.local/lib/python3.8/site-packages/aws_encryption_sdk/internal/crypto/authentication.py", line 144, in from_encoded_point
return cls(
File "/.local/lib/python3.8/site-packages/aws_encryption_sdk/internal/crypto/authentication.py", line 44, in init
self._signature_type = self._set_signature_type()
File "/.local/lib/python3.8/site-packages/aws_encryption_sdk/internal/crypto/authentication.py", line 51, in _set_signature_type
raise NotSupportedError("Unsupported signing algorithm info")
aws_encryption_sdk.exceptions.NotSupportedError: Unsupported signing algorithm info

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants