Skip to content

Commit 5f9a236

Browse files
Sanju98claude
andauthored
Add SPIFFE v2 URI-SAN based principal extraction (DEPEND-89172) (#142)
* Add SPIFFE v2 URI-SAN based principal extraction (DEPEND-89172) Adds server-side support for SPIFFE-issued client certificates in X509 auth. The principal is the ILM UID (path-after-/v2/), aligned with the LinkedIn ILM v2 design: trust-domain stripped, segment-prefix matching for ACL lookup. Key changes: - X509AuthenticationUtil.matchAndExtractSpiffeSAN extracts the ILM UID from v2 SPIFFE URIs. v1 SPIFFE URIs and user-identity URIs (/v<N>/user/...) fall through to existing URN / Subject-DN extraction. - X509AuthenticationConfig adds spiffe.sanMatchRegex as the operator- controlled trust-domain gate. - ZkClientUriDomainMappingHelper recursively walks the znode subtree below each domain. Only leaf znodes are registered as keys (path joined by '/'), letting multi-segment SPIFFE UIDs be expressed as nested znodes (whose names can't contain '/'). getDomains does exact-match then segment-prefix walk-up. clientUriToDomainNames is volatile with in-method snapshot. - Defense-in-depth: SPIFFE path extraction uses URI.getRawPath() and rejects any path containing '%' to prevent URL-decoding bypass of identity checks. Tests: 29/29 pass (X509AuthTest 13, X509SpiffeAuthIntegrationTest 6, ZkClientUriDomainMappingHelperTest 10; includes end-to-end SPIFFE-cert through X509ZNodeGroupAclProvider with real znode mapping). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address review: thread-safe SPIFFE config + wire prefix walk-up to production path Two fixes addressing the code review on PR #142: 1. X509AuthenticationConfig.getSpiffeSanMatchPattern now uses double-checked locking with volatile fields and a dedicated lock object, matching the pattern already used by allowedClientIdAsAclDomains and other lazy-loaded fields in the same class. The previous lazy-init was a data race on the per-handshake hot path. 2. X509ZNodeGroupAclProvider's setDomainAuthUpdater lambda now calls helper.getDomains(clientId) instead of the raw map's getOrDefault, so the segment-prefix walk-up added to ZkClientUriDomainMappingHelper is reachable from the production authentication path. This is the znode-tree analogue of LinkedIn's documented acl-tool wildcard idiom (`--spiffe "application/<mp>/*"`); without this fix, MP-level prefix grants documented in the class javadoc would silently no-op. Adds testA4_SpiffeCertResolvesViaPrefixWalkUpToDomainAuthInfo: real SPIFFE cert with a 4-segment principal resolves to an MP-level leaf grant through the full provider->helper.getDomains path. This test would have failed before fix #2 (the exact-match lookup misses the 4-segment principal against a 2-segment registered prefix). All 30 SPIFFE-related tests pass (X509AuthTest 13 + X509SpiffeAuthIntegrationTest 6 + ZkClientUriDomainMappingHelperTest 11). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address review: accept SPIFFE v1 workload certs alongside v2 Per @rgodha's review comment, the extractor now accepts both SPIFFE versions instead of rejecting v1 outright: - v2 (existing): spiffe://<td>/v2/<path> → principal is the full path after /v2/ (the ILM UID, e.g. "application/foo-mp/bar-app"). - v1 workload (new): spiffe://<td>/v1/wl/<app-name> → strip the "wl/" type prefix; principal is just the app-name. This matches how legacy authZ handled v1 identities. Other v1 paths (e.g. v1/wf/ workflow) still fall through to URN/DN. User-identity URIs (/v<N>/user/...) continue to be rejected for both versions — they must never be promoted to a service principal. The test match regex broadens to ^spiffe://.*/v[12]/.*$ so existing test scaffolding exercises both versions. testSpiffeV1FallsBackToDn becomes testSpiffeV1WlAuth (now asserts extraction). The integration test for v1 likewise flips from fall-back-to-DN to v1/wl extraction. LISPIFFE-ID spec reference: https://github.com/linkedin-multiproduct/gopki/blob/master/LISPIFFE-ID.md#2-uri-path Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix SPIFFE v1/wl principal extraction to reject multi-segment values, and add regression tests for a real Grestin cert's dual urn:li: SAN scenario. * Make SPIFFE URI-SAN principal extraction always-on, not a feature flag Previously SPIFFE detection was gated behind the opt-in ssl.x509.spiffe.sanMatchRegex system property and was a no-op unless an operator explicitly configured it. This removes that config knob entirely: X509AuthenticationUtil#getClientId now checks for a spiffe:// URI SAN unconditionally, before the clientCertIdType-gated legacy URN fallback, regardless of how (or whether) clientCertIdType is configured. - Remove SSL_X509_SPIFFE_SAN_MATCH_REGEX and its lazy-loaded Pattern field/getter/setter from X509AuthenticationConfig. - X509AuthenticationUtil: replace the configurable SPIFFE match pattern with an unconditional constant and run SPIFFE detection first, unconditionally, in getClientId(). - Update SpiffeAuthTestUtil and existing SPIFFE tests to drop references to the removed config property. - Add regression tests proving SPIFFE v1/v2 extraction and SPIFFE user-identity rejection work with zero clientCertIdType configuration (X509AuthTest, X509SpiffeAuthIntegrationTest). * Add v1 application/airflow workload sub-type support to SPIFFE extraction LISPIFFE-ID spec section 2.A lists application/<...> and airflow/<....> as valid v1 workload sub-types alongside wl/, which were not previously recognized. Per rgodha's review comment on PR #142, extend the v1 extractor to accept these forms, retaining the type prefix in the principal (matching v2 semantics), while wl/ keeps its existing bare-app-name behavior. v1/wf/ (Flyte workflow) remains out of scope. Adds 6 new tests across X509AuthTest and X509SpiffeAuthIntegrationTest. * Consolidate SPIFFE auth tests into real-cert integration suite X509AuthTest previously duplicated most SPIFFE URI-SAN extraction scenarios using TestCertificate, a hand-rolled X509Certificate whose getSubjectAlternativeNames() just returns a canned list -- it never exercises real ASN.1/SAN encoding or the JDK's certificate parsing. X509SpiffeAuthIntegrationTest already covered several of the same scenarios using real BouncyCastle-signed certs, but had a few gaps. Changes: - Added 8 real-cert tests to X509SpiffeAuthIntegrationTest to close the coverage gaps: v1/wl zero-config, v1/wl multi-segment fallback, v1/user rejection, v2/workload extraction, v2/user zero-config rejection, non-SPIFFE-SAN-falls-back-to-URN (with URN configured), multiple-SPIFFE-SANs fallback, and SPIFFE-wins-over-URN precedence. - Removed the now-redundant SPIFFE-specific mock tests from X509AuthTest (17 tests across ~230 lines), along with the SPIFFE_V1_URI/SPIFFE_V2_URI fixtures and now-unused imports. X509AuthTest keeps only the generic (non-SPIFFE) auth/SAN-regex tests, which still use the lightweight fake certificate since they don't need real certificate parsing. - X509SpiffeAuthIntegrationTest is now the authoritative, real-cert suite for SPIFFE certificate validation and principal extraction. Verified: X509AuthTest (6 tests) + X509SpiffeAuthIntegrationTest (18 tests, up from 10) all pass. * Trigger CI re-run (no code changes) The previous CI run for this PR is over a month old and can no longer be rerun via the GitHub Actions UI/API (runs older than ~30 days are ineligible for rerun). This empty commit triggers a fresh run to get a current result for the flaky C-client testAuth check. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 1d05536 commit 5f9a236

9 files changed

Lines changed: 1111 additions & 63 deletions

File tree

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/X509AuthenticationConfig.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ public static X509AuthenticationConfig getInstance() {
8383
public static final String SSL_X509_CLIENT_CERT_ID_SAN_EXTRACT_MATCHER_GROUP_INDEX =
8484
SSL_X509_CONFIG_PREFIX + "clientCertIdSanExtractMatcherGroupIndex";
8585
public static final String SUBJECT_ALTERNATIVE_NAME_SHORT = "SAN";
86+
8687
private static final String DEFAULT_REGEX = ".*";
8788
private String clientCertIdType;
8889
private int clientCertIdSanMatchType = -1;
@@ -482,4 +483,5 @@ public static void reset() {
482483
instance = null;
483484
}
484485
}
486+
485487
}

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/X509AuthenticationUtil.java‎

Lines changed: 170 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,13 @@
1818

1919
package org.apache.zookeeper.server.auth;
2020

21+
import java.net.URI;
2122
import java.security.cert.CertificateException;
2223
import java.security.cert.CertificateParsingException;
2324
import java.security.cert.X509Certificate;
2425
import java.util.Collection;
2526
import java.util.List;
27+
import java.util.Optional;
2628
import java.util.regex.Matcher;
2729
import java.util.regex.Pattern;
2830
import java.util.stream.Collectors;
@@ -48,6 +50,39 @@ public class X509AuthenticationUtil extends X509Util {
4850
public static final String SUPERUSER_AUTH_SCHEME = "super";
4951
public static final String X509_SCHEME = "x509";
5052

53+
// Matches any SPIFFE URI, regardless of trust domain or version. SPIFFE detection is always
54+
// active — not gated behind operator config — and relies entirely on the TLS trust manager to
55+
// reject certificates from untrusted issuers before this code is ever reached.
56+
private static final Pattern SPIFFE_URI_PATTERN = Pattern.compile("^spiffe://.*$");
57+
58+
// Matches LISPIFFE user-identity paths of the form "/v<N>/user" or "/v<N>/user/<rest>".
59+
// User-identity SPIFFE certs (issued to humans, not workloads) must NOT be promoted to a
60+
// service principal, otherwise a user credential would be granted service-level ACL access.
61+
// See LISPIFFE-ID spec: https://github.com/linkedin-multiproduct/gopki/blob/master/LISPIFFE-ID.md
62+
private static final Pattern SPIFFE_USER_IDENTITY_PATH_PATTERN =
63+
Pattern.compile("^/v\\d+/user(/.*)?$");
64+
65+
// Matches LISPIFFE v2 paths and captures the ILM UID (the path after "/v2/").
66+
// The canonical ILM v2 principal is the full path-after-v2 (e.g. "application/foo-mp/bar-app");
67+
// ACL matching downstream is segment-prefix on this UID.
68+
private static final Pattern SPIFFE_V2_PATH_PATTERN = Pattern.compile("^/v2/(.+)$");
69+
70+
// Matches the legacy LISPIFFE v1 "wl/<app-name>" workload form and captures the app-name.
71+
// Per LISPIFFE-ID spec, the v1 workload unique-identity is "wl/<app-name>"; we strip the
72+
// "wl/" type prefix and return just the app-name as the principal, matching how legacy authZ
73+
// systems handled v1 identities. The app-name is a single path segment (no "/"); a
74+
// multi-segment value after "wl/" does not match here and falls through to URN/DN extraction
75+
// instead of being misinterpreted as a single app-name.
76+
private static final Pattern SPIFFE_V1_WL_PATH_PATTERN = Pattern.compile("^/v1/wl/([^/]+)$");
77+
78+
// Matches the other LISPIFFE v1 workload sub-types (LISPIFFE-ID spec §2.A: "application/<...>"
79+
// and "airflow/<....>", alongside "wl/"). Unlike "wl/", these keep their type prefix in the
80+
// extracted principal (e.g. "/v1/application/foo-mp/bar-app" -> "application/foo-mp/bar-app"),
81+
// matching how v2 identities are handled. Deliberately excludes "wf/" (v1 Flyte workflow),
82+
// which is out of scope for ZK per PR #142 review discussion.
83+
private static final Pattern SPIFFE_V1_WORKLOAD_PATH_PATTERN =
84+
Pattern.compile("^/v1/(application|airflow)/(.+)$");
85+
5186
@Override
5287
protected String getConfigPrefix() {
5388
return X509AuthenticationConfig.SSL_X509_CONFIG_PREFIX;
@@ -131,6 +166,20 @@ public static X509TrustManager createTrustManager(ZKConfig config) {
131166
* The clientId string is intended to be an URI for client and map the client to certain domain.
132167
*/
133168
public static String getClientId(X509Certificate clientCert) {
169+
// SPIFFE identity extraction always runs, regardless of clientCertIdType configuration —
170+
// it is not a feature flag. Any URI SAN beginning with "spiffe://" is treated as a
171+
// candidate; trust in the issuing CA/trust-domain is established upstream by the TLS
172+
// handshake's trust manager, not by this method.
173+
try {
174+
Optional<String> spiffeId = X509AuthenticationUtil.matchAndExtractSpiffeSAN(clientCert);
175+
if (spiffeId.isPresent()) {
176+
LOG.debug("Extracted SPIFFE identity: {}", spiffeId.get());
177+
return spiffeId.get();
178+
}
179+
} catch (Exception e) {
180+
LOG.warn("Failed to extract SPIFFE identity from SAN. Falling through to legacy extraction.", e);
181+
}
182+
134183
String clientCertIdType = X509AuthenticationConfig.getInstance().getClientCertIdType();
135184
if (clientCertIdType != null && clientCertIdType
136185
.equalsIgnoreCase(X509AuthenticationConfig.SUBJECT_ALTERNATIVE_NAME_SHORT)) {
@@ -140,10 +189,128 @@ public static String getClientId(X509Certificate clientCert) {
140189
LOG.warn("Failed to match and extract a client ID from SAN. Using Subject DN instead.", ce);
141190
}
142191
}
143-
// return Subject DN by default
144192
return clientCert.getSubjectX500Principal().getName();
145193
}
146194

195+
/**
196+
* Attempt to extract a client identity from a LISPIFFE URI SAN. Always active — not gated
197+
* behind any operator configuration. Any URI SAN beginning with {@code spiffe://} is treated
198+
* as a candidate. Supported forms:
199+
* <ul>
200+
* <li><b>v2</b> ({@code spiffe://<td>/v2/<path>}): principal is the full path-after-{@code /v2/}
201+
* (the ILM UID), e.g. {@code spiffe://prod.lipki/v2/application/foo-mp/bar-app} →
202+
* {@code application/foo-mp/bar-app}. ACL matching downstream is segment-prefix on the UID.</li>
203+
* <li><b>v1 workload, {@code wl} form</b> ({@code spiffe://<td>/v1/wl/<app-name>}): principal is
204+
* just the {@code <app-name>} (the "wl/" type prefix is stripped, matching how legacy authZ
205+
* handled v1 identities).</li>
206+
* <li><b>v1 workload, {@code application}/{@code airflow} forms</b>
207+
* ({@code spiffe://<td>/v1/application/<path>} or {@code spiffe://<td>/v1/airflow/<path>}):
208+
* principal is the full path including the type prefix, e.g.
209+
* {@code spiffe://<td>/v1/application/foo-mp/bar-app} → {@code application/foo-mp/bar-app}
210+
* (see LISPIFFE-ID spec §2.A).</li>
211+
* </ul>
212+
*
213+
* <p>Returns {@link Optional#empty()} when no URI SAN begins with {@code spiffe://}, the
214+
* matched URI is a user identity ({@code /v<N>/user/...}, which must never be promoted to a
215+
* service principal), or the matched URI is a non-{v1 wl/application/airflow, v2} path (e.g.
216+
* v1 Flyte workflow {@code /v1/wf/...}, out of scope for ZK). Caller falls through to URN/DN
217+
* extraction.
218+
*
219+
* @throws IllegalArgumentException if multiple URI SANs begin with {@code spiffe://}
220+
*/
221+
private static Optional<String> matchAndExtractSpiffeSAN(X509Certificate clientCert)
222+
throws CertificateParsingException {
223+
String spiffeUri = findSingleMatchingSan(clientCert, 6, SPIFFE_URI_PATTERN, "SPIFFE");
224+
if (spiffeUri == null) {
225+
return Optional.empty();
226+
}
227+
228+
String path;
229+
try {
230+
// getRawPath() returns the literal (un-percent-decoded) path so the principal we accept is
231+
// exactly what the CA validated in the SAN. getPath() would decode %2F → /, allowing a
232+
// single-segment SAN like /v2/foo%2Fbar to be promoted to a multi-segment principal that
233+
// could collide with an unrelated registered identity. Reject any path containing % to
234+
// also block encoded "user" bypass (e.g. /v2/%75ser/alice).
235+
path = URI.create(spiffeUri).getRawPath();
236+
} catch (IllegalArgumentException e) {
237+
LOG.debug("Malformed SPIFFE URI '{}'; falling through to URN/DN extraction.", spiffeUri);
238+
return Optional.empty();
239+
}
240+
if (path == null) {
241+
return Optional.empty();
242+
}
243+
if (path.indexOf('%') >= 0) {
244+
LOG.debug("Rejecting SPIFFE URI with percent-encoded path '{}'; falling through.", spiffeUri);
245+
return Optional.empty();
246+
}
247+
if (SPIFFE_USER_IDENTITY_PATH_PATTERN.matcher(path).matches()) {
248+
LOG.debug("Rejecting SPIFFE user identity '{}' for service-principal extraction.", spiffeUri);
249+
return Optional.empty();
250+
}
251+
Matcher v2Matcher = SPIFFE_V2_PATH_PATTERN.matcher(path);
252+
if (v2Matcher.matches()) {
253+
return Optional.of(v2Matcher.group(1));
254+
}
255+
Matcher v1WlMatcher = SPIFFE_V1_WL_PATH_PATTERN.matcher(path);
256+
if (v1WlMatcher.matches()) {
257+
return Optional.of(v1WlMatcher.group(1));
258+
}
259+
Matcher v1WorkloadMatcher = SPIFFE_V1_WORKLOAD_PATH_PATTERN.matcher(path);
260+
if (v1WorkloadMatcher.matches()) {
261+
return Optional.of(v1WorkloadMatcher.group(1) + "/" + v1WorkloadMatcher.group(2));
262+
}
263+
LOG.debug("SPIFFE URI '{}' is not a v1/wl, v1/application, v1/airflow, or v2 identity; "
264+
+ "falling through to URN/DN extraction.", spiffeUri);
265+
return Optional.empty();
266+
}
267+
268+
/**
269+
* Returns the single SAN value of the given type whose value matches the regex, or null if
270+
* there are zero matches. Throws if there are multiple matches (callers always want exactly one).
271+
*/
272+
private static String findSingleMatchingSan(X509Certificate cert, int sanType, Pattern pattern,
273+
String matchKind) throws CertificateParsingException {
274+
String found = null;
275+
Collection<List<?>> sans = cert.getSubjectAlternativeNames();
276+
if (sans == null) {
277+
return null;
278+
}
279+
for (List<?> san : sans) {
280+
if (!Integer.valueOf(sanType).equals(san.get(0))) {
281+
continue;
282+
}
283+
String value = san.get(1).toString();
284+
if (!pattern.matcher(value).find()) {
285+
continue;
286+
}
287+
if (found != null) {
288+
String errStr = "Expected exactly 1 " + matchKind + " SAN but found more than 1. "
289+
+ "Please fix the match regex so exactly one match is found.";
290+
LOG.error(errStr);
291+
throw new IllegalArgumentException(errStr);
292+
}
293+
found = value;
294+
}
295+
return found;
296+
}
297+
298+
/**
299+
* Applies an extract regex to a SAN value and returns the captured group.
300+
*
301+
* @throws IllegalArgumentException if the regex does not match.
302+
*/
303+
private static String applyExtractRegex(Pattern extractPattern, String value, int groupIndex) {
304+
Matcher matcher = extractPattern.matcher(value);
305+
if (!matcher.find()) {
306+
String errStr = "Failed to extract identity from '" + value
307+
+ "' using regex '" + extractPattern.pattern() + "'";
308+
LOG.error(errStr);
309+
throw new IllegalArgumentException(errStr);
310+
}
311+
return matcher.group(groupIndex);
312+
}
313+
147314
/**
148315
* Extract the authenticated client Id from the specified server connection object.
149316
* @param cnxn Server connection object that contains the certificate.
@@ -204,18 +371,8 @@ private static String matchAndExtractSAN(X509Certificate clientCert)
204371
throw new IllegalArgumentException(errStr);
205372
}
206373

207-
// Extract a substring from the found match using extractRegex
208-
Pattern extractPattern = Pattern.compile(extractRegex);
209-
Matcher matcher = extractPattern.matcher(matched.iterator().next().get(1).toString());
210-
if (matcher.find()) {
211-
// If extractMatcherGroupIndex is not given, return the 1st index by default
212-
String result = matcher.group(extractMatcherGroupIndex);
213-
LOG.debug("Returning extracted client ID: {} using Matcher group index: {}", result, extractMatcherGroupIndex);
214-
return result;
215-
}
216-
String errStr = "Failed to find an extract substring to determine client ID. Please review the extract regex.";
217-
LOG.error(errStr);
218-
throw new IllegalArgumentException(errStr);
374+
return applyExtractRegex(Pattern.compile(extractRegex),
375+
matched.iterator().next().get(1).toString(), extractMatcherGroupIndex);
219376
}
220377

221378
/**

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/znode/groupacl/X509ZNodeGroupAclProvider.java‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
package org.apache.zookeeper.server.auth.znode.groupacl;
2020

2121
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
22-
import java.util.Collections;
2322
import java.util.HashSet;
2423
import java.util.List;
2524
import java.util.Set;
@@ -164,11 +163,14 @@ private ClientUriDomainMappingHelper getUriDomainMappingHelper(ZooKeeperServer z
164163
// Set up AuthInfo updater to refresh connection AuthInfo on any client domain changes.
165164
// TODO Making the anonymous class to a separate updater implementation class if any other Acl provider shares
166165
// the same logic.
167-
helper.setDomainAuthUpdater((cnxn, clientUriToDomainNames) -> {
166+
// Route through helper.getDomains(clientId) so SPIFFE multi-segment principals resolve
167+
// via the segment-prefix walk-up (operator can register an MP-level leaf to grant all
168+
// apps under that MP; see ZkClientUriDomainMappingHelper class javadoc). The map passed
169+
// into the lambda is ignored — kept in the interface signature for backward compat.
170+
helper.setDomainAuthUpdater((cnxn, ignoredMap) -> {
168171
try {
169172
String clientId = X509AuthenticationUtil.getClientId(cnxn, trustManager);
170-
assignAuthInfo(cnxn, clientId,
171-
clientUriToDomainNames.getOrDefault(clientId, Collections.emptySet()));
173+
assignAuthInfo(cnxn, clientId, helper.getDomains(clientId));
172174
} catch (UnsupportedOperationException unsupportedEx) {
173175
LOG.info("Cannot update AuthInfo for session 0x{} since the operation is not supported.",
174176
Long.toHexString(cnxn.getSessionId()));

0 commit comments

Comments
 (0)