Added RS256 alg support via Okta lib #24
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Updated golang to 1.11 (the original docker was on 1.8 and code didn't build on it) * Added instructions to run git-gateway to README.md * Added abstraction to auth.go such that different implementations of auth can be used * Updated example.env
* updated to golang 1.11 * worked around open-source fork problem * fixed lint on auth.go and errors.go
4 tasks
calavera
suggested changes
Dec 10, 2018
There was a problem hiding this comment.
Thanks for the improvement @thomasyip ! I'm concerned about keeping backwards compatibility. I left you a comment that should be easy to address.
| } else if (authenticatorName == "bearer-okta-jwt-token") { | ||
| auth.authenticator = &OktaJWTAuthenticator{name: "bearer-okta-jwt-token", auth: *auth} | ||
| } else { | ||
| if (authenticatorName != "") { |
There was a problem hiding this comment.
In order to keep backwards compatibility, this initialization should not fail. The default behavior when there is no authenticationName should be to use bearer jwt tokens.
There was a problem hiding this comment.
@calavera good catch. I didn't mean to break backward compatibility.
I will address the request soon. Hopefully within this week.
Thank for your time and looking over the PR!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- Summary
This PR adds support for OAuth2 RS256 alg via Okta's golang lib. There were a few typical pitfalls on implementing OAuth, we feel it was better to use the lib developed by our vendor. (eg, dgrijalva/jwt-go#147)
The PR abstracts
auth.goto make the new implementation work alongside with the existingalg. It also makes adding other alg or implementing it with other lib easier later.We can back-out the Okta implementation upon request. But, we like the abstraction on auth.go to stay such to make it easier for us to keep it in-sync.
- Test plan
The Docker on
masterbranch failed to build when we pulled** and had no instruction on how to get it runs. We got it to built and provided some instructions.** (I later found out there is travis setup. I didn't aware of Travis-ci setup in the beginning.)
We didn't add test in this PR, but believe we improves the quality a little bit over existing condition for the Docker use-case.
The PR should pass Travis-CI. I added an workaround to make Travis-ci works for a fork.
- Description for the changelog
Added the option to authenticate using OKTA lib (which use RS256 alg).
* Updated golang to 1.11 (the original docker was on 1.8 and code didn't build on it)
* Added instructions to run git-gateway to README.md
* Added abstraction to auth.go such that different implementations of auth can be used
* Updated example.env
* Updated travis
- A picture of a cute animal (not mandatory but encouraged)
