TCK Challenge: Tests using URL location make a too restrictive assumption

[tomas-langer]

Tests

1. PublicKeyAsPEMLocationURLTest
2. PublicKeyAsJWKLocationURLTest
   Make an assumption that the application is deployed at the time the JWT handling is configured.
   If an implementation configures the JWT Auth handling at startup (as we do) and the key is not available at that time, the startup fails and the application never gets deployed.
   The specification does not define WHEN the URL should be reached, whether it can/should be cached etc., so the implementation may choose.
   As we follow the "fail fast" principle, we fail if the configuration cannot be completed, thus never deploying the application. As a result these TCK tests cannot be executed, even though the implementation is valid and according to the specification.

[radcortez]

+1

[radcortez]

My understanding on the spec is that the first load / key validation needs to happen at deploy time. From the spec:

MicroProfile JWT implementations are required to throw a DeploymentException if the JWK
kty field is missing or JSON text is found, but does not follow either JWK or JWKS
format.

It doesn't make sense to throw a DeploymentException if you load the key after the application is deployed.

Anyway, on TomEE we have the same issue. If the key is part of the deployment, we are unable to reach an endpoint before the application is completely deployed. After that point, as I said, it doesn't make sense to throw a DeploymentException.

Also, I believe on a proper usage scenario, you will not be exposing the JWK Key on the same application that needs it. So, on the TCK, I think we can easily fix this by adding another Arquillian deployment to expose the key and have the main deployment call it to run the test. Here is how we have done it:
apache/tomee@4dee78b#diff-7855d23518772909eb5be87d68093b0b

I'm happy to submit a PR to the TCK to achieve the same result.

[starksm64]

Yes, agreed.

@radcortez Sure, create a PR to fix the test.

[sberyozkin]

@radcortez Hey Roberto, so please do this PR :-)

[radcortez]

Ops, sorry. I guess I've forgotten about this. I'll put this on my priority list :)

[radcortez]

Ok, here is a fix for it:
#148

On the other hand, I've started to think about this and I'm not completely sure if this should be the right behaviour from a spec point of view.

While it makes sense to load, validate the key if you have the key locally (file, classpath, etc) and fail the deployment if we are not able to validate, I'm not so sure about this when you load the key from an external resource. It just means that we are adding a hard dependency to the external service on your application startup (it may be down when the application is starting, but up when it actually needs the key). 

Plus, since there is no definition on when the key should be loaded (implementation can choose), if you defer to be loaded when you actually needed, it doesn't make sense to fail with a DeploymentException.

Maybe the spec should be clarified and specify that implementors can choose when the key is loaded. If it is loaded at the application startup and fails validation you throw DeploymentException. If loaded on demand (or after application startup) it fails with some other exception?

[rdebusscher]

Although the key is only required when validation is required, checking the 'store' at deployment make sense, even if it is an external resource.

When application is deployed, it is ready to accept requests and thus key needs to be accessible for validation.
So if the remote resource is not available at deployment, it will likely not be available when the first request arrives a fraction of a second later.

So early check is better

[sberyozkin]

@radcortez @rdebusscher IMHO it should be left up to the given container at what point the external key is available. Given the concerns of the hard dependency expressed by Roberto. We had to add a connection timeout in Quarkus for the OIDC adapter to wait till the OIDC server is up and I would like to avoid enforcing the same on the pure smallrye-jwt implementations.
And with HTTPs JWKs and the whole key rotation idea there is no guarantee the validation key will be available at the deployment time anyway.

Maybe the spec should be clarified and specify that implementors can choose when the key is loaded. If it is loaded at the application startup and fails validation you throw DeploymentException. If loaded on demand (or after application startup) it fails with some other exception?

Sounds good to me. UnresolvableKeyException ? (Jose4j has the same exception name and it fits the problem well)

[radcortez]

@rdebusscher It highly depends on your system. I think we cannot make that assumption. Just because I'm deploying an application it doesn't mean I'll be receiving request a few fractions of a seconds later. I could be starting nodes to warm them up and I can have rules to enforce dependencies once I'm ready to receive requests and add the app to a load balancer / expose to the actual clients.

What about endpoints that don't require JWT at all? I'm failing my entire deployment and have nothing available because there might be a network glitch and I'm unable to reach my OIDC server.

@sberyozkin I like the UnresolvableKeyException. How about if we relax the constraints a little bit and don't fail the application deployment and just throw the UnresolvableKeyException and a 401? It would be very explicit for the developer what is going on, it would remove the hard dependency and the app could still work.

[rdebusscher]

I can have rules to enforce dependencies once I'm ready to receive requests

So far the myth of microServices which are independent.

The UnresolvableKeyException is then the only option.

[sberyozkin]

Hi @rdebusscher I'd like to clarify that there is no way I'm going to push our own implementation agenda no matter what, I'll try my best anyway :-) and I look forward to discussing the issues with yourself and others..
But returning to what Roberto suggested, I was thinking yesterday after I signed off, and I came to the conclusion it was really MP JWT going a bit too far in trying to enforce at what point and when the keys have to be resolved. MP JWT is about JWT and RBAC and how to validate JWT but not about the key management.
What you said makes sense in that (in most cases) the keys would already be available. But then in our case we load the key and cache it on the 1st request - but I think it is equally valid. Of course it would not be a problem to get most of it loaded at the deployment time but the ambiguity would remain. It really should be up to the application when to load a key of a given type, statically or dynamically.
Also having a DeploymentException only as requirement would likely be a problem with the JWE decryption likely to be done for 2.0 since it would imply caching a private key which may not be good for some cases...
As such, I'd personally favor introducing UnresolvableKeyException and drop a requirement to throw DeploymentException or clarify that if the former is thrown at the deployment time then the cause of the latter must be UnresolvableKeyException.
I feel it is not a 1.2 change though. I'll update the milestone to JWT 2.0

Thanks all

[radcortez]

As such, I'd personally favor introducing UnresolvableKeyException and drop a requirement to throw DeploymentException or clarify that if the former is thrown at the deployment time then the cause of the latter must be UnresolvableKeyException.

+1

[rdebusscher]

If there is a need from the users that failure should only be reported when the endpoints are used, that we should allow for it.

But deployment failure is much easier to diagnose and act upon than an exception for a user request which returns a 401 (and is lost in the logs). So basically, users are then shifting their problems to the end-users.

[sberyozkin]

@rdebusscher A case of the bad log management is not a good reason IMHO not to support the dynamic key acquisition. IMHO there is a good compromise which is being proposed which also works for some key sets which are dynamic in nature

[radcortez]

BTW, my proposal was to throw the exception UnresolvableKeyException anyway so you can see it in the logs, but send the 401 to the user so he doesn't get the exception.

Yes, I know this might be not the correct behaviour, since 4xx codes are usually things that the user got wrong and can correct with a new request. On the other hand, if we think about a user getting wrong credentials, there is nothing he can do to fix the issue, other than having support from the app team. In this case, is similar.

[sberyozkin]

Hey, in the end of the day there is a very practical issue is that the HTTPs based location may only be available few seconds later after the MP JWT server starts, and IMHO we just should not introduce this uncertainty into the spec. I appreciate @rdebusscher's concern, but we can try to figure out how to make things obvious. Perhaps indeed for 2.0 we just always throw UnresolvedKeyException. And discuss if it is really 401 or 500 that should be returned if it happens after the deployment has completed - the key unavailability is a server error unless it was caused by the token kid mismatch which would indeed by a client issue.

I'll create a separate issue for JWT 2.0 it once we resolve the TCK issue (with @radcortez's PR )

[radcortez]

Sure.

[tomas-langer]

What about removing the TCK tests I mentioned, and just leave this decision to the implementation?
As was mentioned before - the spec should describe how to validate authentication and authorization when accessing resources.
The key management itself can be vendor specific.

If you still want to use an URL to retrieve the keys, could you please handle it using WireMock or some other way that would be part of the TCK harness?
Adding a second deployment will not work with implementations that do not support deployment at all. Also we should not be testing the capability to deploy a second application, but the capability to load a key from a URL...