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

OIDC support for Microservices architecture #6435

Merged
merged 11 commits into from
Oct 13, 2017

Conversation

danielpetisme
Copy link
Member

  • Please make sure the below checklist is followed for Pull Requests.

  • Travis tests are green

  • Tests are added where necessary

  • Documentation is added/updated where necessary

  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

@danielpetisme
Copy link
Member Author

danielpetisme commented Sep 29, 2017

As @mraible explained #6435 , I started a microservices support for Oauth2.
This is a global status.

So far, I succeed to generates a Gateway with OAuth2Sso enabled (redirects the user to Keycloak for authentication and successfully load principal and authorities) and ResourceServer (API's gateways's endpoints are accessible via a http client js or curl or whatever). In order combine both features I had to define 2 mutually excluding configurations). The gateway also acts as a token relay to provide jwt to the backend microservices.
I have one known issue, the swagger api pages can't be loaded...
The microservices have a bearer-only oauth2 client configuration meaning that microservice only verifies that the token is valid but can't generate one. The user has to get it before. Microservices also load principal and authorities so the dev can finely manage the microservice enpoints permissions.

In monolith applications when the authenticationType is oauth2 a set of user management files was generated. Currently in a microservice architecture I skip the user related files (everything is delagated to the IdP).

Regarding the code itself, it does work and the existing generator-jhipster test work. I didn't added new ones so far. Regarding the generated apps I didn't run the tests suite or generate all the combination.

@jdubois
Copy link
Member

jdubois commented Sep 29, 2017

Awesome! We need to find a solution for the Swagger doc, but this is a minor point compared to the rest (for a first release, that's not a blocker issue)

@danielpetisme
Copy link
Member Author

If we follow UAA pattern, none of te gateway or services components expose user-management endpoint (/account, /register, etc).

For me there is 3 options:

  1. Change the front app code with the corresponding provider endpoints. I don't like this solutions,
  2. Play with Zuul to reverse proxy the requests (ie change /account into http://localhost:9080/auth/realms/jhipster/protocol/openid-connect/auth). The benefit is that there is no code to maintain but seems it's purely declarative I don't know how far we can go.
  3. Add a intermediate layer on server side (actually on the gateway it would make sense). We coud use generic properties like security.oauth2.resource.userInfoUri to have something quite generic.

At the moment I think option#3 it's the cleaner from an architecture stand point, it's coherent with the gateway orchestration responsability... but it adds responsibility ro this component.

What do you think?

@jdubois
Copy link
Member

jdubois commented Oct 5, 2017

  • I'm very interested by this feature - unfortunately I don't have much time this month, but I'll try to help as much as possible
  • To simplify things, maybe some of those classes shouldn't be generated, and be in https://github.com/jhipster/jhipster instead
  • Unfortunately the Travis build fails -> is it related to your latest comment? I'm not expert here, but indeed point 3 looks good - maybe @mraible you could help on this?

-> outside of those comments, how can I help you?

@sdoxsee
Copy link
Contributor

sdoxsee commented Oct 6, 2017

@danielpetisme not entirely sure if I understand but here are some thoughts. Option 2 and/or 3 sound best to me. Do you have to choose? Here's an example gateway configuration where it uses zuul AND security.oauth2.resource.userInfoUri.

@danielpetisme
Copy link
Member Author

@sdoxsee you're right I can actually use both approaches but I'm not sure it's consistent
First of all, I don't want to build a uaa-proxy thing so only the api gateway should discuss with the oauth provider (keycloak or okta or whatever).
I think it's important to keep the frontend application agnostic of the oauth provider. So I'm targeting to keep the following endpoints:

capture d ecran 2017-10-06 a 23 31 23

That means that I could use to Zuul to map some urls. For instance a Zuul rule to map /api/account to security.oauth2.resource.userInfoUri but when it comes to password management the Jhipster process is not excatly mappable to the keycloak one, or least from what I have seen.
Rather than spreading some part in a config file and the other part in code I prefer to centralize everything.

I'm not a Zuul expert so feel free to correct me.

NOTE: Another option could simply be to say that User management isn't a Jhipster concern anymore and it's up to the dev to integrate it's oauth provider into the generated app. It's not the proposition I prefer but it's a pramatic (radical) option.

@danielpetisme
Copy link
Member Author

danielpetisme commented Oct 6, 2017

@jdubois
Most of all I need some reviews to validate the architecture:

  • Security
    • Are the clients definitions and their usage between the gateway and microservices correct?
    • Is the Token relaying made by the gateway safe (the communication between the gateway and services is made via HTTP so the token isn't encrypted... almost all the gw-services communicatoins are made in a private trusted network but security matters...)
  • General questions
    • To be able to use @EnableOauthSso and @EnableResourceServer I had to use a silly hack to split the api access and the classic HTMLs assets. They are mutually excluded. I'm not comfortable with this snippet but it's the best solution I found, so I would definitively welcome enhancement remarks
    • Rather than hardcoding the principal and authority extraction I've added a default implementation Principal and Authorities Extractor based on a configurable property. It's one more property to document and support but I think it would ease the integration or further Oauth provider. What you think about this.

Regarding the Travis CI status, I don't understand the issues (it failed on app-gradle for instance but I didn't touched anything related to gradle). As former Jenkins Administrators it hurts me to say that but... it works on my machine 😄

I'm currently running the test suite... we'll see

EDIT
The Travis build fails only on the oauth2 now I can now debug efficiently now.
https://travis-ci.org/jhipster/generator-jhipster/jobs/284413190

@sdoxsee
Copy link
Contributor

sdoxsee commented Oct 6, 2017

I'd say that with oidc, user management is not longer a jhipster concern since many oidc providers also have user management but feel free to push back on that...as long as jhipster can handle the role and permissions from the IdP.

@EnableGlobalMethodSecurity(prePostEnabled = true, securedEnabled = true)
public class MicroserviceSecurityConfiguration extends ResourceServerConfigurerAdapter {


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra line break.

}
<%_ } _%>


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra line break.

.and()
.sessionManagement()
.sessionCreationPolicy(SessionCreationPolicy.STATELESS)
.and()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@mraible mraible left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielpetisme After modifying application.yml to have the following, I'm able to login with a monolith again. This is probably why Travis doesn't pass.

oauth2:
    issuer: http://localhost:9080/auth/realms/jhipster

security:
    basic:
        enabled: false
    oauth2:
        client:
            accessTokenUri: ${oauth2.issuer}/protocol/openid-connect/token
            userAuthorizationUri: ${oauth2.issuer}/protocol/openid-connect/auth
            clientId: web_app
            clientSecret: web_app
            clientAuthenticationScheme: form
            scope: openid profile email
        resource:
            userInfoUri: ${oauth2.issuer}/protocol/openid-connect/userinfo
            tokenInfoUri: ${oauth2.issuer}/protocol/openid-connect/token/introspectr
            preferTokenInfo: false

Also, this PR seems to be missing the following files for a monolith. They should be copied when selecting oauth2.

this.skipUserManagement = true;
}

if (this.applicationType === 'uaa') {
this.authenticationType = 'uaa';
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra line break.

resource:
userInfoUri: ${oauth2.issuer}/protocol/openid-connect/userinfo
tokenInfoUri: ${oauth2.issuer}/protocol/openid-connect/token/introspectr
preferTokenInfo: false
jwt:
keyUri: ${oauth2.issuer}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines aren't needed for a monolith, so should only be included for microservice architecture.

oauth2:
client:
accessTokenUri: ${oauth2.issuer}/protocol/openid-connect/token
userAuthorizationUri: ${oauth2.issuer}/protocol/openid-connect/auth
<%_ if (applicationType === 'gateway') { _%>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clientId, clientSecret, clientAuthenticationScheme, and scope should also be included for a monolith, but tokenName is not needed.

oauth2:
issuer: http://localhost:9080/auth/realms/jhipster
principal-attribute: preferred_username
authorities-attribute: roles
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

principal-attribute and authorities-attribute not needed for monolith.

clientSecret: internal
authenticationScheme: header
clientAuthenticationScheme: header
<%_ } _%>
resource:
userInfoUri: ${oauth2.issuer}/protocol/openid-connect/userinfo
tokenInfoUri: ${oauth2.issuer}/protocol/openid-connect/token/introspectr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to http://localhost:9080/auth/realms/jhipster/.well-known/openid-configuration, the tokenInfoUri should be http://localhost:9080/auth/realms/jhipster/protocol/openid-connect/token/introspect. This is a mistake I made in my PR. However,, it doesn't seem to matter what the value is.

@@ -360,6 +360,7 @@ dependencies {
<%_ } _%>
<%_ if (authenticationType === 'oauth2') { _%>
compile "org.springframework.security.oauth:spring-security-oauth2:${spring_security_oauth2_version}"
compile "org.springframework.security:spring-security-jwt"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed when using OAuth 2.0 and a monolith.

<dependency>
<groupId>org.springframework.security</groupId>
<artifactId>spring-security-jwt</artifactId>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed when using OAuth 2.0 and a monolith.

@danielpetisme
Copy link
Member Author

The test about oauth2 are not failing anymore now it's eureka. It's strange because it fails on things like:

2017-10-10 07:28:50.867 ERROR 8527 --- [freshExecutor-0] com.netflix.discovery.DiscoveryClient    : DiscoveryClient_TRAVISDEFAULT/travisDefault:a534e8834a2417a16ad7c40bdec5fa69 - was unable to refresh its cache! status = Cannot execute request on any known server
* Rebuilt URL to: http://localhost:8080/
* Hostname was NOT found in DNS cache
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0*   Trying 127.0.0.1...
* connect to 127.0.0.1 port 8080 failed: Connection refused
* Failed to connect to localhost port 8080: Connection refused
* Closing connection 0

I'm not 100% sure it's related to the code itself.

@mraible
Copy link
Contributor

mraible commented Oct 9, 2017

Just noticed a regression. When selecting oauth2 and monolith, AngularJS should not be listed as a choice. @deepu105 implemented this.

screen shot 2017-10-09 at 3 45 10 pm

@mraible
Copy link
Contributor

mraible commented Oct 9, 2017

More testing... when creating a microservice gateway, shouldn't we prompt for JHipster Registry vs. Consul? The answers don't seem to line up:

? (5/16) Which service discovery server do you want to use? (Use arrow keys)
  Yes
  Consul
  No service discovery

@jdubois
Copy link
Member

jdubois commented Oct 10, 2017

@mraible maybe you're not on the right branch, or had an issue with yarn link (it happens all the time to me!) -> the current code looks correct for the Which service discovery server do you want to use? question

I need all this for my current client: I'm going to do a quick review

@jdubois
Copy link
Member

jdubois commented Oct 10, 2017

  • The build failed because there was a JVM crash, I restarted it and it works OK
  • @danielpetisme I'm not an expert and I can't answer all questions here: yes, there is no problem in exchanging JWT token over HTTP inside the "private" network. 1) that's supposed to be secured at the network level 2) the JWT token are signed and 3) you can easily add HTTPS

@danielpetisme
Copy link
Member Author

@jdubois What I propose in a first release is to remove all the user-management related files (as proposed by @sdoxsee). That means no more account settings page, account creation or users settings and user. The end user will have to go to the IdP to create an account or deal with his settings.
This config would be the equivalent of a future --skip-user-management settings of oauth2.

In a second shot, we can work on a Jhipster support of the account/users UI + IdP API orchestration in the gateway.

What do you think guys?

@jdubois
Copy link
Member

jdubois commented Oct 10, 2017

@danielpetisme yes for me all the user account management is handled by Keycloak/Okta, that's one of the main points of using this method. Like we do in JHipster UAA. So yes you should "skip user management".

@danielpetisme
Copy link
Member Author

The uaa deals with user management but since it's a jhipster app it's registered in the registry and accessible via the gateaway and visible from the gateway UI.

I'll update the code tonight to not generate the user related files.

@jdubois
Copy link
Member

jdubois commented Oct 10, 2017

Oh yes, let me detail:

  • The most common use case for me: users will be created elsewhere, and for example they will be stored in an LDAP server or a database -> the main role of Keycloak and Okta is to aggregate those sources of users, and give authentication tokens to the applications

Now for Keycloak only, as it can be used as a Docker image:

  • Keycloak port should be exposed by default in Docker-Compose -> so people can create a user, etc... directly using it
  • If you use Traefik, Keycloak should also be routed through it (and that would be very close to what you have with a gateway + JHipster UAA)

@mraible
Copy link
Contributor

mraible commented Oct 10, 2017

I agree that users should be created elsewhere. However, in the monolith implementation, I keep some of the user-related classes. My reason is so there can be a User entity that people can create relationships with. How I do this is I sync the User from the IdP with the local database's user when /api/account is called. See the logic here.

The JHipster app is not allowed to update the user, but UserResource/Repository can be used to query the user.

FWIW, I've sent this code to the Spring Security folks for a code review.

@jdubois
Copy link
Member

jdubois commented Oct 10, 2017

I've reviewed the code in more detail, and my main issue is this configuration from @mraible

oauth2:
    issuer: http://localhost:9080/auth/realms/jhipster
    principal-attribute: preferred_username
    authorities-attribute: roles

This configuration is not type safe, and to be consistent with the rest of the code: it should be stored in https://github.com/jhipster/jhipster/blob/master/src/main/java/io/github/jhipster/config/JHipsterProperties.java using a jhipster prefix -> let me do that, and do a new release of jhipster/jhipster so you can use it.

@jdubois
Copy link
Member

jdubois commented Oct 10, 2017

I just released https://github.com/jhipster/jhipster/releases/tag/v1.1.12 that includes the new OAuth2 configuration (see jhipster/jhipster@6ff3a22 )

-> you need to migrate the configuration to the new jhipster.security.authentication.oauth2 prefix, and @mraible the documentation should be changed accordingly
-> @danielpetisme I can help on this part, don't hesitate to ping me

@mraible
Copy link
Contributor

mraible commented Oct 10, 2017

@jdubois oauth2.issuer is not used by any code, it's only a shortcut for the other OAuth-related properties that Spring Security OAuth uses. The other two properties (principal-attribute and authorities-attribute) are only used by the microservices code at the moment.

@jdubois
Copy link
Member

jdubois commented Oct 10, 2017 via email

@danielpetisme
Copy link
Member Author

@jdubois @mraible I've mixed feelings for. Right now oauth2.issuer acts as a variable for the others oauth2 properties. I think we could simply duplicate this value, we're talking about 4 consecutives lines...

If we plan to pilot the IdP provider (registering for instance) from JHipster code, then this variable could be useful but it will raise more issues (It requires an implementation per provider).

Regarding the principal-attribute and authorities-attribute they're used in the extractor pattern I proposed. Matt had a code generation oriented strategy hardcoding the snippet where I tried to have a framework oriented approach using the underneath Spring piping.
Don't know if we should merge the 2 approaches, use one or the other.

The OIDC monolith and microservices implementation are not 100% aligned but for the sake of speed I propose to treat this point as a dedicated issue.

@mraible sounds good to you if we stay as-is in a first release?

@jdubois
Copy link
Member

jdubois commented Oct 10, 2017

yes this is just a variable used 4 times @danielpetisme - could we just remove it? And then probably just copy/paste it, that's OK in the YAML file (and not really our fault, this is just the Spring Security OAuth config that works this way). If that's fine, then I'll remove my config in the jhipster/jhipster library.

@danielpetisme
Copy link
Member Author

+1 for me

@mraible
Copy link
Contributor

mraible commented Oct 10, 2017

@jdubois If we move it, we'll need to change _app.yml from:

- OAUTH2_ISSUER=http://keycloak:9080/auth/realms/jhipster

To:

- SECURITY_OAUTH2_CLIENT_ACCESS_TOKEN_URI = http://keycloak:9080/auth/realms/jhipster/protocol/openid-connect/token
- SECURITY_OAUTH2_CLIENT_USER_AUTHORIZATION_URI = http://keycloak:9080/auth/realms/jhipster/protocol/openid-connect/auth
- SECURITY_OAUTH2_RESOURCE_USER_AUTHORIZATION_URI = http://keycloak:9080/auth/realms/jhipster/protocol/openid-connect/userinfo
- SECURITY_OAUTH2_RESOURCE_TOKEN_INFO_URI = http://keycloak:9080/auth/realms/jhipster/protocol/openid-connect/introspect

We might need to update some other documentation too.

@mraible
Copy link
Contributor

mraible commented Oct 10, 2017

@danielpetisme Can you please provide the steps you're using the ensure this PR works? I want to compare your methodology with the one I'm using. Here's what I expect to be able to do:

  1. Create a gateway with OAuth
  2. Create a microservices app with OAuth
  3. Create an entity in the microservices app
  4. Create the UI for that entity in the gateway
  5. Clone JHipster Registry and run it
  6. Start Keycloak using docker compose (from the Gateway app's directory)
  7. Start the gateway and the microservices app and login successfully

I think it should also be possible to use the docker-compose and kubernetes sub-generators to create a system that'll start all of the above services.

@danielpetisme
Copy link
Member Author

@mraible Indeed is confusing to have a skipUserManagement flag and still manage some user files. Since your code provision the user into Jhipster db for sync purpose can we skip the initial user creation?

@danielpetisme
Copy link
Member Author

@jdubois How did you build and deploy the jhipster lib? I can't find the new properties. I retrieved the jar and disassemble it. I can't see an oauth2 property (only jwt)

@mraible
Copy link
Contributor

mraible commented Oct 12, 2017

@mraible Indeed is confusing to have a skipUserManagement flag and still manage some user files. Since your code provision the user into Jhipster db for sync purpose can we skip the initial user creation?

I could skip the loading of users.csv, but we'd still need to load authorities.csv (since the role names are hard-coded in the client/server) and users_authorities.csv (for the default admin/admin and user/user to work). Does not loading users.csv buy us anything?

@jdubois jdubois mentioned this pull request Oct 12, 2017
4 tasks
@mraible
Copy link
Contributor

mraible commented Oct 12, 2017

@danielpetisme I created #6510 against master to remove oauth2.issuer

@jdubois
Copy link
Member

jdubois commented Oct 12, 2017

@danielpetisme I removed the old "oauth" code, and as we said we wouldn't use this property (as it's copy/pasted), there is no specific JHipster configuration here - that's why you don't see anything. If you want to use the lib, just clone its repo, and you can do a mvn install -> that will install your version in your local .m2 repo, so you will use your version instead of the official one.

@jdubois
Copy link
Member

jdubois commented Oct 12, 2017

@mraible oh I didn't see you were syncing users from OIDC:

  • that's a great feature, as I often have questions on using users from a microservice
  • but at the moment we don't do this with JWT authentication -> I don't want this feature to block us from merging this PR, we can still do it later - I'm not totally sure, is this causing an issue here?
  • and I'm also not sure of the solution: this means each microservice will get a "user" table, which is being copy/pasted from OIDC, and which is not always in sync. I don't have a better idea, but that looks weird to me.

@jdubois
Copy link
Member

jdubois commented Oct 12, 2017

  • I tried to fix the merge conflict
  • I'm testing this -> this is becoming so big, I'd love to merge this tonight, but it depends on what I find

@jdubois
Copy link
Member

jdubois commented Oct 12, 2017

There's a blocking bug at the moment: when you re-generate an app, it changes in your .yo-rc.json: "authenticationType": "oauth2" by "authenticationType": "jwt".
I don't know what changes that variable, that must be some control code in JHipster, but as this is changed, everything fails when you re-generate.

@jdubois jdubois merged commit 4ce5551 into jhipster:master Oct 13, 2017
@jdubois
Copy link
Member

jdubois commented Oct 13, 2017

Oops.... So sorry everyone, I merged this by mistake!!! I'm doing a JHipster training and I was trying to show a few tricks, and of course I messed up badly :-(
This is totally my fault, and I'm not available to clean up (as I'm doing the training...) - hopefully this isn't too bad, and anyway I wanted to merge this during the week-end.

@danielpetisme
Copy link
Member Author

I fixed the bug you mentioned @jdubois + I reuse @mraible user sync as-is for microservice architecture (only for gateways).
https://github.com/danielpetisme/generator-jhipster/tree/oidc-support-microservices

I think it's good enough for a first release.

Regarding the anticipated merge. Do you want me to create a new PR? I can create one to fix the bug and another for the user sync if you want.

@jdubois
Copy link
Member

jdubois commented Oct 13, 2017

Yes please ... really sorry

@danielpetisme
Copy link
Member Author

1 or 2 pr ?

@jdubois
Copy link
Member

jdubois commented Oct 13, 2017

Oh sorry - the easiest for you, so 1 I guess

@adnansenyurt
Copy link

I'm using generator v4.9.0 but I don't see keycloak.yml created under docker directory.
Also, is this ready for cloud deployment, or dev profile only? Shall I go back to 4.8.2 for now?

@jdubois
Copy link
Member

jdubois commented Oct 14, 2017

@adnansenyurt this isn't released yet, this why this ticket is not yet linked to a release version. You should use the master branch, please read the contributing doc.

@jdubois jdubois added this to the 4.10.0 milestone Oct 17, 2017
erikkemperman pushed a commit to erikkemperman/jhipster that referenced this pull request Oct 26, 2017
erikkemperman pushed a commit to erikkemperman/generator-jhipster that referenced this pull request Dec 3, 2017
erikkemperman pushed a commit to erikkemperman/generator-jhipster that referenced this pull request Dec 4, 2017
erikkemperman pushed a commit to erikkemperman/generator-jhipster that referenced this pull request Dec 4, 2017
erikkemperman pushed a commit to erikkemperman/generator-jhipster that referenced this pull request Dec 5, 2017
erikkemperman pushed a commit to erikkemperman/generator-jhipster that referenced this pull request Dec 5, 2017
erikkemperman pushed a commit to erikkemperman/generator-jhipster that referenced this pull request Dec 5, 2017
erikkemperman pushed a commit to erikkemperman/generator-jhipster that referenced this pull request Dec 5, 2017
erikkemperman pushed a commit to erikkemperman/generator-jhipster that referenced this pull request Dec 18, 2017
@mraible mraible mentioned this pull request Mar 13, 2018
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants