-
Notifications
You must be signed in to change notification settings - Fork 26
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
implement ssl-skip-verify to forward to self-signed-certificates #29
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this. I have mixed feelings about this change because:
- I think that TLS verification is a good thing and one should always employ it
- I am not convinced on how the sslskipverify option is passed around
So I summon @jblackman for his feedback :-)
README.md
Outdated
@@ -144,6 +144,7 @@ content-memory-threshold | (Optional) Maximum payload size to keep in RAM. Large | |||
log-file | (Optional) The clammit log file, if ommitted will log to stdout | |||
test-pages | (Optional) If true, clammit will also offer up a page to perform test uploads | |||
debug | (Optional) If true, more things will be logged | |||
ssl-skip-verify | (Optional) If true, will skip SSL validation forwarding connection to use self-signed certitifates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would reword this as:
if true, it will not perform TLS certificate verification, allowing connections to servers using self-signed or otherwise untrusted certificates. Use with care, it is always preferable to perform certificate verification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your very precise documentation
// allow for | ||
// https://stackoverflow.com/questions/12122159/how-to-do-a-https-request-with-bad-certificate | ||
tr := &http.Transport{ | ||
TLSClientConfig: &tls.Config{InsecureSkipVerify: f.sslSkipVerify}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks OK as today we only have a single option. For the future, we’ll want to pass the tls config object in the forwarder struct directly, that’ll allow us to drive any tls config options from clammit configuration without adding more fields to the Forwarder struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - I was not happy also. But I did not wanted to do a full refactoring ....
Any change on getting this merged? |
Also another question/observation: Why is it not possible that self signed certs that have been added to the os level are considered valid? I am not familiar with the go ecosystem but i would expect that a cert that is trusted on the system level would be working. We tried this together with https://github.com/maxsivkov/clammit-docker (and after adding the necessary cert files) and when we curl inside the container it works but when clammit forwards the request it keeps complaining about invalid cert. |
we are using internally also SSL between our servers (nginx and Service fabric / load balancer) but these are self-signed-certiticates. Today clammit does not allow it
the PR allows to configure "ssl-skip-verify" (true, false).
If true, will skip SSL validation forwarding connection to use self-signed certitifates, default = false
ssl-skip-verify = false