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 and microservices current issues #6519

Closed
jdubois opened this Issue Oct 14, 2017 · 34 comments

Comments

Projects
None yet
6 participants
@jdubois
Member

jdubois commented Oct 14, 2017

This is a ticket to follow up #6514 and to help discussing with @danielpetisme @mraible @sendilkumarn who are all helping here.

  • The current build is broken because we don't have UserMapper in a microservice -> @mraible and @danielpetisme introduced a method to sync user data between the gateway and the microservices, which makes the microservices have a "User" object. However, if you choose to use DTOs (another entity which uses a DTO has a relationship to the User), this means you also need a UserMapper and a UserDTO, specific for microservices. This sounds quite complex to me!
  • Once compilation is done, we also have a testing issue. The current code (if it compiled) would fail because resourceServerProperties.getJwt().getKeyUri() is not configured in the test resources application.yml file. Configuring it wouldn't solve the issue: you need an OIDC server to do the tests. That's also pretty bad, as this prevents us to do unit tests unless we run that server: as this works for monoliths without doing this, we need to check how it works for monoliths, and apply the same method to gateways and microservices.
  • We need to test monoliths, gateways and microservices with Cassandra
  • When doing a gateway + microservice: create an entity in the microservice, and then create its front-end on the gateway. There is a CSRF token issue that prevents write requests to work from the gateway to the microservice - I don't know if we should pass the CSRF token or ignore it (not sure OIDC allows CSRF attacks)
  • Swagger UI does not work with OAuth2
  • When doing a monolith, and selecting JHipster Registry, we currently force the use of JWT. We should be able to use OAuth2 also.
  • The JHipster Registry does not work with OIDC. We should do a specific Spring profile for OIDC.
  • For microservice, we need to check that doing csrf().disabled() in the Spring Security configuration doesn't cause a security risk
@mraible

This comment has been minimized.

Show comment
Hide comment
@mraible

mraible Oct 14, 2017

Contributor
Contributor

mraible commented Oct 14, 2017

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 14, 2017

Member

@mraible what, you don't know about microservices?? :-)
Indeed it works for monoliths - I don't know what you did, but we need the same for gateways and microservices -> I'm updating the todo list accordingly

Member

jdubois commented Oct 14, 2017

@mraible what, you don't know about microservices?? :-)
Indeed it works for monoliths - I don't know what you did, but we need the same for gateways and microservices -> I'm updating the todo list accordingly

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 14, 2017

Member

OK for microservices it's a bit different from monoliths as there is a JWT exchange from the OIDC server to the microservice - I think I got this, I'll commit something soon.

Member

jdubois commented Oct 14, 2017

OK for microservices it's a bit different from monoliths as there is a JWT exchange from the OIDC server to the microservice - I think I got this, I'll commit something soon.

@danielpetisme

This comment has been minimized.

Show comment
Hide comment
@danielpetisme

danielpetisme Oct 14, 2017

Contributor

@jdubois when you use uaa the microservice fails to start if the authorization server is down . I using uaa since 6months so yes it's annoying to have an OAuth server running but IMHO it's an understandable constraints.
For the unit test issue, maybe https://www.testcontainers.org/ could be a neat solution. I didn't tested yet but programatatically starting a Keycloak container for unit test could solve the issue.

Again with uaa I was used to create my User representation in service which need one + a uaa Feign client in the service. I didn't sync locally the user it only exist once in the IdP. Duplicating this responsability could be tricky (as always you try to maintain sync 2 things in different places).

Contributor

danielpetisme commented Oct 14, 2017

@jdubois when you use uaa the microservice fails to start if the authorization server is down . I using uaa since 6months so yes it's annoying to have an OAuth server running but IMHO it's an understandable constraints.
For the unit test issue, maybe https://www.testcontainers.org/ could be a neat solution. I didn't tested yet but programatatically starting a Keycloak container for unit test could solve the issue.

Again with uaa I was used to create my User representation in service which need one + a uaa Feign client in the service. I didn't sync locally the user it only exist once in the IdP. Duplicating this responsability could be tricky (as always you try to maintain sync 2 things in different places).

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 14, 2017

Member

Thanks @danielpetisme but it's solved easily, see my commit e4c34c7 above :-)

Member

jdubois commented Oct 14, 2017

Thanks @danielpetisme but it's solved easily, see my commit e4c34c7 above :-)

@danielpetisme

This comment has been minimized.

Show comment
Hide comment
@danielpetisme

danielpetisme Oct 14, 2017

Contributor
Contributor

danielpetisme commented Oct 14, 2017

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 15, 2017

Member

So for the first point, I don't think we can do a relationship to the User, when OIDC is selected, see my comment here. At least not in a first release. WDYT?

Member

jdubois commented Oct 15, 2017

So for the first point, I don't think we can do a relationship to the User, when OIDC is selected, see my comment here. At least not in a first release. WDYT?

@mraible

This comment has been minimized.

Show comment
Hide comment
@mraible

mraible Oct 15, 2017

Contributor

@jdubois I'd vote that we don't allow a relationship to User when microservices is selected, but we do when it's a monolith. I had that working with my user synching code. That way, the following expression works.

@Query("select blog from Blog blog where blog.user.login = ?#{principal.username}")
Contributor

mraible commented Oct 15, 2017

@jdubois I'd vote that we don't allow a relationship to User when microservices is selected, but we do when it's a monolith. I had that working with my user synching code. That way, the following expression works.

@Query("select blog from Blog blog where blog.user.login = ?#{principal.username}")
@danielpetisme

This comment has been minimized.

Show comment
Hide comment
@danielpetisme

danielpetisme Oct 15, 2017

Contributor

+1

Contributor

danielpetisme commented Oct 15, 2017

+1

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 15, 2017

Member

Yes, totally agree. And that would be consistent with JWT authentication which is perfect.

Member

jdubois commented Oct 15, 2017

Yes, totally agree. And that would be consistent with JWT authentication which is perfect.

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 15, 2017

Member

Oh sorry @mraible @danielpetisme I started testing the monolith (I was just working on microservices) but that's the same issue: as long as the User isn't a JPA entity you can't do relationships with it.

  • I don't think that could work
  • With DTOs that's going to be even more complex
  • I'm worried to have a User table in each application, that is just a copy of the OIDC server, that's not good to copy/paste such information everywhere

I'm afraid we are stuck here, unless there's something I don't understand? Otherwise I would like to remove that user sync code, so we can move on with a first release (lots of people and PRs are waiting). If we find a solution later, we can still add it in a future release.

Member

jdubois commented Oct 15, 2017

Oh sorry @mraible @danielpetisme I started testing the monolith (I was just working on microservices) but that's the same issue: as long as the User isn't a JPA entity you can't do relationships with it.

  • I don't think that could work
  • With DTOs that's going to be even more complex
  • I'm worried to have a User table in each application, that is just a copy of the OIDC server, that's not good to copy/paste such information everywhere

I'm afraid we are stuck here, unless there's something I don't understand? Otherwise I would like to remove that user sync code, so we can move on with a first release (lots of people and PRs are waiting). If we find a solution later, we can still add it in a future release.

@mraible

This comment has been minimized.

Show comment
Hide comment
@mraible

mraible Oct 15, 2017

Contributor

@jdubois I'm working on making monoliths work with a User table and JPA entity. This was working before the accidental merge of OIDC microservices. I'll keep the current implementation for microservices.

Contributor

mraible commented Oct 15, 2017

@jdubois I'm working on making monoliths work with a User table and JPA entity. This was working before the accidental merge of OIDC microservices. I'll keep the current implementation for microservices.

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 15, 2017

Member

Oh OK @mraible I'm eager to see your code

Member

jdubois commented Oct 15, 2017

Oh OK @mraible I'm eager to see your code

@sendilkumarn

This comment has been minimized.

Show comment
Hide comment
@sendilkumarn

sendilkumarn Oct 15, 2017

Contributor

I do agree with @mraible here. I saw his code was working well for monolith.

👍 for removing that in microservices

Contributor

sendilkumarn commented Oct 15, 2017

I do agree with @mraible here. I saw his code was working well for monolith.

👍 for removing that in microservices

@danielpetisme

This comment has been minimized.

Show comment
Hide comment
@danielpetisme

danielpetisme Oct 15, 2017

Contributor

This is the snippet I used to code to fullfill the api/account endpoint without syncing the user.

   /**
     * GET  /authenticate : check if the user is authenticated, and return its login.
     *
     * @param request the HTTP request
     * @return the login if the user is authenticated
     */
    @GetMapping("/authenticate")
    @Timed
    public String isAuthenticated(HttpServletRequest request) {
        log.debug("REST request to check if the current user is authenticated");
        return request.getRemoteUser();
    }

   /**
     * GET  /account : get the current user.
     *
     * @return the current user
     * @throws RuntimeException 500 (Internal Server Error) if the user couldn't be returned
     */
    @GetMapping("/account")
    @Timed
    @SuppressWarnings("unchecked")
    public ResponseEntity<User> getAccount(Principal principal) {
        return Optional.ofNullable(principal)
            .filter(it -> it instanceof OAuth2Authentication)
            .map(it -> ((OAuth2Authentication) it).getUserAuthentication())
            .map(authentication -> {
                    Map<String, Object> details = (Map<String, Object>) authentication.getDetails();
                    return new User(
                        authentication.getName(),
                        (String) details.get("given_name"),
                        (String) details.get("family_name"),
                        (String) details.get("email"),
                        (String) details.get("langKey"),
                        (String) details.get("imageUrl"),
                        (Boolean) details.get("email_verified"),
                        authentication.getAuthorities().stream().map(it -> it.getAuthority()).collect(Collectors.toSet())
                    );
                }
            )
            .map(user -> new ResponseEntity<>(user, HttpStatus.OK))
            .orElse(new ResponseEntity<>(HttpStatus.INTERNAL_SERVER_ERROR));
    }
Contributor

danielpetisme commented Oct 15, 2017

This is the snippet I used to code to fullfill the api/account endpoint without syncing the user.

   /**
     * GET  /authenticate : check if the user is authenticated, and return its login.
     *
     * @param request the HTTP request
     * @return the login if the user is authenticated
     */
    @GetMapping("/authenticate")
    @Timed
    public String isAuthenticated(HttpServletRequest request) {
        log.debug("REST request to check if the current user is authenticated");
        return request.getRemoteUser();
    }

   /**
     * GET  /account : get the current user.
     *
     * @return the current user
     * @throws RuntimeException 500 (Internal Server Error) if the user couldn't be returned
     */
    @GetMapping("/account")
    @Timed
    @SuppressWarnings("unchecked")
    public ResponseEntity<User> getAccount(Principal principal) {
        return Optional.ofNullable(principal)
            .filter(it -> it instanceof OAuth2Authentication)
            .map(it -> ((OAuth2Authentication) it).getUserAuthentication())
            .map(authentication -> {
                    Map<String, Object> details = (Map<String, Object>) authentication.getDetails();
                    return new User(
                        authentication.getName(),
                        (String) details.get("given_name"),
                        (String) details.get("family_name"),
                        (String) details.get("email"),
                        (String) details.get("langKey"),
                        (String) details.get("imageUrl"),
                        (Boolean) details.get("email_verified"),
                        authentication.getAuthorities().stream().map(it -> it.getAuthority()).collect(Collectors.toSet())
                    );
                }
            )
            .map(user -> new ResponseEntity<>(user, HttpStatus.OK))
            .orElse(new ResponseEntity<>(HttpStatus.INTERNAL_SERVER_ERROR));
    }
@mraible

This comment has been minimized.

Show comment
Hide comment
@mraible

mraible Oct 15, 2017

Contributor

I just pushed changes to add user synchronization back to an OAuth monolith. PR > #6523.

@danielpetisme I found that (String) details.get("email") throws a NPE. Not sure if you need a null check for microservices too.

Contributor

mraible commented Oct 15, 2017

I just pushed changes to add user synchronization back to an OAuth monolith. PR > #6523.

@danielpetisme I found that (String) details.get("email") throws a NPE. Not sure if you need a null check for microservices too.

@mraible

This comment has been minimized.

Show comment
Hide comment
@mraible

mraible Oct 15, 2017

Contributor

@jdubois Build should be fixed if you merge #6523.

Contributor

mraible commented Oct 15, 2017

@jdubois Build should be fixed if you merge #6523.

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 16, 2017

Member

So the big issues are fixed, I just added a new todo on a CSRF issue - I need to check this a little bit and we'll probably end up removing CSRF on microservices (as it basically doesn't make any sense)

Member

jdubois commented Oct 16, 2017

So the big issues are fixed, I just added a new todo on a CSRF issue - I need to check this a little bit and we'll probably end up removing CSRF on microservices (as it basically doesn't make any sense)

@sdoxsee

This comment has been minimized.

Show comment
Hide comment
@sdoxsee

sdoxsee Oct 16, 2017

Contributor

@jdubois I think you're right. As far as I understand it, CSRF makes sense from browser to gateway for session-based oidc but not from gateway to downstream microservices as those receive JWTs.

Contributor

sdoxsee commented Oct 16, 2017

@jdubois I think you're right. As far as I understand it, CSRF makes sense from browser to gateway for session-based oidc but not from gateway to downstream microservices as those receive JWTs.

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 16, 2017

Member

Thanks @sdoxsee I just checked and indeed we have csrf().disabled() in our Spring Security config from the beginning, both for gateways and microservices.
Here the error message comes from a microservice, and I have no idea how to remove it, as it's already disabled!!! Totally stuck here :-(

Member

jdubois commented Oct 16, 2017

Thanks @sdoxsee I just checked and indeed we have csrf().disabled() in our Spring Security config from the beginning, both for gateways and microservices.
Here the error message comes from a microservice, and I have no idea how to remove it, as it's already disabled!!! Totally stuck here :-(

@danielpetisme

This comment has been minimized.

Show comment
Hide comment
@danielpetisme

danielpetisme Oct 17, 2017

Contributor

@jdubois did you get a specific error message? How can I reproduce?

Contributor

danielpetisme commented Oct 17, 2017

@jdubois did you get a specific error message? How can I reproduce?

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 17, 2017

Member

@danielpetisme thanks for helping, here are:

They both use the code from the current master branch, and need a JHipster Registry and Keycloak to work, so in the gateway you must run:

docker-compose -f src/main/docker/jhipster-registry.yml up -d
docker-compose -f src/main/docker/keycloak.yml up -d

Then start both projects by running ./mvnw

Once you are logged in, go to the Yolo entity and create a new entity. This entity comes from the microservice, and is served through the Zuul proxy.

On the first "create" request you will get:

screenshot 2017-10-17 12 18 05

And if you do a second "create" request:

screenshot 2017-10-17 12 18 14

Member

jdubois commented Oct 17, 2017

@danielpetisme thanks for helping, here are:

They both use the code from the current master branch, and need a JHipster Registry and Keycloak to work, so in the gateway you must run:

docker-compose -f src/main/docker/jhipster-registry.yml up -d
docker-compose -f src/main/docker/keycloak.yml up -d

Then start both projects by running ./mvnw

Once you are logged in, go to the Yolo entity and create a new entity. This entity comes from the microservice, and is served through the Zuul proxy.

On the first "create" request you will get:

screenshot 2017-10-17 12 18 05

And if you do a second "create" request:

screenshot 2017-10-17 12 18 14

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 17, 2017

Member

@danielpetisme this is worrying:

  • In order to work, you must disable CSRF in the OAuth2SsoConfiguration in the gateway. That's weird as this is already disabled in MicroserviceSecurityConfiguration but the conf there looks ignored.
  • From what I see, there is a stateful mechanism here, so does that mean we should keep the CSRF protection?
  • Once this is done, I have an "unauthorized" error on the microservice: it looks like security did not get propagated...
Member

jdubois commented Oct 17, 2017

@danielpetisme this is worrying:

  • In order to work, you must disable CSRF in the OAuth2SsoConfiguration in the gateway. That's weird as this is already disabled in MicroserviceSecurityConfiguration but the conf there looks ignored.
  • From what I see, there is a stateful mechanism here, so does that mean we should keep the CSRF protection?
  • Once this is done, I have an "unauthorized" error on the microservice: it looks like security did not get propagated...
@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 17, 2017

Member

OK I got it working and I'm going to push my changes soon:

  • I'm disabling CSRF in OAuth2SsoConfiguration as mentioned in my comment above: I'm not sure it causes an issue (which is worrying, and which is why this is still BETA), but anyway I don't think this can work in a microservice architecture (as the CSRF token would need to be shared everywhere, which is stupid)
  • I just realized that @danielpetisme only worked on making authentication work with a Feign client, using TokenRelayRequestInterceptor. I just needed to use the same code in the Zuul TokenRelayFilter on the gateway, and you have Zuul working :-)
Member

jdubois commented Oct 17, 2017

OK I got it working and I'm going to push my changes soon:

  • I'm disabling CSRF in OAuth2SsoConfiguration as mentioned in my comment above: I'm not sure it causes an issue (which is worrying, and which is why this is still BETA), but anyway I don't think this can work in a microservice architecture (as the CSRF token would need to be shared everywhere, which is stupid)
  • I just realized that @danielpetisme only worked on making authentication work with a Feign client, using TokenRelayRequestInterceptor. I just needed to use the same code in the Zuul TokenRelayFilter on the gateway, and you have Zuul working :-)
@danielpetisme

This comment has been minimized.

Show comment
Hide comment
@danielpetisme

danielpetisme Oct 17, 2017

Contributor
Contributor

danielpetisme commented Oct 17, 2017

@jdubois jdubois referenced this issue Oct 17, 2017

Merged

Make Zuul requests work with OAuth2 #6548

4 of 4 tasks complete
@danielpetisme

This comment has been minimized.

Show comment
Hide comment
@danielpetisme

danielpetisme Oct 17, 2017

Contributor

after you merge #6548 is there other blocking issues you're aware of?

Contributor

danielpetisme commented Oct 17, 2017

after you merge #6548 is there other blocking issues you're aware of?

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 17, 2017

Member

Not blocker, but Swagger UI is not working

Member

jdubois commented Oct 17, 2017

Not blocker, but Swagger UI is not working

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 17, 2017

Member

@danielpetisme I'm adding some stuff in the todo list on top of this ticket

Member

jdubois commented Oct 17, 2017

@danielpetisme I'm adding some stuff in the todo list on top of this ticket

@danielpetisme

This comment has been minimized.

Show comment
Hide comment
@danielpetisme

danielpetisme Oct 17, 2017

Contributor

@jdubois the swagger UI issue comes from a frame options configuration. It's set to deny.
In OAuth2SsoConfiguration update the method like the following snippet and it work.

@Override
    protected void configure(HttpSecurity http) throws Exception {
        http
            .headers().frameOptions().sameOrigin();
        http
            .requestMatcher(new NegatedRequestMatcher(authorizationHeaderRequestMatcher))
            .authorizeRequests()
            .anyRequest().permitAll();
    }

it looks like a common issue for application using oauth and trying to embed external content into an iframe (like youtube for instance).

Contributor

danielpetisme commented Oct 17, 2017

@jdubois the swagger UI issue comes from a frame options configuration. It's set to deny.
In OAuth2SsoConfiguration update the method like the following snippet and it work.

@Override
    protected void configure(HttpSecurity http) throws Exception {
        http
            .headers().frameOptions().sameOrigin();
        http
            .requestMatcher(new NegatedRequestMatcher(authorizationHeaderRequestMatcher))
            .authorizeRequests()
            .anyRequest().permitAll();
    }

it looks like a common issue for application using oauth and trying to embed external content into an iframe (like youtube for instance).

jdubois added a commit to jdubois/generator-jhipster that referenced this issue Oct 17, 2017

jdubois added a commit to jdubois/generator-jhipster that referenced this issue Oct 17, 2017

Keep the "authorization" ignored header
Some people could use it to abuse the system, as you could have 2 "authorization" headers. It's better not to trust the one from the browser.

See jhipster#6519

jdubois added a commit that referenced this issue Oct 17, 2017

Merge pull request #6549 from jdubois/fix-do-not-force-jwt-6519
Do not force JWT when Eureka is selected, see #6519
@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 18, 2017

Member

I'm adding a new todo item: the JHipster Registry just doesn't work with OIDC. That should be simple to solve, but then do we do a specific JHipster Registry build? Or add a Spring profile?

Member

jdubois commented Oct 18, 2017

I'm adding a new todo item: the JHipster Registry just doesn't work with OIDC. That should be simple to solve, but then do we do a specific JHipster Registry build? Or add a Spring profile?

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 19, 2017

Member

I started to implement OIDC support in the JHipster Registry -> the ticket on the registry is jhipster/jhipster-registry#193

Member

jdubois commented Oct 19, 2017

I started to implement OIDC support in the JHipster Registry -> the ticket on the registry is jhipster/jhipster-registry#193

@danielpetisme

This comment has been minimized.

Show comment
Hide comment
@danielpetisme

danielpetisme Oct 22, 2017

Contributor

I have a working prototype of OIDC with angular-oauth2-oidc that means no more user management code on the backend side (except if you need a user sync). Wondering if it interests anyone? Does it worth a PR?
The implementation requires to have a look to #6565

Contributor

danielpetisme commented Oct 22, 2017

I have a working prototype of OIDC with angular-oauth2-oidc that means no more user management code on the backend side (except if you need a user sync). Wondering if it interests anyone? Does it worth a PR?
The implementation requires to have a look to #6565

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 22, 2017

Member

@danielpetisme I have no idea what this will do - at the moment I'm still stuck with jhipster/jhipster-registry#193 - I spent a few hours on it today again, but no luck :-(

Member

jdubois commented Oct 22, 2017

@danielpetisme I have no idea what this will do - at the moment I'm still stuck with jhipster/jhipster-registry#193 - I spent a few hours on it today again, but no luck :-(

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 27, 2017

Member

Closing as everything is done

Member

jdubois commented Oct 27, 2017

Closing as everything is done

@jdubois jdubois closed this Oct 27, 2017

@jdubois jdubois added this to the 4.10.2 milestone Oct 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment