Skip to content

feat(GraphQL): Validate audience in authorization JWT and change Dgraph.Authorization format. #5927

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

Merged
merged 9 commits into from
Jul 14, 2020

Conversation

arijitAD
Copy link

@arijitAD arijitAD commented Jul 10, 2020

Fixes GRAPHQL-565

  1. Upgrades jwt-go to v4 to support multiple aud claims in JWT fix(GraphQL): Upgrade jwt-go to v4 to support multiple aud claims #5750
  2. Makes JWT token expiry field option in GraphQL.
  3. Changes Dgraph.Authorization format.

The updated Dgraph.Authorization format is as follows:
# Dgraph.Authorization {"VerificationKey":"secretkey","Header":"X-Test-Auth","Namespace":"https://xyz.io/jwt/claims","Algo":"HS256"}


This change is Reviewable

Docs Preview: Dgraph Preview

dpeek and others added 3 commits June 30, 2020 17:54
@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Jul 10, 2020
@arijitAD arijitAD marked this pull request as ready for review July 10, 2020 09:30
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Mostly looking good. Please make some changes and then can have another look before approving.

Reviewed 1 of 6 files at r1, 6 of 7 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @arijitAD and @MichaelJCompton)


graphql/authorization/auth.go, line 95 at r2 (raw file):

	}

	fmt.Println("Falling back to parsing authorization information in old format.")

We don't need this and can tell the user that they need to upgrade their syntax.
Michael mentioned on Slack

also, do this -> if we have a parsing error on the auth, we have an error message like "Unable to parse Dgraph.Authorization.  It may be that you are using a pre-release syntax.  Please check at https://graphql.dgraph.io/authorization/."
10:42
That should cover anyone who's used a pre-release version and needs to update their syntax.
10:43
Obviously if it parses as json, but there's some other error, then we report the specific error because they must know the new syntax at that point.

graphql/authorization/auth.go, line 272 at r2 (raw file):

			}
			return nil, errors.Errorf("couldn't parse signing method from token header: %s", algo)
		}, jwt.WithoutClaimsValidation())

Add a comment that library only supports comparing against one audience and we do this validation ourselves below.


testutil/graphql.go, line 203 at r2 (raw file):

func AppendAuthInfo(schema []byte, algo, publicKeyFile string) ([]byte, error) {
	if algo == "HS256" {
		authInfo := `# Dgraph.Authorization {"PublicKey":"secretkey","Header":"X-Test-Auth","Namespace":"https://xyz.io/jwt/claims","Algo":"HS256","Audience":["aud1","63do0q16n6ebjgkumu05kkeian","aud5"]}`

Check that JSON parsing works fine with spaces before and after the JSON.


testutil/graphql.go, line 203 at r2 (raw file):

func AppendAuthInfo(schema []byte, algo, publicKeyFile string) ([]byte, error) {
	if algo == "HS256" {
		authInfo := `# Dgraph.Authorization {"PublicKey":"secretkey","Header":"X-Test-Auth","Namespace":"https://xyz.io/jwt/claims","Algo":"HS256","Audience":["aud1","63do0q16n6ebjgkumu05kkeian","aud5"]}`

Change PublicKey to VerificationKey

@CLAassistant
Copy link

CLAassistant commented Jul 13, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Author

@arijitAD arijitAD left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 8 files reviewed, 4 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)


graphql/authorization/auth.go, line 95 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We don't need this and can tell the user that they need to upgrade their syntax.
Michael mentioned on Slack

also, do this -> if we have a parsing error on the auth, we have an error message like "Unable to parse Dgraph.Authorization.  It may be that you are using a pre-release syntax.  Please check at https://graphql.dgraph.io/authorization/."
10:42
That should cover anyone who's used a pre-release version and needs to update their syntax.
10:43
Obviously if it parses as json, but there's some other error, then we report the specific error because they must know the new syntax at that point.

Done.


graphql/authorization/auth.go, line 272 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a comment that library only supports comparing against one audience and we do this validation ourselves below.

Done.


testutil/graphql.go, line 203 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Check that JSON parsing works fine with spaces before and after the JSON.

Test with spaces. It works.


testutil/graphql.go, line 203 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Change PublicKey to VerificationKey

Done.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 5 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @arijitAD and @MichaelJCompton)


graphql/authorization/auth.go, line 112 at r2 (raw file):

		return meta, errors.Errorf("error while parsing jwt authorization info: %v", err)
	}
	idx := authMetaRegex.FindAllStringSubmatchIndex(authInfo, -1)

the authMetaRegex can be deleted as well now?


graphql/authorization/auth.go, line 93 at r4 (raw file):

		return meta, fmt.Errorf("Unable to parse Dgraph.Authorization. " +
			" It may be that you are using the pre-release syntax. " +
			"Please check at https://graphql.dgraph.io/authorization/")

Please check the correct syntax at ...


graphql/schema/wrappers_test.go, line 985 at r4 (raw file):

			nil,
			"",
			errors.New("Unable to parse Dgraph.Authorization. " +

nice

Copy link
Author

@arijitAD arijitAD left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 8 files reviewed, 3 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)


graphql/authorization/auth.go, line 112 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

the authMetaRegex can be deleted as well now?

I have already deleted it.


graphql/authorization/auth.go, line 93 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Please check the correct syntax at ...

Done.


graphql/schema/wrappers_test.go, line 985 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

nice

Done.

@arijitAD
Copy link
Author

@dpeek Can you sign CLA so that we can merge this PR.

@dpeek
Copy link
Contributor

dpeek commented Jul 14, 2020

Hi @pawanrawal, sorry for radio silence – things got busy at work! I though I had signed the CLA already, but signed again. Still says pending though?

@parasssh
Copy link
Contributor

Hi @pawanrawal, sorry for radio silence – things got busy at work! I though I had signed the CLA already, but signed again. Still says pending though?

Yes, it shows pending without which we cannot merge.

@dpeek
Copy link
Contributor

dpeek commented Jul 14, 2020

Seems like it picked it up now, had to ask it to recheck :)

@arijitAD arijitAD merged commit 5d789f0 into master Jul 14, 2020
@arijitAD arijitAD deleted the arijitAD/JWT-aud-claims branch July 14, 2020 03:15
arijitAD pushed a commit that referenced this pull request Jul 14, 2020
…aph.Authorization` format. (#5927)

1. Upgrades jwt-go to v4 to support multiple aud claims in JWT
2. Makes JWT token expiry field option in GraphQL.
3. Changes Dgraph.Authorization format.

Co-authored-by: David Peek <mail@dpeek.com>
(cherry picked from commit 5d789f0)
parasssh pushed a commit that referenced this pull request Jul 14, 2020
…aph.Authorization` format. (#5927) (#5980)

1. Upgrades jwt-go to v4 to support multiple aud claims in JWT
2. Makes JWT token expiry field option in GraphQL.
3. Changes Dgraph.Authorization format.

Co-authored-by: David Peek <mail@dpeek.com>
(cherry picked from commit 5d789f0)
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
…aph.Authorization` format. (hypermodeinc#5927)

1. Upgrades jwt-go to v4 to support multiple aud claims in JWT
2. Makes JWT token expiry field option in GraphQL.
3. Changes Dgraph.Authorization format.

Co-authored-by: David Peek <mail@dpeek.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

5 participants