Conversation
mraerino
left a comment
mraerino
left a comment
There was a problem hiding this comment.
Thank you for submitting this PR and being patient with our response.
I added some remarks. You also want to make sure to rebase against master, we recently moved to go mod.
| return loadRSAPublicKeyFromDisk(config.JWT.Keyfile), nil | ||
| default: | ||
| return nil, unauthorizedError("Invalid Signing Method: %s", signMethod) | ||
| } |
There was a problem hiding this comment.
token.Method will give you the signing method the JWT uses. Consider also checking if that matches - error messages for mismatch conditions would be great.
Please also check if Keyfile or Secret are empty and return an error if they should be present.
| panic(e.Error()) | ||
| } | ||
| return key | ||
| } |
There was a problem hiding this comment.
Please do not panic on errors. They should always be returned from the function so the caller can decide to handle errors.
| Secret string `json:"secret" required:"true"` | ||
| Method string `envconfig:"SIGNING_METHOD" json:"method"` | ||
| Secret string `envconfig:"SECRET" json:"secret"` | ||
| Keyfile string `envconfig:"KEYFILE" json:"keyfile"` |
There was a problem hiding this comment.
Since this service has support for a multi instance mode it should be possible to have a different singing key for every instance. The settings of an instance are persisted in the database and therefore cannot be loaded from a file dynamically. It would be great if there also was an option to pass in the PEM-encoded key as a string.
See these files for a similar implementation in gotrue:
| GITGATEWAY_JWT_SECRET="CHANGE-THIS! VERY IMPORTANT!" | ||
| GITGATEWAY_JWT_SIGNING_METHOD="HS256" # [HS256|RS256] | ||
| GITGATEWAY_JWT_SECRET="CHANGE-THIS! VERY IMPORTANT!" # HS256 | ||
| #GITGATEWAY_JWT_KEYFILE="/path/to/keyfile.pub" # RS256 |
There was a problem hiding this comment.
If you would like to test for backwards compat, discard this change, so the tests run on a previous config where method did not exist.
2 participants
Closes Issue #32
- Summary
This adds the RS256 Signing Method to the valid Parser methods in
api/auth.go.There are two new properties on the JWTConfiguration struct:
MethodandKeyfile.Secretis no longer required, since the RS256 method will be looking at theKeyfileproperty. TheKeyfileshould be a path to a public keyfile.Since
Methodis a new property, it is not required, so as to maintain backwards compatibility.api/auth.godefaults to HS256 when that property is not present.- Motivation
I'm using a non-GoTrue authentication service which does not currently support HMAC signing of access tokens - only RSA.
- Test plan
Existing instances of Git Gateway should continue to function normally upon update.
Using RS256:
.envkey.pub- Description for the changelog
Allow for tokens to be signed with RS256
- A picture of a cute animal (not mandatory but encouraged)