diff --git a/wolftls/config.go b/wolftls/config.go index d07c5ef..6b3e231 100644 --- a/wolftls/config.go +++ b/wolftls/config.go @@ -72,13 +72,19 @@ type Certificate struct { // Config structures a TLS connection's parameters. type Config struct { - // ServerName is the value sent in the SNI extension. For clients, - // it is also used for hostname verification unless InsecureSkipVerify is set. + // ServerName is the value sent in the SNI extension and, for clients, is + // used for hostname verification. A client that verifies the peer + // (InsecureSkipVerify == false) must set ServerName: otherwise the + // certificate name is never checked, so doHandshake rejects the connection + // before the handshake, matching crypto/tls. Set InsecureSkipVerify to + // connect without a ServerName. ServerName string // InsecureSkipVerify disables wolfSSL's built-in certificate verification. - // When true, the VerifyConnection callback (if set) is still called after - // the handshake so the caller can perform custom verification. + // It is also the alternative to setting ServerName: when false, a client + // must provide ServerName so the certificate name can be checked. When + // true, the VerifyConnection callback (if set) is still called after the + // handshake so the caller can perform custom verification. // // This determines how a client verifies the server, setting it on a // server is a no-op. diff --git a/wolftls/conn.go b/wolftls/conn.go index 33e2a70..2051679 100644 --- a/wolftls/conn.go +++ b/wolftls/conn.go @@ -169,6 +169,14 @@ func (c *Conn) doHandshake() error { return errors.New("wolftls: ServerName contains NUL byte") } + // A client with verification enabled must set ServerName; without it the + // certificate name is never checked, so reject rather than proceed. This is + // enforced to match crypto/tls, which requires either ServerName or + // InsecureSkipVerify when verifying. + if c.isClient && !c.config.InsecureSkipVerify && c.config.ServerName == "" { + return errors.New("wolftls: either ServerName or InsecureSkipVerify must be specified") + } + // Create CTX with version-flexible method if c.isClient { c.ctx = wolfSSL.WolfSSL_CTX_new_v23_client() diff --git a/wolftls/tls_test.go b/wolftls/tls_test.go index 42baef2..b04c91c 100644 --- a/wolftls/tls_test.go +++ b/wolftls/tls_test.go @@ -594,6 +594,136 @@ func TestVerifyHostnameMismatch(t *testing.T) { <-errc } +// TestVerifyFailsClosedWhenServerNameEmpty checks that a client with peer +// verification enabled (InsecureSkipVerify == false) but no ServerName is +// rejected before the handshake, matching crypto/tls. +// +// The server presents a certificate that chains to the trusted CA but is +// issued for example.com / 127.0.0.1. doHandshake only performs the +// certificate name check when ServerName is non-empty, so with an empty +// ServerName the name is never checked while the trusted chain is still +// accepted. crypto/tls rejects this configuration ("either ServerName or +// InsecureSkipVerify must be specified"); wolftls should do the same. +func TestVerifyFailsClosedWhenServerNameEmpty(t *testing.T) { + certPEM := loadFile(t, certPath("server-cert.pem")) + keyPEM := loadFile(t, certPath("server-key.pem")) + caPEM := loadFile(t, certPath("ca-cert.pem")) + + serverConfig := &Config{ + Certificates: []Certificate{{ + CertPEM: certPEM, + KeyPEM: keyPEM, + }}, + } + + // Verification ON, but no ServerName. + clientConfig := &Config{ + ServerName: "", + InsecureSkipVerify: false, + RootCAPEMs: [][]byte{caPEM}, + } + + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + defer ln.Close() + + errc := make(chan error, 1) + go func() { + conn, err := ln.Accept() + if err != nil { + errc <- err + return + } + tlsConn := Server(conn, serverConfig) + defer tlsConn.Close() + errc <- tlsConn.Handshake() + }() + + conn, err := net.Dial("tcp", ln.Addr().String()) + if err != nil { + t.Fatalf("dial: %v", err) + } + tlsConn := Client(conn, clientConfig) + defer tlsConn.Close() + + err = tlsConn.Handshake() + if err == nil { + t.Fatal("client handshake succeeded with verification enabled but " + + "empty ServerName — the certificate name was never checked. " + + "Expected the handshake to be rejected, as crypto/tls does.") + } + // Assert it is the fail-closed guard rejecting the config, not an unrelated + // failure, so the test keeps pinning the guard if the handshake path changes. + if !strings.Contains(err.Error(), + "either ServerName or InsecureSkipVerify must be specified") { + t.Fatalf("expected the ServerName-required guard error, got: %v", err) + } + t.Logf("failed closed as expected: %v", err) + + // The client rejected the config before sending a ClientHello, so close + // the connection to unblock the server's handshake read, then drain it. + tlsConn.Close() + <-errc +} + +// A client with InsecureSkipVerify set may omit ServerName: the fail-closed +// guard must not fire and the handshake must complete. This pins the guard's +// !InsecureSkipVerify condition so it is not later simplified into rejecting +// every empty-ServerName client. +func TestInsecureSkipVerifyAllowsEmptyServerName(t *testing.T) { + certPEM := loadFile(t, certPath("server-cert.pem")) + keyPEM := loadFile(t, certPath("server-key.pem")) + + serverConfig := &Config{ + Certificates: []Certificate{{ + CertPEM: certPEM, + KeyPEM: keyPEM, + }}, + } + + // Verification OFF and no ServerName: allowed, guard must not fire. + clientConfig := &Config{ + ServerName: "", + InsecureSkipVerify: true, + } + + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + defer ln.Close() + + errc := make(chan error, 1) + go func() { + conn, err := ln.Accept() + if err != nil { + errc <- err + return + } + tlsConn := Server(conn, serverConfig) + defer tlsConn.Close() + errc <- tlsConn.Handshake() + }() + + conn, err := net.Dial("tcp", ln.Addr().String()) + if err != nil { + t.Fatalf("dial: %v", err) + } + tlsConn := Client(conn, clientConfig) + defer tlsConn.Close() + + if err := tlsConn.Handshake(); err != nil { + t.Fatalf("client handshake should succeed with InsecureSkipVerify and "+ + "empty ServerName, got: %v", err) + } + + if err := <-errc; err != nil { + t.Fatalf("server handshake failed: %v", err) + } +} + func TestMinMaxVersion(t *testing.T) { certPEM := loadFile(t, certPath("server-cert.pem")) keyPEM := loadFile(t, certPath("server-key.pem"))