Skip to content

Fix static zero IV in InternalSecurityAccessor #7374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aajisaka wants to merge 3 commits into from
+20 −13
Closed

Fix static zero IV in InternalSecurityAccessor #7374

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f5a840b
Fix static zero IV in InternalSecurityAccessor
aajisaka Mar 23, 2026
96351c8
Move random into object
aajisaka Apr 6, 2026
abfe628
Throw IllegalArgumentException for malformed tokens. Specify UTF-8 ch…
aajisaka Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions ...on/src/main/scala/org/apache/kyuubi/service/authentication/InternalSecurityAccessor.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.kyuubi.service.authentication

import java.security.SecureRandom
import javax.crypto.Cipher
import javax.crypto.spec.{IvParameterSpec, SecretKeySpec}

Expand All @@ -32,23 +33,17 @@ class InternalSecurityAccessor(conf: KyuubiConf, val isServer: Boolean) {
val cryptoKeyAlgorithm = conf.get(ENGINE_SECURITY_CRYPTO_KEY_ALGORITHM)
val cryptoCipher = conf.get(ENGINE_SECURITY_CRYPTO_CIPHER_TRANSFORMATION)

private val random = new SecureRandom()
Copy link
Copy Markdown
Member

@pan3793 pan3793 Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move it to object InternalSecurityAccessor?

Copy link
Copy Markdown
Member Author

@aajisaka aajisaka Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Moved it.

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)
Comment on lines +42 to +47
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@aajisaka aajisaka Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in abfe628

}

def issueToken(): String = {
Expand All @@ -69,11 +64,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)
random.nextBytes(nonce)
encryptor.init(Cipher.ENCRYPT_MODE, secretKeySpec, new IvParameterSpec(nonce))
byteArrayToHexString(nonce ++ encryptor.doFinal(value.getBytes))
}
Comment on lines 67 to 72
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@aajisaka aajisaka Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in abfe628


private[authentication] def decrypt(value: String): String = synchronized {
new String(decryptor.doFinal(hexStringToByteArray(value)))
val bytes = hexStringToByteArray(value)
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
val bytes = hexStringToByteArray(value)
val bytes = hexStringToByteArray(value)
if (bytes.length <= cryptoIvLength) {
throw new IllegalArgumentException(
"Malformed engine access token: ciphertext is shorter than the IV length")
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@aajisaka aajisaka Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in abfe628

val nonce = bytes.take(cryptoIvLength)
decryptor.init(Cipher.DECRYPT_MODE, secretKeySpec, new IvParameterSpec(nonce))
new String(decryptor.doFinal(bytes.drop(cryptoIvLength)))
}
Comment on lines 74 to 83
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@aajisaka aajisaka Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in abfe628


private def normalizeSecret(secret: String): String = {
Expand Down
Loading