chore(README): update README.md #1940
Conversation
|
|
||
| // Encrypt the data | ||
| // | ||
| final CryptoResult<byte[], KmsMasterKey> encryptResult = crypto.encryptData(prov, plaintext.getBytes(StandardCharsets.UTF_8), context); | ||
| final CryptoResult<byte[], ?> encryptResult = crypto.encryptData(kmsKeyring, plaintext.getBytes(StandardCharsets.UTF_8), encryptionContext); |
There was a problem hiding this comment.
why the ? in
CryptoResult<byte[], ?>
There was a problem hiding this comment.
The ? is a wild card to keep encryptData API compatible with previous releases.
ESDK-Java's CryptoResult includes a definition of the actual Cryptographic Material Provider (MasterKey or MasterKeyProvider) as a Generic that can be refined at class initialization.
I am not sure if we ever refactored that CryptoResult class so
IKeyring or ICryptographicMaterialProvider could refine the Generic...
There was a problem hiding this comment.
We clearly did not:
https://github.com/aws/aws-encryption-sdk-java/blob/master/src/main/java/com/amazonaws/encryptionsdk/CryptoResult.java#L31
This means that following APIs are now broken for Keyring users:
Which... is not Great.
@imabhichow we should bring this up at Stand up.
Those APIs are NOT critical; one of them is even marked as deprecated.
There was a problem hiding this comment.
Which... is not Great.
Looking into the details here, this is not bad or even an issue.
K is would be an explicit MasterKey.
If no MaterKey or MasterKeyProviders are used,
then no one SHOULD expect a list of MasterKeyIDs to be populated.
We should probably write a test to assert that nothing wild happens,
like an Exception.
There was a problem hiding this comment.
When using keyrings, the CryptoResult.getMasterKeyIds method returns an empty list during encryption.
However, during decryption, CryptoResult.getMasterKeyIds throws a NULL POINTER EXCEPTION. This could be due to returning a DataKey with the master key set to null. I suppose we should expect similar behavior to encryption which is returning an empty llist of masterkeys. I'll try to look more into this.
I also think we should deprecate this method CryptoResult.getMasterKeyIds
There was a problem hiding this comment.
We should have, yes.
And we should cut a PR to prevent the NPE.
There was a problem hiding this comment.
With this pull request, CryptoResult.getMasterKeyIds is now deprecated and will return an empty list when we use keyrings.
imabhichow
force-pushed
the
imabhichow/readme
branch
from
d4b44e3 to
b7f40a8
Compare
## [3.0.1](v3.0.0...v3.0.1) (2024-06-12) ### Fixes * Add Locale.ROOT to String uppercase conversions ([#1880](#1880)) ([9a9950e](9a9950e)), closes [#1879](#1879) * Update DecryptionMaterials code to support legacy custom CMMs ([#2037](#2037)) ([8807d79](8807d79)) ### Maintenance * deprecate getMasterKeyIds() in CryptoResult ([#1976](#1976)) ([1890ebb](1890ebb)) * **deps:** bump bcprov-jdk18on from 1.77 to 1.78.1 ([#2032](#2032)) ([713ca11](713ca11)) * **deps:** udpate org.bouncycastle to bcprov-jdk18on ([#1891](#1891)) ([32a92a9](32a92a9)) * **deps:** update dependencies ([#1973](#1973)) ([800bd01](800bd01)) * **Examples:** Customize KMS Client ([#2001](#2001)) ([e94ee85](e94ee85)) * fix release script ([#1912](#1912)) ([57e8a0b](57e8a0b)) * **README:** update README.md ([#1940](#1940)) ([7a0899e](7a0899e)) * update node version in version step ([#1959](#1959)) ([905385d](905385d)) * Update SUPPORT_POLICY.rst ([#1924](#1924)) ([57e40b5](57e40b5))
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: