SNOW-1858529 Implement FLOE #2006
Conversation
sfc-gh-pfus
changed the title
sfc-gh-pfus
force-pushed
the
SNOW-1858529-floe
branch
2 times, most recently
from
dd4ff7d to
52d061f
Compare
sfc-gh-pfus
force-pushed
the
SNOW-1858529-floe
branch
from
52d061f to
68b11d2
Compare
There was a problem hiding this comment.
Just some quick early feedback so it's easy to incorporate if needed.
As a general note, I suspect this implementation will be less performant than the reference implementation due to extra byte copies and such. We'll want to measure and profile it to see how much we care.
| @@ -41,8 +41,7 @@ public class EncryptionProvider { | |||
| private static final String FILE_CIPHER = "AES/CBC/PKCS5Padding"; | |||
| private static final String KEY_CIPHER = "AES/ECB/PKCS5Padding"; | |||
| private static final int BUFFER_SIZE = 2 * 1024 * 1024; // 2 MB | |||
| private static ThreadLocal<SecureRandom> secRnd = | |||
| new ThreadLocal<>().withInitial(SecureRandom::new); | |||
| private static ThreadLocal<SecureRandom> secRnd = ThreadLocal.withInitial(SecureRandom::new); | |||
There was a problem hiding this comment.
I'd make this final. No reason not to. Also, as it is now static final it should probably also BE_IN_CAPS.
| import net.snowflake.client.jdbc.MatDesc; | ||
| import net.snowflake.common.core.RemoteStoreFileEncryptionMaterial; | ||
|
|
||
| class GcmEncryptionProvider { | ||
| @SnowflakeJdbcInternalApi | ||
| public class GcmEncryptionProvider { |
There was a problem hiding this comment.
Is this ever being shipped? I thought we designed FLOE because we determined GCM wouldn't work?
| @@ -99,7 +99,7 @@ private static CipherInputStream encryptContent( | |||
| NoSuchAlgorithmException { | |||
| SecretKey fileKey = new SecretKeySpec(keyBytes, 0, keyBytes.length, AES); | |||
There was a problem hiding this comment.
NitPick
| SecretKey fileKey = new SecretKeySpec(keyBytes, 0, keyBytes.length, AES); | |
| SecretKey fileKey = new SecretKeySpec(keyBytes, AES); |
| Cipher fileCipher = Cipher.getInstance(FILE_CIPHER); | ||
| Cipher fileCipher = Cipher.getInstance(JCE_CIPHER_NAME); |
There was a problem hiding this comment.
Why doesn't this method use decryptContentFromStream()?
Also, decrypting the file in place seems risky. It would be far safer to decrypt to a temporary file and then rename the temporary file over the input file only upon completion/success.
| private final byte[] aad; | ||
|
|
||
| FloeAad(byte[] aad) { | ||
| this.aad = Optional.ofNullable(aad).orElse(new byte[0]); |
There was a problem hiding this comment.
NitPick: It's often better to have a hidden constant EMPTY_ARRAY = new byte[0] because otherwise creating and managing these empty arrays is unneeded GC pressure.
| byte[] headerTag = | ||
| keyDerivator.hkdfExpand( | ||
| this.floeKey, floeIv, this.floeAad, HeaderTagFloePurpose.INSTANCE, headerTagLength); | ||
| if (!Arrays.equals(headerTag, headerTagFromHeader)) { |
There was a problem hiding this comment.
This is insecure. You must use a constant-time method for this comparison. This is documented in the primary FLOE document and pseudo-code:
assert ExpectedHeaderTag == HeaderTag // Must be constant time
Please use MessageDigest.isEqual() instead.
| verifySegmentSizeMarker(inputBuf); | ||
| AeadKey aeadKey = getKey(floeKey, floeIv, floeAad, segmentCounter); | ||
| AeadIv aeadIv = AeadIv.from(inputBuf, parameterSpec.getAead().getIvLength()); | ||
| AeadAad aeadAad = AeadAad.nonTerminal(segmentCounter++); |
There was a problem hiding this comment.
NitPick: The psuedo-code only increments the counter after successful decryption while yours increments it before. This does mean that the reference implementation and the current JDBC implementation will act differently after a failed decryption if decryption attempts continue.
The reference implementation acts as if the failed decryption were never attempted (because state doesn't change). Yours increments the counter and thus cannot return decryption of the same segment.
I note this as a nitpick because it isn't actually a problem, but it is a deviation from the provided algorithm description.
| AeadIv aeadIv = | ||
| AeadIv.generateRandom( | ||
| parameterSpec.getFloeRandom(), parameterSpec.getAead().getIvLength()); | ||
| AeadAad aeadAad = AeadAad.nonTerminal(segmentCounter++); |
There was a problem hiding this comment.
Same nitpick as above related to where the counter is incremented.
Overview
SNOW-XXXXX
Pre-review self checklist
masterbranchmvn -P check-style validate)mvn verifyand inspecttarget/japicmp/japicmp.html)SNOW-XXXX:External contributors - please answer these questions before submitting a pull request. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Issue: #NNNN
Fill out the following pre-review checklist:
@SnowflakeJdbcInternalApi(note that public/protected methods/fields in classes marked with this annotation are already internal)Please describe how your code solves the related issue.
Please write a short description of how your code change solves the related issue.