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

Code review tweaks for InsecureSkipVerify support #2

Merged
merged 1 commit into from
Sep 9, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 12 additions & 9 deletions tlsdialer.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,31 +78,34 @@ func DialWithDialer(dialer *net.Dialer, network, addr string, sendServerName boo
serverName = hostname
}

c := new(tls.Config)
*c = *config
// copy config so we can tweak it
configCopy := new(tls.Config)
*configCopy = *config

if sendServerName {
config.ServerName = serverName
// Set the ServerName and rely on the usual logic in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some comments while I was in here

// tls.Conn.Handshake() to do its verification
configCopy.ServerName = serverName
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to modify the copy, not the original

Copy link
Contributor

Choose a reason for hiding this comment

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

It just depends on which we pass where though right? If we want to pass the copy to the tls layer, we should modify the copy. Otherwise, we should modify the original. Similarly, in that case we would check the copy when deciding whether or not to verify the certs in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My requirement here is that the original remain unchanged, since it was passed in to us and we want to be a polite API. Also, your original change made a copy, but modified and passed the original, so the only effect of the copy was to remember the original InsecureSkipVerify setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should add that my original code failed to make a copy when it should have, and it's not clear that the copy it was making was made correctly :( Hence the added unit test condition :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense on not modifying what users pass in. I think in practice people rarely reuse that config, but I guess you never know.

Also, your original change made a copy, but modified and passed the original, so the only effect of the copy was to remember the original InsecureSkipVerify setting.

Sure, it could just as easily have pulled out that boolean.

} else {
// Don't verify, we'll verify manually after handshaking
config.InsecureSkipVerify = true
// Disable verification in tls.Conn.Handshake(). We'll verify manually
// after handshaking
configCopy.InsecureSkipVerify = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here

}

conn := tls.Client(rawConn, config)
conn := tls.Client(rawConn, configCopy)

if timeout == 0 {
err = conn.Handshake()
} else {
go func() {
errChannel <- conn.Handshake()
}()

err = <-errChannel
}

if !sendServerName && err == nil && !c.InsecureSkipVerify {
if !sendServerName && err == nil && !config.InsecureSkipVerify {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we reference InsecureSkipVerify from the original config

// Manually verify certificates
err = verifyServerCerts(conn, serverName, config)
err = verifyServerCerts(conn, serverName, configCopy)
}
if err != nil {
rawConn.Close()
Expand Down
19 changes: 18 additions & 1 deletion tlsdialer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,25 @@ func TestOKWithServerName(t *testing.T) {
}

func TestOKWithoutServerName(t *testing.T) {
_, err := Dial("tcp", ADDR, false, &tls.Config{
config := &tls.Config{
RootCAs: cert.PoolContainingCert(),
}
_, err := Dial("tcp", ADDR, false, config)
if err != nil {
t.Errorf("Unable to dial: %s", err.Error())
}
serverName := <-receivedServerNames
if serverName != "" {
t.Errorf("Unexpected ServerName on server: %s", serverName)
}
if config.InsecureSkipVerify {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added condition to make sure that our original config remains untouched.

t.Errorf("Original config shouldn't have been modified, but it was")
}
}

func TestOKWithInsecureSkipVerify(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New test for InsecureSkipVerify

_, err := Dial("tcp", ADDR, false, &tls.Config{
InsecureSkipVerify: true,
})
if err != nil {
t.Errorf("Unable to dial: %s", err.Error())
Expand Down