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

New flag "-ssl-upstream-insecure-skip-validation" #234

Merged
merged 13 commits into from
Aug 7, 2019
Merged

New flag "-ssl-upstream-insecure-skip-validation" #234

merged 13 commits into from
Aug 7, 2019

Conversation

jansinger
Copy link
Contributor

New flag to skip SSL validation for upstreams with self generated / invalid SSL certificates.

Motivation and Context

Upstream with self-signed certificate (such as default kubernetes dashboard deployment) should work if oauth2-proxy.

Fixes #75

How Has This Been Tested?

Setup an oauth2_proxy for a Kubernetes dashboard as described in Issue #75, produced the errors as mentioned without the new flag, works setting the flag.

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.

…ion for upstreams with self generated / invalid SSL certificates.
@jansinger jansinger requested a review from a team August 5, 2019 11:05
@steakunderscore
Copy link
Member

Hi @jansinger 👋 Thanks for your PR. Did you look in to why the tests are failing?

@jansinger
Copy link
Contributor Author

Hi @steakunderscore, sorry, I just missed to push the updated test after changing the method signature of NewReverseProxy(). It's fixed now.

loshz
loshz previously approved these changes Aug 6, 2019
Copy link
Member

@loshz loshz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@loshz loshz left a comment

Choose a reason for hiding this comment

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

Once you've added this change to the changelog we should be good to go!

@jansinger
Copy link
Contributor Author

Can anyone give me a hint why this is failing now when trying to download the mods?

@loshz
Copy link
Member

loshz commented Aug 6, 2019

It could be due to Travis just timing out as this is something I've experienced before with go modules. I've restarted the build, but if this issue persists then I'll investigate further.

loshz
loshz previously approved these changes Aug 6, 2019
@jansinger jansinger closed this Aug 7, 2019
@jansinger jansinger reopened this Aug 7, 2019
@loshz
Copy link
Member

loshz commented Aug 7, 2019

Just a thought on this, why do we need to add a new flag? Can we not just pass the -ssl-insecure-skip-verify flag to the NewReverseProxy func instead? As I assume needing to skip verification will always require both -ssl-insecure-skip-verify and -ssl-upstream-insecure-skip-verify being set to true.

@jansinger
Copy link
Contributor Author

We want to validate the SSL certificate of our OIDC provider, which has an official certificate and can be validated and treated as secure. But the upstreams in our Kubernetes Cluster have self signed certs, so validation need to be skipped for them.
If all agree that it makes sense to always skip validation of SSL certificates for both providers and upstreams, I can change it accordingly. We will then continue to use our own fork to have this seperated.

@JoelSpeed
Copy link
Member

Personally I would keep them separate. There are two different "sets" of certificates and I can see valid reason for wanting to validate one set and not validate the others

@loshz
Copy link
Member

loshz commented Aug 7, 2019

Yeah after reading this again it definitely makes sense to keep them separate!

@loshz
Copy link
Member

loshz commented Aug 7, 2019

Are you happy with this @JoelSpeed @steakunderscore?

CHANGELOG.md Outdated Show resolved Hide resolved
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.

Once the issue with the changelog is fixed this is good to merge

@loshz loshz merged commit 7134d22 into oauth2-proxy:master Aug 7, 2019
@loshz
Copy link
Member

loshz commented Aug 7, 2019

Thank you @jansinger and congrats on your first contribution! 🎉

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.

Skip validation of upstream certificate
4 participants