FrankChen021 commented on code in PR #19828:
URL: https://github.com/apache/druid/pull/19828#discussion_r3914092046


##########
processing/src/main/java/org/apache/druid/crypto/CryptoService.java:
##########
@@ -106,41 +122,52 @@ public CryptoService(
   public byte[] encrypt(byte[] plain)
   {
     try {
-      byte[] salt = new byte[saltSize];
+      final byte[] salt = new byte[saltSize];
       SECURE_RANDOM_INSTANCE.nextBytes(salt);
 
-      SecretKey tmp = getKeyFromPassword(passPhrase, salt);
-      SecretKey secret = new SecretKeySpec(tmp.getEncoded(), cipherAlgName);
+      final SecretKey tmp = getKeyFromPassword(passPhrase, salt);
+      final SecretKey secret = new SecretKeySpec(tmp.getEncoded(), 
AUTHENTICATED_CIPHER_ALGORITHM);
 
-      // error-prone warns if the transformation is not a compile-time constant
-      // since it cannot check it for insecure combinations.
-      @SuppressWarnings("InsecureCryptoUsage")
-      Cipher ecipher = Cipher.getInstance(transformation);
-      ecipher.init(Cipher.ENCRYPT_MODE, secret);
-      return new EncryptedData(
+      final byte[] iv = new byte[GCM_IV_SIZE];
+      SECURE_RANDOM_INSTANCE.nextBytes(iv);
+
+      final Cipher ecipher = 
Cipher.getInstance(AUTHENTICATED_CIPHER_TRANSFORMATION);
+      ecipher.init(Cipher.ENCRYPT_MODE, secret, new 
GCMParameterSpec(GCM_TAG_LENGTH_BITS, iv));
+      ecipher.updateAAD(AUTHENTICATED_FORMAT_HEADER);
+
+      final byte[] encryptedData = new EncryptedData(
           salt,
-          
ecipher.getParameters().getParameterSpec(IvParameterSpec.class).getIV(),
+          iv,
           ecipher.doFinal(plain)
       ).toByteAray();
+      return 
ByteBuffer.allocate(Math.addExact(AUTHENTICATED_FORMAT_HEADER.length, 
encryptedData.length))

Review Comment:
   [P1] New cookies break mixed-version rolling upgrades
   
   New nodes now emit the versioned AES-GCM envelope, but pre-upgrade nodes 
only parse the legacy length-prefixed format. When a user authenticates through 
a new node and is then routed to an old node, the old parser interprets the 
negative magic value as a salt length and fails, causing authentication-cookie 
reads to error until the user reauthenticates or reaches a new node. 
Legacy-read support only covers old cookies sent to new nodes. Use a staged 
format rollout, dual-cookie/format compatibility, or another deployment 
coordination mechanism, and add a test proving old-node readers handle cookies 
emitted during rollout.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to