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

TLS server name not set when hostInCertificate not set #93

Closed
jwbargsten opened this issue Feb 25, 2023 · 0 comments · Fixed by #94
Closed

TLS server name not set when hostInCertificate not set #93

jwbargsten opened this issue Feb 25, 2023 · 0 comments · Fixed by #94

Comments

@jwbargsten
Copy link

Describe the bug
I have issues with connecting to a MSSQL server using TLS. I get the error

could not ping db	{"error": "TLS Handshake failed: tls: either ServerName or InsecureSkipVerify must be specified in the tls.Config"}

To Reproduce
using a connection string comparable to this:

sqlserver://someuser:somepass@somehost?TrustServerCertificate=false&encrypt=true

and the code

db, err := sqlx.Open("sqlserver", connectionString)
db.Ping()

I get the error shown above.

The issue can be reproduced by this test (add it to msdsn/conn_str_test.go)

func TestServerNameInTLSConfig(t *testing.T) {
	connStr := "sqlserver://someuser:somepass@somehost?TrustServerCertificate=false&encrypt=true"
	cfg, err := Parse(connStr)
	if err != nil {
		t.Errorf("Could not parse valid connection string %s: %v", connStr, err)
	}
	if cfg.TLSConfig.ServerName != "somehost" || cfg.Host != "somehost" {
		t.Errorf("Expected somehost as host %s and TLS server, but got %s", cfg.Host, cfg.TLSConfig.ServerName)
	}
}

Expected behavior
db.Ping() works

Additional context
I think the bug was introduced in fd44003

The code for setting p.Host was moved down in the parse function, resulting in an empty p.Host value at the time of the creation of p.TLSConfig.

jwbargsten added a commit to jwbargsten/go-mssqldb that referenced this issue Feb 27, 2023
A change in fd44003 moved the code for setting p.Host down in the parse
function, resulting in an empty p.Host value at the time of the creation
of p.TLSConfig. I extracted the TLS parsing into a separate function and
moved the TLSConfig creation to the end of the parsing function.
shueybubbles pushed a commit that referenced this issue Feb 27, 2023
* Fixed #93: servername was not supplied to TLS cfg

A change in fd44003 moved the code for setting p.Host down in the parse
function, resulting in an empty p.Host value at the time of the creation
of p.TLSConfig. I extracted the TLS parsing into a separate function and
moved the TLSConfig creation to the end of the parsing function.

* Added extra test scenario with hostnameincertificate parameter
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 a pull request may close this issue.

1 participant