Proposal for AuthN/Z API Refactor#119
Conversation
|
Hey Tom, thanks for the mention. I can confirm the statements about Apicurio Registry are factually correct. For our fine-grained per-resource authorization (Apicurio/apicurio-registry#7724) draft work (PR #7829 (Apicurio/apicurio-registry#7829)), we evaluated the Kroxylicious Authorizer API and found the interface design to be exactly what we needed, with the Authorizer, AuthorizeResult, Action, Decision, ResourceType all fit our use case well. However, the transitive dependency on kroxylicious-api (and through it kafka-clients, Jackson, compression codecs) was a non-starter for us, since our authz module is meant to be a lightweight, zero-dependency library that can be consumed by Registry and potentially other projects. We ended up copying the authorizer API source into our own io.apicurio.authz module and stripping out the dependent code. It works, but it means we're now maintaining a fork of interfaces that could be shared. Our ADR explicitly notes: "If the Kroxylicious team extracts their API, migrating is mechanical." This proposal does exactly that. With kroxylicious-authentication-api as a standalone module, we could drop our copied interfaces and depend on the real thing. +1 from the Apicurio Registry side. |
| @@ -1,2 +1,3 @@ | |||
| .idea | |||
| .claude/settings.local.json | |||
| *.swp | |||
There was a problem hiding this comment.
For those lonely few who still use the one-true-editor.
There was a problem hiding this comment.
Quit it! Oh wait you can't... 😜
|
Thanks for writing this up, Tom — the core motivation is solid and the proposal is thorough. I've left some initial inline comments but I'm out of time for a full review today. I'll come back with further feedback. |
|
|
||
| ### Use of a new package | ||
|
|
||
| The new types live in `io.kroxylicious.authentication`, not `io.kroxylicious.proxy.authentication`. |
There was a problem hiding this comment.
The rationale for avoiding a split package is sound. But `io.kroxylicious.authentication` is a very broad package name for a module that currently contains two interfaces and an annotation.
If the Kroxylicious project later introduces other authentication-related types — say, credential providers, token validators, or authentication flow abstractions — the natural home for those would also be `io.kroxylicious.authentication`. If they land in a different module, we're back to a split package. If they're forced into this module to avoid the split, the "minimal, zero-dependency" property is lost.
Does the package need a narrower name that more precisely reflects what's in this module — the identity model rather than authentication as a whole? Something like `io.kroxylicious.authentication.identity` or `io.kroxylicious.identity` would leave room for other authentication concerns to live in sibling packages without splitting.
There was a problem hiding this comment.
I agree with you that we need a more focused name for the package. io.kroxylicious.identity seems the cleanest. The module could then be called kroxylicious-identity-api and remain standalone if other authentication related types are introduced in future that the authorizer API does not strictly need.
| - `<exclude>` entries for: the removed `Principal` class, the removed `Unique` annotation, the removed `Subject` class (renamed to `ProxySubject`), the changed `PrincipalFactory#newPrincipal` return type, and the changed method signatures in `TransportSubjectBuilder#buildTransportSubject`, `SaslSubjectBuilder#buildSaslSubject`, `FilterContext#clientSaslAuthenticationSuccess`, `FilterContext#authenticatedSubject`, and `RouterContext#authenticatedSubject`. | ||
| - An `<ignoreMissingClassesByRegularExpressions>` entry for `io.kroxylicious.proxy.authentication.Principal`, because `japicmp` cannot resolve old bytecode signatures that reference the deleted class without this. | ||
|
|
||
| - The concrete `ProxySubject` record retains all its existing behaviour, including the `User`-principal validation in its constructor, and its `uniquePrincipalOfType`, `allPrincipalsOfType`, and `isAnonymous` methods. |
There was a problem hiding this comment.
The proposal keeps uniquePrincipalOfType, allPrincipalsOfType, isAnonymous, and anonymous() on the concrete ProxySubject, treating them as proxy-specific. But looking at the Apicurio PoC (apicurio-registry#7829), they copied our Subject and kept these same methods — principalOfType, isAnonymous, anonymous() — because they need them in their own GrantsAuthorizer to extract a username from a Subject.
That suggests these methods are fundamental to working with Subject, not proxy-specific behaviour. If the primary external consumer of this API needs them, they belong on the Subject interface in the new module.
The rename to ProxySubject is still right — having a Subject interface and a Subject class would be painful regardless of package. But if the convenience methods move to the interface, ProxySubject doesn't need to appear in the signatures of FilterContext.authenticatedSubject(), RouterContext.authenticatedSubject(), etc. — those could return Subject (the interface). ProxySubject becomes an internal implementation detail rather than a type that leaks across the whole proxy API surface, and the blast radius of the rename shrinks from "every module that touches authentication" to "just the places that construct a ProxySubject".
There was a problem hiding this comment.
You make a good point. I wanted to keep the interface as simple as possible. However, given that Apicurio are actually using those methods and how moving them to the interface would actually reduce the changes to the rest of the codebase I think it would make sense to move them. This would mean that @Unique would have to move to the package aswell.
I will update the proposal.
There was a problem hiding this comment.
Thinking about the @unique tension between this thread and the earlier one: @unique's javadoc is already framed entirely in terms of Principal and Subject — it's not actually generic, just generically named. If we rename it to something like @UniqueIdentity it belongs naturally in the identity module and the T2 concern goes away. @unique in kroxylicious-api doesn't need to exist.
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Thomas Cooper <code@tomcooper.dev>
Signed-off-by: Thomas Cooper <code@tomcooper.dev>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Thomas Cooper <code@tomcooper.dev>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Thomas Cooper <code@tomcooper.dev>
f998f00 to
93fa410
Compare
|
The downstream changes section says More broadly: is |
| This triggers Maven's analyzer. | ||
| Because several API surfaces (e.g. `FilterContext.authenticatedSubject()`) directly reference `io.kroxylicious.identity.Subject`, some modules will have genuine bytecode references that make the dependency required rather than a false positive. The exact set of modules needing an `ignoredNonTestScopedDependencies` override will be determined during implementation. | ||
|
|
||
| - **Dependency enforcer allowlists**: `kroxylicious-identity-api` must be added to `bannedDependencies` allowlists in the `kroxylicious-filters`, `kroxylicious-kms-providers`, and `kroxylicious-kubernetes` parent POMs. |
There was a problem hiding this comment.
Nit: I found this paragraph hard to follow. It describes a false positive (bytecode doesn't reference identity-api types directly, but needs them for super-interface resolution), then says some modules will have genuine bytecode references.
But for modules that only use ProxySubject — do they actually need to declare kroxylicious-identity-api at all? It'll be a transitive compile-scope dependency of kroxylicious-api (because ProxySubject implements Subject). If they don't declare it explicitly, the analyzer has nothing to complain about. And if they do declare it unnecessarily, the analyzer is right to flag it — that's not a false positive.
I think this section needs reworking to clarify which modules actually need an explicit dependency and why.
|
I've been thinking about the breaking change story here. If this only broke the authorizer API, I'd be comfortable with a clean break — the adoption is minimal and the case is well made. But the change leaks into The rejected alternative ("extract concrete types into the new module") correctly identifies the split-package problem with keeping the same package across two modules. But there's a third option the proposal doesn't consider: deprecate and migrate. This doesn't require a split package because the old types stay where they are:
The authorizer API gets its lightweight module and Apicurio gets what they need, without breaking the filter plugin contract. The cost is carrying deprecated bridge types for a release cycle and living with I think that trade-off is worth it to preserve FilterContext stability. |
| ## Current situation | ||
|
|
||
| The `Authorizer` interface and `AuthorizeResult` record in `kroxylicious-authorizer-api` reference `io.kroxylicious.proxy.authentication.Subject`, which is defined in `kroxylicious-api`. | ||
| This means any project that wants to implement the `Authorizer` plugin interface must depend on `kroxylicious-api`, which transitively pulls in `kafka-clients`, `jackson-annotations`, and compression codec libraries (`zstd-jni`, `lz4-java`, `snappy-java`). |
There was a problem hiding this comment.
aside: I expect that #116 will allow the kroxylicious-api' s dependency on kafka-clients to go away as it should eliminate the use of Kafka's ApiKeys, RequestHeaderData, ApiMessage etc.
That's not to say I don't appreciate that a dedicated -tightly defined- kroxylicious-identity-api may be more attractive for re-use.
| Introduce a new module, `kroxylicious-identity-api`, containing the `Subject` and `Principal` interfaces and the `@Unique` annotation. | ||
| The `Subject` interface defines one abstract method (`principals()`) and provides default convenience methods (`uniquePrincipalOfType`, `allPrincipalsOfType`, `isAnonymous`) and a static factory (`anonymous()`), giving any implementation full subject-querying capability without additional dependencies. | ||
|
|
||
| The existing concrete `Subject` record in `kroxylicious-api` is renamed to `ProxySubject` and implements the new `Subject` interface. |
There was a problem hiding this comment.
Could we avoid the need to rename io.kroxylicious.proxy.authentication.Subject (and thus the Kroxylicious API change), by just having the existing record implement io.kroxylicious.identity.Subject? io.kroxylicious.proxy.authentication.Principal would need to extend io.kroxylicious.identity.Principal
kroxylicious-api would need to depend on kroxylicious-identity-api
Edit: Ah - it is the annotations that make this awkward. Annotations can't extend annotations so we couldn't have io.kroxylicious.proxy.authentication.Unique extend io.kroxylicious.identity.Unique
There was a problem hiding this comment.
I think I'm basically suggesting the same thing as #119 (comment)
|
Thanks for the proposal @tomncooper. @SamBarker you are right to be concerned about the compatibility story. However, the new
I wondered if we could plan to use an interface (let's call it something else than |
|
@tombentley thanks for thinking through the compatibility story — your concerns about On My main concern however is with the Full analysis here if you want the detailed walkthrough. |
Not sure what exactly you mean by query time, but I don't think it's a "proxy construction choice" at all. I think it's an aspect of a particular
Where did
That's intentional and specifically deferred to a major version, where breaking changes don't need to be clean. The benefit is that the end state is much cleaner than having a pointless, and difficult to understand interface lingering in the API (and being confusing by its existence) forever. |
|
FWIW, since I had the plan anyway I thought I'd let Claude implement it and kroxylicious/kroxylicious#4408 is the result. |
This PR contains a proposal to extract the
SubjectandPrincipalconcepts fromkroxylicious-apiinto a new lightweight module,kroxylicious-authentication-api, so thatkroxylicious-authorizer-apican be consumed independently of the full proxy API and its transitive dependencies (kafka-clients, jackson-annotations, compression codecs).The authorizer API was designed for general-purpose reuse, but its dependency on
kroxylicious-apimakes adoption impractical for non-Kroxylicious projects. Apicurio Registry, for example, has resorted to copying the authorizer API source into their own module to avoid the dependency tree. This refactor removes that barrier.The proposed
kroxylicious-authentication-apimodule will contain three types in theio.kroxylicious.authenticationpackage:Principalinterface with a singlename()methodSubjectinterface with a singleprincipals()method@UniqueannotationA new package is proposed, rather than placing the interfaces in the existing
io.kroxylicious.proxy.authenticationpackage, as it signals their independence from the Kroxy internals and also because that would result in a split package and block future JPMS adoption.The existing concrete
Subjectrecord inkroxylicious-apiwill be renamed toProxySubjectand will implement the proposedSubjectinterface. This rename avoids ambiguity when both the interface and the concrete type are in scope.ProxySubjectwill retain all existing behavior includingUser-principal validation,uniquePrincipalOfType,allPrincipalsOfType, andisAnonymous.The
kroxylicious-authorizer-apimodule will switch its compile dependency fromkroxylicious-apitokroxylicious-authentication-api.Authorizer.authorize()andAuthorizeResultwill reference theSubjectinterface rather than the concrete type. Method signatures inFilterContext,RouterContext,TransportSubjectBuilder, andSaslSubjectBuilderwill be updated to useProxySubjectwhere the concrete type is required.These are binary-incompatible and in some cases source-incompatible changes. The project is at 0.x where breaking changes carry lower migration cost, and the authorizer API was introduced recently with minimal external adoption. Downstream changes will follow two mechanical patterns: renaming
SubjecttoProxySubjectand updatingPrincipal/@Uniqueimports fromio.kroxylicious.proxy.authenticationtoio.kroxylicious.authentication.A PoC branch containing these proposed changes is available on my fork: https://github.com/tomncooper/kroxylicious/tree/auth-api-refactor