Add an OAuth2 extension #3078

loicmathieu · 2019-07-03T09:05:16Z

This PR add an OAuth 2 extentions, based on wildfly-elytron-realm-token that allow to implements a simple OAUth2 authentication flow, without JWT nor Keyclock.

It covers the case of classic OAuth authentication flow like client_credential or resource_owner_password without OIDC.

loicmathieu · 2019-07-03T09:07:50Z

ping @sberyozkin can you review it and give some feedback.

If it is decided to be included in Quarkus, I will need to add documentation & integration tests. But I don't want to work on these if it will not be incuded ...

emmanuelbernard · 2019-07-03T09:32:00Z

CC @sebastienblanc too

extensions/elytron-security-oauth2/pom.xml

...ty-oauth2/runtime/src/main/java/io/quarkus/elytron/security/oauth2/runtime/OAuth2Config.java

...-oauth2/runtime/src/main/java/io/quarkus/elytron/security/oauth2/runtime/OAuth2Template.java

...main/java/io/quarkus/elytron/security/oauth2/runtime/oauth/ElytronOAuth2CallerPrincipal.java

...runtime/src/main/java/io/quarkus/elytron/security/oauth2/runtime/oauth/Oauth2Credential.java

sberyozkin · 2019-07-03T12:11:54Z

Hi @loicmathieu, All
IMHO it would be a worthy feature to have. I'd suggest avoiding the oauth2 qualifier in the extension name and use realm-token instead to avoid some possible ambiguities (however it is not a big deal, if -oauth2 works for others then it would be OK for me as well, since OIDC is built on top of OAuth2, which may suggest that this extension can take care of all kind of tokens).

As I have mentioned earlier, smallrye-jwt should have a token introspection option available as well but at this moment of time it is only a plan which would also require refactoring the Quarkus smallrye-jwt extension to use the factories and the users would be expected to have a service loader resource pointing to the introspection endpoint factory and some details are not yet clear. As such I don't think it is a good reason not to accept this PR. Instead, combining the extension proposed by this PR and smallrye-jwt to have the introspected tokens exposed as MP JWT tokens can be a viable alternative and consistent with the similar idea for combining KC adapter and smallrye-jwt.

CC-ing @starksm64 as well.

sberyozkin · 2019-07-04T12:23:07Z

@starksm64, @emmanuelbernard, @gsmet, @sebastienblanc Can you please indicate to @loicmathieu if the PR can make it. I recommend for it to go in (once the tests are in and some minor tweaks are completed), please decide

emmanuelbernard · 2019-07-04T12:42:05Z

I am deferring to Scott and Sebi on this one. I declare incompetency :D

…

On Thu, Jul 4, 2019 at 2:23 PM sberyozkin ***@***.***> wrote: @starksm64 <https://github.com/starksm64>, @emmanuelbernard <https://github.com/emmanuelbernard>, @gsmet <https://github.com/gsmet>, @sebastienblanc <https://github.com/sebastienblanc> Can you please indicate to @loicmathieu <https://github.com/loicmathieu> if the PR can make it. i recommend for it to go in (once the tests are in and some minor tweaks are completed), please decide — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3078?email_source=notifications&email_token=AACJNWCHOWM7WHIDT3ENQIDP5XTTHA5CNFSM4H5DR52KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZHISPY#issuecomment-508463423>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACJNWDXJ5QUU7L45JENRLDP5XTTHANCNFSM4H5DR52A> .

sberyozkin · 2019-07-04T14:56:08Z

@emmanuelbernard np :-), all this PR does is it sends (indirectly) a simple HTTP POST request to the token introspection endpoint which returns a simple JSON response with the token properties, and this PR then takes the name and roles (from the usual scope property the name can be customized in the future) and populates a Principal and that is it. Should make quite a few users happy for whom the KC adapter does not suit

extensions/elytron-security-oauth2/deployment/pom.xml

sberyozkin · 2019-07-04T15:03:13Z

@loicmathieu Hi, can you please spend a bit of time on the integration test (not sure, may be you can check what is happening with the KC integration tests and just use the KC server to introspect the (JWT) token ? It does not really matter it won't be a binary one, the concept is the same).

loicmathieu · 2019-07-04T15:55:49Z

@sberyozkin I will work on it tomorrow.
Is it OK to use wiremock to mock an introspection endpoint?

The Keycloack integration test are skipped, I assume because they need a Keycloack installed ;), so I prefer to mock the response of the introspection endpoint, it's way more easy for me.

sberyozkin · 2019-07-04T16:52:42Z

@loicmathieu sure, mocking it would be fine I guess for a start, may be copy a JSON response fragment from the introspection spec text (or use whatever you prefer :-)).
FYI, I've opened #3092. @pedroigor @sebastienblanc hope you don't mind :-)

loicmathieu · 2019-07-05T09:39:41Z

@sberyozkin I added an integration test ... and fixes one bug that the test discover ;)
I could write a guide next week if it's a GO for the extention.

sebastienblanc · 2019-07-05T09:52:05Z

I need to find some time to review this. I understand the need of having an OAuth2 extension but this is not going to ease the consolidation of the security story ;)
I see also a lot of duplicate code with the keycloak-undertow adapter .
@loicmathieu Is it correct that what you really need is an extension that can validate an opaque token using an introspection endpoint ? OIDC does not make it mandatory to have an JWT token for the access token , although Keycloak does it this is something we should change on our side to support this.
I would also really like the feedback of @pedroigor and @patriot1burke on this.

sberyozkin · 2019-07-05T10:29:24Z

@sebastienblanc Expecting Keycloak to start supporting the binary tokens is unrealistic IMHO in the short/medium term, though would be glad to be proven wrong. The reality is the Keycloak adapter can not handle all the tokens in the world :-) and it is not a limitation of the KC in anyway.

By the way, @loicmathieu has mentioned that this is not an OIDC flow he is dealing with, but a client credentials or similar with the tokens possibly being issued by the home-grown or some other 3rd party Oauth2 provider.

I agree there is a certain overlap and I've talked about it above with the respect the similar option be eventually (possibly) offered by smallrye-jwt, but I honestly don't think it conflicts with the higher-end KC adapter dealing with the variety of OIDC flows.

pedroigor · 2019-07-05T13:10:46Z

I agree with @sberyozkin. The extension is basically leveraging Elytron Token Realm and OAuth2 Token Validator. The latter enables OAuth2 support to the token realm so that access tokens sent as a bearer can be validated using a token introspection endpoint.

Spec-wise, OAuth2 access tokens are opaque and need a call to the introspection endpoint in order to check their validity as well as the claims within the token, mainly the scopes granted to a client acting on behalf of the user. Just like in Elytron the extension provides a pure OAuth2 bearer authorization. On the other hand, the Keycloak extension assumes that access tokens are JWTs and validation is only performed locally (no call to introspection endpoint).

The Keycloak adapter also provides specific features that are implementation specific. So, as we discussed before, it is mainly targeted for those using Keycloak as OIDC OP, providing the same experience in terms of functionalities for those coming from Spring or Wildfly.

In our last team meeting, Stian mentioned that we should probably drop our adapter in favor of standard implementations of the specs that Keycloak support. So, I can think of this extension (or improvements to smallrye-jwt) as an important move in this direction.

Regarding the capabilities provided by this extension, I think we could also consider:

Support for JSON Web Token (JWT) Profile for OAuth 2.0 Access Tokens. This should be possible given that Elytron also supports a JWT validator that can be used to validate bearer tokens using JWT format. See http://docs.wildfly.org/16/WildFly_Elytron_Security.html.
More flexible claims to role mapping, so that developers can map claims from tokens to roles accordingly with their requirements.

...me/src/main/java/io/quarkus/elytron/security/oauth2/runtime/oauth/OAuth2IdentityManager.java

...nt/src/main/java/io/quarkus/elytron/security/oath2/deployment/OAuth2DeploymentProcessor.java

...me/src/main/java/io/quarkus/elytron/security/oauth2/runtime/oauth/OAuth2IdentityManager.java

integration-tests/elytron-security-oauth2/pom.xml

sberyozkin · 2019-07-17T15:28:03Z

...t/src/main/java/io/quarkus/elytron/security/oauth2/deployment/OAuth2DeploymentProcessor.java

+     *
+     * @return
+     */
+    @BuildStep(providesCapabilities = "io.quarkus.quarkus-elytron-security-oauth2")


@loicmathieu It should be a package name, see the Capabilities sub section

done, I use io.quarkus.elytron.security.oauth2

sberyozkin · 2019-07-17T15:33:20Z

@loicmathieu one thing I'd like to experiment with once this PR makes it, is to update this extension to look for something like PrincipalProvider and if it is found then pass the Elytron Attributes to it and PrincipalProvider will return a Principal which will have to be used instead of the default one offered by this extension. This way smallrye-jwt would complement this extension and make the tokens obtained by it exposed via Mp JWT API. (and repeat the same for the KC extension)

pedroigor · 2019-07-17T15:40:38Z

@sberyozkin I also think is a good addition. Although, like you and @sebastienblanc said, not really helps with a unification of the security story in Quarkus.

From a user perspective, the extension provides an easy and nice way to enable a standard-based OAuth2 Bearer Authorization. There are more to consider but I think they could be part of the second round of changes.

loicmathieu · 2019-07-17T15:44:42Z

@sberyozkin thanks for your support :). After this first version merged, I will opens followup issues for RoleDecoder, native SSL support and PrincipalProvider.

sberyozkin · 2019-07-17T15:53:28Z

@pedroigor thanks, I was less concerned about the unification at this point of time :-) as it seems this extension fits its own niche as we discussed earlier.

Please also see my last comment to @loicmathieu, I feel like this path will help us unify things, example, this extension + smallrye-jwt, or KC + smallrye-jwt, the only other thing would be needed for smallrye-jwt to check the BuildStep capabilities (to determine if either KC or this extension has been loaded) and offer an Attributes to JsonWebToken principal converter, the KC or this extension would have the populated Attributes.
Something like that :-), I'm sure we will figure out the minor details when we start dealing with it... I'll certainly keep you all up to date once I'll start looking at it from the smallrye-jwt point of view
thanks

pedroigor · 2019-07-17T16:18:02Z

Sounds like a good plan. Please, let me know if I can help with something.

sberyozkin · 2019-07-17T16:21:50Z

@pedroigor sure, thanks

gsmet

This needs to be squashed before merging.

Disclaimer: I haven't read all the code as I'm not really versed in the security layer but have reviewed the doc and made a scan through the code.

I haven't validated the architecture either as the others have the vision of what we want for security.

gsmet · 2019-07-17T16:23:23Z

docs/src/main/asciidoc/oauth2-guide.adoc

+
+include::./attributes.adoc[]
+:extension-name: Elytron Security OAuth2
+:oauth2-token: OAuth2 tokens


Hmmm, what's the purpose of this one?

I create this guide based on the JWT one and kept the variables in place, I can remove them if you want.

We can keep the extension name but really oauth2-token should go away.

docs/src/main/asciidoc/oauth2-guide.adoc

gsmet · 2019-07-17T16:24:44Z

docs/src/main/asciidoc/oauth2-guide.adoc

+[cols="<m,<m,<2",options="header"]
+|===
+|Property Name|Default|Description
+|quarkus.oauth2.enabled|true|Determine if the OAuth2 extension is enabled. Enabled by default if you include the elytron-security-oauth2 dependency, so this would be used to disable it.


backticks around the dependency name would be nice.

Maybe also around the properties, depending on how it is done in the other guides.

done.

backticks around properties when use a table is not the common usage on the other guides.

gsmet · 2019-07-17T16:25:24Z

docs/src/main/asciidoc/oauth2-guide.adoc

+
+This command generates the Maven project with a REST endpoint and imports the `elytron-security-oauth2` extension, which includes the {auth2-token} support.
+
+If you don't want to use the maven plugin, you can just include the dependency in your pom.xml:


Suggested change

If you don't want to use the maven plugin, you can just include the dependency in your pom.xml:

If you don't want to use the Maven plugin, you can just include the dependency in your pom.xml:

gsmet · 2019-07-17T16:27:12Z

docs/src/main/asciidoc/oauth2-guide.adoc

+
+* user name is anonymous
+* isSecure is false as https is not used
+* authScheme is null


Some backticks around isSecure and authScheme would be nice.

gsmet · 2019-07-17T16:41:51Z

...time/src/main/java/io/quarkus/elytron/security/oauth2/runtime/oauth/OAuth2AuthMechanism.java

+                            credential, exchange.getRequestPath(), account.getRoles());
+                    return AuthenticationMechanismOutcome.AUTHENTICATED;
+                } else {
+                    UndertowLogger.SECURITY_LOGGER.info("Failed to authenticate Oauth2 bearer token");


Is it an info, an error, or something that can happen in a normal behavior?

From the code, it looks like a normal behavior so we shouldn't log anything.

This is the same code as in the JWT extension, I prefere to keep it consistent.

Authentication failures are normal, but it's important to be able to know that a failure occurs so I always love to see those logs. A user can disabled them by raising the level to WARN.

In fact, I found the current extension not loggin enought in case of authentication failures (for example, if it cannot access the introspection endpoint there is no error on the log). Maybe I will revisit it later to raise more error (but again it's the same as the JWT extension)

gsmet · 2019-07-17T16:41:58Z

...time/src/main/java/io/quarkus/elytron/security/oauth2/runtime/oauth/OAuth2AuthMechanism.java

+                            credential, exchange.getRequestPath(), account.getRoles());
+                    return AuthenticationMechanismOutcome.AUTHENTICATED;
+                } else {
+                    UndertowLogger.SECURITY_LOGGER.info("Failed to authenticate Oauth2 bearer token");


Suggested change

UndertowLogger.SECURITY_LOGGER.info("Failed to authenticate Oauth2 bearer token");

UndertowLogger.SECURITY_LOGGER.info("Failed to authenticate OAuth2 bearer token");

gsmet · 2019-07-17T16:42:09Z

...time/src/main/java/io/quarkus/elytron/security/oauth2/runtime/oauth/OAuth2AuthMechanism.java

+                    return AuthenticationMechanismOutcome.NOT_AUTHENTICATED;
+                }
+            } catch (Exception e) {
+                UndertowLogger.SECURITY_LOGGER.infof(e, "Failed to validate Oauth2 bearer token");


Suggested change

UndertowLogger.SECURITY_LOGGER.infof(e, "Failed to validate Oauth2 bearer token");

UndertowLogger.SECURITY_LOGGER.infof(e, "Failed to validate OAuth2 bearer token");

gsmet · 2019-07-17T16:43:35Z

integration-tests/elytron-security-oauth2/pom.xml

+            <dependency>
+                <groupId>org.eclipse.jetty</groupId>
+                <artifactId>jetty-servlet</artifactId>
+                <version>9.4.17.v20190418</version><!-- or 9.4.15.v20190215 -->


Why do we want Jetty while we have Undertow?

And in any case, it should be in the bom.

This is a used by wiremock and there is a dependency management issue in wiremock itself. I can open an issue on wiremock and reference it here to be able to remove this dependency management override later

I openned an issue and add the link as a comment in the pom

The issue is not on wiremock, it's because we define some jetty dependencies in the bom and not all. I added the missing one.

gsmet · 2019-07-17T16:43:45Z

integration-tests/elytron-security-oauth2/pom.xml

+        <dependency>
+            <groupId>com.github.tomakehurst</groupId>
+            <artifactId>wiremock-jre8</artifactId>
+            <version>2.23.2</version><!-- TODO add it to parent POM ?-->


It should be in the BOM.

docs/src/main/asciidoc/oauth2-guide.adoc

...t/src/main/java/io/quarkus/elytron/security/oauth2/deployment/OAuth2DeploymentProcessor.java

...ty-oauth2/runtime/src/main/java/io/quarkus/elytron/security/oauth2/runtime/OAuth2Config.java

...-oauth2/runtime/src/main/java/io/quarkus/elytron/security/oauth2/runtime/OAuth2Recorder.java

loicmathieu · 2019-07-18T10:18:41Z

@gsmet, @machi1990 thanks for the hard work on reviewing the doc and the implementation details :)
I correct a lot of your feedback and let some questions ... can you close what is OK for you ?

loicmathieu · 2019-07-23T08:18:58Z

@gsmet @machi1990 do you have more feedback? can you close what's OK ?

machi1990 · 2019-07-23T08:29:14Z

@gsmet @machi1990 do you have more feedback? can you close what's OK ?

hey @loicmathieu feel free to close my comments. I cannot seem to find the Resolve button.

loicmathieu · 2019-07-25T16:09:32Z

@gsmet I rebase on master to resolves the conflict and squash some commits

gsmet · 2019-07-26T16:26:09Z

@loicmathieu so on the documentation side, the last thing to fix is https://github.com/quarkusio/quarkus/pull/3078/files#r304518743 . Please include your proposal + my fix to "OAuth2".

As for the extension itself, it seems @sberyozkin thinks it would be a good addition to the security layer so I suppose we should merge it. I haven't reviewed the code though.

Note: please squash everything into one commit, better to have one atomic commit for the feature.

loicmathieu · 2019-07-29T08:05:42Z

@gsmet I update the documentation as requested and squashed everything in a single commit.

gsmet

Approved on behalf of the Security team.

emmanuelbernard requested a review from starksm64 July 3, 2019 09:28