search

corretto / amazon-corretto-crypto-provider

The Amazon Corretto Crypto Provider is a collection of high-performance cryptographic implementations exposed via standard JCA/JCE interfaces.
Apache License 2.0
238 stars 56 forks source link

AES-CBC #380

Closed amirhosv closed 6 months ago

amirhosv commented 7 months ago

Description of changes:

Adding support for AES-CBC with no padding and with PKCS7 padding.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

geedo0 commented 7 months ago

I did a first pass, but still need to go through how the buffering works and how we handle the padding/non-padding cases.

I normally don't like to police commit messages and PR descriptions, but this PR could use some more context and explanation around what's going on. This helps with the review process and also helps future developers figure out what happened in the code. This is especially important for large diffs like this. Things that would be helpful to explain:

The README.md file needs to be updated to include the new algorithm.

Curious on your thoughts why we pulled in the NIST test vectors for this algorithm. ACCP doesn't consistently pull-in NIST vectors and tests for all algorithms. This PR accounts for half the rsp.gz files that we've imported. It's unclear to me what decision criteria is used for when we do vector testing in ACCP in addition to AWS-LC's extensive vector testing.

amirhosv commented 6 months ago

I did a first pass, but still need to go through how the buffering works and how we handle the padding/non-padding cases.

I normally don't like to police commit messages and PR descriptions, but this PR could use some more context and explanation around what's going on. This helps with the review process and also helps future developers figure out what happened in the code. This is especially important for large diffs like this. Things that would be helpful to explain:

  • Everything that's going on with the buffers.
  • Why PKCS5 aliases PKCS7.
  • Why we're adding this algorithm or a reference to a ticket.
  • What testing you're doing and why.
  • The refactoring you did around AES GCM and this new CBC class.

The README.md file needs to be updated to include the new algorithm.

Curious on your thoughts why we pulled in the NIST test vectors for this algorithm. ACCP doesn't consistently pull-in NIST vectors and tests for all algorithms. This PR accounts for half the rsp.gz files that we've imported. It's unclear to me what decision criteria is used for when we do vector testing in ACCP in addition to AWS-LC's extensive vector testing.

I think I've answered all your comments.

geedo0 commented 6 months ago

I think I've answered all your comments.

Yes, I see it all in the code. To clarify though, I was really hoping to see some of these design decisions documented in the PR itself. That helps me a lot when trying to reason out why certain code looks the way it does.