-
Notifications
You must be signed in to change notification settings - Fork 12
Conversation
It looks like this app as a whole is just a very, very thin wrapper around Warden? I guess I'm not sure what exactly you're looking for review on. |
The JWT's headers/payload (for reference) Header:
Payload:
I don't like using custom headers to identify the issuer. Instead look for and use the issuer claim ( If the signature algorithm is asymmetric (e.g RS256) ... we should fetch / cache the public key to prevent more look ups. If the signature algorithm is symmetric, we need to save the password somewhere. I think FxA might use symmetric keys... will need to check . |
also committing ./vendor in a PR makes it really hard to review ... |
Is it possible to consider not vendoring libs? Btw @mostlygeek if you look at only the first commit, you don't have the vendor files. |
@mozbhearsum just to make sure we are talking about the same thing, the content of the warden folder in this PR is the code of the project @leplatrem wrote, not sure if you were refering to the ory/warden project here. |
warden/jwt.go
Outdated
) | ||
|
||
func verifyJWT(request *http.Request) (*jwt.Claims, error) { | ||
domain := request.Header.Get("Auth0-Domain") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be part of the config of the instance? Probably read from the ENV variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for this one indeed, but not for the audience right? (each Relying Party like Balrog have their own audience, unless I'm mistaken...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes then would we be able to grab it from the referer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caller (Relying Party, like balrog) is not a browser, so it has to send it explicitly
warden/jwt.go
Outdated
} | ||
|
||
jwksURI := fmt.Sprintf("https://%s.auth0.com/.well-known/jwks.json", domain) | ||
apiIssuer := fmt.Sprintf("https://%s.auth0.com/", domain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be specific to auth0 but most likely specific to OpenID Connect IDP, we should probably also use the config to store the domain including auth0.com
@@ -11,6 +11,7 @@ import ( | |||
"github.com/ory/ladon" | |||
manager "github.com/ory/ladon/manager/memory" | |||
log "github.com/sirupsen/logrus" | |||
jwt "gopkg.in/square/go-jose.v2/jwt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I guess that since we use |
This branch was demoed. No one seemed to react with a funny face, so I'll add some tests and merge :) |
Then I query
/allowed
with a JWT token, the token is verified, and the subject is used to match policies.I pass the
Auth0-Domain
andAuth0-Audience
in the headers, to be able to validate tokens from different services.I takes around ~200ms, but I guess
JWKClient
could be cached by domain... Later the authorization could be in charge of fetching LDAP userid and groups to match the policies (instead of just of the JWT subject)@Natim @mozbhearsum @mostlygeek @peterbe Any thought about this POC?
Thanks a lot!