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

Restore the 0.2 TLS verification behavior. #233

Merged
merged 2 commits into from Jul 1, 2014

Conversation

Projects
None yet
2 participants
@nelhage
Copy link
Contributor

commented Jun 28, 2014

cc @armon -- I'd probably like to write more tests before merging this, but can you ACK that this looks like the right track to you before I do so?


Namely, don't check the DNS names in TLS certificates when connecting to
other servers.

As of golang 1.3, crypto/tls no longer natively supports doing partial
verification (verifying the cert issuer but not the hostname), so we
have to disable verification entirely and then do the issuer
verification ourselves. Fortunately, crypto/x509 makes this relatively
straightforward.

If the "server_name" configuration option is passed, we preserve the
existing behavior of checking that server name everywhere.

No option is provided to retain the current behavior of checking the
remote certificate against the local node name, since that behavior
seems clearly buggy and unintentional, and I have difficulty imagining
it is actually being used anywhere. It would be relatively
straightforward to restore if desired, however.

Restore the 0.2 TLS verification behavior.
Namely, don't check the DNS names in TLS certificates when connecting to
other servers.

As of golang 1.3, crypto/tls no longer natively supports doing partial
verification (verifying the cert issuer but not the hostname), so we
have to disable verification entirely and then do the issuer
verification ourselves. Fortunately, crypto/x509 makes this relatively
straightforward.

If the "server_name" configuration option is passed, we preserve the
existing behavior of checking that server name everywhere.

No option is provided to retain the current behavior of checking the
remote certificate against the local node name, since that behavior
seems clearly buggy and unintentional, and I have difficulty imagining
it is actually being used anywhere. It would be relatively
straightforward to restore if desired, however.
@armon

This comment has been minimized.

Copy link
Member

commented Jun 29, 2014

This looks good. I agree with all of your reasoning. I can't say I fully get wrapTLSClient but I'm definitely no expert in TLS.

Add some basic smoke tests for wrapTLSclient.
Check the success case, and check that we reject a self-signed
certificate.
@nelhage

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2014

@armon Thanks! I think these are the tests I wanted, so I'm now happy with this.

wrapTLSclient is mostly copied from http://golang.org/src/pkg/crypto/tls/handshake_client.go#L235 if that makes you feel any better about it.

@armon

This comment has been minimized.

Copy link
Member

commented Jul 1, 2014

Awesome! Thanks for all your hard work on this!

armon added a commit that referenced this pull request Jul 1, 2014

Merge pull request #233 from nelhage/tls-no-subjname
Restore the 0.2 TLS verification behavior.

@armon armon merged commit 746449f into hashicorp:master Jul 1, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details

@nelhage nelhage deleted the nelhage:tls-no-subjname branch Jul 1, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.