OBO tokens: load the encryption key from a keystore and harden the crypto

[beanuwave]

Description

Category: Bug fix, Enhancement

The OBO key path was audited end to end. The encryption rewrite is the FIPS trigger; the rest are weaknesses found alongside it. A second commit keeps OBO tokens valid across a rolling upgrade despite the wire-format change the rewrite introduces.

Key changes

• Encryption rewrite. EncryptionDecryptionUtil built its cipher with a bare Cipher.getInstance("AES") and a key forced to 16 bytes via Arrays.copyOf (silently truncating or zero-padding). Now AES-256-GCM (random 12-byte IV, 128-bit tag) with the key derived via HKDF-SHA256. A bare "AES" transformation resolves to the provider's default mode - ECB - which for data confidentiality violates NIST SP 800-38A^[2] (ECB is approved only for key wrapping, SP 800-38F^[3]). The mode was never spelled out or configurable, and BC FIPS permits the ECB primitive at runtime, so this had to be caught by review rather than by the provider.
• Entropy floor (SP 800-133r2^[5]). HKDF cannot create entropy, so a short encryption_key would yield a nominal AES-256 key with sub-112-bit strength. A FIPS guard rejects input keying material < 32 bytes and zeroes the IKM after derivation.
• Signing-key length check. Validates the decoded key bytes; measuring the Base64 string would over-count by ~4/3 and let a 384-bit key pass a 512-bit gate.
• Lazy, fail-closed init. OnBehalfOfAuthenticator initialises lazily and atomically: the constructor is inert, the first token request triggers init, and a failure logs once and declines. A bad key can no longer throw out of the constructor, propagate through config reload and retry forever. Mirrors ApiTokenAuthenticator.
• Keystore support. Signing and encryption keys can be loaded from a keystore (KeyUtils.loadKeyFromKeystore -> PemKeyReader.loadSecretKeyFromKeystore) instead of inline config, keeping secrets out of cluster state. Relative *_keystore_path values resolve against the node config dir, consistent with other security file settings. This is SP 800-57^[6] key-at-rest hardening, not a FIPS 140-3^[1] requirement. PKCS#12 and BCFKS both hold SecretKey entries, JKS does not (engineSetKeyEntry requires a PrivateKey); in FIPS mode BCFKS is the only option (Enforce FIPS-approved cryptography across hashing, TLS, tokens and the CLI #6398).
• Secret redaction (CWE-532). OnBehalfOfSettings.toString() redacts signing_key / encryption_key so they no longer leak into logs.
• OBO tokens stay valid across a rolling upgrade. Moving the encrypted_roles claim from AES/ECB to AES-GCM changes its wire format; in a mixed cluster a token issued by one node and verified by another would be rejected with 401, in both directions, for as long as the upgrade takes plus one token lifetime. Now:
  • Read: decryption tries AES-GCM first and falls back to the legacy format on a tag mismatch. The JWT signature is already verified at that point, so the fallback is no forgery risk. The fallback stays open for one maximum token lifetime (10 min) after the last pre-upgrade node leaves, measured per node from the cluster-state change that removed it - not from the last legacy token read.
  • Write: while any node in the cluster lacks the node attribute security.internal.obo_roles_aes_gcm, encryption keeps writing the legacy format so that node can read what upgraded nodes issue. The attribute is set by the plugin itself, so upgraded nodes are recognised by capability rather than version, whatever release this lands in or is backported to; a configured value other than the plugin's own is refused at startup. The switch to AES-GCM happens by itself once the last such node leaves - no restart, no config change.
  • The legacy transformation is spelled out as AES/ECB/PKCS5Padding instead of relying on the provider default for "AES".
  • The transitional code lives in LegacyRolesClaimFormat and its tests, with small hooks in EncryptionDecryptionUtil, OnBehalfOfAuthenticator, SecurityTokenManager, ClusterInfoHolder and the plugin; it is to be removed with the next major version.

Reviewer call-outs

1. FIPS: one-directional 401 during a rolling upgrade. Under FIPS the legacy format is read but never written (SP 800-38A^[2]), and the first legacy token read is logged as a WARN. Compliance is chosen over compatibility: pre-upgrade nodes in a FIPS cluster answer 401 to tokens from upgraded nodes until they are upgraded too. This only affects clusters with OBO enabled whose nodes are started with OPENSEARCH_FIPS_MODE=true - the variable core's launcher uses to load fips_java.security and BC FIPS approved-only mode, and which the plugin has read since Validate password hashing algorithm for FIPS #6126 (PBKDF2 required) - and it fails loudly rather than as a silent loss of roles - one line in the release note. Walkthrough in Enforce FIPS-approved cryptography across hashing, TLS, tokens and the CLI #6398.

Testing

./gradlew test integrationTest

LegacyRolesClaimFormatTest / LegacyRolesClaimFormatWindowTest cover both formats, the write gate and the read window; the integration test LegacyRolesClaimFormatNodeAttributeTest checks that every node carries the attribute. The first two *FipsTests variants of the stack land here: LegacyRolesClaimFormatFipsTests and SecurityTokenManagerFipsTests assert that FIPS never writes the legacy format, even with a pre-upgrade node present.

Manual test: OnBehalfOf (OBO) token, non-FIPS

Exercises OBO token issuance and verification against an already-running non-FIPS cluster, in three modes: keys inline in the dynamic config (Scenario A), held in a PKCS#12 keystore out of cluster state (Scenario B), or in a BCFKS keystore (Scenario C - works on a non-FIPS node too, because the plugin still registers the BC FIPS provider at load on this branch; from #6398 on this depends on core registering the BCFIPS provider on non-FIPS nodes). No restart needed - -t config pushes only the dynamic on_behalf_of block, picked up live. Pick ONE scenario, edit config.yml, then run the apply / issue / use steps. All paths are relative to $OPENSEARCH_HOME.

cd $OPENSEARCH_HOME
export ADMIN_AUTH="admin:<admin-password>"

# === Apply / issue / use (the verify loop - run after editing config.yml) ====
# apply: push the dynamic config (-t config = on_behalf_of block only, live reload).
sh ./plugins/opensearch-security/tools/securityadmin.sh \
  -f ./config/opensearch-security/config.yml \
  -t config \
  -icl \
  -nhnv \
  -cacert config/root-ca.pem \
  -cert config/kirk.pem \
  -key config/kirk-key.pem \
  -h localhost \
  -p 9200

# issue: generate a token.
export OBO_TOKEN=$(curl -sk \
  -u "$ADMIN_AUTH" \
  -X POST \
  -H 'Content-Type: application/json' \
  https://localhost:9200/_plugins/_security/api/generateonbehalfoftoken \
  -d '{
        "description": "obo test",
        "service": "test-service",
        "durationSeconds": "300"
      }' | jq -r '.authenticationToken')
echo "$OBO_TOKEN"

# use: a populated user_name / roles proves the verify side loaded the key,
#      checked the signature, and decrypted the roles (AES-GCM round trip).
curl -sk \
  -H "Authorization: Bearer $OBO_TOKEN" \
  https://localhost:9200/_plugins/_security/authinfo?pretty

# === Scenario A - inline keys ================================================
# signing_key >= 512 bits (64 bytes) for HS512.
export OBO_SIGNING_KEY=$(openssl rand 64 | base64 -w0)
export OBO_ENCRYPTION_KEY=$(openssl rand 32 | base64 -w0)

# Put under config.dynamic.on_behalf_of in config/opensearch-security/config.yml:
#   on_behalf_of:
#     enabled: true
#     signing_key: "<value of $OBO_SIGNING_KEY>"
#     encryption_key: "<value of $OBO_ENCRYPTION_KEY>"

# === Scenario B - PKCS#12 keystore ===========================================
# PKCS#12 has no per-entry passwords, so use one value for -storepass and -keypass.
keytool \
  -genseckey \
  -alias obo-signing \
  -keyalg HmacSHA512 \
  -keysize 512 \
  -storetype PKCS12 \
  -keystore config/obo.p12 \
  -storepass kspass \
  -keypass kspass

keytool \
  -genseckey \
  -alias obo-enc \
  -keyalg AES \
  -keysize 256 \
  -storetype PKCS12 \
  -keystore config/obo.p12 \
  -storepass kspass \
  -keypass kspass

# Reference the keystore in config.yml (no inline keys). <key>_keystore_path is
# config-dir-relative; the loader falls back to the keystore password when
# _keystore_key_password is absent.
#   on_behalf_of:
#     enabled: true
#     signing_key_keystore_path: "obo.p12"
#     signing_key_keystore_type: "PKCS12"
#     signing_key_keystore_alias: "obo-signing"
#     signing_key_keystore_password: "kspass"
#     encryption_key_keystore_path: "obo.p12"
#     encryption_key_keystore_type: "PKCS12"
#     encryption_key_keystore_alias: "obo-enc"
#     encryption_key_keystore_password: "kspass"
#
# Same result as Scenario A, but no key material in the config index. GET
# /_plugins/_security/api/securityconfig shows only keystore references.

# === Scenario C - BCFKS keystore =============================================
keytool \
  -genseckey \
  -alias obo-signing \
  -keyalg HmacSHA512 \
  -keysize 512 \
  -storetype BCFKS \
  -providername BCFIPS \
  -keystore config/obo.bcfks \
  -storepass kspass \
  -keypass keypass \
  -providerClass org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider \
  -providerPath lib/bc-fips-2.1.2.jar

keytool \
  -genseckey \
  -alias obo-enc \
  -keyalg AES \
  -keysize 256 \
  -storetype BCFKS \
  -providername BCFIPS \
  -keystore config/obo.bcfks \
  -storepass kspass \
  -keypass keypass \
  -providerClass org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider \
  -providerPath lib/bc-fips-2.1.2.jar

# Reference it as in Scenario B, with per-entry key passwords:
#   on_behalf_of:
#     enabled: true
#     signing_key_keystore_path: "obo.bcfks"
#     signing_key_keystore_type: "BCFKS"
#     signing_key_keystore_alias: "obo-signing"
#     signing_key_keystore_password: "kspass"
#     signing_key_keystore_key_password: "keypass"
#     encryption_key_keystore_path: "obo.bcfks"
#     encryption_key_keystore_type: "BCFKS"
#     encryption_key_keystore_alias: "obo-enc"
#     encryption_key_keystore_password: "kspass"
#     encryption_key_keystore_key_password: "keypass"

Negative checks (each one, then restore a working config):

• Tampered token. Change a character in the token's last segment - the use step returns no credentials.
• Lazy, fail-closed init. Scenario B/C with a wrong alias (signing_key_keystore_alias: "nope"). apply still succeeds and the cluster stays healthy (_cluster/health); on the first OBO request the node logs On-behalf-of authentication is misconfigured; OBO tokens will be rejected: ... No key found at alias 'nope' in keystore once, and OBO requests are declined.
• Signing-key strength on decoded bytes. Scenario A with a 48-byte key (openssl rand 48 | base64 -w0 - 64 Base64 chars, 384 bits) is rejected with Signing key size was 384 bits, which is not secure enough.
• No FIPS-only restriction leaks into non-FIPS. Scenario A with a short encryption_key (e.g. ZW5jcnlwdGlvbktleQ==, 13 bytes) is accepted here; the 32-byte floor applies only in FIPS mode (tested in Enforce FIPS-approved cryptography across hashing, TLS, tokens and the CLI #6398).

Manual test: OnBehalfOf (OBO) tokens across a rolling upgrade (3.8 -> 3.9), non-FIPS

Upgrades a three-node cluster one node at a time from the previous release (3.8) to a build of this branch (3.9), and after every step issues a token on each node and verifies it on each node - nine combinations - because the point is which version issued a token and which one verifies it. A token is always sent to a named node, never through a load balancer.

Setup (three nodes on one host, obouser holds a role with security:obo/create):

$BASE/
|-- 3.7.0/node{1,2,3}/            opensearch-min 3.7.0 + security 3.7.0.0 (scenario M only)
|-- 3.8.0/node{1,2,3}/            opensearch-min 3.8.0 + security 3.8.0.0
|-- 3.9.0-SNAPSHOT/node{1,2,3}/   opensearch-min 3.9.0-SNAPSHOT + this branch's plugin
|-- conf/node{1,2,3}/             OPENSEARCH_PATH_CONF, shared by all versions
|-- state/node{1,2,3}/            data/ logs/ tmp/
`-- run/node{1,2,3}.pid

export BASE=~/obo-rolling
export ADMIN_AUTH="admin:<admin-password>"
export OBO_AUTH="obouser:<obouser-password>"
NODES=(https://127.0.0.1:9250 https://127.0.0.1:9251 https://127.0.0.1:9252)

# === Upgrade node n (the per-phase step) =====================================
# stop: kill it and wait until the process is really gone.
n=1
PID=$(cat $BASE/run/node$n.pid)
kill "$PID"
while kill -0 "$PID" 2>/dev/null; do sleep 1; done

# start: the same config and data, from the 3.9 installation (its own bundled JDK).
env -u JAVA_HOME \
  OPENSEARCH_JAVA_HOME=$BASE/3.9.0-SNAPSHOT/node$n/jdk \
  OPENSEARCH_PATH_CONF=$BASE/conf/node$n \
  OPENSEARCH_TMPDIR=$BASE/state/node$n/tmp \
  $BASE/3.9.0-SNAPSHOT/node$n/bin/opensearch \
    -d \
    -p $BASE/run/node$n.pid \
    >> $BASE/state/node$n/logs/startup.log 2>&1

# wait: until every node reports its version and the cluster is "green 3"
#       (ask a node that is not the one being restarted).
curl -sk -u "$ADMIN_AUTH" "${NODES[2]}/_cat/nodes?h=name,version&s=name"
curl -sk -u "$ADMIN_AUTH" "${NODES[2]}/_cat/health?h=status,node.total"

# === Verify (after every phase) ==============================================
# nine combinations: issue on src, use on dst. The body is captured in a variable
# rather than a file, so a failed request cannot report the previous answer.
for src in "${NODES[@]}"; do
  TOKEN=$(curl -sk \
    -u "$OBO_AUTH" \
    -X POST \
    -H 'Content-Type: application/json' \
    "$src/_plugins/_security/api/generateonbehalfoftoken" \
    -d '{
          "description": "rolling upgrade",
          "durationSeconds": "600"
        }' | jq -r '.authenticationToken')
  for dst in "${NODES[@]}"; do
    RESP=$(curl -sk \
      -w '\n%{http_code}' \
      -H "Authorization: Bearer $TOKEN" \
      "$dst/_plugins/_security/authinfo")
    printf '%s -> %s  %s %s\n' "$src" "$dst" "${RESP##*$'\n'}" \
      "$(jq -c '{user_name, roles}' <<< "${RESP%$'\n'*}" 2>/dev/null)"
  done
done

# format written by a node, without any key: AES/ECB is deterministic, AES-GCM never is.
# Two tokens for the same user -> 1 distinct claim = legacy, 2 = AES-GCM.
for i in 1 2; do
  curl -sk \
    -u "$OBO_AUTH" \
    -X POST \
    -H 'Content-Type: application/json' \
    "${NODES[0]}/_plugins/_security/api/generateonbehalfoftoken" \
    -d '{
          "description": "format check",
          "durationSeconds": "600"
        }' \
  | jq -r '.authenticationToken | split(".")[1] | gsub("-"; "+") | gsub("_"; "/") | @base64d | fromjson | .encrypted_roles'
done | sort -u | wc -l

# which nodes advertise the capability (only nodes running this change):
curl -sk -u "$ADMIN_AUTH" "${NODES[2]}/_cat/nodeattrs?h=node,attr,value" | grep obo_roles_aes_gcm

# tail token: before the last 3.8 node is upgraded, keep one of its tokens ...
TAIL_TOKEN=$(curl -sk \
  -u "$OBO_AUTH" \
  -X POST \
  -H 'Content-Type: application/json' \
  "${NODES[2]}/_plugins/_security/api/generateonbehalfoftoken" \
  -d '{
        "description": "tail token",
        "durationSeconds": "600"
      }' | jq -r '.authenticationToken'); date

# ... and after it, within 10 minutes, use it on every node.
for dst in "${NODES[@]}"; do
  RESP=$(curl -sk \
    -w '\n%{http_code}' \
    -H "Authorization: Bearer $TAIL_TOKEN" \
    "$dst/_plugins/_security/authinfo")
  printf '%s  %s %s\n' "$dst" "${RESP##*$'\n'}" \
    "$(jq -c '{user_name, roles}' <<< "${RESP%$'\n'*}" 2>/dev/null)"
done

Read the roles, not just the status code. A node that cannot decrypt the claim answers 401 - loud. The quiet failure is a claim that yields no roles (e.g. a node without encryption_key): 200 with roles: [], which only shows up later as 403. Every row below expects 200 with roles: ["obo-test-role"].

Phase	Step	Nodes (1, 2, 3)	Expected
0	-	3.8, 3.8, 3.8	9 x 200 + role; format: legacy; no attribute. Baseline - if this fails, the OBO config is wrong, not the upgrade
1	upgrade node 1	3.9, 3.8, 3.8	9 x 200 + role. 3.9 -> 3.8: node 1 still writes legacy (format check on node 1: legacy). 3.8 -> 3.9: node 1 reads legacy via the fallback and logs the INFO below once. Attribute: node 1
2	upgrade node 2	3.9, 3.9, 3.8	same - one 3.8 node is enough to keep the legacy writer on. Attribute: nodes 1, 2. Before phase 3, keep TAIL_TOKEN from node 3, the last 3.8 node
3	upgrade node 3	3.9, 3.9, 3.9	9 x 200 + role; format: AES-GCM, switched by itself - no restart, no config change. Attribute: all three. Tail window: $TAIL_TOKEN (issued by 3.8, valid 10 min) used on each node -> 200 + role on all three. This is the case a naive version check gets wrong

1   (node 1 log, once)  INFO  Decrypted an on-behalf-of token that still uses the legacy AES/ECB format. Such tokens
                              were issued by nodes running an earlier version and stop appearing once every node is
                              upgraded and the tokens issued before the upgrade have expired.
    grep -F 'legacy AES/ECB format' $BASE/state/node1/logs/obo-rolling.log

Scenario M - three versions in one cluster (3.7 -> 3.9 with a 3.8 node in between). A rolling upgrade may skip a minor, and one abandoned halfway and replaced by another leaves three versions in one cluster; both are valid. This shows the gates look at every node, not just the oldest. Start a fresh cluster (stop all nodes, move the three data directories aside, start all from a 3.7 installation built like the 3.8 one), create role and obouser again, then per step: upgrade, wait for green, nine combinations, format check on a 3.9 node, attribute listing.

Step	Upgrade	Nodes (1, 2, 3)	Expected
M.0	-	3.7, 3.7, 3.7	9 x 200 + role; legacy; no attribute
M.1	node 2 -> 3.8, then node 3 -> 3.9	3.7, 3.8, 3.9	9 x 200 + role; node 3 writes legacy; attribute: node 3
M.2	node 1 -> 3.9	3.9, 3.8, 3.9	same; attribute: nodes 1, 3. Keep TAIL_TOKEN from node 2, the last pre-upgrade node (issue against ${NODES[1]})
M.3	node 2 -> 3.9	3.9, 3.9, 3.9	9 x 200 + role; AES-GCM; $TAIL_TOKEN -> 200 + role on all three

M.4 - the reserved attribute. An upgraded node refuses the attribute in its own config (unit tests cover the check, this covers that the node really does not start):

# stop node 3 as above, then:
echo 'node.attr.security.internal.obo_roles_aes_gcm: false' >> $BASE/conf/node3/opensearch.yml
# start node 3 from 3.9 as above -> it exits; the reason is in the log:
grep -h 'must not be configured' $BASE/state/node3/logs/{startup,obo-rolling}.log | tail -1
# remove the line and start again -> joins normally.
sed -i '/^node.attr.security.internal.obo_roles_aes_gcm:/d' $BASE/conf/node3/opensearch.yml

M.4  [node.attr.security.internal.obo_roles_aes_gcm] is internal to the security plugin, which sets it to mark
     nodes that read AES-GCM on-behalf-of tokens; it must not be configured

The same line on a 3.7 or 3.8 node is accepted - the check lives in the new plugin.

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 715a21c.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java	465	medium	@JsonAnySetter/@JsonAnyGetter on OnBehalfOfSettings allows any unrecognised JSON key from the security config document to be stored in additionalSettings and serialised back out via configAsJson(). Those key-value pairs flow into downstream Settings consumers (JwtVendor, OnBehalfOfAuthenticator, EncryptionDecryptionUtil) without explicit allow-listing or type validation. An actor with write access to the security config index — or one who has already compromised it — could inject unexpected settings names that influence plugin behaviour in ways not intended by the API surface.
src/main/java/org/opensearch/security/authtoken/jwt/LegacyRolesClaimFormat.java	135	low	hasPreUpgradeNode() returns true for any DiscoveryNode that lacks the INTERNAL_AES_GCM_NODE_ATTRIBUTE, including non-security-plugin nodes or nodes whose attribute was deliberately suppressed. This permanently forces issuance to use the weaker AES/ECB format (writes() returns true) as long as such a node is present. An operator or attacker who can introduce a node without the plugin — or who can prevent the attribute from being advertised — can keep the cluster writing AES/ECB indefinitely without triggering an error.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 715a21c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: AES-GCM/HKDF encryption rewrite, keystore-backed key loading, and signing-key validation Relevant files: src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java src/main/java/org/opensearch/security/authtoken/jwt/claims/OBOJwtClaimsBuilder.java src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java src/main/java/org/opensearch/security/util/KeyUtils.java src/main/java/org/opensearch/security/support/PemKeyReader.java src/test/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtilsTest.java src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java src/test/java/org/opensearch/security/support/PemKeyReaderLoadKeyStoreTest.java src/test/java/org/opensearch/security/support/PemKeyReaderLoadSecretKeyTest.java src/test/java/org/opensearch/security/util/KeyUtilsLoadKeyFromKeystoreTest.java Sub-PR theme: Rolling-upgrade legacy AES/ECB compatibility layer and node-attribute gate Relevant files: src/main/java/org/opensearch/security/authtoken/jwt/LegacyRolesClaimFormat.java src/main/java/org/opensearch/security/configuration/ClusterInfoHolder.java src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java src/test/java/org/opensearch/security/authtoken/jwt/LegacyRolesClaimFormatTest.java src/test/java/org/opensearch/security/authtoken/jwt/LegacyRolesClaimFormatFipsTests.java src/test/java/org/opensearch/security/authtoken/jwt/LegacyRolesClaimFormatWindowTest.java src/integrationTest/java/org/opensearch/security/http/LegacyRolesClaimFormatNodeAttributeTest.java

⚡ Recommended focus areas for review Cluster-state race in issuance gate issuanceGate treats a null DiscoveryNodes as "upgraded" and writes AES-GCM. On a newly started node, cs.state().nodes() may briefly contain only the local (upgraded) node before it discovers the pre-upgrade peer, causing hasPreUpgradeNode to return false and issuance to write AES-GCM tokens that the pre-upgrade node cannot read. The comment says an unknown cluster counts as upgraded, but the far more common transient case is a state that only knows about the local node. Consider gating on cluster-state-recovered / minimum node count, or defaulting to "legacy" until at least one non-local peer has been observed. public static BooleanSupplier issuanceGate(final Supplier<DiscoveryNodes> nodes) { return () -> { final DiscoveryNodes current = nodes.get(); return current != null && hasPreUpgradeNode(current); }; } Ambiguous ciphertext decryption decrypt distinguishes AES-GCM from legacy AES/ECB by GCM tag failure. If a value that is actually an AES-GCM ciphertext (or arbitrary bytes) whose decoded length is a multiple of 16 happens to pass GCM tag verification failure and then legacy AES/ECB with PKCS5 padding by chance produces valid padding, decrypt will return arbitrary garbage as roles rather than throwing. False-positive probability is small but non-zero (~1/256 for padding alone), and the returned string then feeds into role assignment. Consider using a length/format marker to positively identify legacy values, or at least validating that the decrypted plaintext looks like a comma-separated role list before accepting it. public String decrypt(final String encryptedString) { byte[] decodedBytes = Base64.getDecoder().decode(encryptedString); try { byte[] nonce = Arrays.copyOfRange(decodedBytes, 0, GCM_NONCE_LENGTH); byte[] ciphertext = Arrays.copyOfRange(decodedBytes, GCM_NONCE_LENGTH, decodedBytes.length); Cipher cipher = Cipher.getInstance(AES_GCM_NO_PADDING); cipher.init(Cipher.DECRYPT_MODE, aesKey, new GCMParameterSpec(GCM_TAG_LENGTH, nonce)); return new String(cipher.doFinal(ciphertext), StandardCharsets.UTF_8); } catch (final AEADBadTagException e) { return legacyFormat.decrypt(decodedBytes, e); } catch (final Exception e) { throw new RuntimeException("Error processing data with cipher", e); } } One-shot init never retries ensureInitialized sets initialized = true before attempting init and only logs on failure, so a transient failure at startup (e.g. keystore file momentarily unreadable) permanently disables OBO authentication for the lifetime of the process — no token will ever succeed until the node is restarted. Consider only marking initialized on success, or resetting the flag on RuntimeException so a later request can retry. private synchronized boolean ensureInitialized() { if (!initialized) { initialized = true; try { jwtParser = AccessController.doPrivileged(this::buildJwtParser); encryptionUtil = EncryptionDecryptionUtil.fromSettings(settings, ENCRYPTION_KEY, configPath, legacyFormatWindow); } catch (final RuntimeException e) { log.error("On-behalf-of authentication is misconfigured; OBO tokens will be rejected: {}", e.toString(), e); } } return jwtParser != null; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 715a21c

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle short ciphertext before GCM parsing When decodedBytes.length < GCM_NONCE_LENGTH (e.g. a short legacy ciphertext), Arrays.copyOfRange will not throw but Cipher.init/doFinal will throw a non-AEADBadTagException (e.g. InvalidAlgorithmParameterException or IllegalBlockSizeException), which bypasses the legacy fallback and yields a generic error. Catch these too and route to legacy decryption so short legacy tokens remain readable during the upgrade window. src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [144-157] public String decrypt(final String encryptedString) { byte[] decodedBytes = Base64.getDecoder().decode(encryptedString); try { + if (decodedBytes.length <= GCM_NONCE_LENGTH) { + throw new AEADBadTagException("Ciphertext too short for AES-GCM"); + } byte[] nonce = Arrays.copyOfRange(decodedBytes, 0, GCM_NONCE_LENGTH); byte[] ciphertext = Arrays.copyOfRange(decodedBytes, GCM_NONCE_LENGTH, decodedBytes.length); Cipher cipher = Cipher.getInstance(AES_GCM_NO_PADDING); cipher.init(Cipher.DECRYPT_MODE, aesKey, new GCMParameterSpec(GCM_TAG_LENGTH, nonce)); return new String(cipher.doFinal(ciphertext), StandardCharsets.UTF_8); } catch (final AEADBadTagException e) { return legacyFormat.decrypt(decodedBytes, e); } catch (final Exception e) { throw new RuntimeException("Error processing data with cipher", e); } } Suggestion importance[1-10]: 7 __ Why: Valid concern: a short legacy ciphertext (< 12 bytes) would throw InvalidAlgorithmParameterException or similar before reaching the AEADBadTagException catch, bypassing the legacy fallback path. The fix ensures short legacy tokens remain readable during the upgrade window.	Medium
	Reset state fully on init failure If buildJwtParser succeeds but EncryptionDecryptionUtil.fromSettings throws (e.g. an invalid Base64 encryption_key or bad keystore), jwtParser remains set while encryptionUtil is null. Subsequent calls that rely on encryptionUtil (e.g. decrypting encrypted_roles) will then NPE instead of failing cleanly. Clear both fields on failure so the authenticator uniformly rejects tokens until reconfigured. src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java [94-105] private synchronized boolean ensureInitialized() { if (!initialized) { initialized = true; try { jwtParser = AccessController.doPrivileged(this::buildJwtParser); encryptionUtil = EncryptionDecryptionUtil.fromSettings(settings, ENCRYPTION_KEY, configPath, legacyFormatWindow); } catch (final RuntimeException e) { + jwtParser = null; + encryptionUtil = null; log.error("On-behalf-of authentication is misconfigured; OBO tokens will be rejected: {}", e.toString(), e); } } return jwtParser != null; } Suggestion importance[1-10]: 6 __ Why: Reasonable defensive improvement: if fromSettings throws after jwtParser is set, encryptionUtil remains null while ensureInitialized returns true, potentially causing NPE later during decryption. Clearing both fields ensures uniform rejection.	Low
Security	Reject empty secret in legacy key Arrays.copyOf with a length larger than the source pads with zeros — matching the legacy behavior for a short key. However, if secretBytes is empty or null this constructor either produces an all-zero key or throws NPE. Since the caller in EncryptionDecryptionUtil invokes this before deriveKey (which validates length only in FIPS mode), a very short/empty secret in non-FIPS mode silently yields a predictable all-zero AES key for the legacy path. Consider enforcing a minimum non-empty secret length here as well. src/main/java/org/opensearch/security/authtoken/jwt/LegacyRolesClaimFormat.java [88-91] LegacyRolesClaimFormat(final byte[] secretBytes, final BooleanSupplier inUse) { + if (secretBytes == null || secretBytes.length == 0) { + throw new IllegalArgumentException("encryption_key must not be empty"); + } this.key = new SecretKeySpec(Arrays.copyOf(secretBytes, LEGACY_KEY_LENGTH_BYTES), "AES"); this.inUse = inUse; } Suggestion importance[1-10]: 4 __ Why: The concern about empty/null secrets producing predictable all-zero keys is valid but edge-case, as callers generally validate the key material earlier via Base64 decoding. Minor defensive improvement.	Low
General	Fail clearly when config path missing If configPath is null (e.g. the test-only SecurityTokenManager constructor passes null) and a relative keystore path is configured, this throws NPE with no context. Fail with a clearer error, or require configPath non-null when a keystore alias is set. src/main/java/org/opensearch/security/util/KeyUtils.java [76-81] private static String resolveKeystorePath(final String pathStr, final Path configPath) { if (pathStr == null || pathStr.isEmpty()) { return pathStr; } + if (configPath == null) { + throw new IllegalStateException("Cannot resolve keystore path '" + pathStr + "': node config path is not available"); + } return configPath.resolve(pathStr).toAbsolutePath().toString(); } Suggestion importance[1-10]: 4 __ Why: Minor usability improvement: replacing an NPE with a clearer error message helps debugging when configPath is null and a relative keystore path is used, but the scenario is uncommon in production.	Low

---

Previous suggestions

Suggestions up to commit 4e3396c

Category	Suggestion	Impact
Possible issue	Handle short ciphertexts before AES-GCM decrypt If decodedBytes is shorter than GCM_NONCE_LENGTH (12 bytes), Cipher.doFinal will throw a generic exception rather than AEADBadTagException, so a truncated/short legacy value gets reported as "Error processing data with cipher" instead of falling back to legacy decryption. Guard for the minimum length and route short inputs (which cannot be AES-GCM) to the legacy path or a clear error. src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [144-157] public String decrypt(final String encryptedString) { byte[] decodedBytes = Base64.getDecoder().decode(encryptedString); try { + if (decodedBytes.length < GCM_NONCE_LENGTH) { + throw new AEADBadTagException("ciphertext too short for AES-GCM"); + } byte[] nonce = Arrays.copyOfRange(decodedBytes, 0, GCM_NONCE_LENGTH); byte[] ciphertext = Arrays.copyOfRange(decodedBytes, GCM_NONCE_LENGTH, decodedBytes.length); Cipher cipher = Cipher.getInstance(AES_GCM_NO_PADDING); cipher.init(Cipher.DECRYPT_MODE, aesKey, new GCMParameterSpec(GCM_TAG_LENGTH, nonce)); return new String(cipher.doFinal(ciphertext), StandardCharsets.UTF_8); } catch (final AEADBadTagException e) { return legacyFormat.decrypt(decodedBytes, e); } catch (final Exception e) { throw new RuntimeException("Error processing data with cipher", e); } } Suggestion importance[1-10]: 5 __ Why: Valid observation: a ciphertext shorter than the 12-byte nonce would throw a generic exception rather than falling through to the legacy path. The fix improves robustness of the format-detection trial decryption, though the impact is minor since malformed tokens are rejected either way.	Low
General	Wrap keystore access in doPrivileged encryptionUtil is assigned via AccessController.doPrivileged was removed for the encryption util, but EncryptionDecryptionUtil.fromSettings reads a keystore file, which typically requires file-read permissions. Wrap the whole initialization in doPrivileged so keystore access does not fail under a security manager. src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java [94-105] private synchronized boolean ensureInitialized() { if (!initialized) { initialized = true; try { - jwtParser = AccessController.doPrivileged(this::buildJwtParser); - encryptionUtil = EncryptionDecryptionUtil.fromSettings(settings, ENCRYPTION_KEY, configPath, legacyFormatWindow); + AccessController.doPrivileged(() -> { + jwtParser = buildJwtParser(); + encryptionUtil = EncryptionDecryptionUtil.fromSettings(settings, ENCRYPTION_KEY, configPath, legacyFormatWindow); + return null; + }); } catch (final RuntimeException e) { log.error("On-behalf-of authentication is misconfigured; OBO tokens will be rejected: {}", e.toString(), e); } } return jwtParser != null; } Suggestion importance[1-10]: 4 __ Why: Reasonable defensive measure since EncryptionDecryptionUtil.fromSettings reads a keystore file that may need privileged access, but it's uncertain whether the current SecurityManager context requires this given the plugin's existing permission model.	Low
	Handle null configPath when resolving keystore configPath.resolve(pathStr) will throw NullPointerException when configPath is null (e.g., some test paths). Also, when pathStr is an absolute path, Path.resolve already returns it unchanged, but a null configPath still NPEs before that logic runs. Guard against a null configPath to keep absolute paths working without a config directory. src/main/java/org/opensearch/security/util/KeyUtils.java [76-81] private static String resolveKeystorePath(final String pathStr, final Path configPath) { if (pathStr == null || pathStr.isEmpty()) { return pathStr; } + if (configPath == null) { + return Paths.get(pathStr).toAbsolutePath().toString(); + } return configPath.resolve(pathStr).toAbsolutePath().toString(); } Suggestion importance[1-10]: 3 __ Why: A minor robustness improvement for edge cases where configPath may be null. In production the config path is always set, so this is mostly relevant for tests or unusual configurations.	Low
	Warn on short key material for legacy Arrays.copyOf with a target length larger than the source pads with zero bytes. If secretBytes is shorter than 16 bytes, this silently constructs a weak AES key (zero-padded), matching the pre-upgrade behavior — but the new EncryptionDecryptionUtil requires ≥32 bytes only in FIPS mode. In non-FIPS mode a very short key would be accepted here, potentially producing an insecure legacy key. Consider validating a minimum input length or documenting this behavior explicitly. src/main/java/org/opensearch/security/authtoken/jwt/LegacyRolesClaimFormat.java [88-91] LegacyRolesClaimFormat(final byte[] secretBytes, final BooleanSupplier inUse) { + if (secretBytes.length < LEGACY_KEY_LENGTH_BYTES) { + LOG.warn("Encryption key material is shorter than {} bytes; legacy AES/ECB key will be zero-padded (matches pre-upgrade behavior).", + LEGACY_KEY_LENGTH_BYTES); + } this.key = new SecretKeySpec(Arrays.copyOf(secretBytes, LEGACY_KEY_LENGTH_BYTES), "AES"); this.inUse = inUse; } Suggestion importance[1-10]: 2 __ Why: The zero-padding behavior is intentional to match pre-upgrade behavior, as documented in the class Javadoc. Adding a warning is optional and low-impact since this only affects the transitional legacy read path.	Low

Suggestions up to commit f6b50ef

Category	Suggestion	Impact
Possible issue	Handle short ciphertexts before AES-GCM parsing When the decoded ciphertext is shorter than GCM_NONCE_LENGTH (12 bytes), Arrays.copyOfRange will throw IllegalArgumentException (from an invalid range) which is not an AEADBadTagException, bypassing the intended legacy-format fallback and instead surfacing as a generic error. Guard the length up front and route short inputs to the legacy path (or a clean error), so that a malformed or legacy-formatted input never triggers the wrong branch. src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [144-157] public String decrypt(final String encryptedString) { byte[] decodedBytes = Base64.getDecoder().decode(encryptedString); + if (decodedBytes.length < GCM_NONCE_LENGTH) { + return legacyFormat.decrypt(decodedBytes, new AEADBadTagException("ciphertext too short for AES-GCM")); + } try { byte[] nonce = Arrays.copyOfRange(decodedBytes, 0, GCM_NONCE_LENGTH); byte[] ciphertext = Arrays.copyOfRange(decodedBytes, GCM_NONCE_LENGTH, decodedBytes.length); Cipher cipher = Cipher.getInstance(AES_GCM_NO_PADDING); cipher.init(Cipher.DECRYPT_MODE, aesKey, new GCMParameterSpec(GCM_TAG_LENGTH, nonce)); return new String(cipher.doFinal(ciphertext), StandardCharsets.UTF_8); } catch (final AEADBadTagException e) { return legacyFormat.decrypt(decodedBytes, e); } catch (final Exception e) { throw new RuntimeException("Error processing data with cipher", e); } } Suggestion importance[1-10]: 7 __ Why: Valid concern: if the decoded bytes are shorter than GCM_NONCE_LENGTH, Arrays.copyOfRange behavior and the subsequent GCM init may throw a non-AEADBadTagException, bypassing the legacy fallback. Routing short inputs to the legacy path improves robustness during the upgrade window.	Medium
General	Fix read order to match documented write ordering The comment above declares that preUpgradeNodeLastSeenMs must be written before nodes, so that a reader that observes a new nodes also observes its sighting timestamp. The current order writes nodes last, which is correct for that invariant — but the read side in legacyFormatReadable() reads nodes first and then preUpgradeNodeLastSeenMs, so a reader can see a fresh nodes value with a stale timestamp and close the window one tick early. Read the timestamp first, then nodes, to mirror the write order. src/main/java/org/opensearch/security/authtoken/jwt/LegacyRolesClaimFormat.java [262-272] -@Override -public void clusterChanged(final ClusterChangedEvent event) { - final DiscoveryNodes current = event.state().nodes(); - if (hasPreUpgradeNode(event.previousState().nodes()) || hasPreUpgradeNode(current)) { - preUpgradeNodeLastSeenMs = clock.getAsLong(); +public boolean legacyFormatReadable() { + final long lastSeen = preUpgradeNodeLastSeenMs; + final DiscoveryNodes current = nodes; + if (current == null || hasPreUpgradeNode(current)) { + return true; } - nodes = current; + return clock.getAsLong() - lastSeen < TimeUnit.SECONDS.toMillis( + CreateOnBehalfOfTokenAction.OBO_MAX_EXPIRY_SECONDS + ); } Suggestion importance[1-10]: 6 __ Why: The write ordering comment says timestamp is written before nodes, but the read side reads nodes first and then the timestamp. Reversing the read order mirrors the documented invariant and avoids closing the window one tick early in a rare race.	Low
	Set initialized flag after field assignments jwtParser and encryptionUtil are read outside the synchronized block (in extractCredentials0), but neither is declared volatile here — they are written inside synchronized but reads are not synchronized, so under the JMM another thread may observe stale/null values even after initialized has been flipped. Since initialized, jwtParser, and encryptionUtil are all declared volatile in the field section, this is fine, but ensure the write ordering places initialized = true last (after jwtParser/encryptionUtil are assigned), otherwise a concurrent caller that sees initialized == true on a fast path could still see a null parser. src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java [94-105] private synchronized boolean ensureInitialized() { if (!initialized) { - initialized = true; try { jwtParser = AccessController.doPrivileged(this::buildJwtParser); encryptionUtil = EncryptionDecryptionUtil.fromSettings(settings, ENCRYPTION_KEY, configPath, legacyFormatWindow); } catch (final RuntimeException e) { log.error("On-behalf-of authentication is misconfigured; OBO tokens will be rejected: {}", e.toString(), e); + } finally { + initialized = true; } } return jwtParser != null; } Suggestion importance[1-10]: 5 __ Why: Setting initialized = true before assigning jwtParser/encryptionUtil could let a concurrent reader observe initialized == true while the fields are still null. Moving the flag write to after the assignments (or into a finally) improves the visibility guarantee, although fields are volatile.	Low
	Handle null config path and absolute keystore paths configPath may legitimately be null in some code paths (tests pass it through, and callers such as EncryptionDecryptionUtil.fromSettings accept it without null-checks); if a keystore alias is set but configPath is null, this will throw an opaque NullPointerException. Fail explicitly with a clear message, or fall back to treating an absolute path as-is when configPath is null. src/main/java/org/opensearch/security/util/KeyUtils.java [76-81] private static String resolveKeystorePath(final String pathStr, final Path configPath) { if (pathStr == null || pathStr.isEmpty()) { return pathStr; } - return configPath.resolve(pathStr).toAbsolutePath().toString(); + final Path p = Path.of(pathStr); + if (p.isAbsolute() || configPath == null) { + return p.toAbsolutePath().toString(); + } + return configPath.resolve(p).toAbsolutePath().toString(); } Suggestion importance[1-10]: 5 __ Why: Reasonable defensive improvement: a null configPath or an absolute path would otherwise cause an opaque NPE or ignore the absolute path. Handling both cases yields clearer errors and correct behavior for absolute paths.	Low

Suggestions up to commit 9f8fb30

Category	Suggestion	Impact
Possible issue	Route short ciphertexts to legacy decryption path When decodedBytes.length < GCM_NONCE_LENGTH (e.g., a short legacy ciphertext or corrupted input), Arrays.copyOfRange succeeds but cipher.doFinal(ciphertext) will throw a generic exception (not AEADBadTagException), so the legacy fallback is skipped and legitimate short legacy tokens fail. Guard the length and route short inputs directly to the legacy path (or catch broader GeneralSecurityException to trigger the fallback). src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [144-157] public String decrypt(final String encryptedString) { byte[] decodedBytes = Base64.getDecoder().decode(encryptedString); try { + if (decodedBytes.length < GCM_NONCE_LENGTH + GCM_TAG_LENGTH / Byte.SIZE) { + return legacyFormat.decrypt(decodedBytes, new AEADBadTagException("ciphertext too short for AES-GCM")); + } byte[] nonce = Arrays.copyOfRange(decodedBytes, 0, GCM_NONCE_LENGTH); byte[] ciphertext = Arrays.copyOfRange(decodedBytes, GCM_NONCE_LENGTH, decodedBytes.length); Cipher cipher = Cipher.getInstance(AES_GCM_NO_PADDING); cipher.init(Cipher.DECRYPT_MODE, aesKey, new GCMParameterSpec(GCM_TAG_LENGTH, nonce)); return new String(cipher.doFinal(ciphertext), StandardCharsets.UTF_8); } catch (final AEADBadTagException e) { return legacyFormat.decrypt(decodedBytes, e); } catch (final Exception e) { throw new RuntimeException("Error processing data with cipher", e); } } Suggestion importance[1-10]: 6 __ Why: Valid concern: a legacy ciphertext shorter than GCM_NONCE_LENGTH + tag bytes would throw a generic exception rather than AEADBadTagException, bypassing the legacy fallback. The fix improves robustness of the format detection.	Low
	Fix initialization ordering and visibility initialized is set to true before the potentially-failing calls, so if buildJwtParser throws, encryptionUtil is never initialized and no subsequent attempt is made — even after the operator fixes the configuration and reloads dynamic config. Also jwtParser/encryptionUtil are read outside synchronized without volatile, so other threads may see a stale null. Mark them volatile and only set initialized = true after both fields are assigned (or on a terminal error), and consider allowing retry on transient failure. src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java [94-105] +private volatile JwtParser jwtParser; +private volatile EncryptionDecryptionUtil encryptionUtil; + private synchronized boolean ensureInitialized() { if (!initialized) { - initialized = true; try { jwtParser = AccessController.doPrivileged(this::buildJwtParser); encryptionUtil = EncryptionDecryptionUtil.fromSettings(settings, ENCRYPTION_KEY, configPath, legacyFormatWindow); + initialized = true; } catch (final RuntimeException e) { + initialized = true; log.error("On-behalf-of authentication is misconfigured; OBO tokens will be rejected: {}", e.toString(), e); } } return jwtParser != null; } Suggestion importance[1-10]: 5 __ Why: The fields are already declared volatile in the diff, so the visibility part is moot. The ordering concern (setting initialized=true before assignment) is minor since a failure still leaves jwtParser null and the code checks for that.	Low
General	Guard against null configPath during resolution configPath may be null in some code paths (e.g., the SecurityTokenManager test constructor passes null, and other callers may too if the plugin has not fully initialized), which would cause a NullPointerException on configPath.resolve(...) when a relative keystore path is configured. Guard against a null configPath and fall back to treating the path as absolute, or fail with a clear error. src/main/java/org/opensearch/security/util/KeyUtils.java [76-81] private static String resolveKeystorePath(final String pathStr, final Path configPath) { if (pathStr == null || pathStr.isEmpty()) { return pathStr; } + if (configPath == null) { + return Path.of(pathStr).toAbsolutePath().toString(); + } return configPath.resolve(pathStr).toAbsolutePath().toString(); } Suggestion importance[1-10]: 5 __ Why: Valid defensive check: configPath can be null (e.g., in test constructors), and a NullPointerException would occur if a relative keystore path is configured. The fallback improves robustness.	Low
Security	Reject empty key material for legacy format Arrays.copyOf(secretBytes, 16) zero-pads when secretBytes.length < 16, but if secretBytes is empty or null this silently produces an all-zero AES key, which would accept any legacy ciphertext encrypted with the zero key. Validate that secretBytes has at least some minimum length (or is non-empty) before constructing the legacy key, to avoid inadvertently trusting tokens forged against a trivially-derived key. src/main/java/org/opensearch/security/authtoken/jwt/LegacyRolesClaimFormat.java [88-91] LegacyRolesClaimFormat(final byte[] secretBytes, final BooleanSupplier inUse) { + if (secretBytes == null || secretBytes.length == 0) { + throw new IllegalArgumentException("encryption_key is empty; cannot derive legacy AES/ECB key"); + } this.key = new SecretKeySpec(Arrays.copyOf(secretBytes, LEGACY_KEY_LENGTH_BYTES), "AES"); this.inUse = inUse; } Suggestion importance[1-10]: 4 __ Why: The concern about an empty/null key producing a zero-key is theoretically valid, but callers upstream (EncryptionDecryptionUtil.fromSettings, FIPS check requiring 32 bytes) already ensure non-empty key material, so this is largely defensive.	Low

Suggestions up to commit 094236e

Category	Suggestion	Impact
Possible issue	Guard AES-GCM decrypt against short inputs If the input is shorter than GCM_NONCE_LENGTH (12 bytes), Arrays.copyOfRange on the ciphertext range will succeed but cipher.doFinal may throw an IllegalArgumentException/ArrayIndexOutOfBoundsException (or a non-AEAD exception) rather than an AEADBadTagException, so the legacy fallback path is never tried for short legacy payloads. Guard the length before slicing and route short/invalid GCM inputs through the legacy decrypt path (or fail cleanly), to ensure legacy AES/ECB ciphertexts of any block-multiple length are still readable. src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [144-157] public String decrypt(final String encryptedString) { byte[] decodedBytes = Base64.getDecoder().decode(encryptedString); + if (decodedBytes.length <= GCM_NONCE_LENGTH) { + // Too short to be an AES-GCM payload; treat as legacy AES/ECB attempt. + return legacyFormat.decrypt(decodedBytes, new AEADBadTagException("input too short for AES-GCM")); + } try { byte[] nonce = Arrays.copyOfRange(decodedBytes, 0, GCM_NONCE_LENGTH); byte[] ciphertext = Arrays.copyOfRange(decodedBytes, GCM_NONCE_LENGTH, decodedBytes.length); Cipher cipher = Cipher.getInstance(AES_GCM_NO_PADDING); cipher.init(Cipher.DECRYPT_MODE, aesKey, new GCMParameterSpec(GCM_TAG_LENGTH, nonce)); return new String(cipher.doFinal(ciphertext), StandardCharsets.UTF_8); } catch (final AEADBadTagException e) { return legacyFormat.decrypt(decodedBytes, e); } catch (final Exception e) { throw new RuntimeException("Error processing data with cipher", e); } } Suggestion importance[1-10]: 6 __ Why: Valid concern: inputs shorter than GCM_NONCE_LENGTH may throw non-AEADBadTagException exceptions, bypassing the legacy fallback. The guard improves robustness for legacy short ciphertexts, though in practice legacy AES/ECB produces multi-block outputs so the impact is moderate.	Low
	Fix initialization ordering under concurrency jwtParser and encryptionUtil are declared volatile but assigned inside a synchronized block whose read (return jwtParser != null) is on the same synchronization; the extractCredentials path reads jwtParser/encryptionUtil without synchronization. Because initialized is set to true before jwtParser is assigned, a concurrent caller can see initialized == true on the next invocation and skip initialization while jwtParser is still null. Set initialized = true only after successful assignment (or don't rely on it for the null-check path). src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java [94-105] private synchronized boolean ensureInitialized() { if (!initialized) { - initialized = true; try { jwtParser = AccessController.doPrivileged(this::buildJwtParser); encryptionUtil = EncryptionDecryptionUtil.fromSettings(settings, ENCRYPTION_KEY, configPath, legacyFormatWindow); } catch (final RuntimeException e) { log.error("On-behalf-of authentication is misconfigured; OBO tokens will be rejected: {}", e.toString(), e); + } finally { + initialized = true; } } return jwtParser != null; } Suggestion importance[1-10]: 3 __ Why: The ensureInitialized() method is synchronized, so concurrent callers cannot skip initialization while jwtParser is null—every caller enters the synchronized block. The concern about concurrent readers is largely unfounded here, making this suggestion of limited value.	Low
General	Verify tracker window initialization semantics The comment above states "Written after preUpgradeNodeLastSeenMs, so a reader that sees a new state also sees its sighting." However, since nodes is volatile and preUpgradeNodeLastSeenMs is also volatile, but the reader in legacyFormatReadable() reads nodes first and then preUpgradeNodeLastSeenMs, the ordering here is correct only if the write to preUpgradeNodeLastSeenMs happens-before the write to nodes. That works, but if a pre-upgrade node is present in current and the sighting is recorded, a concurrent reader might read the new nodes (which still contains a pre-upgrade node) — fine. But if the previous state had the pre-upgrade node and current does not, the sighting is stamped then nodes is set to the upgraded one — also fine. Consider whether the initial preUpgradeNodeLastSeenMs = clock.getAsLong() combined with a first clusterChanged event where previousState has no nodes could reset the window incorrectly and shorten it below one token lifetime. src/main/java/org/opensearch/security/authtoken/jwt/LegacyRolesClaimFormat.java [253-259] +@Override +public void clusterChanged(final ClusterChangedEvent event) { + final DiscoveryNodes current = event.state().nodes(); + if (hasPreUpgradeNode(event.previousState().nodes()) || hasPreUpgradeNode(current)) { + preUpgradeNodeLastSeenMs = clock.getAsLong(); + } + nodes = current; +} - Suggestion importance[1-10]: 2 __ Why: The suggestion is speculative and asks the author to "verify" semantics without proposing a concrete change; improved_code is identical to existing_code, offering no actionable improvement.	Low

[github-actions]

Persistent review updated to latest commit 9f8fb30

[github-actions]

Persistent review updated to latest commit f6b50ef

[github-actions]

Persistent review updated to latest commit 4e3396c

[github-actions]

Persistent review updated to latest commit 715a21c