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 #124

Closed
daniel-ac-martin opened this issue Sep 8, 2020 · 36 comments
Closed

OIDC support #124

daniel-ac-martin opened this issue Sep 8, 2020 · 36 comments

Comments

@daniel-ac-martin
Copy link
Owner

We should build in support for authentication with OpenID Connect.

This is complicated by isomorphic rendering. i.e. Really the initial login should be done via SSR but the client should take over the handling of tokens as well as allow us to inspect the login information from the token. Meanwhile, any API endpoints need to support inspecting tokens outside of React. (And in fact, that is the most important point to get right in terms of security.)

It would probably be best if the SSR React component would put the tokens in a cookie. I think this is how louketo-proxy works. I'd like to avoid forcing a server-side session store if at all possible.

@daniel-ac-martin
Copy link
Owner Author

There are a few options in terms of existing React libraries. Although many of them seem to be poorly documented.

  1. https://github.com/bjerkio/oidc-react
    Nice and simple interface. Uses React Context API and provides a hook. Less popular. Not clear if it supports SSR.
  2. https://github.com/AxaGuilDEv/react-oidc
    Provides React Context API option. Not clear if it supports SSR.
  3. https://github.com/maxmantz/redux-oidc
    Requires Redux but should support SSR.

They all seem to use the same underlying library: https://github.com/IdentityModel/oidc-client-js
(So perhaps we should consider that for protecting API endpoints.)

@daniel-ac-martin
Copy link
Owner Author

Whilst the use of Redux for this feels wrong to me, (it only needs to store some tokens doesn't it? I wouldn't think there would be that much async stuff going on. Then again, maybe there is...) perhaps it wouldn't be the end of the world if we already need Redux for #125.

@daniel-ac-martin
Copy link
Owner Author

Possibly, Redux is useful for handling the updating of access tokens.

Then again, I wonder if that would be so infrequent that it could be done synchronously.

@daniel-ac-martin
Copy link
Owner Author

Another concern for this work is the security of tokens. I suspect that cookies might be better than local storage.

Or perhaps we could be even more cautious. Perhaps the server could return the tokens in headers (or a cookie) and the app could just put them in memory and delete any cookies.

@daniel-ac-martin
Copy link
Owner Author

oidc-react appears to not support SSR. Issue raised at: bjerkio/oidc-react#329

@daniel-ac-martin
Copy link
Owner Author

It occurs to me that this isn't necessarily a React issue. We could solve most of this problem with just a middleware on the server.

The problem would only arise once the access token expires. As at that point queries to GraphQL would start to fail.

@daniel-ac-martin
Copy link
Owner Author

Another contender might be https://github.com/react-keycloak/react-keycloak .

However this would make the solution specific to Keycloak and I'm not sure what the benefit of that would be. - If Keycloak implements OIDC then surely we can just work with that?

@daniel-ac-martin
Copy link
Owner Author

So far the libraries appear to be oriented towards the client-side and even require a client with a 'public' access type (no client secret). - We would normally only use 'confidential' on UIs and 'bearer-only' on APIs.

I wonder if it would be better to write our own one specifically for isomorphic applications. To my mind the ideal process would be:

  1. User visits website without a valid access token.
  2. Server redirects user to Keycloak to login
  3. User logs in, gets redirected back with an auth code
  4. Server exchanges the auth code with Keycloak for a access and refresh tokens, using its client id and secret, and places the access token in a cookie. (We'd also want to send the refresh token to the client but I'm not sure how. We should check what louketo-proxy does.)
  5. The client hydrates and takes over responsibility for keeping the access token up to date using the refresh token.

i.e. The client would never need to do an actual login as it would be receiving tokens from the server.

I'm not sure anything out there works like this currently. It's also possible that I've misunderstood something.

@daniel-ac-martin
Copy link
Owner Author

Looks like louketo-proxy puts the refresh token in a separate cookie called kc-refresh: https://github.com/louketo/louketo-proxy/blob/26d77f1635fba0a0503b5da584929f43d95b5a47/doc.go#L68

It looks like it is encrypted: https://github.com/louketo/louketo-proxy/blob/master/docs/user-guide.md#encryption-key

It also looks like it's possible to use a server-side data store for them instead.

It makes sense to take more care over handling the refresh token as it is a longer-lived credential. However, if we want the client to handle updating the access token we will need to find a way of sharing the refresh token with it in an unencrypted form. We could have a special endpoint that the client could hit in order to receive the unencrypted refresh token?

@daniel-ac-martin
Copy link
Owner Author

(I say unencrypted, but I only mean unencrypted at rest; TLS would be in place for the transit, at least in production.)

@daniel-ac-martin
Copy link
Owner Author

Another alternative might be to not have the client even do the refreshing of access tokens. Perhaps the client could simply refresh the page when the access token expires and let the server do it.

Although there's probably a big risk of losing the users work with this strategy...

@daniel-ac-martin
Copy link
Owner Author

daniel-ac-martin commented Nov 4, 2020

Typically, refresh tokens are only used with confidential clients. However, since it is possible to use the authorization code flow without a client secret, the refresh grant may also be used by clients that don’t have a secret. If the client was issued a secret, then the client must authenticate this request. Typically the service will allow either additional request parameters client_id and client_secret, or accept the client ID and secret in the HTTP Basic auth header. If the client does not have a secret, then no client authentication will be present in this request.

https://www.oauth.com/oauth2-servers/access-tokens/refreshing-access-tokens/

@daniel-ac-martin
Copy link
Owner Author

daniel-ac-martin commented Nov 4, 2020

Perhaps, rather than ever holding the (unencrypted) refresh token, the client could hit a special endpoint on the server to get a new access token? - This might be a better fit given the server will be using the auth-code flow.

@daniel-ac-martin
Copy link
Owner Author

The server then checks whether the refresh token is valid, and has not expired. If the refresh token was issued to a confidential client, the service must ensure the refresh token in the request was issued to the authenticated client.

https://www.oauth.com/oauth2-servers/access-tokens/refreshing-access-tokens/

This suggests that it is the server that should get the new access token.

It does sort of imply that the request token is the real credential here. i.e. What's the point of the access token?

@daniel-ac-martin
Copy link
Owner Author

daniel-ac-martin commented Nov 4, 2020

What's the point of the access token?

I suppose it means that you are checking in with the auth / SSO service. i.e. The Auth service can refuse you a new access token if the session has been revoked.

@daniel-ac-martin
Copy link
Owner Author

The question then becomes, should we have a special endpoint for getting a new access token or should the server simply try to replace expired access tokens as and when it receives them. (Which is what louketo-proxy does, I think.)

If we go down the route of doing this automatically, there's really nothing for the client to do on the auth side. (Maybe we could even just use louketo-proxy and be done with it.) We would want get the information from the token in to the client though.

@daniel-ac-martin
Copy link
Owner Author

The automatic way feels a bit wrong for API endpoints (including /graphql) but I can't think of anything actually wrong with it.

@daniel-ac-martin
Copy link
Owner Author

Perhaps the first pass for implementing this is the following:

  1. Just use louketo-proxy
  2. Place louketo-proxy's userinfo (from 'X-Auth-*` headers) into the application props (this comes from the token so shouldn't be confidential)
  3. Set up a React context (with hook) for accessing the userinfo data

@daniel-ac-martin
Copy link
Owner Author

daniel-ac-martin commented Nov 4, 2020

Upsides (to this 'first pass'):

  • Quick and easy
  • Simple to do functional testing on apps
    ... as one can test different access levels simply by adding role headers

Downsides:

  • No auth on prototypes in Netlify
    If we do the auth flow in the Node.js app we can protect even things in Netlify which would make it a more suitable deployment target for prototypes.

@daniel-ac-martin
Copy link
Owner Author

Doing auth in the app could be a 'second pass' and be enabled (or disabled!) with a feature flag to maintain ease of testing.

@RobinKnipe
Copy link
Collaborator

They all seem to use the same underlying library: https://github.com/IdentityModel/oidc-client-js
(So perhaps we should consider that for protecting API endpoints.)

Part of my suggesting the hapi toolset was because of how it dealt with authentication and security, might be worth a spike?

@RobinKnipe
Copy link
Collaborator

Whilst the use of Redux for this feels wrong to me, (it only needs to store some tokens doesn't it? I wouldn't think there would be that much async stuff going on. Then again, maybe there is...) perhaps it wouldn't be the end of the world if we already need Redux for #125.

It feels like trying to React without Redux is something you can't really do for long, and the longer you try, the worse your problems become when you finally given...

@RobinKnipe
Copy link
Collaborator

Another concern for this work is the security of tokens. I suspect that cookies might be better than local storage.

Or perhaps we could be even more cautious. Perhaps the server could return the tokens in headers (or a cookie) and the app could just put them in memory and delete any cookies.

Is part of the goal here to not use KeyCloak gatekeeper (or whatever they're calling KeyCloak proxy these days)?

@RobinKnipe
Copy link
Collaborator

It occurs to me that this isn't necessarily a React issue. We could solve most of this problem with just a middleware on the server.

The problem would only arise once the access token expires. As at that point queries to GraphQL would start to fail.

This whole issue feels like a server-side problem that is at a higher level than the rendering library; all the RBAC stuff should be handled before any React work, all that React should be doing is rendering one page when RBAC has successfully finished or an error when there's a problem with the roles/access.

@RobinKnipe
Copy link
Collaborator

Another alternative might be to not have the client even do the refreshing of access tokens. Perhaps the client could simply refresh the page when the access token expires and let the server do it.

Although there's probably a big risk of losing the users work with this strategy...

Why would you bother refreshing the page when you can use ajax or an iframe?

@RobinKnipe
Copy link
Collaborator

What's the point of the access token?

I suppose it means that you are checking in with the auth / SSO service. i.e. The Auth service can refuse you a new access token if the session has been revoked.

...or has expired, in which case the client would have to re-login (a common activity when using KC)

@RobinKnipe
Copy link
Collaborator

The question then becomes, should we have a special endpoint for getting a new access token or should the server simply try to replace expired access tokens as and when it receives them. (Which is what louketo-proxy does, I think.)

If we go down the route of doing this automatically, there's really nothing for the client to do on the auth side. (Maybe we could even just use louketo-proxy and be done with it.) We would want get the information from the token in to the client though.

The client should be essentially dumb as far as RBAC goes, it should just pass back whatever tokens the server issued after first (or most recent) login. This means an access token will work until it expires, then the refresh token will mean new access tokens can be issued, until that too expires at which point the client will have to login again (but this login should be possible without interfering with the user's session).

@RobinKnipe
Copy link
Collaborator

Perhaps the first pass for implementing this is the following:

  1. Just use louketo-proxy
  2. Place louketo-proxy's userinfo (from 'X-Auth-*` headers) into the application props (this comes from the token so shouldn't be confidential)
  3. Set up a React context (with hook) for accessing the userinfo data

I'm not sure I see why React would need access to information about the user, all RBAC work should be handled outside of the scope of React. All React should be doing is rendering the resulting information.

@RobinKnipe
Copy link
Collaborator

Downsides:

  • No auth on prototypes in Netlify
    If we do the auth flow in the Node.js app we can protect even things in Netlify which would make it a more suitable deployment target for prototypes.

Again, look at how hapi handles security

@daniel-ac-martin
Copy link
Owner Author

Part of my suggesting the hapi toolset was because of how it dealt with authentication and security, might be worth a spike?

What was it about HAPI's handling of auth and security that you liked?

It feels like trying to React without Redux is something you can't really do for long, and the longer you try, the worse your problems become when you finally given...

I'm not sure that's true though. I've been doing pretty well without it. People are saying that React's context API fulfils most of the use cases for Redux. So I suppose the question is, what do you think we need it for?

Is part of the goal here to not use KeyCloak gatekeeper (or whatever they're calling KeyCloak proxy these days)?

I think it would be nice to not require it, in the long-run. So far we support serverless deployment so it would be nice to keep that rather than require a proxy is in place.

This means an access token will work until it expires, then the refresh token will mean new access tokens can be issued, until that too expires at which point the client will have to login again (but this login should be possible without interfering with the user's session).

Well the question is how we pull that off without having the app just break when things expire. (Refreshing the page would be a cheap way to achieve this.)

I'm not sure I see why React would need access to information about the user, all RBAC work should be handled outside of the scope of React. All React should be doing is rendering the resulting information.

It's fairly normal to show who you are currently logged in as. So that would be one use case.

The other would be to preempt what the user will be able to do and provide them with a helpful message.

@RobinKnipe
Copy link
Collaborator

The different schemes and strategies for authentication seems relevant here, you can configure basic auth, session cookies, or deferred auth through simple config. Enabling the security features flexibly by route/path was also straight forward. I'd really need to re-watch that video I posted in slack to remind me...
Maybe Redux can be avoided, but it's so widely used by the react community that lots of things assume you're already using it for your data store.
Yes I agree, it would be much better (especially for NGU) not to rely on anything KC!
Rather than refreshing the page, we should be able to just fire off an ajax request and handle the process without a reload.
Basic user information (by which I mean essentially static, non-role/access based details) could be stored locally, but anything authoritative should only be handled on the server. Nothing should be "preempted", the server should always be part of CRUD or access processes, even if it's handled as a background process.

@daniel-ac-martin
Copy link
Owner Author

Maybe Redux can be avoided, but it's so widely used by the react community that lots of things assume you're already using it for your data store.

I think that's the thing. For simple data-stores the new 'context API' can be used. The rest of Redux's use-case seems to be around managing events and I'm not sure that we need that if we are sticking to having SSR as a fall-back.

Rather than refreshing the page, we should be able to just fire off an ajax request and handle the process without a reload.

Can we though? What if the user needs to log-in?

@daniel-ac-martin
Copy link
Owner Author

The first pass of this (relying on headers from louketo-proxy) is now done. Specifically we can:

  • access information on the current user (via a React hook)
  • apply fine-grained (role-based) access control in GraphQL resolvers

This should allow us to protect our data and provide the user with helpful information when they don't have permission to perform certain actions.

We can also apply different authentication modes depending on the situation which should help with local development and functional testing.

For the second pass I'm considering using Passport.js.

@RobinKnipe
Copy link
Collaborator

Passport.js looks a lot like hapi security. It does say specifically 'Express-based' web apps, do you think the port to restify is trivial?

@daniel-ac-martin
Copy link
Owner Author

Yes, I think so. (Famous last words!)

The main difference with Restify is that you have to ensure that next() is called.

@daniel-ac-martin
Copy link
Owner Author

This is now done but will need future improvement as detailed in #158.

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

No branches or pull requests

2 participants