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

Conversation

@jansinger
Copy link
Contributor

commented Aug 5, 2019

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.
Jan Singer
New flag "-ssl-upstream-insecure-skip-validation" to skip SSL validat…
…ion for upstreams with self generated / invalid SSL certificates.

@jansinger jansinger requested review from steakunderscore, syscll and pusher/cloud-team as code owners Aug 5, 2019

@steakunderscore

This comment has been minimized.

Copy link
Collaborator

commented Aug 6, 2019

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

@jansinger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

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

@syscll
Copy link
Collaborator

left a comment

LGTM 👍

@syscll
Copy link
Collaborator

left a comment

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

Jan Singer added 2 commits Aug 6, 2019
Jan Singer
Merge remote-tracking branch 'origin/skip-upstream-ssl-validation' in…
…to skip-upstream-ssl-validation

# Conflicts:
#	CHANGELOG.md
@jansinger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

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

@syscll

This comment has been minimized.

Copy link
Collaborator

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.

@jansinger jansinger closed this Aug 7, 2019

@jansinger jansinger reopened this Aug 7, 2019

@syscll

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

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

@syscll

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

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

@syscll

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

Are you happy with this @JoelSpeed @steakunderscore?

CHANGELOG.md Outdated Show resolved Hide resolved
@JoelSpeed
Copy link
Collaborator

left a comment

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

Jan Singer added 2 commits Aug 7, 2019
@syscll
syscll approved these changes Aug 7, 2019

@syscll syscll merged commit 7134d22 into pusher:master Aug 7, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@syscll

This comment has been minimized.

Copy link
Collaborator

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
Projects
None yet
4 participants
You can’t perform that action at this time.