Support for OIDC SPIFFE Client Authentication#53773
Support for OIDC SPIFFE Client Authentication#53773sberyozkin wants to merge 1 commit intoquarkusio:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
8998167 to
1e43f3b
Compare
|
I forgot to add a test resource, it caused the compilation failure. |
Status for workflow
|
|
🎊 PR Preview 7db3de4 has been successfully built and deployed to https://quarkus-pr-main-53773-preview.surge.sh/version/main/guides/
|
Status for workflow
|
|
I'll review sometimes tomorrow. |
| } | ||
| if (clientAssertion == null) { | ||
| String errorMessage = String.format( | ||
| "%s OidcClient can not complete the %s grant request because a JWT bearer client_assertion is missing", |
There was a problem hiding this comment.
this error message cannot say only "JWT bearer"
| final String clientAssertion = clientAssertionProvider.getClientAssertion(); | ||
| if (clientAssertion == null) { | ||
| throw new OIDCException(String.format( | ||
| "Cannot get token for tenant '%s' because a JWT bearer client_assertion is not available", |
There was a problem hiding this comment.
now it is not just a JWT bearer, hence this err msg should be updated
| this.responseFilters = responseFilters; | ||
| this.clientSecretBasicAuthScheme = clientCredentials.clientSecretBasicAuthScheme; | ||
| this.jwtBearerAuthentication = oidcClientConfig.credentials().jwt().source() == Source.BEARER; | ||
| this.jwtBearerAuthentication = oidcClientConfig.credentials().jwt().source() != Source.CLIENT; |
There was a problem hiding this comment.
you are free to keep it as is, but if you can find a better name that reflects it is not only the jwt bearer auth, better
|
|
||
| /** | ||
| * Path to a file with a JWT bearer token that should be used as a client assertion. | ||
| * This path can only be set when JWT source ({@link #source()}) is set to {@link Source#BEARER}. |
| } | ||
|
|
||
| conf.put("code-flow-jwt-bearer-token-path", jwtBearerTokenPath.toString()); | ||
| conf.put("code-flow-jwt-bearer-token", jwtBearerToken); |
There was a problem hiding this comment.
I didn't find this used anywhere?
| conf.put("code-flow-jwt-bearer-token-path", jwtBearerTokenPath.toString()); | ||
| conf.put("code-flow-jwt-bearer-token", jwtBearerToken); | ||
| conf.put("code-flow-spiffe-token-path", spiffeSvidTokenPath.toString()); | ||
| conf.put("code-flow-spiffe-token", spiffeSvidToken); |
There was a problem hiding this comment.
I also didn't find this to be used anywhere?
| private static void verifySpiffeId(JsonObject claims) { | ||
| String sub = claims != null ? claims.getString(Claims.sub.name()) : null; | ||
| if (sub == null || !sub.startsWith(SPIFFE_ID_SCHEME)) { | ||
| throw new IllegalStateException( |
There was a problem hiding this comment.
I think I had a good reason why I didn't want to throw an exception. Now I didn't verify this today, but I think this will be silently (or not silently?) swallowed and the old assertion will be kept. That is not what we want, we want it to be null, so that it is later considered io.quarkus.oidc.common.runtime.KubernetesServiceClientAssertionProvider#isInvalid (invalid)
There was a problem hiding this comment.
Hi @michalvavrik there is a test but I will check
There was a problem hiding this comment.
Hi @michalvavrik there is a test but I will check
so maybe it is considered as "expired", but my error log messages are logged. please check this exception is at least logged for user to know that something has happened, in which case it is fine
| private static final Logger LOG = Logger.getLogger(KubernetesServiceClientAssertionProvider.class); | ||
| private static final String SPIFFE_ID_SCHEME = "spiffe://"; | ||
| private final Vertx vertx; | ||
| private final Path bearerTokenPath; |
There was a problem hiding this comment.
that bearer doesn't really work? but I am fine with keeping it unless you know a better way?
There was a problem hiding this comment.
never mind. I meant to comment on "jwtBearerToken" variables which are definitely elsewhere. this is member property is fine.
There was a problem hiding this comment.
basically - we have a source which can be bearer or spiffe or .. and yes SPIFFE JWT-SVID is a JWT Bearer token, but it is also bit confusing to have a JWT bearer token marking both SPIFFE and BEARER.
but this is not a right place, let's keep it as is. thanks
There was a problem hiding this comment.
Np, thanks @michalvavrik, I'll have a look and see what can be renamed. May be we can even make this class abstract and have typical bearer and spiffe token extensions, may be later
michalvavrik
left a comment
There was a problem hiding this comment.
I only run quickly through it + asked Claude to check it. I'll read it properly again when you address my comments. I already did a lot of unrelated reading today, so I need to check tonight. But I presume it will be fine. Thanks
|
Thanks @michalvavrik , I'll have a look |
| } | ||
| ---- | ||
|
|
||
| ==== SPIFFE SVID |
There was a problem hiding this comment.
Are you certain that we should be using "SPIFFE SVID"? The way I see it the SVID document can be in JWT or x509 certificate format and you are only talking in this section about JWT-SVID. I tried to look it up and "SVID token" is almost never used. Only in https://spiffe.io/docs/latest/spiffe-specs/jwt-svid/ it says "token-based SVID".
The same term "SPIFFE SVID token" is used below as well. Same question for these occurrences.
michalvavrik
left a comment
There was a problem hiding this comment.
I checked again, looks fine. Let's address at least some of my comments and then we can ask George to review.

Fixes #52232
This PR adds support for using SPIFFE JWT tokens for the client authentication between Quarkus OIDC and providers such as Keycloak.
It builds upon the work done by @michalvavrik, with the demo work from @sabre1041 and @maia-iyer at https://github.com/sabre1041/keycloakcon-spiffe/ clarifying a typical pattern for using such tokens for the client authentication.
This PR will fix the linked issue.
As far as X509 client authentication is concerned, it is expected to work out of the box where the OIDC client certificate is located in the file, though I think we'll need to figure out how to handle the dynamism associated with such certificates being regularly refreshed. It will be another enhancement request