[jira] [Updated] (KAFKA-20921) ScramImage exposes mutable nested credential maps

["GiminKim (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/KAFKA-20921?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

GiminKim updated KAFKA-20921:
-----------------------------
    Description: 
h3. Problem

{{ScramImage}} documents itself as thread-safe, but its constructor previously wrapped only the outer mechanisms map with {{Collections.unmodifiableMap}} without copying it. The nested per-mechanism credential maps remained mutable. A caller could clear a nested map returned by {{mechanisms()}} and remove SCRAM users from an already constructed image.

{code:java}
image.mechanisms()
    .get(ScramMechanism.SCRAM_SHA_256)
    .clear();
{code}

{{ScramDelta.apply()}} supplies mutable nested {{HashMap}} instances to {{ScramImage}}. Changes to maps supplied to the constructor can also alter an existing image.

h3. Impact

The contents of a SCRAM metadata image can change after construction through its returned nested maps, rather than through a metadata update. This violates the image's intended snapshot semantics. No remote mutation path is currently known.

h3. Expected behavior

* The outer mechanisms map and every nested credential map are immutable snapshots.
* Mutating constructor input maps or maps returned from {{mechanisms()}} does not alter an existing image.

h3. Proposed fix

* Snapshot-copy and make immutable the outer and nested maps in {{ScramImage}}.
* Add regression tests for constructor input changes and returned-map mutation.

Scope: Defensive copying of {{ScramCredentialData}} byte arrays was considered during review but is intentionally not part of this issue. Credential array behavior is unchanged under the existing metadata ownership convention.

Related work: KAFKA-19305 mentioned {{ScramImage}} immutability, but PR #19847 only updated {{ClientQuotaImage}} and {{TopicImage}}.

  was:
h3. Problem

{{ScramImage}} and {{ScramCredentialData}} are documented as thread-safe, but both expose mutable state.

{{ScramImage}} only wraps the outer mechanisms map with {{Collections.unmodifiableMap}}. The wrapper does not copy the input map, and the nested per-mechanism maps remain mutable.

{code:java}
image.mechanisms()
    .get(ScramMechanism.SCRAM_SHA_256)
    .clear();
{code}

The image produced by {{ScramDelta.apply()}} is affected because its nested maps are mutable {{HashMap}} instances.

{{ScramCredentialData}} stores its {{byte[]}} components without copying them, and the record accessors expose the same arrays. Its conversion methods also pass those arrays directly to {{UserScramCredentialRecord}} and {{ScramCredential}}.

{code:java}
int before = data.hashCode();
data.salt()[0] ^= 1;
int after = data.hashCode();
// before != after
{code}

h3. Impact

Metadata images can change after construction, their {{equals}} and {{hashCode}} results are not stable, and concurrent readers may observe mutation of SCRAM users or credential bytes. This is a correctness and thread-safety issue in authentication metadata, but no remote mutation path is currently known.

h3. Expected behavior

* The outer mechanisms map and every nested credential map are immutable snapshots.
* Constructor inputs cannot mutate an existing image or credential value.
* Array accessors and conversion methods do not expose internal credential arrays.

h3. Proposed fix

* Snapshot-copy and make immutable the outer and nested maps in {{ScramImage}}.
* Defensively copy arrays on construction and when returning or converting {{ScramCredentialData}}.
* Add regression tests for constructor inputs, accessors, nested maps, {{toRecord()}}, and {{toCredential()}}.

Related work: KAFKA-19305 mentioned {{ScramImage}} immutability, but PR #19847 only updated {{ClientQuotaImage}} and {{TopicImage}}.

        Summary: ScramImage exposes mutable nested credential maps  (was: ScramImage and ScramCredentialData are not deeply immutable)

Scope narrowed during review of PR #23128 (https://github.com/apache/kafka/pull/23128#discussion_r4008394035). Defensive copying of the `ScramCredentialData` byte arrays was considered but withdrawn to follow the existing metadata ownership convention; this does not make those arrays immutable. This issue and PR now cover only `ScramImage`'s outer and nested mechanism maps.

> ScramImage exposes mutable nested credential maps
> -------------------------------------------------
>
>                 Key: KAFKA-20921
>                 URL: https://issues.apache.org/jira/browse/KAFKA-20921
>             Project: Kafka
>          Issue Type: Bug
>          Components: kraft
>         Environment: Apache Kafka trunk at 2225a16c7e2658b4db207c89c477693373a52073 (4.4.0-SNAPSHOT)
>            Reporter: GiminKim
>            Assignee: GiminKim
>            Priority: Major
>
> h3. Problem
> {{ScramImage}} documents itself as thread-safe, but its constructor previously wrapped only the outer mechanisms map with {{Collections.unmodifiableMap}} without copying it. The nested per-mechanism credential maps remained mutable. A caller could clear a nested map returned by {{mechanisms()}} and remove SCRAM users from an already constructed image.
> {code:java}
> image.mechanisms()
>     .get(ScramMechanism.SCRAM_SHA_256)
>     .clear();
> {code}
> {{ScramDelta.apply()}} supplies mutable nested {{HashMap}} instances to {{ScramImage}}. Changes to maps supplied to the constructor can also alter an existing image.
> h3. Impact
> The contents of a SCRAM metadata image can change after construction through its returned nested maps, rather than through a metadata update. This violates the image's intended snapshot semantics. No remote mutation path is currently known.
> h3. Expected behavior
> * The outer mechanisms map and every nested credential map are immutable snapshots.
> * Mutating constructor input maps or maps returned from {{mechanisms()}} does not alter an existing image.
> h3. Proposed fix
> * Snapshot-copy and make immutable the outer and nested maps in {{ScramImage}}.
> * Add regression tests for constructor input changes and returned-map mutation.
> Scope: Defensive copying of {{ScramCredentialData}} byte arrays was considered during review but is intentionally not part of this issue. Credential array behavior is unchanged under the existing metadata ownership convention.
> Related work: KAFKA-19305 mentioned {{ScramImage}} immutability, but PR #19847 only updated {{ClientQuotaImage}} and {{TopicImage}}.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)