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

[Key Vault] Add local crypto support for AES-CBCPAD #17762

Merged
merged 6 commits into from
Apr 5, 2021

Conversation

mccoyp
Copy link
Member

@mccoyp mccoyp commented Apr 2, 2021

Resolves #17425, at least to achieve parity with Javascript.

This adds local crypto support for encryption and decryption with AES-CBCPAD algorithms. AES-CBC support is on hold until we find a way to perform zero-padding -- the closest thing in cryptography is ANSIX923 padding, but the final byte in that padding is nonzero.

This also fixes a bug that I had accidentally introduced in async tests by removing preparers: async test cases weren't being awaited due to the preparer's absence, and green test runs greatly exaggerated their success. This decorates affected tests with a dummy PowerShellPreparer that gets them noticed by the test runner without affecting execution otherwise.

@mccoyp mccoyp added KeyVault Client This issue points to a problem in the data-plane of the library. labels Apr 2, 2021
encryptor = algorithm.create_encryptor(key=self._key, iv=iv)
return encryptor.transform(plain_text, **kwargs)
raise_if_incorrect_key_size(algorithm, len(self._key))
encryptor = algorithm.create_encryptor(key=self._key, iv=kwargs.pop("iv", None))
Copy link
Member

Choose a reason for hiding this comment

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

Won't this just raise when iv is None?

Copy link
Member Author

Choose a reason for hiding this comment

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

An error will raise from a check in aes_cbc.py, yes. I didn't enforce that check here partially because there are some symmetric algorithms (AES-GCM) that don't accept an IV for encryption. This could be a case of planning for functionality we don't yet have, but I think it makes sense to validate that while validating the key and IV size. Is that what you were asking about?

Copy link
Member

Choose a reason for hiding this comment

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

I thought it was odd to use a default value that always gets an error. If AES-GCM is the only such algorithm supported by Key Vault, I'd forego the flexibility because we won't ever do AES-GCM locally.

supported_key_wrap_algorithms.append(AesKw192.name())
if key_size >= key_size_256:
supported_encryption_algorithms.append(Aes256Cbc.name())
supported_encryption_algorithms.append(Aes256CbcPad.name())
supported_encryption_algorithms.append(Aes128CbcHmacSha256.name())
Copy link
Member

Choose a reason for hiding this comment

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

This makes me nervous: we don't intend to support HMAC algorithms locally but may now do so in an undocumented way.

@mccoyp mccoyp requested a review from chlowell April 5, 2021 22:59
@mccoyp mccoyp merged commit 2264517 into Azure:master Apr 5, 2021
@mccoyp mccoyp deleted the local-crypto branch April 5, 2021 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Key Vault] Add local crypto capabilities for new algorithms
2 participants