SAP / cloud-security-services-integration-library

Integration libraries and samples for authenticating users and clients bound to XSUAA authentication and authorization service or Identity authentication service.
Apache License 2.0
151 stars 136 forks source link

JWT validation pattern with `jku` and `kid` claims #242

Closed artemkovalyov closed 4 years ago

artemkovalyov commented 4 years ago

Hi @nenaraab,

In SAP Cloud SDK, we're integrating your library to handle XSUAA authentication topics. Currently, we're fixing JWT validation that fails because of this change: tenant specific JWT signing keys

In regard to this, I have a couple of questions to ensure our implementation is secure and compliant:

Thank you in advance for your help! I hope this makes sense. cc @MatKuhr @newtork

Best, Artem

nenaraab commented 4 years ago

Hi @artemkovalyov ,

thanks for your request! You questions make perfectly sense!

Does this answer your questions? Nena

artemkovalyov commented 4 years ago

Thank you for the quick reply and detailed answer, @nenaraab, it clarifies a lot! Let me a bit of time to regroup with my colleagues and see if we might have any follow-up questions about potential edge cases.

MatKuhr commented 4 years ago

Thank you, some follow up questions from my side:

nenaraab commented 4 years ago
MatKuhr commented 4 years ago

Thank you so much for your help! We were pointed towards this post which indicates that reading the token keys from the jku is mandatory in a multi tenant setup. Together with the information you provided above I believe the following behaviour would be correct and are implemented by the XSUAA Java library:

  1. If the jku property is present AND it points towards a resource on the XSUAA domain -> we try to fetch the token keys from there.
    1. The results are cached, isolated by tenant
    2. If there is a kid present then it is used to select the correct public key
  2. Otherwise we use the oidc endpoint

As Artem said we are replacing our own implementation for JWT validation with the XSUAA Java library and need to understand possible changes in behaviour so that we can communicate that to our consumers clearly.

nenaraab commented 4 years ago

Hi,

Almost correct. Currently, there is a restriction to Option 1 for tokens issued by XSUAA (also because it provides the jku). See also here: https://github.com/SAP/cloud-security-xsuaa-integration/blob/master/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtSignatureValidator.java#L108

Best regards, Nena

MatKuhr commented 4 years ago

Thank you so Nena much for the detailed information, that helped us a lot.

With that we were able to integrate the XSUAA library to perform JWT validation and first consumers reported that it works, so I think we can close this issue. Thanks again for the great & fast help 👍

nenaraab commented 4 years ago

Okay, cool. Thanks. Happy to join for a code review if you like.

MatKuhr commented 4 years ago

Thanks, that would actually be great.

We currently use the library like this:

final Token token = new XsuaaToken(encodedJwt);
final OAuth2ServiceConfiguration xsuaaConfiguration = Environments.getCurrent().getXsuaaConfiguration();

if( xsuaaConfiguration == null ) {
    throw new AuthTokenAccessException("Unable to fetch XSUAA configuration from VCAP services.");
}
final CombiningValidator<Token> validator = JwtValidatorBuilder.getInstance(xsuaaConfiguration).build();

final ValidationResult result = validator.validate(token);
if( result.isValid() ) {
    return JWT.decode(encodedJwt);
}
throw new AuthTokenAccessException("The token is invalid: " + result.getErrorDescription());

In case this validation fails we fall back to our "old" implementation and try again. That way our own mocking still works and nothing can break for the consumer.

So far one of our consumers confirmed that this works. Next steps would probably be leveraging the mocking capabilites of the XSUAA lib to better test this and also to deprecate our custom implementation.

nenaraab commented 4 years ago

cool. Perfect and good migration idea :-)