hasura / graphql-engine

Blazing fast, instant realtime GraphQL APIs on your DB with fine grained access control, also trigger webhooks on database events.
https://hasura.io
Apache License 2.0
31.2k stars 2.77k forks source link

jwt claims without x-hasura prefix #1502

Closed SunChero closed 3 years ago

SunChero commented 5 years ago

is it possible to have jwt claims standard without the artificial x-hasura-* prefix ? this force gateways to have specific claims for hasura and standard claims for other backends!

doing this will only help making hasura easier to integrate and talk with other/existing backends

thanks

ecthiender commented 5 years ago

I totally understand your concern and it kind of makes sense.

The problem is Hasura needs atleast two values - a list of allowed roles and the default role, to enforce the ACL (and optionally other values that you can specify that will be used in ACL).

There were two ways of achieving it:

  1. Having a convention of the name of claims (x-hasura-* prefix). That way everything just works out of the box.
  2. Having configuration where you specify what keys of claims correspond to list of allowed roles and a default role. Hasura would use that configuration to pick values from the JWT claims.

We opted for the first option as it seemed simpler.

What are your thoughts?

@coco98 @0x777 @shahidhk thoughts?

SunChero commented 5 years ago

i agree it can be simpler, but this adds some complexity if you want it to be seemly integrated with other stacks. imagine if your idp have to generate a new set of claims for every backend ? how many claims would you be adding ? + testing ...etc. our team actually evaluating either fork it or forget it

thanks

coco98 commented 5 years ago

@SunChero On another note, you can always use the webhook method in this case. The webhook can pick up the JWT that is sent, verify it and then return the x-hasura-role and x-hasura-* variables as you'd like.

Here's an example of a webhook for firebase (instead of using a firebase jwt): https://github.com/hasura/graphql-engine/blob/master/community/boilerplates/auth-webhooks/nodejs-express/firebase/firebaseHandler.js

SunChero commented 5 years ago

a webhook to me is +1 hop for every http request and another external dependency i agree, this should work but how hard it is to implement such a thing. i saw this project is part of hasura platform and i think you want things to stay conform with the other microservices which use the same set of headers for authn/authz. anyway , i guess there is no willing to change this anytime soon and thats ok , i like the project and how its evolving

thanks

coco98 commented 5 years ago

@SunChero Thanks for your kind words and glad you like direction of the project! :)

Well, it's not really because of the platform, but it is more because the design of the authz system expects claims in a specific way. This should be easy to implement if we have a good spec to go ahead with. Do you have a rough idea of what this configuration would look like?

As @ecthiender suggested, you'd need to pass 2 important variables to hasura:

  1. The name of the role that the JWT user represents
  2. Other session variables that will come from the standard jwt claims Basically some kind of "remapping" config that maps/renames standard claims to x-hasura-role and x-hasura-* variables. That would not break the hasura authz system and will allow easy integration with an existing jwt. Would be great if you can give us an example of how you'd use your existing JWT structure in this way.

Also, regarding the webhook system, you're right about the extra hop. Hasura maintains a connection pool to the auth-webhook server to minimise that impact and ideally if you're co-located that jump should be +1-5ms so the impact may not be severe in practice.

SunChero commented 5 years ago

@coco98 just cloned the repo and tried to understand the code behind this (not easy with haskell :) ) basically if im not mistaken we filter headers to keep only x-hasura-*

 let claimsMap = Map.filterWithKey (\k _ -> T.isPrefixOf "x-hasura-" k)
                $ Map.fromList $ map (first T.toLower)
                $ Map.toList hasuraClaims

then

parseHasuraClaims claimsMap = do
  let mAllowedRolesV = Map.lookup allowedRolesClaim claimsMap
  allowedRolesV <- maybe missingAllowedRolesClaim return mAllowedRolesV
  allowedRoles <- parseJwtClaim (A.fromJSON allowedRolesV) errMsg

  let mDefaultRoleV = Map.lookup defaultRoleClaim claimsMap
  defaultRoleV <- maybe missingDefaultRoleClaim return mDefaultRoleV
  defaultRole <- parseJwtClaim (A.fromJSON defaultRoleV) errMsg

where allowedRolesClaim and defaultRoleClaim has been hardcoded as strings

allowedRolesClaim :: T.Text
allowedRolesClaim = "x-hasura-allowed-roles"

defaultRoleClaim :: T.Text
defaultRoleClaim = "x-hasura-default-role"

cant we just get them from env vars ? like running docker with -e x-hasura-var= var-to-be-extracted-from-http-headers

thanks again

revskill10 commented 5 years ago

Should jwt prefix be configured via environment variables ?

SunChero commented 5 years ago

@revskill10 i think the idea is to give users more control over which claim is being used for authentication ... configuring only the prefix will still limit them in some usecases. i would opt for a solution which make the claim totally configured via env

mnlbox commented 5 years ago

I also think an environment variable for JWT prefix is very useful. User also can use Hasura without this and use default value like: x-hasura

SunChero commented 5 years ago

good to see this still alive .. keep up good work .. and let me know if you need some testing .. .thanks again

mnlbox commented 5 years ago

@SunChero I think a simple environment variable is enough as a start point for close this issue. For more advanced use case maybe Hasura teams needs more work and I think maybe it's better to talking in another issue.

SunChero commented 5 years ago

yes would be nice!

mnlbox commented 5 years ago

@dsandip Any news?

dsandip commented 5 years ago

@mnlbox The use-case for this is well understood, but factoring in the demand for this in the community and the bandwidth available, we'll only be able to pick this up after a couple of months.

mnlbox commented 5 years ago

@dsandip I think it's an old request and I guess simple prefix environment variable need small PR. But I agree with you and know your workload bro. OK I can wait for this man :wink: Thanks for your reply.

jbek7 commented 4 years ago

This PR should solve this issue too

mnlbox commented 4 years ago

@jbek7 I didn't find any way to customize JWT claims prefix in that PR :thinking: Can you explain more how it will be possible after that PR?

tirumaraiselvan commented 3 years ago

Solved in https://github.com/hasura/graphql-engine/pull/3575 and released in v1.3.3