feat: add Key Committing AES-GCM and AES-CBC support#147
Conversation
| ##= specification/s3-encryption/client.md#key-commitment | ||
| ##% If the configured Encryption Algorithm is incompatible with the key commitment policy, then it MUST throw an exception. | ||
| def __attrs_post_init__(self): | ||
| if self.algorithm_suite.is_legacy: |
There was a problem hiding this comment.
Blocking/Question: Isn't this too bold a claim?
attrs_post_init fires for both the encrypt and decrypt path.
But this error should only be thrown on the encrypt path.
For the decrypt path, it needs to do a check for enable_legacy_unathenticated_modes.
Which I recognize may not be implemented yet, but shouldn't we at least put a TODO here to acknowledge that this is incorrect?
There was a problem hiding this comment.
you are correct,
this parameter needs to be encryption_algorithm instead;
this is what Java et al do.
| Returns: | ||
| The pseudorandom key (PRK). | ||
| """ | ||
| return hmac.new(salt, ikm, "sha512").digest() |
There was a problem hiding this comment.
Mild Suggestion: Should sha512 be a constant?
| cmm: AbstractCryptoMaterialsManager = field() | ||
|
|
||
| def encrypt(self, plaintext, encryption_context=None): | ||
| def encrypt(self, plaintext, encryption_context=None, algorithm_suite=None): |
There was a problem hiding this comment.
Suggestion: if there is going to be a default algorithm suite, do not bury it here, but put it in init.
It is nice to have defaults all in one place, as compared to scattered across init, materials, and pipeline.
| ##% The S3EC MUST validate the configured Encryption Algorithm against the provided key commitment policy. | ||
| ##= specification/s3-encryption/client.md#key-commitment | ||
| ##% If the configured Encryption Algorithm is incompatible with the key commitment policy, then it MUST throw an exception. | ||
| def __attrs_post_init__(self): |
There was a problem hiding this comment.
Praise: Award winning error experience.
| if algorithm_suite is None: | ||
| algorithm_suite = AlgorithmSuite.ALG_AES_256_GCM_IV12_TAG16_NO_KDF |
There was a problem hiding this comment.
Issue: I am still against hiding defaults in pipeline.
Move this to init and make algorithm_suite a required argument.
There was a problem hiding this comment.
Agreed. Also it should be KC GCM as default.
| commitment_policy: CommitmentPolicy = field( | ||
| default=CommitmentPolicy.REQUIRE_ENCRYPT_REQUIRE_DECRYPT | ||
| ) | ||
| enable_legacy_unauthenticated_modes: bool = field(default=False) |
There was a problem hiding this comment.
Question: It appears we are recreating the config object here.
Should we just pass the config object in as a required parameter?
i.e: the config object is a field of the pipeline?
There was a problem hiding this comment.
Maybe? I think it could go either way. So I'm inclined to leave it as is.
There was a problem hiding this comment.
As you know, I am generally against burying or having duplicate definitions of a default.
commitment_policy and enable_legacy_unauthenticated_modes should be required arguments here, to ensure that they are always plumbed through.
kessplas
changed the title
texastony
left a comment
texastony
left a comment
There was a problem hiding this comment.
I really do not like duplicate definitions of a default.
That seems like an anti-pattern.
How opposed is @kessplas to making Algorithm Suite a required argument to encryption materials and the pipeline methods, and only an optional argument to the config?
| encryption_algorithm: AlgorithmSuite = field( | ||
| default=AlgorithmSuite.ALG_AES_256_GCM_HKDF_SHA512_COMMIT_KEY | ||
| ) | ||
| commitment_policy: CommitmentPolicy = field( | ||
| default=CommitmentPolicy.REQUIRE_ENCRYPT_REQUIRE_DECRYPT | ||
| ) |
There was a problem hiding this comment.
Are there no Duvet annotations for these defaults?
You would think there would be.
| commitment_policy: CommitmentPolicy = field( | ||
| default=CommitmentPolicy.REQUIRE_ENCRYPT_REQUIRE_DECRYPT | ||
| ) | ||
| enable_legacy_unauthenticated_modes: bool = field(default=False) |
There was a problem hiding this comment.
As you know, I am generally against burying or having duplicate definitions of a default.
commitment_policy and enable_legacy_unauthenticated_modes should be required arguments here, to ensure that they are always plumbed through.
2 participants
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.