Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NimbusReactiveJwtDecoder should accept a custom processor #5937

Closed
jzheaux opened this issue Oct 10, 2018 · 11 comments

Comments

@jzheaux
Copy link
Contributor

commented Oct 10, 2018

The Nimbus JwtDecoders, both servlet and reactive, simplify Nimbus configuration by exposing some basic use cases like using a JWK Set Uri or by using a single public key.

Many scenarios are more complex than this, and Nimbus is a full-featured API that allows users to configure for these more complex scenarios.

#5648 proposes addressing this on the servlet side by introducing a constructor that accepts a JWTProcessor instance.

Because of certain preprocessing that NimbusJwtReactiveDecoder does in order to maintain its non-blocking nature as it interacts with Nimbus, it is not yet clear how we might expose the JWTProcessor there; though coordinating with the Nimbus team may bear some fruit.

UPDATE: Creating a Converter from scratch that reactively exercises the Nimbus API can be a challenge:

Converter<SignedJWT, Mono<JWTClaimSet>> processor = // ... tricky business
NimbusReactiveJwtDecoder jwtDecoder = new NimbusReactiveJwtDecoder(processor);

So, as part of this ticket, let's also expose a builder for simply working with the keyset:

Function<JWT, Flux<JWK>> jwkSource = // ... reactive way to obtain key set
NimbusReactiveJwtDecoder jwtDecoder = 
        NimbusReactiveJwtDecoder.withJwkSource(jwkSource).build();
@GregoireW

This comment has been minimized.

Copy link

commented Jan 8, 2019

Hello,

I made an attempt to solve this: PR #6367

From the fact that:

  • JWTProcessor can depends on reactive result. We need something to provide those
  • JWTProcessor is only used to provides JwtClaimsSet

For me there is only two solution. Either the decoder take a function SignedJWT -> Mono<jwtClaimsSet> but it means that the decoder will do almost nothing.

Or the decoder take an optional option that provide the context (computed from the JWT header) so that we can call processor process method correctly.

I choose to write this code.

Now writing the nimbus reactive jwt decoder can be painful.
It is why I provides a "factory" class that can create the instance from the previous existing constructor (from RSA public key or JWKS url).

I'm convince that there could be a "technically" better solution, but I'm not sure it will better to use it.

@jzheaux

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

I'm not sure that exposing NimbusReactiveJwtDecoder's context-creating is a good idea. This is definitely a workaround for Nimbus lacking a non-blocking JWK retrieval API.

Ideally, Nimbus would add a non-blocking API, and perhaps there is an opportunity for the community to help with that.

Another possibility might be to introduce ReactiveJWTProcessor into Spring Security:

public interface ReactiveJWTProcessor<C extends SecurityContext> {
    Mono<JWTClaimSet> process(SignedJWT signedJWT, C context);
}

with a default implementation that performs the same workaround that NimbusReactiveJwtDecoder does today. Then, NimbusReactiveJwtDecoder could take ReactiveJWTProcessor as a constructor parameter.

It would help to know what particular problem you are trying to solve, @GregoireW. What is your specific need for passing in your own JWTProcessor to NimbusReactiveJwtDecoder?

@GregoireW

This comment has been minimized.

Copy link

commented Jan 8, 2019

I wrote in my previous message that I saw 2 way of solving the issue. First is like you say with the interface.
Note that the context is not required in this case as it is linked to the processor and will be hidden in the implementation (else we need to get a context from somewhere so it is like the implementation I wrote)

public interface ReactiveJWTProcessor {
    Mono<JWTClaimSet> process(SignedJWT signedJWT);
}

or the one I write in the PR with a processor and a initializer. In this use case, processor and initializer are coupled a little bit (not so great yes but ...)

I choose the processor + initializer only because

  • sometime initializer is not needed
  • I guess we can have a key selection business much more complex than the default code (what happens if I got a JWKS that reference RS256 key, then RS512 key then ECDSA-P256) followed by a simple processor.

But It is clearly a pure tail/head choice.
If you prefer an interface with

Mono process(SignedJWT signedJWT);

it is a simple refactoring. I can do it. ( who validate? )

Now going back to my usecase:

  • I'm not sure my identity provider will stay on the RS256 signature mode.
  • Most of the application we wrote in my organization are behind corporate proxy. I need to push the key to the context in a reactive way, then call the nimbus processor. Today, the RemoteSource is not so great to setup. I add a small "WebClient" parameter to be able to fix that.

PS: I just saw that I got 2 errors in the build, I will check that...
EDIT: It was 1 typo + the test with springboot... On this one, as I did something with breaking change, there is no way the process will be ok without a really dirty thing....(breaking change or dirty hack) ... which I'm really not proud to have done.... ( one way, protected getter on the factory is better but if it is a deprecated thing in xxx version (no need to set versions here :D ) , I hope quality guy are a little bit blind :D )

       @Deprecated
	public NimbusReactiveJwtDecoder(String jwksUri){
		NimbusReactiveJwtDecoder r= NimbusReactiveJwtDecoderFactory.fromJWKS(jwksUri);
		this.jwtProcessor=r.jwtProcessor;
		this.initializer=r.initializer;
	}

@rwinch rwinch changed the title NimbusJwtReactiveDecoder should accept a Nimbus JWTProcessor NimbusReactiveJwtDecoder should accept a Nimbus JWTProcessor Jan 8, 2019

@jzheaux

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

I think let's go for our ideal first, which is to coordinate with Nimbus. They've been open to our suggestions and PRs in the past.

If we can't find a path forward with them, then we can look at a PR.

In the meantime, I think that your code will end up being fairly minimal to implement ReactiveJwtDecoder yourself so that you can retrieve your key in the way that you need to.

@jzheaux

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

I've opened a ticket with Nimbus and we'll see where that goes: https://bitbucket.org/connect2id/nimbus-jose-jwt/issues/296/consider-having-jwtprocessor-accept-key

@GregoireW

This comment has been minimized.

Copy link

commented Jan 9, 2019

I read the ticket you open on Nimbus side, and I'm a little bit confuse on what you have in mind.

You ask them to simply get a new method process(JWT, List<JWK>, SecurityContext)
It means you plan to have a constructor like: NimbusReactiveJwtDecoder(Flux<JWK>, JWTProcessor) and an optional function JWT -> SecurityContext ?

@jzheaux

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

My point in the ticket was to highlight how Spring Security solves the problem today, via a custom SecurityContext, and to gauge whether or not that was contrary to its intended use.

Vladimir confirmed in his reply that this kind of scenario is what SecurityContext was meant for, which actually was contrary to my expectations.

To your question, my point was not to ask for a specific method signature, but instead to be able to pass a key set as a parameter since "it seems that passing in a parameter is more amenable than changing several Nimbus interfaces to be reactive." And Vladimir's response is that the SecurityContext is that parameter.

@GregoireW

This comment has been minimized.

Copy link

commented Jan 9, 2019

Ok, then how you want to play this?

I can refactor the code with:

A simple interface:

public interface ReactiveJWTProcessor {
    Mono<JWTClaimSet> process(JWT jwt);
}

means people need to take care of SignedJWT and EncryptedJWT.

or a specific

public interface ReactiveJWTProcessor {
    default Mono<JWTClaimSet> process(SignedJWT jwt){
        throw new JwtException("Signed JWT not supported");
    }

    default Mono<JWTClaimSet> process(EncryptedJWT jwt){
        throw new JwtException("Signed JWT not supported");
    }
}

Or another approach ?

@jzheaux

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

@GregoireW I think the next step is to work with Nimbus to see if we can get some of the Spring Security code accepted there, in order to legitimize and clarify the approach. Since it's Nimbus's position that it's okay, let's see if we can get a stronger API definition on that side of things.

At the very least, JWKContext or something similar should live in Nimbus. I'll submit a PR to Nimbus as report back here.

In the meantime, the work that NimbusReactiveJwtDecoder does is pretty minimal. You should be able to roll your own implementation of ReactiveJwtDecoder that hits your corporate proxy as you described. Let me know if you run into problems with that.

@GregoireW

This comment has been minimized.

Copy link

commented Jan 10, 2019

I already do something on my projects to make it works (and it is ok),

My main issue is that some "common" classes are protected, it means I need to copy a lot of code (or create a beautiful package org.springframework.security.oauth2.jwt in my applications ) to be able to make this work.

It is why I'm so motivated to upgrade this on spring side to drop those things as soon as possible ;)

@GregoireW

This comment has been minimized.

Copy link

commented Jan 11, 2019

Anyway I refactor stuff to be cleaner with somehow what I use on my project now

jzheaux added a commit to jzheaux/spring-security that referenced this issue Mar 5, 2019

jzheaux added a commit to jzheaux/spring-security that referenced this issue Mar 5, 2019

jzheaux added a commit to jzheaux/spring-security that referenced this issue Mar 5, 2019

jzheaux added a commit to jzheaux/spring-security that referenced this issue Mar 15, 2019

@jzheaux jzheaux closed this in 55e8df1 Mar 18, 2019

@jzheaux jzheaux modified the milestones: 5.2.x, 5.2.0.M2 Apr 16, 2019

@jzheaux jzheaux changed the title NimbusReactiveJwtDecoder should accept a Nimbus JWTProcessor NimbusReactiveJwtDecoder should accept a custom processor Apr 16, 2019

@jzheaux jzheaux self-assigned this Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.