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