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

implement ssl-skip-verify to forward to self-signed-certificates #29

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Nov 5, 2021

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

2021/11/04 23:18:04 Interceptor passed this request
2021/11/04 23:18:04 Forwarding to https://10.57.2.4:8001
2021/11/04 23:18:04 Failed to forward request: 'post' "https://10.57.2.4:8001/api/Files?api-version=1.0": x509: certificate signed by unknown authority

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

2021/11/04 23:22:26 Interceptor passed this request
2021/11/04 23:22:26 Forwarding to https://10.57.2.4:8001
2021/11/04 23:22:26 Request forwarded, response 200 OK
  • test cases ran
  • test cases implemented
  • documentation adapted
  • (some build documentation adapted)
  • GitHub build action run
  • build v0.7.3+ssl-skip-verify on our forked repository

Copy link
Contributor

@vjt vjt left a 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 :-)

Makefile Outdated Show resolved Hide resolved
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
Copy link
Contributor

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.

Copy link
Author

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

clammit.cfg.example Show resolved Hide resolved
// 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},
Copy link
Contributor

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.

Copy link
Author

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 ....

@butsjoh
Copy link

butsjoh commented Jan 12, 2022

Any change on getting this merged?

@butsjoh
Copy link

butsjoh commented Jan 12, 2022

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.

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