CRYPTO-60: opensslCipher support GCM mode#70
Conversation
| this.padding = padding; | ||
|
|
||
| // context should be initialized | ||
| if (context != 0) { |
There was a problem hiding this comment.
According the previous code, if context is zero, we still pass it to OpenSslCommonMode? Is the behavior changed now?
| // context should be initialized | ||
| if (context != 0) { | ||
| if (algorithm == AlgorithmMode.AES_GCM.ordinal()) { | ||
| opensslBlockCipher = new OpenSslGaloisCounterMode(context, algorithm, padding); |
There was a problem hiding this comment.
Nit: OpenSslGaloisCounterMode: Is OpenSslGaloisCounterCipher better?
| if (algorithm == AlgorithmMode.AES_GCM.ordinal()) { | ||
| opensslBlockCipher = new OpenSslGaloisCounterMode(context, algorithm, padding); | ||
| } else { | ||
| opensslBlockCipher = new OpenSslCommonMode(context, algorithm, padding); |
There was a problem hiding this comment.
Nit: OpenSslCommonMode: is OpenSslCommonCipher better?
|
|
||
| /** Checks whether context is initialized. */ | ||
| private void checkState() { | ||
| Utils.checkState(context != 0); |
There was a problem hiding this comment.
Same question, why not use context for checking State? Is the behavior changed now?
There was a problem hiding this comment.
-
behavior is not changed. The context was moved to to the Cipher(OpenSslFeedbackCipher).
The cipher will check its own state. -
Well, I am considering to remove OpenSsl.java. From my point of view, now this layer(OpenSsl.java) seems redundant , we can move the logic(paramenters check) in OpenSsl.java to OpenSslCipher.
Then, the codebase will be clear:
- OpenSslCipher which implements the CryptoCipher interface which is exposed to the users.
- OpenSslFeedbackCipher and its sub classes (OpenSslGaloisCounterMode,OpenSslCommonMode) are private classes. they do the real work(encription/decription) for different modes.
- OpenSslNative: JNI class
Maybe we can do this later.
| inputLen, output, outputOffset, output.length - outputOffset); | ||
|
|
||
| len += OpenSslNative.doFinalByteArray(context, output, outputOffset + len, | ||
| output.length - outputOffset); |
There was a problem hiding this comment.
Should output.length - outputOffset be output.length - outputOffset - len ?
| #define DECRYPT_MODE 0 | ||
|
|
||
| /** Currently only support AES/CTR/NoPadding. */ | ||
| /** Currently only support AES/CTR/NoPadding, AES/CBC/NoPadding. AES/GCM/NoPadding */ |
There was a problem hiding this comment.
AES/CTR/NoPadding, AES/CBC/NoPadding, AES/CBC/PKCS5Padding, AES/GCM/NoPadding
| } | ||
|
|
||
| len += OpenSslNative.doFinalByteArray(context, output, outputOffset + len, | ||
| output.length - outputOffset); |
There was a problem hiding this comment.
should output.length - outputOffset be output.length - outputOffset - len
|
Hi, I am very interested in this PR - any reason why it was not merged? |
|
@yaronlev171, This PR implements GCM only in Cipher level. it needs to be tested more, but I was busy on other stuffs. I will be back on this PR soon, i will refine the code and do more testing. now, Stream level api does not support GCM. We need to do more work on stream API. |
|
@kexianda thanks for the reply. |
| class OpenSslGaloisCounterMode extends OpenSslFeedbackCipher { | ||
|
|
||
| // buffer for AAD data; if consumed, set as null | ||
| private ByteArrayOutputStream aadBuffer = new ByteArrayOutputStream(); |
There was a problem hiding this comment.
this should be set inside init() so the cipher with aad is reusable after clean(). [tested]
There was a problem hiding this comment.
thank you for this comment. I'll update the patch.
|
+1, thank @kexianda for the contribution! |
|
Thank @sundapeng. I will update the testcases with samples for GCM/GMAC soon. |
support AES-GCM cipher, also we has GMAC. code refactor.
|
BUILD SUCCESS on my local dev enviroment. but failed on travis-ci. |
|
Thank @kexianda, I will fix it. |
|
Is there a timeline for the release of this feature. The version is 3 years old. |
5 participants
CRYPTO-60
support GCM mode