apache / apache/pulsar

[fix][admin] PIP-478: PulsarAdminBuilderImpl.build() shares its configuration, so a second admin can inherit the first's OAuth2 IdP trust material

Open
#26,398 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
15.3k
Forks
3.8k
Avg merge
1d 14h
Merged PRs (30d)
160

Description

### Motivation

`PulsarAdminBuilderImpl.build()` hands the builder's own `ClientConfigurationData` to the admin without
copying it:

```java
// PulsarAdminBuilderImpl:49
public PulsarAdmin build() throws PulsarClientException {
return new PulsarAdminImpl(conf.getServiceUrl(), conf,
clientBuilderClassLoader, acceptGzipCompression, sharedResources);
}
```

and `PulsarAdminImpl.foldOAuth2IdpPolicy` writes into it during construction:

```java
// PulsarAdminImpl:514
oauth2.idpTlsPolicy(clientDefault.jsseProvider(), clientDefault.jcaProvider()).ifPresent(policy -> {
Map policies = conf.getTlsPolicyMap();
if (policies == null) {
policies = new LinkedHashMap<>();
conf.setTlsPolicyMap(policies);
}
policies.putIfAbsent(TlsPurpose.CLIENT_OAUTH2, policy);
});
```

The first `build()` installs the IdP policy into a map that stays reachable from the builder. On a second
`build()` the map is already present and already has a `CLIENT_OAUTH2` entry, so `putIfAbsent` keeps the
first one:

```java
PulsarAdmin a = builder.authentication(oauth2ForIdpA).build();
PulsarAdmin b = builder.authentication(oauth2ForIdpB).build(); // b resolves against IdP A's policy
```

Admin *b* then fetches its token against IdP B while trusting IdP A's material — the wrong private CA, or
the wrong mTLS client key — with no error and no log line.

### Scope

Narrower than the client-side version of this bug. The admin never stores a composed `PulsarTlsFactory`
back onto `conf`, so there is no closed-factory failure and no cross-client teardown; a plain second
`build()` with unchanged authentication is idempotent. It needs two builds from one builder with
*different* OAuth2 credentials, which is unusual — but the outcome is silent wrong-trust rather than a
visible failure, which is why it is worth closing rather than documenting.

### Suggested fix

The same one-line change the two client builders received in #26326 — hand the admin a copy:

```java
return new PulsarAdminImpl(conf.getServiceUrl(), conf.clone(), ...);
```

`ClientConfigurationData.clone()` is a shallow copy, which is enough here: the defect is the shared
*mutation target*, and every caller that mutates the builder's configuration does so before `build()`.
Note that a shallow copy still shares the `tlsPolicyMap` instance itself, so the copy should be paired
with either a defensive copy of that map or a `put` rather than `putIfAbsent` at the fold — worth
deciding when fixing, and worth a test that builds two admins with different OAuth2 IdP material from one
builder and asserts each gets its own `CLIENT_OAUTH2` policy.

### Context

Reported by @david-streamlio reviewing #26326, alongside the same defect on `ClientBuilderImpl` and
`PulsarClientBuilderV5` — both of which are fixed in that PR. Split out here on his suggestion, since the
consequence and the reasoning differ enough to take on their own merits rather than for symmetry.

Verified against `master` before filing: both call sites are as quoted.

Contributor guide

Open the contributing guide

Research direction

Start in PulsarAdminBuilderImpl.build() and PulsarAdminImpl.foldOAuth2IdpPolicy(), then inspect ClientConfigurationData.clone() and the client-side fix in #26326. Add a regression test that builds two admins from one builder with different OAuth2 IdP material and verifies each has its own CLIENT_OAUTH2 policy; resolve the shallow tlsPolicyMap handling as part of the fix.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
authentication, backend, security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.