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

Add support for CBC without padding #210

Merged
merged 3 commits into from
Dec 5, 2023
Merged

Add support for CBC without padding #210

merged 3 commits into from
Dec 5, 2023

Conversation

lovetodream
Copy link
Contributor

@lovetodream lovetodream commented Nov 16, 2023

This PR adds support for CBC de- and encryption without padding, as discussed in #209

Checklist

  • I've run tests to see all new and existing tests pass
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

If you've made changes to gyb files

  • I've run .script/generate_boilerplate_files_with_gyb and included updated generated files in a commit of this pull request

Motivation:

As described in #209, I personally need this to migrate an oracle driver from a third party crypto lib to swift-crypto. I think other users might benefit from this addition too.

Modifications:

I've added an overload to the encrypt and decrypt methods of AES._CBC, allowing the user to configure if padding should be added or not. With noPadding set to true, an error will be thrown if the plaintext isn't a multiple of the block size. I've added the corresponding inline documentation.

I've also added tests to ensure both encrypting and decrypting without padding work as expected. Although those tests might not be sufficient enough, because I couldn't find good resources online. I've created a bunch of random hex strings and encrypted/decrypted them using another implementation of paddingless CBC and checked if I receive the expected results. To further validate the feature, I've tested it as part of the authentication in oracle-nio, which worked in all test scenarios I've been running.

Result:

After merging this, it will be possible to use CBC without padding. This closes #209

@Lukasa Lukasa added the semver/minor Adds new public API. label Nov 17, 2023
@Lukasa
Copy link
Collaborator

Lukasa commented Nov 17, 2023

@swift-server-bot test this please

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Generally looks good, got one small note for you.

Sources/_CryptoExtras/AES/AES_CBC.swift Outdated Show resolved Hide resolved
@Lukasa
Copy link
Collaborator

Lukasa commented Nov 17, 2023

@swift-server-bot test this please

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

One final suggestion here: these tests encrypt and decrypt only inputs that are a single block in size, or that are not an even multiple. Can you add a few tests that use multi-block-size inputs?

@lovetodream
Copy link
Contributor Author

One final suggestion here: these tests encrypt and decrypt only inputs that are a single block in size, or that are not an even multiple. Can you add a few tests that use multi-block-size inputs?

Sure, I've added a few more with 2-16x block sizes

@lovetodream
Copy link
Contributor Author

Anything left for me to do @Lukasa?

@Lukasa
Copy link
Collaborator

Lukasa commented Dec 5, 2023

Hi @lovetodream, sorry about the delay, I've been swamped. I'll take a look now.

@Lukasa
Copy link
Collaborator

Lukasa commented Dec 5, 2023

@swift-server-bot test this please

@Lukasa
Copy link
Collaborator

Lukasa commented Dec 5, 2023

Nice change, thanks for your hard work!

@Lukasa Lukasa merged commit b793a1e into apple:main Dec 5, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to set CBC padding behavior?
2 participants