You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2025/03/12 18:28:02 UTC
[PR] prevents creating overlapping encryption streams [accumulo]
keith-turner opened a new pull request, #5400:
URL: https://github.com/apache/accumulo/pull/5400
Modified AESCryptoService to detect creation of multiple encryptions streams at the same time. The implementation in AESCryptoService does not support this because it uses shared Cipher object and shrared intialization vectors. The code that uses CryptoService does not seem to have a need for creating more that one encryption stream at a time. If the usage pattern of the code change, then the detection added in this commit should catch that.
fixes #5386
--
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
Re: [PR] prevents creating overlapping encryption streams [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5400:
URL: https://github.com/apache/accumulo/pull/5400#discussion_r2009176259
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java:
##########
@@ -371,6 +373,10 @@ public OutputStream encryptStream(OutputStream outputStream) throws CryptoExcept
throw new CryptoException(
"Key/IV reuse is forbidden in AESGCMCryptoModule. Too many RBlocks.");
}
+ if (!openTracker.compareAndSet(false, true)) {
+ throw new CryptoException("Attempted to obtain new stream without closing previous one.");
+ }
Review Comment:
The encryption stream is created more than once when writing to rfiles (not sure about walogs, but I don't think that is the case w/ those). For rfiles each rfile block written creates a new encrypter and the impl will one up the initialization vector. I believe the AES impl of the SPI provided by Accumulo is trying to ensure that each encrypted rfile block is using a different initialization vector and I suspect this is important because the same per file key is used for all rfile blocks. Maybe if the same IV was used for different rfile block it would make cryptanalysis easier. The code goes to a lot of trouble to make sure the IVs are unique, in addition to one up it will also check if the IVs ever wrap back to the beginning.
The way Accumulo uses this SPI probably needs to be better captured in the SPI docs, seems really important for getting a correct impl.
--
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
Re: [PR] prevents creating overlapping encryption streams [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5400:
URL: https://github.com/apache/accumulo/pull/5400#discussion_r2009177265
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/FileEncrypter.java:
##########
@@ -21,13 +21,14 @@
import java.io.OutputStream;
/**
- * Class implementation that will encrypt a file. Make sure implementation is thread safe.
+ * Class implementation that will encrypt a file.
*
* @since 2.0
*/
public interface FileEncrypter {
/**
- * Encrypt the OutputStream.
+ * Encrypt the OutputStream. Only one OutputStream is expected to be active at a time. Before a
+ * new encryption OutputStream is created the previous one is expected to be closed.
Review Comment:
Based on another comment I think we need better overall documention on the SPI about how Accumulo will use the SPI for its two use cases RFiles and walogs.
--
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
Re: [PR] prevents creating overlapping encryption streams [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5400:
URL: https://github.com/apache/accumulo/pull/5400#discussion_r2009176259
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java:
##########
@@ -371,6 +373,10 @@ public OutputStream encryptStream(OutputStream outputStream) throws CryptoExcept
throw new CryptoException(
"Key/IV reuse is forbidden in AESGCMCryptoModule. Too many RBlocks.");
}
+ if (!openTracker.compareAndSet(false, true)) {
+ throw new CryptoException("Attempted to obtain new stream without closing previous one.");
+ }
Review Comment:
The encryption stream is created more than once when writing to rfiles (not sure about walogs, but I don't think that is the case w/ those). For rfiles each rfile block written creates a new encrypter and the impl will one up the initialization vector. I believe the AES impl of the SPI provided by Accumulo is trying to ensure that each encrypted rfile block is using a different initialization vector and I suspect this is important because the same per file key is used for all rfile blocks. Maybe if the same IV was used for different rfile block it would make cryptanalysis easier. The code goes to a lot of trouble to make sure the IVs are unique, in addition to one up it will also check if the IVs every wrap back to the beginning.
The way Accumulo uses this SPI probably needs to be better captured in the SPI docs, seems really important for getting a correct impl.
--
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
Re: [PR] prevents creating overlapping encryption streams [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5400:
URL: https://github.com/apache/accumulo/pull/5400#discussion_r1993363970
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java:
##########
@@ -351,6 +352,7 @@ public class AESGCMFileEncrypter implements FileEncrypter {
private final byte[] initVector = new byte[GCM_IV_LENGTH_IN_BYTES];
private final Cipher cipher;
private final byte[] decryptionParameters;
+ private final AtomicInteger openCounter = new AtomicInteger();
Review Comment:
Is there a reason not to use AtomicBoolean?
--
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
Re: [PR] prevents creating overlapping encryption streams [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5400:
URL: https://github.com/apache/accumulo/pull/5400#discussion_r2009179541
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/CryptoService.java:
##########
@@ -37,7 +37,8 @@ public interface CryptoService {
/**
* Initialize the FileEncrypter for the environment and return. This will get called once per
- * R-File or Write Ahead Log. FileEncrypter implementation must be thread safe.
+ * R-File or Write Ahead Log. FileEncrypter implementation is not expected be called by multiple
+ * threads.
Review Comment:
This is getting into the weeds of the AES impl, but its actually a bit more than multiple threads. Even a single thread using an output stream in an overlapping way would cause really bad things to happen. For example the following single thread use case would be bad, the changes in this PR were to defend against multiple thread and single thread having multiple output streams open at the same time.
```java
CryptoService cs = new AESCryptoService();
var fileEncrypter = cs.getFileEncrypter(env);
var encOut1 = fileEncrypter.encryptStream(out1);
var encOut2 = fileEncrypter.encryptStream(out2);
encOut1.write(data1);
// this will cause problems because encOut1 and encOut2 use shared instance vars from fileEncypter
encOut2.write(data2);
```
--
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
Re: [PR] prevents creating overlapping encryption streams [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #5400:
URL: https://github.com/apache/accumulo/pull/5400
--
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
Re: [PR] prevents creating overlapping encryption streams [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5400:
URL: https://github.com/apache/accumulo/pull/5400#discussion_r2009167926
##########
core/src/main/java/org/apache/accumulo/core/crypto/streams/BlockedOutputStream.java:
##########
@@ -113,5 +117,6 @@ public void write(byte[] b) throws IOException {
public void close() throws IOException {
flush();
out.close();
+ openTracker.compareAndSet(true, false);
Review Comment:
Yeah there is no point in this case. I did that for symmetry in the code because when it sets it to true it uses compare and set and actually cares if it succeeded.
--
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
Re: [PR] prevents creating overlapping encryption streams [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5400:
URL: https://github.com/apache/accumulo/pull/5400#discussion_r2006428717
##########
core/src/main/java/org/apache/accumulo/core/crypto/streams/BlockedOutputStream.java:
##########
@@ -113,5 +117,6 @@ public void write(byte[] b) throws IOException {
public void close() throws IOException {
flush();
out.close();
+ openTracker.compareAndSet(true, false);
Review Comment:
@keith-turner I don't see any point in compareAndSet here if you're not checking the returned value. You might as well just do `openTracker.set(false)`, right?
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/CryptoService.java:
##########
@@ -37,7 +37,8 @@ public interface CryptoService {
/**
* Initialize the FileEncrypter for the environment and return. This will get called once per
- * R-File or Write Ahead Log. FileEncrypter implementation must be thread safe.
+ * R-File or Write Ahead Log. FileEncrypter implementation is not expected be called by multiple
+ * threads.
Review Comment:
Since this is an SPI, I think it might be better to phrase this differently, to guide user implementations. Something like:
> once per R-File or Write Ahead Log for use in a single thread. Each call to this method should return a new FileEncrypter instance if contains any state that prevents use by multiple threads.
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java:
##########
@@ -371,6 +373,10 @@ public OutputStream encryptStream(OutputStream outputStream) throws CryptoExcept
throw new CryptoException(
"Key/IV reuse is forbidden in AESGCMCryptoModule. Too many RBlocks.");
}
+ if (!openTracker.compareAndSet(false, true)) {
+ throw new CryptoException("Attempted to obtain new stream without closing previous one.");
+ }
Review Comment:
While this prevents concurrent reuse, I think the design was that no reuse should be allowed. That's why getFileEncrypter() always returns a new instance.
Rather than bothering to track closed states, it'd be simpler to just track that the encryptStream method is never called more than once.
The same design intent is on the decrypters, but I suppose they don't have the risk of being stateful that the encrypters do... at least not the built-in ones (this is still an SPI, and a user could provide a different implementation that is stateful).
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/FileEncrypter.java:
##########
@@ -21,13 +21,14 @@
import java.io.OutputStream;
/**
- * Class implementation that will encrypt a file. Make sure implementation is thread safe.
+ * Class implementation that will encrypt a file.
*
* @since 2.0
*/
public interface FileEncrypter {
/**
- * Encrypt the OutputStream.
+ * Encrypt the OutputStream. Only one OutputStream is expected to be active at a time. Before a
+ * new encryption OutputStream is created the previous one is expected to be closed.
Review Comment:
I think this method is expected to be called exactly once. Having it closed first so it can be reused adds flexibility, I suppose, but I don't think that's expected. Implementers should really assume it's a one-time use.
--
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
Re: [PR] prevents creating overlapping encryption streams [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5400:
URL: https://github.com/apache/accumulo/pull/5400#discussion_r2009176259
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java:
##########
@@ -371,6 +373,10 @@ public OutputStream encryptStream(OutputStream outputStream) throws CryptoExcept
throw new CryptoException(
"Key/IV reuse is forbidden in AESGCMCryptoModule. Too many RBlocks.");
}
+ if (!openTracker.compareAndSet(false, true)) {
+ throw new CryptoException("Attempted to obtain new stream without closing previous one.");
+ }
Review Comment:
The encryption stream is created more than once when writing to rfiles (not sure about walogs, but I don't think that is the case w/ those). For rfiles each rfile block written creates a new encrypter and the impl will one up the initialization vector. I believe the AES impl of the SPI provided by Accumulo is trying to ensure that each encrypted rfile block is using a different initialization vector and I suspect this is important because the same per file key is used for all rfile blocks. Maybe if the same IV was used for different rfile block it would make cryptanalysis easier. The code goes to a lot of trouble to make the IVs are unique, in addition to one up it will also check if the IVs every wrap back to the beginning.
The way Accumulo uses this SPI probably needs to be better captured in the SPI docs, seems really important for getting a correct impl.
--
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
Re: [PR] prevents creating overlapping encryption streams [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5400:
URL: https://github.com/apache/accumulo/pull/5400#discussion_r1993904950
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java:
##########
@@ -351,6 +352,7 @@ public class AESGCMFileEncrypter implements FileEncrypter {
private final byte[] initVector = new byte[GCM_IV_LENGTH_IN_BYTES];
private final Cipher cipher;
private final byte[] decryptionParameters;
+ private final AtomicInteger openCounter = new AtomicInteger();
Review Comment:
That may be cleaner, can give that a try would just need to rename the variable to something besides counter.
--
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