Configurable authentication converter for resource-servers with token introspection #11665

ch4mpy · 2022-08-05T20:44:09Z

This could close #11661

It adds configurable authentication converter for resource-servers with token introspection (something very similar to what JwtAuthenticationConverter does for resource-servers with JWT decoder).

The new (Reactive)OpaqueTokenAuthenticationConverter is given responsibility for converting successful token introspection result into an Authentication instance (which is currently done by private methods of OpaqueTokenAuthenticationProvider and OpaqueTokenReactiveAuthenticationManager).

(Reactive)BearerTokenAuthenticationConverter, the default (Reactive)OpaqueTokenAuthenticationConverter, behave the same as current private convert(OAuth2AuthenticatedPrincipal principal, String token) methods: map authorities from scope attribute and return a BearerTokenAuthentication.

New unit test classes are created for default authentication converters and tests related to authorities mapping are moved there.

Existing tests for OpaqueTokenAuthenticationProvider and OpaqueTokenReactiveAuthenticationManager are modified to simply assert that introspector and authenticationConverter are called and that resulting Authentication is propagated.

P.S.
I set 6.0 as target but can of course modify this to 5.8 or whatever depending on how you schedule the feature (additional XSDs could need an update depending on the branch this is merged to).

ch4mpy · 2022-08-11T02:14:18Z

@sjohnr and @marcusdacoregio I believe this PR to be ready for review.

sjohnr · 2022-08-11T14:36:31Z

Thanks @ch4mpy! As we're working through some priority tasks for 5.8 and 6.0, it may be a slight delay before I'm able to take a look at this.

ch4mpy · 2022-08-11T14:58:01Z

@sjohnr I understand. Just put it together right now because I had a little time and the idea was fresh in my mind.

I couldn't find the issue / PR I'm duplicating. Could you please point me to it?

sjohnr · 2022-08-11T21:11:20Z

Thanks for taking the time! We use the duplicate label for book-keeping so only one issue (either the issue or the PR) goes into the release notes, and so whoever did the work gets credit. Since you opened the issue AND the PR, I just marked this PR as the duplicate.

ch4mpy · 2022-08-25T01:28:00Z

@sjohnr I added a separate commit for 5.8 XSD and doc, just in case you have some time to review any time soon. The commit is easy to remove otherwise.

There is no breaking change with this PR, default behavior should remain the same.

sjohnr · 2022-08-25T21:33:52Z

Thanks @ch4mpy. Apologies, I haven't been able to review it thoroughly yet, as I'm still heads down. However, glancing at it, I wanted to mention one thing to think about. You may want to consider the Converter interface from spring-core. It's used fairly extensively throughout the framework already, and we'd probably want to prefer that over introducing new interfaces. Switching that over will get you a bit ahead on review feedback.

I'll let you know when I have a bit more time to review.

ch4mpy · 2022-08-25T22:38:13Z

Actually, I had considered using the Converter interface but had two reserves :

current convertion methods have two parameters (token string and OAuth2AuthenticatedPrincipal) => I'd have to create a new class, let's say IntrospectionResult, to wrap the two parameters into a single one.
due to generics erasure or something, in an application or two where I wanted to override several converters, I had to curry it for beans to be correctly wired (do semething like public interface ABConverter extends Converter<A, B> {}).

I'll update the PR with:

Converter<IntrospectionResult, ? extends Authentication> instead of OpaqueTokenAuthenticationConverter
Converter<IntrospectionResult, Mono<? extends Authentication>> instead of ReactiveOpaqueTokenAuthenticationConverter

Please confirm that IntrospectionResult is fine for naming the converter source type.

sjohnr · 2022-08-26T14:41:07Z

Please confirm that IntrospectionResult is fine for naming the converter source type.

I'll spend some time thinking about that.

sjohnr

Thanks for the PR @ch4mpy! I'm excited about this one. As I mentioned, I'm still a little behind on tasks, but I wanted to give this a bit of attention. See below comments for some suggestions to get you moving forward.

Also, I mention this below but putting it at the top so you can focus on it first:

At the moment, I'm re-thinking my position on using a Converter here. Would you mind reverting the changes to switch to this interface, so we have an OpaqueTokenAuthenticationConverter again? (Sorry for being indecisive!)

.../org/springframework/security/oauth2/server/resource/authentication/IntrospectionResult.java

...mework/security/oauth2/server/resource/authentication/OpaqueTokenAuthenticationProvider.java

...security/oauth2/server/resource/authentication/OpaqueTokenReactiveAuthenticationManager.java

docs/modules/ROOT/pages/servlet/oauth2/resource-server/opaque-token.adoc

sjohnr · 2022-08-29T22:53:58Z

docs/modules/ROOT/pages/servlet/oauth2/resource-server/opaque-token.adoc

-Or, if you're not using Spring Boot at all, then both of these components - the filter chain and a <<oauth2resourceserver-opaque-architecture-introspector,`OpaqueTokenIntrospector`>> can be specified in XML.
+If the application doesn't expose an `OpaqueTokenAuthenticationConverter` bean, then Spring Boot will expose a `BearerTokenAuthenticationConverter`.
+
+Or, if you're not using Spring Boot at all, then all of these components - the filter chain, an <<oauth2resourceserver-opaque-architecture-introspector,`OpaqueTokenIntrospector`>> and an `OpaqueTokenAuthenticationConverter` can be specified in XML.


Ideally, no changes in Spring Boot should be required, so I think this should be made a separate (standalone) paragraph that just describes the new configuration option with no mention of Spring Boot or other components.

I tried to rephrase, but I'm not native English speaker. Please provide me with what you want if it still not what you expect.

Ok. Will do!

...ava/org/springframework/security/web/servletapi/SecurityContextHolderAwareRequestFilter.java

config/src/main/resources/org/springframework/security/config/spring-security-6.0.xsd

...ework/security/config/annotation/web/configurers/AbstractAuthenticationFilterConfigurer.java

ch4mpy · 2022-08-30T19:42:15Z

@sjohnr I:

rebased (twice, was lucky enough to pick an unstable main the first time)
reverted to OpaqueTokenAuthenticationConverter interface and made it a @FunctionalInterface
found a way to share code between servlet and reactive confs for default OpaqueTokenAuthenticationConverter
added a test to assert XML conf for optional OpaqueTokenAuthenticationConverter is actually used
removed breaking change by adding constructors with OpaqueTokenAuthenticationConverter argument (and keeping constructors without such an argument)
removed var and record occurences

ch4mpy · 2022-08-31T19:01:13Z

Rebased on main after #11773 was merged and squashed again.

ch4mpy · 2022-09-07T00:45:12Z

@sjohnr, do you expect more changes from me?

Also, what about OpaqueTokenAuthenticationFactory instead of OpaqueTokenAuthenticationConverter?

sjohnr

This is definitely developing nicely @ch4mpy! Let's leave the existing interface name for now.

I have more suggestions to some specific areas of the PR, see below. Once we work through this round, any additional (small) changes could be applied as polish by the team when we're ready to merge. Please note that we will pause at that point and wait on this PR until after November (after the 5.8/6.0 release).

...mework/security/oauth2/server/resource/authentication/OpaqueTokenAuthenticationProvider.java

...security/oauth2/server/resource/authentication/OpaqueTokenReactiveAuthenticationManager.java

...ity/oauth2/server/resource/authentication/OpaqueTokenReactiveAuthenticationManagerTests.java

...k/security/oauth2/server/resource/authentication/OpaqueTokenAuthenticationProviderTests.java

ch4mpy · 2022-09-07T16:49:35Z

we will pause at that point and wait on this PR until after November (after the 5.8/6.0 release).

So I should remove modifications to 5.8 xsd and rnc too, maybe? And also modify @Since comments. Do you know what the target version will be?

ch4mpy · 2022-09-07T20:55:50Z

@sjohnr I think I fixed everything as you requested, but static BearerTokenAuthentication convert(String introspectedToken, OAuth2AuthenticatedPrincipal authenticatedPrincipal) for which I set visibility to package instead of private to keep code de-duplication

sjohnr · 2022-09-07T21:00:50Z

So I should remove modifications to 5.8 xsd and rnc too, maybe? And also modify @since comments. Do you know what the target version will be?

We can wait on that, but thanks for asking! I'll try and keep you updated if anything changes.

sjohnr

It's looking really good @ch4mpy! I did find a few small things below. Sorry for missing them earlier!

...mework/security/oauth2/server/resource/authentication/OpaqueTokenAuthenticationProvider.java

...ecurity/oauth2/server/resource/introspection/ReactiveOpaqueTokenAuthenticationConverter.java

...mework/security/oauth2/server/resource/introspection/OpaqueTokenAuthenticationConverter.java

sjohnr · 2022-09-09T20:25:21Z

Thanks for the updates @ch4mpy. After discussing with the team, I think we're good to merge PRs from the community into 5.8. As I mentioned, we've switched to merge-forward as a strategy, so we ultimately want to target 5.8 for this PR. If you happen to have a bit more time, you could squash commits and move the commit to the 5.8.x branch to see if it moves without conflicts (I believe you can edit the base of this PR without opening a new one). If you don't have time, don't worry about it and I can try and find time in the next week or two to get it moved over.

Adds configurable authentication converter for resource-servers with token introspection (something very similar to what JwtAuthenticationConverter does for resource-servers with JWT decoder). The new (Reactive)OpaqueTokenAuthenticationConverter is given responsibility for converting successful token introspection result into an Authentication instance (which is currently done by a private methods of OpaqueTokenAuthenticationProvider and OpaqueTokenReactiveAuthenticationManager). The default (Reactive)OpaqueTokenAuthenticationConverter, behave the same as current private convert(OAuth2AuthenticatedPrincipal principal, String token) methods: map authorities from scope attribute and build a BearerTokenAuthentication.

ch4mpy · 2022-09-12T18:08:44Z

@sjohnr squashed and rebased on 5.8.x.

Of course, 6.0 xsd and rnc modifications where lost in the process.

@configuration

* Add authentication-converter-ref to 6.0 * Add @configuration to test configs

sjohnr · 2022-09-14T15:44:49Z

Merged to 5.8.x as 1efb633. I added polish in 355ef21 and 1aee40d.

Issue spring-projectsgh-11665

Issue gh-11665

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 5, 2022

ch4mpy force-pushed the gh-11661 branch from e52bf2e to 60c52f4 Compare August 8, 2022 21:12

ch4mpy mentioned this pull request Aug 8, 2022

Configurable authentication converter for resource-servers with token introspection #11661

Closed

ch4mpy changed the title ~~Gh 11661~~ Gh 11661 Configurable authentication converter for resource-servers with token introspection Aug 10, 2022

sjohnr self-assigned this Aug 11, 2022

sjohnr added status: duplicate A duplicate of another issue type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Aug 11, 2022

ch4mpy force-pushed the gh-11661 branch from bcc3f01 to 8784961 Compare August 12, 2022 17:03

ch4mpy force-pushed the gh-11661 branch from 8784961 to 013c8c7 Compare August 25, 2022 00:49

ch4mpy force-pushed the gh-11661 branch from d736014 to 2557e39 Compare August 29, 2022 18:35

sjohnr requested changes Aug 29, 2022

View reviewed changes

sjohnr removed the status: waiting-for-triage An issue we've not yet triaged label Aug 29, 2022

ch4mpy force-pushed the gh-11661 branch from 731eef2 to a962ebc Compare August 30, 2022 19:35

ch4mpy requested a review from sjohnr August 30, 2022 20:03

ch4mpy force-pushed the gh-11661 branch 2 times, most recently from 6d19f16 to f32af34 Compare August 31, 2022 18:58

sjohnr requested changes Sep 7, 2022

View reviewed changes

sjohnr changed the title ~~Gh 11661 Configurable authentication converter for resource-servers with token introspection~~ Configurable authentication converter for resource-servers with token introspection Sep 7, 2022

ch4mpy force-pushed the gh-11661 branch from 166813a to a236d6f Compare September 9, 2022 18:41

ch4mpy changed the base branch from main to 5.8.x September 9, 2022 20:46

ch4mpy changed the base branch from 5.8.x to main September 9, 2022 20:51

ch4mpy force-pushed the gh-11661 branch 2 times, most recently from dbb003e to b436143 Compare September 9, 2022 23:24

ch4mpy changed the base branch from main to 5.8.x September 9, 2022 23:24

ch4mpy force-pushed the gh-11661 branch from b436143 to 8f75b16 Compare September 9, 2022 23:50

ch4mpy force-pushed the gh-11661 branch from 8f75b16 to d2b2d66 Compare September 10, 2022 00:11

sjohnr added a commit that referenced this pull request Sep 14, 2022

Polish gh-11665

355ef21

sjohnr added a commit that referenced this pull request Sep 14, 2022

Polish gh-11665

1aee40d

* Add authentication-converter-ref to 6.0 * Add @configuration to test configs

sjohnr closed this Sep 14, 2022

sjohnr added this to the 5.8.0-M3 milestone Sep 14, 2022

jzheaux added a commit to jzheaux/spring-security that referenced this pull request Oct 6, 2022

Polish OpaqueTokenAuthenticationConverterTests

8fdd7fc

Issue spring-projectsgh-11665

jzheaux added a commit that referenced this pull request Oct 6, 2022

Polish OpaqueTokenAuthenticationConverterTests

7043ef6

Issue gh-11665

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable authentication converter for resource-servers with token introspection #11665

Configurable authentication converter for resource-servers with token introspection #11665

ch4mpy commented Aug 5, 2022 •

edited

Loading

ch4mpy commented Aug 11, 2022

sjohnr commented Aug 11, 2022

ch4mpy commented Aug 11, 2022

sjohnr commented Aug 11, 2022

ch4mpy commented Aug 25, 2022 •

edited

Loading

sjohnr commented Aug 25, 2022

ch4mpy commented Aug 25, 2022 •

edited

Loading

sjohnr commented Aug 26, 2022

sjohnr left a comment

sjohnr Aug 29, 2022

ch4mpy Aug 31, 2022

sjohnr Aug 31, 2022

ch4mpy commented Aug 30, 2022 •

edited

Loading

ch4mpy commented Aug 31, 2022

ch4mpy commented Sep 7, 2022 •

edited

Loading

sjohnr left a comment

ch4mpy commented Sep 7, 2022

ch4mpy commented Sep 7, 2022

sjohnr commented Sep 7, 2022 •

edited

Loading

sjohnr left a comment

sjohnr commented Sep 9, 2022

ch4mpy commented Sep 12, 2022

sjohnr commented Sep 14, 2022

Configurable authentication converter for resource-servers with token introspection #11665

Configurable authentication converter for resource-servers with token introspection #11665

Conversation

ch4mpy commented Aug 5, 2022 • edited Loading

ch4mpy commented Aug 11, 2022

sjohnr commented Aug 11, 2022

ch4mpy commented Aug 11, 2022

sjohnr commented Aug 11, 2022

ch4mpy commented Aug 25, 2022 • edited Loading

sjohnr commented Aug 25, 2022

ch4mpy commented Aug 25, 2022 • edited Loading

sjohnr commented Aug 26, 2022

sjohnr left a comment

Choose a reason for hiding this comment

sjohnr Aug 29, 2022

Choose a reason for hiding this comment

ch4mpy Aug 31, 2022

Choose a reason for hiding this comment

sjohnr Aug 31, 2022

Choose a reason for hiding this comment

ch4mpy commented Aug 30, 2022 • edited Loading

ch4mpy commented Aug 31, 2022

ch4mpy commented Sep 7, 2022 • edited Loading

sjohnr left a comment

Choose a reason for hiding this comment

ch4mpy commented Sep 7, 2022

ch4mpy commented Sep 7, 2022

sjohnr commented Sep 7, 2022 • edited Loading

sjohnr left a comment

Choose a reason for hiding this comment

sjohnr commented Sep 9, 2022

ch4mpy commented Sep 12, 2022

sjohnr commented Sep 14, 2022

ch4mpy commented Aug 5, 2022 •

edited

Loading

ch4mpy commented Aug 25, 2022 •

edited

Loading

ch4mpy commented Aug 25, 2022 •

edited

Loading

ch4mpy commented Aug 30, 2022 •

edited

Loading

ch4mpy commented Sep 7, 2022 •

edited

Loading

sjohnr commented Sep 7, 2022 •

edited

Loading