You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/08/25 18:55:10 UTC
[GitHub] [accumulo] milleruntime opened a new pull request, #2895: Improvements to AESCryptoService
milleruntime opened a new pull request, #2895:
URL: https://github.com/apache/accumulo/pull/2895
* Reduce Cipher.getInstance calls to once per file vs once per block
* Move wrap cipher to ThreadLocal thread
* Add flush at the end of createCryptoParameters() before returning the byte array
* Add logger and init checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a diff in pull request #2895: Improvements to AESCryptoService
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2895:
URL: https://github.com/apache/accumulo/pull/2895#discussion_r955962163
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java:
##########
@@ -516,48 +559,45 @@ public static Key generateKey(SecureRandom random, int size) {
return new SecretKeySpec(bytes, "AES");
}
- @SuppressFBWarnings(value = "CIPHER_INTEGRITY",
- justification = "integrity not needed for key wrap")
- public static Key unwrapKey(byte[] fek, Key kek) {
- Key result = null;
+ public static synchronized Key unwrapKey(byte[] fek, Key kek) {
try {
- Cipher c = Cipher.getInstance(KEY_WRAP_TRANSFORM);
+ final Cipher c = KEY_UNWRAP_CIPHER.get();
c.init(Cipher.UNWRAP_MODE, kek);
- result = c.unwrap(fek, "AES", Cipher.SECRET_KEY);
- } catch (InvalidKeyException | NoSuchAlgorithmException | NoSuchPaddingException e) {
+ return c.unwrap(fek, "AES", Cipher.SECRET_KEY);
+ } catch (InvalidKeyException | NoSuchAlgorithmException e) {
throw new CryptoException("Unable to unwrap file encryption key", e);
}
- return result;
}
- @SuppressFBWarnings(value = "CIPHER_INTEGRITY",
- justification = "integrity not needed for key wrap")
- public static byte[] wrapKey(Key fek, Key kek) {
- byte[] result = null;
+ public static synchronized byte[] wrapKey(Key fek, Key kek) {
Review Comment:
Fixed in https://github.com/apache/accumulo/pull/2895/commits/5aba17923c94b184cfe6fb3b428339ad6ce6d59b
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java:
##########
@@ -516,48 +559,45 @@ public static Key generateKey(SecureRandom random, int size) {
return new SecretKeySpec(bytes, "AES");
}
- @SuppressFBWarnings(value = "CIPHER_INTEGRITY",
- justification = "integrity not needed for key wrap")
- public static Key unwrapKey(byte[] fek, Key kek) {
- Key result = null;
+ public static synchronized Key unwrapKey(byte[] fek, Key kek) {
Review Comment:
Fixed in https://github.com/apache/accumulo/pull/2895/commits/5aba17923c94b184cfe6fb3b428339ad6ce6d59b
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2895: Improvements to AESCryptoService
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2895:
URL: https://github.com/apache/accumulo/pull/2895#issuecomment-1227653632
> @dlmarion I noticed you ignored this error in the change you made against my branch:
>
> [ERROR] High: The cipher does not provide data integrity [org.apache.accumulo.core.spi.crypto.AESCryptoService] At AESCryptoService.java:[line 95] CIPHER_INTEGRITY
> Do you know why this is being flagged for the use in `ThreadLocal`?
I resolved that [here](https://github.com/milleruntime/accumulo/pull/19/commits/d798f29009fb96fc25a688481ddbf9ce5ba5152d)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime merged pull request #2895: Improvements to AESCryptoService
Posted by GitBox <gi...@apache.org>.
milleruntime merged PR #2895:
URL: https://github.com/apache/accumulo/pull/2895
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2895: Improvements to AESCryptoService
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2895:
URL: https://github.com/apache/accumulo/pull/2895#issuecomment-1228511968
Tested encryption using Uno and CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2895: Improvements to AESCryptoService
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2895:
URL: https://github.com/apache/accumulo/pull/2895#issuecomment-1227648938
@dlmarion I noticed you ignored this error in the change you made against my branch:
<pre>
[ERROR] High: The cipher does not provide data integrity [org.apache.accumulo.core.spi.crypto.AESCryptoService] At AESCryptoService.java:[line 95] CIPHER_INTEGRITY
</pre>
Do you know why this is being flagged for the use in `ThreadLocal`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2895: Improvements to AESCryptoService
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2895:
URL: https://github.com/apache/accumulo/pull/2895#discussion_r955320506
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java:
##########
@@ -516,48 +559,45 @@ public static Key generateKey(SecureRandom random, int size) {
return new SecretKeySpec(bytes, "AES");
}
- @SuppressFBWarnings(value = "CIPHER_INTEGRITY",
- justification = "integrity not needed for key wrap")
- public static Key unwrapKey(byte[] fek, Key kek) {
- Key result = null;
+ public static synchronized Key unwrapKey(byte[] fek, Key kek) {
try {
- Cipher c = Cipher.getInstance(KEY_WRAP_TRANSFORM);
+ final Cipher c = KEY_UNWRAP_CIPHER.get();
c.init(Cipher.UNWRAP_MODE, kek);
- result = c.unwrap(fek, "AES", Cipher.SECRET_KEY);
- } catch (InvalidKeyException | NoSuchAlgorithmException | NoSuchPaddingException e) {
+ return c.unwrap(fek, "AES", Cipher.SECRET_KEY);
+ } catch (InvalidKeyException | NoSuchAlgorithmException e) {
throw new CryptoException("Unable to unwrap file encryption key", e);
}
- return result;
}
- @SuppressFBWarnings(value = "CIPHER_INTEGRITY",
- justification = "integrity not needed for key wrap")
- public static byte[] wrapKey(Key fek, Key kek) {
- byte[] result = null;
+ public static synchronized byte[] wrapKey(Key fek, Key kek) {
Review Comment:
Same comment here.
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java:
##########
@@ -516,48 +559,45 @@ public static Key generateKey(SecureRandom random, int size) {
return new SecretKeySpec(bytes, "AES");
}
- @SuppressFBWarnings(value = "CIPHER_INTEGRITY",
- justification = "integrity not needed for key wrap")
- public static Key unwrapKey(byte[] fek, Key kek) {
- Key result = null;
+ public static synchronized Key unwrapKey(byte[] fek, Key kek) {
Review Comment:
I don't think this needs the `synchronized` modifier. This may have been included with an earlier change of mine and then not removed when I changed the code to use a ThreadLocal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org