Support one-way SPIFFE compatibility with legacy identities - #149
Open
bellatrix007 wants to merge 15 commits into
Open
bellatrix007 wants to merge 15 commits into
bellatrix007 wants to merge 15 commits into
Conversation
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>
…oduction path Two fixes addressing the code review on PR linkedin#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 linkedin#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>
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>
… and add regression tests for a real Grestin cert's dual urn:li: SAN scenario.
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).
…tion 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 linkedin#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.
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.
DEPEND-89163 follow-up: X509AuthenticationUtil.getClientId() now
recognizes LinkedIn's legacy Grestin-issued
'urn:li:servicePrincipal(<name>;...)' URI SAN automatically and
resolves it to the bare <name>, mirroring how the SPIFFE v1/wl
extraction strips its 'wl/' type prefix. This removes the need for
every cluster to hand-author a clientCertIdSanExtractRegex just to
get parity between legacy Grestin certs and SPIFFE certs during
migration (e.g. Kafka's 'servicePrincipal(kafka' vs SPIFFE's 'kafka'
znode-name mismatch).
The new pattern only runs when clientCertIdType=SAN was never
configured, so clusters that already tuned a SAN extractRegex (e.g.
one that intentionally keeps the 'servicePrincipal(' prefix, or one
that falls back to Subject DN on a misconfigured regex) keep their
existing, unchanged behavior.
Adds 4 unit tests to X509AuthTest covering: auto-extraction with no
config, correct behavior with a sibling servicePrincipalMetadata(...)
SAN present, non-interference with an already-configured
clientCertIdType=SAN cluster, and the existing no-match fallback to
Subject DN.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove company and internal-ticket references from the added comments and use generic certificate fixtures. Clarify that configured SAN extraction failures retain the Subject DN fallback; extraction behavior is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace automatic certificate-side URN normalization with type-aware domain lookup. Return an immutable certificate-type/client-ID pair while preserving the original SPIFFE, configured SAN and Subject DN strings. Only SPIFFE v1/wl identities may fall back to exact application names parsed from legacy service-principal mapping keys; explicit mappings and full-path matching retain precedence and behavior. Cover typed identities, legacy mapping compatibility, exact-match precedence, principal/type isolation and mapping refresh. Use independent temporary databases for the mapping fixture. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Retain the authenticated certificate-type/client-ID pair on each connection and forward connection context through a backward-compatible authentication-provider matcher overload. Share legacy service-principal parsing between domain lookup and direct ACL checks. Allow SPIFFE v1/wl identities to match legacy service-principal ACLs and typed legacy service principals to match bare application ACLs. Bind compatibility to the original authenticated ID so mapped domains, other principal kinds and v2 paths are not reinterpreted. Preserve AuthInfo, creator ACLs, permission checks and exact superuser handling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Try exact and segment-prefix domain mappings before legacy service-principal fallback. Require an explicit application/product/app path with an optional tag before comparing the application segment; retain existing v1/wl support. Reuse the same rule for direct ACLs without changing certificate extraction, stored identities, permissions or authenticated-ID binding. Preserve legacy-to-bare-name compatibility without inferring product or tag context. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep exact superuser matches first and share typed SPIFFE-to-legacy service-principal selection between both X509 providers. The new zookeeper.ssl.x509.legacySuperUserCompatibilityEnabled setting defaults to false. Preserve the original certificate identity and use the matched configured ID as the super marker, retaining explicit-superuser ACL handling without changing cross-domain policy. Document activation and privilege scope and add focused regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Allow eligible SPIFFE application identities to match bare or formatted legacy targets without changing certificate-derived IDs. Restrict new bare aliases to conservative ASCII application names and remove the reverse alias introduced by this branch, preserving original legacy matching. Retain authenticated-ID binding, exact-match precedence and default-off superuser compatibility. Document the boundaries and cover structured targets and legacy exact matches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Depends on #142. This branch is built on Sanay's SPIFFE branch and targets
branch-3.6. Merge #142 first, then rebase this branch if needed.The current upstream diff includes Sanay's unmerged SPIFFE implementation (18 files total). The description below covers the 16-file compatibility increment on top of it. Review that increment separately in the compatibility-only diff; this PR uses the same head commit,
c7dbf32495c5ceb1274fd6e3a558b1f61dc6c270.Preserve certificate-derived client ID strings while supporting one-way SPIFFE-to-legacy application-name compatibility in URI-domain mappings, direct znode ACLs, and opt-in legacy superuser-ID selection. Eligible SPIFFE applications can use bare application names or formatted service-principal targets; legacy SAN/DN clients retain their original pre-PR matching behavior.
getClientId(...)returns an immutableClientIdentitywith the original ID and its extraction type:SPIFFE_V1_WL,SPIFFE_V1_WORKLOAD,SPIFFE_V2,LEGACY_SAN, orSUBJECT_DN. The original SPIFFE/configured SAN/Subject DN extraction order and strings are unchanged. The earlier automatic certificate-side URN normalization is removed.Successful authentication retains this pair on the connection, separately from
AuthInfo. Authentication failure clears it. Direct ACL checks therefore use authenticated type information without reparsing certificates per request or changing stored identity strings.URI-domain matching
Lookup order:
Eligible identities:
SPIFFE_V1_WLkafkakafkaSPIFFE_V1_WORKLOADapplication/example-mp/kafkakafkaSPIFFE_V2application/example-mp/kafkakafkaapplication/example-mp/kafka/cluster-akafkaApplication paths must be exactly
application/<mp>/<app>[/<tag>], with nonempty segments. Product-only paths such asapplication/example-mp, missing app segments, arbitrary v2 paths, and user/group/airflow/workload paths do not gain legacy service-principal compatibility. The product or tag is never used in place of the app name.Bare names (
kafka), truncated names (servicePrincipal(kafka), closed names (servicePrincipal(kafka)), and complete service-principal URNs are supported. Compatible legacy keys contribute a union of their domain sets only on fallback. Legacy SAN/DN and string-only lookup retain their existing behavior.New bare-name aliases must match
^[A-Za-z0-9][A-Za-z0-9._-]*$and are compared literally. This is a conservative compatibility grammar, not a change to certificate extraction or an assertion about every valid application name. For example,application/example-mp/CN=admincannot acquire an alias toCN=admin; the same applies tourn:example:adminand other structured-looking targets. Existing exact matching and formatted service-principal parsing remain unchanged. A full application-path target is never shortened.The existing znode can remain:
A legacy SAN ID
servicePrincipal(kafkamatches its exact key. A SPIFFE v1/wl IDkafka, or a v1/v2 IDapplication/example-mp/kafka, can reach the same domain through the compatibility fallback. A bare mapping such as/zookeeper/uri-domain-map/broker-access/kafkais also eligible for a full SPIFFE application identity's fallback; neither mapping needs renaming.Legacy compatibility compares app names only; it does not distinguish products or tags. Full-path or prefix mappings take precedence when product/tag-specific domain selection is configured. The full authenticated identity is retained.
Direct znode ACL matching
(SPIFFE_V1_WL, "kafka")x509:kafka(exact), orx509:servicePrincipal(kafkaapplication/example-mp/kafka[/cluster-a]x509:kafkaorx509:servicePrincipal(kafka(LEGACY_SAN, "kafka")x509:kafka(existing exact match)(LEGACY_SAN, "servicePrincipal(kafka")x509:servicePrincipal(kafka(existing exact match)Closed and full-URN service-principal ACL forms are supported as well. An application identity is not rewritten into a bare ID. The reverse legacy-service-principal-to-bare-name alias added in an earlier iteration of this PR is removed, restoring original pre-PR legacy-client behavior. Legacy IDs do not gain new aliases to bare or full product/tag-qualified application ACLs.
A bare ACL ID can also denote a domain. This intentionally allows an eligible SPIFFE application to match a same-named bare domain ACL; app-name compatibility does not distinguish products or tags.
LegacyServicePrincipalMatchershares the rules between domain lookup and direct ACL checks. The optional connection-awareAuthenticationProvider.matches(...)overload is forwarded by the existing provider wrapper; other providers retain their original string matcher. Both X509 providers use the authenticated identity pair.The candidate AuthInfo ID must equal the original certificate-derived ID. A mapped domain cannot be reinterpreted as another service principal. Ordinary
x509identities and creator ACL expansion rules are unchanged; no syntheticx509aliases are added. Superuser role markers are described below.User/group/metadata principal names, untyped identities, unrelated application names, and Subject DN identities do not gain compatibility access. Matching is case-sensitive, and normal ACL permission checks remain enforced.
Opt-in legacy superuser-ID compatibility
New shared Java system property, disabled by default:
This applies to
zookeeper.X509AuthenticationProvider.superUserandzookeeper.X509ZNodeGroupAclProvider.superUserIdselection. Exact configured client-ID matches always take precedence and do not require the option. When enabled, eligible SPIFFE v1/wl or complete v1/v2 application identities can instead match a configured bare application name or formatted legacy service principal using the same compatibility rules. Multiple compatible configured IDs select a stable marker.The authenticated certificate-derived identity is unchanged. The
superAuthInfo entry uses the matched configured ID:Keeping the configured marker is important:
PrepRequestProcessor.fixupACL()still recognizes the connection as an explicit superuser rather than a cross-domain component, so it does not accidentally replace that superuser's requested ACLs under the group provider's automatic ACL policy.Only forward SPIFFE-to-legacy compatibility is added for superuser selection, just as for domain and direct-ACL fallback. User/group/airflow/arbitrary-v2/DN identities do not gain superuser aliases, and mapped domain names are not substituted for the authenticated identity. Existing exact legacy SAN/DN superuser settings remain valid, including structured ID strings excluded from the new bare-name alias grammar.
This flag does not gate existing URI-domain or direct-ACL compatibility and does not change cross-domain grants. A mapping into a configured cross-domain domain continues to produce its existing
superrole regardless of the new flag.Privilege scope: legacy app names do not distinguish products or tags. Enable this only when all eligible trusted identities with that app name should have full superuser privileges. Treat it as a startup setting and restart servers or reconnect clients when changing it; it is not an immediate revocation switch for already authenticated connections.
No workflow, dependency, quorum-TLS, or unrelated refactoring changes are included.
Tests
ZooKeeperServer.checkACLand both X509 providers' matching pathsThe mapping fixture uses the existing temporary-directory helper to isolate its test database.
Latest focused run:
mvn -q -pl zookeeper-server -am \ -Dtest=X509DirectAclTest,X509AuthTest,X509AuthFailureTest,X509SpiffeAuthIntegrationTest,ZkClientUriDomainMappingHelperTest,X509ZNodeGroupAclProviderTest,AuthUtilTest,ClientSSLTest,SSLAuthTest \ -DfailIfNoTests=false -Dsurefire-forkcount=1 testResults: 96 tests passed (19 + 10 + 1 + 19 + 23 + 14 + 2 + 6 + 2), with no failures, errors, or skipped tests. The single-fork setting avoids the existing provider fixture's intermittent parallel port-binding conflicts.
Known inherited gaps
The broader review reproduced two behaviors in code inherited from the base: deleting the last nested mapping leaf can cause its parent to become a broader prefix grant, and the basic X509 provider's DN-only ACL validator rejects explicit non-DN ACL authoring (including bare names, service principals, and application paths) even though existing ACL matching succeeds. The passing suites are not evidence that these cases are fixed. This focused compatibility change does not resolve either issue; they remain rollout considerations.
Changes that Break Backward Compatibility (Optional)
Both public
getClientId(...)overloads returnClientIdentityrather thanString. In-repository callers are updated; external callers must recompile and use.getId()for the previous string result.The connection-aware
matches(...)method is a default interface overload, preserving existing authentication-provider implementations. String-only domain lookup retains its original behavior.The intended authorization change is one-way compatibility from eligible SPIFFE application identities to equivalent bare or formatted legacy application names. Certificate validation, raw extracted IDs, stored AuthInfo/ACL formats, normal ACL permission checks, and quorum configuration are unchanged. Superuser-ID compatibility is separately opt-in; existing superuser settings and exact matching remain supported. Original pre-PR legacy-client matching is preserved; the reverse alias introduced earlier in this PR is not retained.
For the domain provider, direct principal ACL compatibility applies when the original client ID is present in AuthInfo under the existing configuration; domain-only AuthInfo is not promoted to a principal.
Documentation (Optional)
The typed identity result, lookup order, application-name requirements, direct-ACL rules, and compatibility boundaries are documented above and in the implementation. The new superuser option, its scope, marker semantics, and activation caveats are documented in
zookeeperAdmin.md.Generated with GitHub Copilot CLI