Use AWS CRT instead of cryptography for CloudTrail log validation and verify query results #9105
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kdaily
force-pushed
the
cloudtrail-log-validation-to-crt
branch
6 times, most recently
from
0f50af1
to
7d31972
Compare
kdaily
changed the title
kdaily
commented
Nov 27, 2024
| digest=hashlib.sha256(self._create_string_to_sign(sign_file)).digest(), | ||
| signature=signature_bytes | ||
| ) | ||
| if not result: |
There was a problem hiding this comment.
Should add same comment from validate module about the previously raised cryptography error.
Suggested change
| if not result: | |
| if not result: | |
| # The previous implementation caught a cryptography.exceptions.InvalidSignature | |
| # exception here and re-raised the DigestSignatureError to avoid the stack trace | |
| # leaking any information about the key. |
kdaily
force-pushed
the
cloudtrail-log-validation-to-crt
branch
from
7d31972
to
d630ced
Compare
kdaily
commented
Nov 27, 2024
| @@ -41,10 +41,14 @@ dependencies = [ | |||
| "ruamel.yaml.clib>=0.2.0,<=0.2.8", | |||
| "prompt-toolkit>=3.0.24,<3.0.39", | |||
| "distro>=1.5.0,<1.9.0", | |||
| "awscrt>=0.19.18,<=0.22.0", | |||
| "awscrt==0.23.1", | |||
There was a problem hiding this comment.
Note: this change is currently causing the dependency tests to fail, because the requirements-doc-lock.txt is not updated, which needs to occur manually. However, on my developer laptop (Apple M3 Pro) there is no wheel for awscrt on Python 3.8, which is what the lockfiles are generated with. Need to investigate further the appropriate solution for that. Since this PR is targeting the feature branch, it should be safe to merge and will resolve before merging to the v2 branch.
kdaily
force-pushed
the
cloudtrail-log-validation-to-crt
branch
2 times, most recently
from
a96070a
to
252f416
Compare
Porting customizations from `cryptography` to `awscrt` requires `awscrt>=0.23.1`. Also, used GH Action to: - Regenerate lock files for macOS and Linux - Regenerate lock files for Windows In order to pass the dependency tests, `zipp` needs to be pinned for support in Python 3.8. Our strategy has been to pin to the minimum supported dependency version across Python versions so all Python environments are consistent.
The existing test used convenience functions from cryptography to create a private/public key pair. Without these functions available, the key pair needs to exist a priori. This commit adds the appropriately formatted keys expected: a PEM-encoded PKCS1 private key and a DER-encoded PKCS1 public key.
The main change is the verify method returns True or False, where previously it raised an error if validation failed.
This commit makes two changes that were difficult to make separately due to a change in the interface of signing and verifying with public and private keys. First, it migrates all remaining use of cryptography to CRT for the CloudTrail verify query results. Second, the existing test cases relied on utilities from the cryptography package to create a temporary public key from a given private key file. This has been refactored to move the public key to a DER-encoded PKCS1 file that is similarly read in and used directly through the utility module `PublicPrivateKeyLoader`. A DER-encoded file was chosen because it is the format used in the API, and it could easily be read in directly without other cryptographic file manipulation. The `PublicPrivateKeyLoader` class is now used in the CloudTrail log validation tests to keep them as similar as possible. I left the data files separately for each test suite (unit and functional), even though they are identical since there was no preexisting structure for sharing data files across the test suites. This entailed duplicating the helper file loading functions as well, since they relied on the test module directory at import time.
kdaily
force-pushed
the
cloudtrail-log-validation-to-crt
branch
from
252f416
to
71472d3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
Description of changes:
Replace use of
cryptographymethods with AWS CRT methods to perform CloudTrail log validation when calling theaws cloudtrail validate-logscommand, and verifying query results iwth theaws cloudtrail verify-query-resultscommand.The methods relied on to load DER-encoded key files were added to
awscrt==0.23.1, so this change bumps the floor of that dependency.This also required changes to the test suite to not rely on
cryptographyhelper methods for public/private keys. The test logic and coverage remains the same.Testing:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.