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

feat(auth): add jwt support in auth middleware #15152

Merged
merged 1 commit into from Sep 27, 2019

Conversation

GeorgeMac
Copy link
Contributor

@GeorgeMac GeorgeMac commented Sep 16, 2019

Closes #14972

This PR adds support for JWT authorization to the http auth middleware.

This uses the new jsonweb package authorizer types.

When Token based authorization is used an attempt is first made to deserialize the payload as a JWT. If the token is well-formed, it is assumed that JWT based authorization is being attempted. The token is then validated and if successful the permission claims are passed down via the usual Authorizer on context strategy.
Given the token is not a well-formed JWT, then it is regarded as a lookup token for an associated Authorization in the AuthorizationService and treated as such. The Authorization is looked up using the token payload as the key.

Currently there is no decision as to how to configure a signing key, so this PR just introduces the mechanisms required for that work in the future. Currently an attempt to use a JWT will result in a no key found error. The token parser's KeyStore must be replace with a key store which returns a valid signing key for a given key id if JWT support is to be enabled.

UPDATE:

This also now transforms each of our Authorizer types into the new PermissionsAuthorizer type which is an Authorizer which can returns its set of permissions. This is required to create a token from an Authorizer.

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Well-formatted commit messages
  • Rebased/mergeable
  • Tests pass
  • http/swagger.yml updated (if modified Go structs or API)
  • Documentation updated or issue created (provide link to issue/pr)
  • Signed CLA (if not already signed)

@GeorgeMac GeorgeMac changed the base branch from gm/jsonweb to master September 19, 2019 11:34
@GeorgeMac GeorgeMac changed the title JWT support in http auth middleware feat(auth): add jwt support in auth middleware Sep 19, 2019
@GeorgeMac GeorgeMac marked this pull request as ready for review September 19, 2019 11:43
@GeorgeMac GeorgeMac requested a review from a team September 19, 2019 11:44
@ghost ghost requested review from docmerlin and removed request for a team September 19, 2019 11:44
@GeorgeMac
Copy link
Contributor Author

See UPDATE in PR description

@docmerlin
Copy link
Contributor

@GeorgeMac This seems to be failing IDPE compatibility. you may want top update IDPE as well.

Copy link
Contributor

@docmerlin docmerlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs IDPE compatibility (fix stuff in IDPE to support it?)

@GeorgeMac
Copy link
Contributor Author

Cheers @docmerlin
As per external discussion we have decided to remove this new PermissionsAuthorizer interface now anyway. As its original intention has now been solved without any additional change being required.

@GeorgeMac
Copy link
Contributor Author

GeorgeMac commented Sep 24, 2019

I re-requested review and added @jademcgough to this review as I needed to rebase over some recent changes to the auth middleware and could do with a sanity check.

After discussions with the compute team it was decided that permission based auth fits well with the JWT token. The Authorizer interface exposes both permission checking capabilities (via Allowed()) and an identity (via GetUserID()). Which gets confusing. So I decided to make the JWT Authorizer represent purely permissions and return an invalid ID when asked for a UserID.

The change that just went in just made the authentication middleware dependent on the UserID returned by the Authorizer type. Now it has a lookup on the user ID to ensure the associated User is active. However, for the JWT token route we are purely interested in the permissions of the authorization and not the associated identity. They are for internal use and will expire shortly after issuing. This is how revokation ultimately will occur.

TL;DR

I updated my change to lean on the validity of the user ID in order to decided whether or not to make a user lookup for activity.
Given an invalid user ID (for example a JWT authorizer) no lookup is required.

Copy link
Contributor

@docmerlin docmerlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this better. It feels cleaner than the previous implementation.

@GeorgeMac GeorgeMac merged commit 9f5390e into master Sep 27, 2019
@GeorgeMac GeorgeMac deleted the gm/http-jsonweb-integration branch September 27, 2019 11:12
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

Successfully merging this pull request may close these issues.

Add support to authorization middleware for jwt based Authorizer
2 participants