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

added jwt-key-file option #111

Merged
merged 1 commit into from
Apr 16, 2019

Conversation

timothy-spencer
Copy link
Contributor

Description

This adds the ability to have a JWT Key file for login.gov to be specified by a filesystem path as well as the environment variable. It also adds some stuff to the build process so that a jwt key file can be copied in at build time.

Motivation and Context

This is useful for GCP App Engine custom runtime builds, because you cannot use multiline variables in their app.yaml, so you have to build the key into the container and then tell it where it is by setting OAUTH2_PROXY_JWT_KEY_FILE=/etc/ssl/private/jwt_signing_key.pem in app.yaml instead.

How Has This Been Tested?

I have tested this through go test ./... and through using this code in my own GCP pilot project.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@timothy-spencer timothy-spencer requested a review from a team March 20, 2019 22:19
@timothy-spencer
Copy link
Contributor Author

Just in case it's not clear, the empty jwt key file that is generated in the Dockerfiles should have zero effect on anything. It's only there so that you can copy a signing key in while doing a custom build and use it in GCP with the login.gov provider. An example of that can be found here: https://github.com/18F/gcp-appengine-template/blob/3c9c979c1f519b7546c77feecfeca4189e79e835/.circleci/config.yml#L311-L341

@timothy-spencer
Copy link
Contributor Author

Any comments here? This is the last change remaining that is preventing me from using upstream for my project. No rush or anything, but I just wanted to make sure that this wasn't forgotten. :-)

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

I've added a couple of comments in-line

I think we should handle the case where jwt-key and jwt-key-file are set as an error, I think just saying that jwt-key takes precedence in the code would be a bit surprising to the average user perhaps?

Also, could you please update the documentation in the ReadMe to reflect the changes and explain the two parameters?

Dockerfile Show resolved Hide resolved
http_test.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@timothy-spencer
Copy link
Contributor Author

OK. I think I have incorporated all your suggestions. The commandline option list in the README looks big, but I just updated it to the output of oauth_proxy -h from master, which seems to have a different format now. Let me know what you think!

Dockerfile Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
http_test.go Outdated Show resolved Hide resolved
options.go Show resolved Hide resolved
@timothy-spencer
Copy link
Contributor Author

OK. Travis is unstuck now. Should be ready to go!

@JoelSpeed JoelSpeed self-requested a review April 15, 2019 09:13
@JoelSpeed JoelSpeed dismissed their stale review April 15, 2019 09:13

Requested changes fixed

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Could you possibly rebase this on top of our master branch and squash down the commits? If travis gets stuck again please let me know and I'll kick it.

The changelog has been updated since we cut a release last week, but for some reason it's not showing as a conflict, the changelog modification will need fixing in this before we can merge. Other than that it's all good 😄

@timothy-spencer
Copy link
Contributor Author

Done! Rebasing/squashing is not something that I normally do, so please check my work. :-)

Thanks for being responsive!

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your work on this and patience with my reviews 🙈

@JoelSpeed JoelSpeed merged commit 46a0d7c into oauth2-proxy:master Apr 16, 2019
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.

2 participants