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

Add setting to skip ssl certificate verification for HTTP checks #1984

Merged
merged 4 commits into from
Nov 3, 2016

Conversation

kylemcc
Copy link
Contributor

@kylemcc kylemcc commented Apr 25, 2016

This pull request adds the same functionality as #1897, but does so by adding a setting to the check definition so that verification can be turned off on a per-check basis. I've also included some unit tests and updated the documentation to reflect the change.

E.g.:

{
  "check": {
    "id": "api",
    "name": "HTTPS check on localhost",
    "http": "https://localhost/health",
    "interval": "10s",
    "timeout": "1s",
    "tls_skip_verify": true
  }
}

@kylemcc kylemcc force-pushed the https-check-skip-verify branch from 67113f9 to e290599 Compare April 26, 2016 20:46
@kylemcc kylemcc force-pushed the https-check-skip-verify branch from e290599 to b5744e6 Compare June 29, 2016 03:51
check.Stop()
server.Close()

server = mockTLSHTTPServer(500)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need to go into this in the tests, since we want to test that we are failing at the transport layer really. Doesn't hurt to keep it, though.

@ryanuber
Copy link
Member

ryanuber commented Jul 5, 2016

Hey @kylemcc, this looks great! Just a few minor comments but otherwise LGTM. Sorry for the delay!

@kylemcc
Copy link
Contributor Author

kylemcc commented Jul 5, 2016

Great! I'll get on those tonight.

@kylemcc kylemcc force-pushed the https-check-skip-verify branch from b5744e6 to 9bf99d6 Compare July 7, 2016 16:03
t.Fatalf("should be critical %v", mock.state)
}

if !strings.Contains(mock.output["skipverify_false"], "certificate signed by unknown authority") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryanuber Based on your feedback, I've updated this test to make sure we're failing at the transport layer

@kylemcc
Copy link
Contributor Author

kylemcc commented Jul 7, 2016

@ryanuber I've made a few changes based on your feedback. The test failure here seems unrelated to my changes - it may be due to my rebasing on top of master. They do pass for me locally though.

Let me know if you have any other feedback.

@ryanuber
Copy link
Member

ryanuber commented Jul 7, 2016

@kylemcc great, will review again shortly!

@cavemandaveman
Copy link

@kylemcc I added a PR to your branch to include TLSSkipVerify in the api. Please review before this gets merged!

@raphaelSch
Copy link

raphaelSch commented Jul 29, 2016

Hey guys,

are there any information when this PR will be merged? We are very interested in using the new option!

@kylemcc kylemcc force-pushed the https-check-skip-verify branch from a4045ca to 6a596da Compare July 29, 2016 14:38
@kylemcc kylemcc force-pushed the https-check-skip-verify branch from 6a596da to 7f208d7 Compare October 17, 2016 16:41
@slackpad slackpad added this to the 0.7.1 milestone Oct 27, 2016
@slackpad slackpad merged commit 73b281a into hashicorp:master Nov 3, 2016
@slackpad
Copy link
Contributor

slackpad commented Nov 3, 2016

Thanks!

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.

5 participants