jhipster / generator-jhipster

JHipster is a development platform to quickly generate, develop, & deploy modern web applications & microservice architectures.
https://www.jhipster.tech
Apache License 2.0
21.47k stars 4.02k forks source link

Access/Refresh Token Handling for UAA Authentication #5752

Closed Metavirulent closed 7 years ago

Metavirulent commented 7 years ago

I want to propose a pull request improving token handling for microservice gateways with authenticationType==='uaa' fixing critical security issues with the current implementation. The proposed changes are:

This fixes a number of security and usability problems as described below.

Currently, for authenticationType==='uaa', only the access token is used, the refresh token is discarded. Furthermore, the access token is stored in the localStorage/sessionStorage and thus accessible from within Javascript.

This has the following security problems:

For security-sensitive applications that require an external audit, these are show-stoppers.

In addition to these security issues, there are a few additional problems:

n/a

n/a

I'd prepare a pull request that

The only impact would be that the JS code won't have access to the token anymore, in particular:

Let me know what you think.

jdubois commented 7 years ago
cbornet commented 7 years ago

An old related discussion : https://github.com/jhipster/generator-jhipster/issues/3405

cbornet commented 7 years ago

This one also: #4278

Metavirulent commented 7 years ago

@cbornet thanks for the links. I see that the XSS vulnerability and other issues have been discussed already, but it's not just about where to store the OAuth2 tokens, but also about which tokens. In a microservice architecture with UAA authentication, the refresh token is returned during the password grant but it is discarded. That puts all the burden on the poor access token which shouldn't even have to be stored persistently nor live nearly as long.

It also means your session duration is hardcoded and being active in the application doesn't prevent the session from expiring. Like I explained above, it also means that you cannot easily share tokens across gateways belonging to the same top level domain.

@jdubois regarding keycloak: it's been a while since I last was looking into it but AFAIK, it has ready-made adapters that do the job for various languages and environments. When it comes to token storage, you get the choice between HTTP sessions and cookies, IIRC. What I didn't like about their cookies back then is that they were not set on the top-level domain making SSO impossible.

Using their adapters would also result in a very keycloak specific solution. I'm proposing a more Spring OAuth2-friendly solution here that works well with all the Spring OAuth2 code the JHipster microservice architecture currently relies upon.

It should theoretically also work for another token infrastructure like keycloak, even though there are a number of different issues to be solved that don't exist with UAA yet, the most prominent being multi-tenancy (or realms in keycloak terms).

I'd start out with making refresh tokens work with UAA first and make sure not to lock it into being too UAA specific so it can be easily adapted later (e.g. by changing the token endpoint URL).

sdoxsee commented 7 years ago

Great ideas. How would you get SSO without a login view on UAA itself? Redirection is key to SSO...or would we provide a login view on UAA now?

xetys commented 7 years ago

@sdoxsee I assume, after doing a password grant authentication, the server stores the token into a cookie, which the browser will add on further requests automatically, while still returning the tokens like before. This cookie, when "HttpOnly" is not accessible over JavaScript, implying there is no need to handle token stuff in the angular client.

In general, I think this is a good step towards a better security and "remember me" function, as it is now. About the "SSO" benefit, this is not that easy. It's not the classical SSO from OAuth2. What @Metavirulent means, is that SSO like experience. Consider all your gateways are on the same hostname and separated via paths. Once you sign on the host "example.com/someGateway/uaa", the cookie is valid for all other gateways, too. So there is no redirection needed.

So, if any help is needed here, let me know!

jdubois commented 7 years ago

Thanks @Metavirulent I have just discussed those Keycloak issues with @bleporini on Friday, and as far as I understand he wanted to do the same thing as you -> any chance that you both could team up on this?

Metavirulent commented 7 years ago

Full keycloak integration is a different beast, as the current UI is also talking to UAA directly, e.g. in Account service. I'd rather limit this issue #5752 to the scope of the missing refresh grant.

I'll make sure that the same mechanism can be used with keycloak by abstracting the token endpoint client so we can hook in a keycloak specific implementation.

I'll also get rid of the limitation that UAA is required during microservice startup as this really impacts availability. Microservices probably cannot talk to each other without UAA being there, but they should still launch properly.

sdoxsee commented 7 years ago

@Metavirulent removing the check for the UAA's public key will mean each service will need to manually configure the key. It's a tradeoff. I prefer the reference but can see why one might not like it.

Any other opinions before we pull it?

Metavirulent commented 7 years ago

@sdoxsee actually, I want each microservice to try fetch the key when it starts but fail gracefully if it cannot, then go fetch it later when needed. Obviously, if UAA continues to be down, then no token check will be possible and thus all requests will be denied, but at least once UAA comes back up, the microservice can be back in action.

On a similar note, it should fetch it again if the verification fails because the key could have been replaced on UAA and we don't want to reboot all services in that case. Maybe we add some kind of time limit to avoid spamming UAA.

sdoxsee commented 7 years ago

Excellent! Even better. I know there's been some work on key-uri config and key rotation that's progressed since the key lookup was coded in jhipster. It might help... but maybe not. See https://github.com/spring-projects/spring-boot/issues/4437 and I'm sure you're aware of the re-write of oauth2 handling that's under way https://spring.io/blog/2017/05/11/spring-security-5-0-0-m1#new-support-for-oauth-2-0-and-openid-connect-1-0

Thanks for looking into this.

On Tue, May 16, 2017, 4:15 PM Markus Öllinger notifications@github.com wrote:

@sdoxsee https://github.com/sdoxsee actually, I want each microservice to try fetch the key when it starts but fail gracefully if it cannot, then go fetch it later when needed. Obviously, if UAA continues to be down, then no token check will be possible and thus all requests will be denied, but at least once UAA comes back up, the microservice can be back in action.

On a similar note, it should fetch it again if the verification fails because the key could have been replaced on UAA and we don't want to reboot all services in that case. Maybe we add some kind of time limit to avoid spamming UAA.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/5752#issuecomment-301901878, or mute the thread https://github.com/notifications/unsubscribe-auth/ABHncqPm_xiCtBuMbv04D2MqGnXLI5VKks5r6gPBgaJpZM4NZZSe .

Metavirulent commented 7 years ago

I need a bit of help on how to get travis run again. A recent commit changed yo jhipster to jhipster in travis/scripts/02-generate-project.sh and now travis won't work on my machine anymore.

What am I missing?

pascalgrimaud commented 7 years ago

@Metavirulent : We added JHipster CLI. See https://github.com/jhipster/generator-jhipster/pull/5726 and https://github.com/jhipster/generator-jhipster/issues/5747

Metavirulent commented 7 years ago

@pascalgrimaud thanks, I already though so. This is completely broken on my machine. I tried it with yarn on a Mac and on Windows but to no avail. I can't even do a yarn link on my generator-jhipster folder anymore without getting an EPERM: operation not permitted, symlink .... On Windows this may have to do with me not having admin rights.

I don't have time to figure out why this is broken so I'll just insert the yo manually for now and hope this gets sorted out before release (or becomes optional, as it doesn't seem to be stable for all the different evnironments out there).

deepu105 commented 7 years ago

Could you give some more info like stack trace etc? Coz it looks like you dont have install rights. Doesn npm i -g generator-jhipster work for you?

On Thu, 18 May 2017, 10:10 am Markus Öllinger, notifications@github.com wrote:

@pascalgrimaud https://github.com/pascalgrimaud thanks, I already though so. This is completely broken on my machine. I tried it with yarn on a Mac and on Windows but to no avail. I can't even do a yarn link on my generator-jhipster folder anymore without getting an EPERM: operation not permitted, symlink .... On Windows this may have to do with me not having admin rights.

I don't have time to figure out why this is broken so I'll just insert the yo manually for now and hope this gets sorted out before release (or becomes optional, as it doesn't seem to be stable for all the different evnironments out there).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/5752#issuecomment-302332984, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlFxaRd6xXb5pJw-1mSBS9doPm8akgks5r6_0QgaJpZM4NZZSe .

deepu105 commented 7 years ago

Also did you follow the instructions to set it up? You need to run npm i -g on the generator-jhipster folder first and then do npm link

On Thu, 18 May 2017, 11:12 am Deepu K Sasidharan, < deepu.k.sasidharan@gmail.com> wrote:

Could you give some more info like stack trace etc? Coz it looks like you dont have install rights. Doesn npm i -g generator-jhipster work for you?

On Thu, 18 May 2017, 10:10 am Markus Öllinger, notifications@github.com wrote:

@pascalgrimaud https://github.com/pascalgrimaud thanks, I already though so. This is completely broken on my machine. I tried it with yarn on a Mac and on Windows but to no avail. I can't even do a yarn link on my generator-jhipster folder anymore without getting an EPERM: operation not permitted, symlink .... On Windows this may have to do with me not having admin rights.

I don't have time to figure out why this is broken so I'll just insert the yo manually for now and hope this gets sorted out before release (or becomes optional, as it doesn't seem to be stable for all the different evnironments out there).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/5752#issuecomment-302332984, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlFxaRd6xXb5pJw-1mSBS9doPm8akgks5r6_0QgaJpZM4NZZSe .

Metavirulent commented 7 years ago

Everything used to work until I pulled the changes. Here's the debug output of npm i -g.

npm-debug.zip

If I try the same with yarn, then no error shows up but it says tabtab:commands User shell undefined not supported, skipping completion install +0ms. yarn-error.zip

This is all on Windows, I don't have access to my Mac right now.

deepu105 commented 7 years ago

Ok let me check. I think its a windows specific issue. This shouldn't happen on a unix like env

On Thu, 18 May 2017, 12:51 pm Markus Öllinger, notifications@github.com wrote:

Everything used to work until I pulled the changes. Here's the debug output of npm i -g.

npm-debug.zip https://github.com/jhipster/generator-jhipster/files/1010616/npm-debug.zip

If I try the same with yarn, then no error shows up but it says tabtab:commands User shell undefined not supported, skipping completion install +0ms.

This is all on Windows, I don't have access to my Mac right now.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/5752#issuecomment-302370322, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlF-Rl1KJyTHpEmOvK9NC_04viqmyGks5r7CLHgaJpZM4NZZSe .

Metavirulent commented 7 years ago

Not sure. Here's the result of npm i -g on my Mac.

npm-debug.log.zip

ThomasWilhelmUR commented 7 years ago

In response to @jdubois top-most answer ("There is also an effort under way to integrate Keycloak with JHipster [...]"):

We are currently using Keycloak with JHipster. We scaffolded the whole architecture using a UAA-based setup (UAA + Services + Gateway + Console etc.). Afterwards we (or rather my colleague) removed the UAA and rewired the gateway to use Keycloak's openID endpoints (and consequently tokens).

In order to know where to start and how to get it running, I really dug deep into the JHipster code. Our progress was a little bit messy but when it clicked it was "easy" - thanks to well written code and good documentation on your part.

I cannot speak to the quality of our solution but if @xetys or the others in this thread are interested, I can share the changes and go into details. (We could PR this, but I would need some guidance there).

greetings

xetys commented 7 years ago

hey,

my big aim with UAA was to build a most compatible solution, so all mechanisms are built using OAuth 2.0 protocol. So I am really sure, there should be no problems of run a keycloak instance instead of UAA. The only part is, you either need some changes on the UI in favor of skipping the Account pages, as they are managed by keycloak, or doing something like "User Profile Service" which aggregates over the user view from keycloak to fit into the existing APIs. However it would be cool for me to see a working setup. A good working demo setup is enough for me, to make a keycloak PR to JHipster, if this appreciated so much.

Addressing the rest of the discussion, I'm happy to see any progress. Especially I want to get rid of the UAA requirement on boot up. We could use the same mechanism, as we currently use to share the secret key of JWT. We can do the same thing to the public key, and that would make thinks a lot easier. Alternatively, we could look how to use Vault, Lemur or other secret sharing systems to achieve this, as this is a security question, and especially for our JWT secret key (which is symmetric, so I really dislike that fact), which can be used to create valid JWT. Both, basic auth for cloud config, or token endpoint exposure are too unsafe for thus purpose.

So if @Metavirulent is stuck on any part, let me know.

Metavirulent commented 7 years ago

Things look pretty good, here's a quick overview of the improvements I've implemented:

It's quite a bit of code but I think things are better now. I'll give it a few more tests and then send the PR. There's still more we could do security-wise, there's no real CSRF protection, no protection vs. click-jacking and no content-security-policy to name a few, but that'd probably for another PR.

ghost commented 7 years ago

So when access token expire, and we are asking for new one then we should send new refresh token too ? Or keep refresh token until he expire ?

I mean configuration like that


@Primary
public DefaultTokenServices tokenServices() {
    DefaultTokenServices defaultTokenServices = new DefaultTokenServices();
    defaultTokenServices.setTokenStore(tokenStore());
    defaultTokenServices.setSupportRefreshToken(true);
    defaultTokenServices.setReuseRefreshToken(false);  //return new refresh token ? 
}
Metavirulent commented 7 years ago

@lukasw44 good point. Refresh tokens won't be reused because they now get a standard "iat" claim holding the UTC timestamp in seconds when they have been created. This is used for session inactivity timeout. If we reuse the refresh tokens, then you'll eventually run into the timeout even though you have been active.

If you don't plan to use session inactivity timeout, then reuse is possible but by default, UAA will be configured to create a new refresh token.

guido8686 commented 7 years ago

I like the idea of using keycloak integration

rosefirefish commented 7 years ago

Um...I think my question may relevant to this one...I want to set the expire time for the token on the uaa architect, how can I do it? Now, the token just keep working without need of refresh, one can get access to login page as long as the browser tab is open... I have UAA server with jhipster version 4.6.2, and gateway server with version 4.6.2 too. I tried to read the 'UaaConfiguration.java', didn't find 'AccessTokenValiditySeconds' setting in it... if I manually add it using .accessTokenValiditySeconds(jHipsterProperties.getSecurity().getAuthentication().getOauth().getTokenValidityInSeconds()) it will work as described in this issue, refresh will not update the token validity, so, one could get kicked out any time during operation.

Can anybody tell me, if this will get updated in later version of jHipster UAA, or, am I doing something wrong?

Metavirulent commented 7 years ago

I'm currently on vacation in le Var (been evacuated from wood fire). I'll look into it as soon as I'll be vack.

deepu105 commented 7 years ago

PR #5812 merged