JWT federated client authentication #3219
Conversation
* WIP: JWT federated client authentication * Implement JWT credential * Add documentation
There was a problem hiding this comment.
First of all, great work, this is a complicated topic. I had a hard time wrapping my head around it 🤯
My main issue is that this JWT authentication + grant type is complex and hard to understand. There are three things handled in the same place (OAuth2 jwt-bearer grant type + OAuth2 client authentication + OIDC client authentication). It makes it really hard to understand what should happen and when.
[suggestion] While we are touching this, would be super helpful to decompose the whole flow a bit rather than add more code in the same place. Maybe have multiple ClientJwtAuthentication classes or methods that map to a specific flow with a link to the spec.
I don't have an easy suggestion of what should be done but we should at least consider it.
I also left a few suggestions (use assertJ, a typo here and there, suggestions for method names), some are easy fixes and some are just non-blocking suggestions.
model/src/main/java/org/cloudfoundry/identity/uaa/oauth/client/ClientJwtCredential.java
Outdated
Show resolved
Hide resolved
model/src/test/java/org/cloudfoundry/identity/uaa/oauth/client/ClientJwtCredentialTest.java
Outdated
Show resolved
Hide resolved
| @@ -71,10 +71,10 @@ protected void additionalAuthenticationChecks(UserDetails userDetails, UsernameP | |||
| setAuthenticationMethod(authentication, CLIENT_AUTH_NONE); | |||
| break; | |||
| } else if (isPrivateKeyJwt(authentication.getDetails())) { | |||
| setAuthenticationMethod(authentication, CLIENT_AUTH_PRIVATE_KEY_JWT); | |||
There was a problem hiding this comment.
[question, non-blocking] Is the order important here? If so, why?
uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/clients/ClientAdminEndpointDocs.java
Outdated
Show resolved
Hide resolved
uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/clients/ClientAdminEndpointDocs.java
Outdated
Show resolved
Hide resolved
| if (!clientId.equals(getClientId(clientAssertion))) { | ||
| JWT clientJWT = parseClientAssertion(clientAssertion); | ||
| JWTClaimsSet clientClaims = parseClientJWT(clientJWT); | ||
| if (!clientId.equals(getClientId(clientClaims))) { |
There was a problem hiding this comment.
[suggestion] Worth clarifying that this is for "authorization grant" case of the OAuth2 flow, where the sub is the subject of the user who got the token, not the client ID.
...java/org/cloudfoundry/identity/uaa/audit/JdbcUnsuccessfulLoginCountingAuditServiceTests.java
Outdated
Show resolved
Hide resolved
| @@ -37,7 +37,7 @@ void log_format_whenThereIsAnAuthType() { | |||
| ArgumentCaptor<String> stringCaptor = ArgumentCaptor.forClass(String.class); | |||
| verify(mockLogger).info(stringCaptor.capture()); | |||
| String logMessage = stringCaptor.getValue(); | |||
| assertThat(logMessage, is("PasswordChangeFailure ('theData'): principal=thePrincipalId, origin=[theOrigin], identityZoneId=[theZoneId], authenticationType=[theAuthType]")); | |||
| assertThat(logMessage, is("PasswordChangeFailure ('theData'): principal=thePrincipalId, origin=[theOrigin], identityZoneId=[theZoneId], authenticationType=[theAuthType], detailedDescription=[theDescription]")); | |||
There was a problem hiding this comment.
[suggestion] use assertJ instead, see other comments - there will probably be conflicts here with gh-3195
| map.put("authorities", "uaa.none"); | ||
| map.put("redirect-uri", "http://localhost/callback"); | ||
| map.put("jwt_creds", "[{\"iss\":\"http://localhost:8080/uaa/oauth/token\",\"sub\":\"foo-jwt\"}]"); | ||
| UaaClientDetails clientDetails = (UaaClientDetails) doSimpleTest(map, clientAdminBootstrap, multitenantJdbcClientDetailsService, clients); |
There was a problem hiding this comment.
[thought, non-blocking] doSimpleTest is a questionable name 😅
| @@ -137,35 +150,121 @@ public boolean validateClientJwt(Map<String, String[]> requestParameters, Client | |||
| if (GRANT_TYPE.equals(UaaStringUtils.getSafeParameterValue(requestParameters.get(CLIENT_ASSERTION_TYPE)))) { | |||
There was a problem hiding this comment.
[suggestion] noticed that this is NOT the grant_type as per spec, it's the client_assertion_type ; the grant type exist and is defined in TokenConstants#GRANT_TYPE_JWT_BEARER. We should probably use this PR to rename this parameter, maybe CLIENT_ASSERTION_TYPE_JWT_BEARER or something similar.
Support integration of so called workload identities
Allow client authentication based on RFC 7523, see https://oauth.net/private-key-jwt/
Scenario: Integrate JWT from another systems to authentication within UAA / CF landscape.
e.g. github action token like https://docs.github.com/en/actions/security-for-github-actions/security-hardening-your-deployments/about-security-hardening-with-openid-connect#understanding-the-oidc-token