OIDC: Add bearer token step up authentication

[michalvavrik]

• closes: Support for OAuth2 Step-up authentication challenge protocol #46213

[github-actions]

🙈 The PR is closed and the preview is expired.

[sberyozkin]

Hi @michalvavrik, thanks very much for agreeing to rework your original PR, it is close to being merged.

I propose a couple of minor updates: always require setting ACRs with @AuthenticationContext, and for now, leave ID tokens out of it.
For the tests, I'd propose to add either another integration-tests/oidc tenant test or add extensions/oidc/deployment/ test. Keycloak supports ACRs AFAIK, though testing it can be hard, so may be add another wiremock step up authentication. I'd would also be good to confirm it works when a custom Jose4 Validator throws the exception.

Have a look during the next couple of weeks please.

Thanks

[michalvavrik]

I'd would also be good to confirm it works when a custom Jose4 Validator throws the exception.

This can't work with custom Jose4 Validator. How would you propagate information that acr value was wrong? Jose4 throws org.jose4j.jwt.consumer.InvalidJwtException and doesn't set cause, so unless we are going to parse exception message, there is nothing we can do. I played with it for some time before I opened this PR.

[sberyozkin]

@michalvavrik What if the user throws an exception from the validator, either InvalidJwtException with the cause or AuthenticationException directly ? I'm just pretty sure that once we offer an annotation based solution, someone will ask, well, we have a dynamic situation, impossible to capture it with the annotations. The required-claims PR is actually related as well, where users may want to set it in properties

[michalvavrik]

@michalvavrik What if the user throws an exception from the validator, either InvalidJwtException with the cause or AuthenticationException directly ? I'm just pretty sure that once we offer an annotation based solution, someone will ask, well, we have a dynamic situation, impossible to capture it with the annotations.

That is not possible either, please have a look here https://bitbucket.org/b_c/jose4j/src/756257eb92352600d5dde08c2b8ed25db13a8952/src/main/java/org/jose4j/jwt/consumer/JwtConsumer.java#lines-447. Users may want it, but that's how Jose library is implemented. I think the only thing that would work is to throw Error or even Throwable but it feels nasty.

The required-claims PR is actually related as well, where users may want to set it in properties

That's not the same, it is our own custom validator, yes, the exception is going to be silenced, but as we own the custom validator, we can just propagate the exception as field or whatever. It won't be nice, but it is not the same situation and is solvable.

[sberyozkin]

Yeah, you are right. Given that Jose4j accumulates results from all the validators it can be tricky to make it right.

Hmm, but our custom validator is run as part of that Jose4j Validators iteration.

I think in the short term we can just say, if you want to use Jose4j Validator such that a Step Up authentication protocol is initiated, then do throw new MalformedClaimException("acr:acr1,acr2"); naming acr as a malformed exception and using : to list required acrs - our own validator will do it if it is acrs as well.

But sure, let's not worry about it now, we can follow up with another PR for an annotation alternative later

[sberyozkin]

Hi Michal @michalvavrik, thanks for working hard on this enhancement, I've added quite a big number of comments but they are all mostly cosmetic, not major suggestions, primarily in the docs, with only a few in the code. IMHO it is getting closer, thanks for your patience.
Thanks

[sberyozkin]

Hi Michal @michalvavrik, I think the Bearer access token related part is complete, thanks.

There is one more thing that I believe we should deal with before merging.

I believe we need to have some suggestion to users how they can enforce Step Up authentication without using annotations. Your PR to support the required claims as arrays was merged, so may be this PR can be rebased on top of it and we can try come up with some hack as suggested earlier to have the error propagated from our own custom Jose4j validator when acrs are not available ? May be just using Throwable which might do for the internal code, or try a suggested MalformedClaimException hack.

[michalvavrik]

I believe we need to have some suggestion to users how they can enforce Step Up authentication without using annotations. Your PR to support the required claims as arrays was merged, so may be this PR can be rebased on top of it and we can try come up with some hack as suggested earlier to have the error propagated from our own custom Jose4j validator when acrs are not available ? May be just using Throwable which might do for the internal code, or try a suggested MalformedClaimException hack.

I'll take care of it, sure, I believe there is a way to do it.

[sberyozkin]

@michalvavrik Perhaps, we can do

public class AuthenticationContextException extends Throwable {
    public AuthenticationContextException(String acr) {
    }
    public AuthenticationContextException(Set<String> acrs) {
    }
}

and throw it from our own Validator if the required claims map does not have it but also ask users to do it from their custom Validators when these checks may be more dynamic for a given tenant ? We can then convert it to AuthenticationFailedException ?

[michalvavrik]

@sberyozkin we can go to that, but I'll first present you with my solution, I am already done, just writing tests.

[sberyozkin]

@michalvavrik Sure

[sberyozkin]

Perfect, thanks @michalvavrik

[sberyozkin]

@michalvavrik Once this is released, please consider experimenting with some custom SPA which will do Keycloak login, be able to access some Quarkus endpoint and then be required to step up once it tries to access some path which requires a stronger authentication. It should be interesting to blog about

[sberyozkin]

@michalvavrik Hi Michal this test is a bit flaky right now, please have a look

[michalvavrik]

@michalvavrik Hi Michal this test is a bit flaky right now, please have a look

Basically, it behaves differently depending on the order in which tests are executed and passes on the second attempt because it is the only test run. The issue is not in mechanism added in this PR.

I think I have fixed it by adding "reset" for the simple OIDC server, let's see.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 735a043.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 735a043.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.