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

Upgrade Spring Security's OIDC Support #9276

Closed
mraible opened this issue Feb 16, 2019 · 18 comments

Comments

Projects
None yet
5 participants
@mraible
Copy link
Contributor

commented Feb 16, 2019

Overview of the feature request

Spring Security 5.1 and (native with Spring Boot 2.1) has a new way to configure OAuth 2.0 Login. Rather than using @EnableOAuth2Ssso, it's a regular part of the web security configuration. This issue is to track that upgrade progress.

  • Create monolith example using Spring Security 5.1's OIDC Support
  • Create microservices example using Spring Security 5.1's OIDC Support
  • Update Spring Security configuration for OAuth 2.0 monolith and microservices into generator
  • Ensure all integration tests pass (across all servers)

Motivation for or Use Case

Using @EnableOAuth2Sso and @EnableResourceServer is no longer recommended in Spring Boot 2.1. The reasons for the change can be found in Josh Long's Bootiful Podcast, published on Jan 25, 2019. It's an interview with Madhura Bhave and the discussion starts at 21:30.

The pull request below shows the changes necessary to modify a v5.8.1 monolith with defaults + OAuth 2.0 for authentication. Please have a look and let me know if you see any issues.

mraible/jhipster-oidc-improved#1

Known Issues

  • The principal's authorities don't get updated by the user authorities mapper. The authorities are correct in the OAuth2AuthenticationToken, but not in its principal.

screen shot 2019-02-16 at 1 11 32 pm

@yelhouti

This comment has been minimized.

Copy link

commented Feb 17, 2019

@mraible this implementation (as well as the current one) has security issues, please let's fix them before moving forward.

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2019

@yelhouti Please provide a pull request to my PR’s branch with your suggested changes. I’ll be happy to review your fixes. Saying there’s issues without providing a fix doesn’t help me.

@yelhouti

This comment has been minimized.

Copy link

commented Feb 17, 2019

Sorry @mraible I forgot to delete this comment, and in order to put everything in the long issue we already have.
The main issue here is that OIdC is used the some way you would with OAuth2 Authorization flow, on the other hand the spec clearly says that after validating the token the same way you would with OAuth2 (step 1) you MUST:

1- Follow the validation rules in RFC 6749, especially those in Sections 5.1 and 10.12.
2- Follow the ID Token validation rules in Section 3.1.3.7.
3- Follow the Access Token validation rules in Section 3.1.3.8.

Step two 2 (and 3) clearly mention that after decryptin and signature verification the SP MUST verify the audience field...
I could argue with you that this is not that necessary because the token is retrieved with a private (not public) client, this why this approach might also be secure, but the spec is the spec and we should follow it.

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2019

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2019

@yelhouti

This comment has been minimized.

Copy link

commented Feb 17, 2019

@mraible Spring security handles audience validation, please check in this commit, in my demo jhipster app using stateless Keycloak/Okta .

And we really shouldn't use the Okta starter since some people use keycloak or even another IdP.

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2019

Are you sure audience validation is necessary when using oauth2Login()? This issue, and your example code, only seems to apply to resource servers.

spring-projects/spring-security#5226

JHipster doesn’t ship with a resource server by default. I’m happy to try it, just not sure that it’ll be used.

@yelhouti

This comment has been minimized.

Copy link

commented Feb 17, 2019

@mraible you are right this won't be used if added to your code since your are using the authorization flow with a private client and therefore (as I said earlier) one could say that there is now need to check the aud claim. But again, the spec (as I understand it, and I might be missing something here) says that validation of tokens should be done locally, and therefore the current implementation doesn't really follow the OIdC spec.
To summarize:

  • I don't see a security hole
  • OIdC specs says we MUST do things in a different fashion, so maybe there is one.
@VinodAnandan

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

The aud check/implementation can help to reduce attacks where an attacker can reuse tokens that are
intended to be used on a different client.

Example: An attacker reused the Facebook video uploader token for Facebook mobile application.

@yelhouti

This comment has been minimized.

Copy link

commented Feb 18, 2019

@VinodAnandan is right, but it mainly protects against malicious clients using token their received from user in other clients. This is why OAuth2 (not OIdC) should never be used for authentication. But in our case since we use the Auth flow and tokens are received directly from the IdP after checking the client's credentials I don't see hoe the same attack can apply. But again, the spec is the spec and we must follow it.

@VinodAnandan

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

Each and every attributes in the spec has its own importance. The great authors of the spec already considered many security use cases.

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

I don't believe it's necessary to validate the audience when using oauth2Login(). Yes, if you're setting up a resource server, it's important, but JHipster doesn't do that by default.

If you do want to setup a resource server in JHipster, see Spring Security's resource server docs:

To specify which authorization server to use, simply do:

security:
  oauth2:
    resourceserver:
      jwt:
        issuer-uri: https://idp.example.com

Where https://idp.example.com is the value contained in the iss claim for JWT tokens that the authorization server will issue. Resource Server will use this property to further self-configure, discover the authorization server’s public keys, and subsequently validate incoming JWTs.

The aforementioned docs have more information on the validation process.

So it looks like Spring Security handles validating incoming JWTs (when setting up a resource server). If you want to make this more robust, you can include audience validation like Okta does:

  1. Define a OAuth2TokenValidator:
    https://github.com/okta/okta-spring-boot/blob/master/oauth2/src/main/java/com/okta/spring/boot/oauth/TokenUtil.java#L80-L92
  2. Expose it as a bean:
    https://github.com/okta/okta-spring-boot/blob/master/oauth2/src/main/java/com/okta/spring/boot/oauth/OktaOAuth2ResourceServerAutoConfig.java#L44-L53

The reactive side of things is very similar.

Since JHipster doesn't define a resource server by default (for monoliths), this extra configuration makes no sense. Microservices might need it, but I haven't implemented that yet.

For modules that create resource servers (JHipster Ignite and Ionic for JHipster), I believe it's up to those projects to implement the resource server properly. Luckily, @ruddell and myself are the leads for those projects, so we'll do our best to do it right.

@yelhouti

This comment has been minimized.

Copy link

commented Feb 20, 2019

@mraible I might agree with you that may be audience validation (and even signature validation) is not necessary in your scenario since the introspection is done by the IdP, the client is verified by the IdP before giving the token since the client is private communication is done through SSL with hopefully certificate pining.
On the other hand i don't agree with you arguments, if the server is responsible for validating the token because it might come from entrusted source, verifying the audience is as importante and a malicious client would be able to impersonate a user.
OAuth2 doesn't contain the audience field this why Spring OAuth doesn't check it. checking it is the main difference between OIdC and OAuth2 and it's why OIdC is suitable for authentication while OAuth2 only for authorization.
The fact that you are using resource servers for Ionic and ignite proves that they are many uses cases when the authorization flow with private clients and sessions is not suitable, this why i'm proposing to add it the right way: officially. This can be done as I suggested by tweaking how UAA works and adding the flag --skip-user-management

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

The implementation I'm working on right now only pertains to monoliths and microservices with OAuth 2.0 / OIDC for authentication. If I encounter a resource server as part of that, I'll go ahead and implement audience validation. I'm not concerned about UAA. I did not code that part of JHipster, nor do I maintain it. I believe @ruddell does.

@ruddell

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Actually @xetys maintains the UAA code, I just help out anywhere I can.

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

POC for microservices created at mraible/jhipster-ms-oidc-improved#1

@mraible mraible referenced this issue Mar 11, 2019

Merged

Upgrade to Spring Security 5.1's OIDC Support #9416

4 of 4 tasks complete
@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

Integration in generator provided by #9416.

@mraible

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

This work has been completed in JHipster. Ionic for JHipster and Ignite JHipster still need to update their OIDC implementations for JHipster 6.

@mraible mraible closed this Mar 16, 2019

@DanielFran DanielFran referenced this issue Mar 24, 2019

Merged

Remove AUTH2 classes not used. #9478

4 of 4 tasks complete

@jdubois jdubois added this to the 6.0.0-beta.0 milestone Apr 4, 2019

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