Copilot commented on code in PR #7374:
URL: https://github.com/apache/kyuubi/pull/7374#discussion_r3037837102
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/InternalSecurityAccessor.scala:
##########
@@ -34,21 +35,14 @@ class InternalSecurityAccessor(conf: KyuubiConf, val
isServer: Boolean) {
private val tokenMaxLifeTime: Long =
conf.get(ENGINE_SECURITY_TOKEN_MAX_LIFETIME)
private val provider: EngineSecuritySecretProvider =
EngineSecuritySecretProvider.create(conf)
- private val (encryptor, decryptor) =
+ private val (secretKeySpec, encryptor, decryptor) =
initializeForAuth(cryptoCipher, normalizeSecret(provider.getSecret()))
- private def initializeForAuth(cipher: String, secret: String): (Cipher,
Cipher) = {
+ private def initializeForAuth(cipher: String, secret: String):
(SecretKeySpec, Cipher, Cipher) = {
val secretKeySpec = new SecretKeySpec(secret.getBytes, cryptoKeyAlgorithm)
- val nonce = new Array[Byte](cryptoIvLength)
- val iv = new IvParameterSpec(nonce)
-
val _encryptor = Cipher.getInstance(cipher)
- _encryptor.init(Cipher.ENCRYPT_MODE, secretKeySpec, iv)
-
val _decryptor = Cipher.getInstance(cipher)
- _decryptor.init(Cipher.DECRYPT_MODE, secretKeySpec, iv)
-
- (_encryptor, _decryptor)
+ (secretKeySpec, _encryptor, _decryptor)
Review Comment:
`SecretKeySpec` is built from `secret.getBytes` using the platform-default
charset. For deterministic key material across environments, use an explicit
charset (e.g., UTF-8) when converting the normalized secret string to bytes.
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/InternalSecurityAccessor.scala:
##########
@@ -69,11 +63,17 @@ class InternalSecurityAccessor(conf: KyuubiConf, val
isServer: Boolean) {
}
private[authentication] def encrypt(value: String): String = synchronized {
- byteArrayToHexString(encryptor.doFinal(value.getBytes))
+ val nonce = new Array[Byte](cryptoIvLength)
+ InternalSecurityAccessor.random.nextBytes(nonce)
+ encryptor.init(Cipher.ENCRYPT_MODE, secretKeySpec, new
IvParameterSpec(nonce))
+ byteArrayToHexString(nonce ++ encryptor.doFinal(value.getBytes))
}
private[authentication] def decrypt(value: String): String = synchronized {
- new String(decryptor.doFinal(hexStringToByteArray(value)))
+ val bytes = hexStringToByteArray(value)
Review Comment:
`decrypt` assumes the decoded byte array is at least `cryptoIvLength` bytes
long. If the ciphertext is shorter, `bytes.take(cryptoIvLength)` yields a short
IV and `Cipher.init(...)` will throw an `InvalidAlgorithmParameterException`.
Add an explicit length check (e.g., `bytes.length <= cryptoIvLength`) and throw
a regular exception with a clear message so callers consistently get the
expected error for malformed tokens.
```suggestion
val bytes = hexStringToByteArray(value)
if (bytes.length <= cryptoIvLength) {
throw new IllegalArgumentException(
"Malformed engine access token: ciphertext is shorter than the IV
length")
}
```
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/InternalSecurityAccessor.scala:
##########
@@ -69,11 +63,17 @@ class InternalSecurityAccessor(conf: KyuubiConf, val
isServer: Boolean) {
}
private[authentication] def encrypt(value: String): String = synchronized {
- byteArrayToHexString(encryptor.doFinal(value.getBytes))
+ val nonce = new Array[Byte](cryptoIvLength)
+ InternalSecurityAccessor.random.nextBytes(nonce)
+ encryptor.init(Cipher.ENCRYPT_MODE, secretKeySpec, new
IvParameterSpec(nonce))
+ byteArrayToHexString(nonce ++ encryptor.doFinal(value.getBytes))
}
Review Comment:
Encryption currently uses platform-default charset via `value.getBytes`. For
portability and consistent token generation across different default encodings,
specify an explicit charset (commonly UTF-8 in this codebase; e.g.,
`SignUtils.scala:52,68`).
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/InternalSecurityAccessor.scala:
##########
@@ -69,11 +63,17 @@ class InternalSecurityAccessor(conf: KyuubiConf, val
isServer: Boolean) {
}
private[authentication] def encrypt(value: String): String = synchronized {
- byteArrayToHexString(encryptor.doFinal(value.getBytes))
+ val nonce = new Array[Byte](cryptoIvLength)
+ InternalSecurityAccessor.random.nextBytes(nonce)
+ encryptor.init(Cipher.ENCRYPT_MODE, secretKeySpec, new
IvParameterSpec(nonce))
+ byteArrayToHexString(nonce ++ encryptor.doFinal(value.getBytes))
}
private[authentication] def decrypt(value: String): String = synchronized {
- new String(decryptor.doFinal(hexStringToByteArray(value)))
+ val bytes = hexStringToByteArray(value)
+ val nonce = bytes.take(cryptoIvLength)
+ decryptor.init(Cipher.DECRYPT_MODE, secretKeySpec, new
IvParameterSpec(nonce))
+ new String(decryptor.doFinal(bytes.drop(cryptoIvLength)))
}
Review Comment:
Decryption constructs a `String` with the platform-default charset. To avoid
inconsistent behavior across JVM default encodings, decode using an explicit
charset (commonly UTF-8 in this codebase; e.g., `SignUtils.scala:52,68`).
--
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]
