microsoft / go-sqlcmd

The new sqlcmd, CLI for SQL Server and Azure SQL (winget install sqlcmd / sqlcmd create mssql / sqlcmd open ads)
https://learn.microsoft.com/sql/tools/sqlcmd/go-sqlcmd-utility
MIT License
335 stars 59 forks source link

Generated connection strings are incorrect #249

Open yorek opened 1 year ago

yorek commented 1 year ago

When generating the connection strings via sqlcmd config connection-strings, the ADO.NET doesn't work as is:

Reference to connection string keywords is here: https://learn.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlconnection.connectionstring?view=dotnet-plat-ext-7.0

Also, the connection string comes with the default for TrustServerCertificate to false, which make it hard to use it with a local SQL Server. sqlcmd should detect if the connection string is generated for a local machine and automatically set that property to true, to make usage frictionless.

stuartpa commented 1 year ago

Great feedback, does this look right (addresses all 4 issues above):

C:\src\go-sqlcmd\cmd\modern>sqlcmd config cs ADO.NET: Server=tcp:localhost,1435;Initial Catalog=master;Persist Security Info=False;User ID=username;Password=REDACTED;MultipleActiveResultSets=False;Encrypt=True;TrustServerCertificate=True;Connection Timeout=30; JDBC: jdbc:sqlserver://localhost:1435;database=master;user=username;password=REDACTED;encrypt=true;trustServerCertificate=true;loginTimeout=30; ODBC: Driver={ODBC Driver 13 for SQL Server};Server=tcp:localhost,1435;Database=master;Uid=username;Pwd=REDACTED;Encode=yes;TrustServerCertificate=yes;Connection Timeout=30; SQLCMD: SET "SQLCMDPASSWORD=REDACTED" & sqlcmd -S localhost,1435 -U username

I was outputting it as YAML, which was putting the newlines in the output, which I didn't know it did. Now just outputting as text.

shueybubbles commented 1 year ago

how about adding a string for go-mssqldb? We'd prefer the url syntax for the Go driver because the pseudo-ODBC and pseudo-ADO syntax support don't support passwords with escaped separator characters etc.

Which reminds me - we should make sure to properly escape such characters in the ado and odbc strings, since it's not as straightforward as escaping a URL component.

yorek commented 1 year ago

Great feedback, does this look right (addresses all 4 issues above):

Everything looks good at first glance, I'll try the ADO.NET and ODBC connection string ASAP. I'm not an expert in Java so let's make sure someone can test it properly too.

yorek commented 1 year ago

Another suggestion. Can you make the TrustServerCertificate parameter set to false only if connecting to an Azure SQL Database? (you can take a look at the server name. If it ends with .database.windows.net then set that option to false) The goal is to make the experience frustration free.

stuartpa commented 1 year ago

how about adding a string for go-mssqldb? We'd prefer the url syntax for the Go driver because the pseudo-ODBC and pseudo-ADO syntax support don't support passwords with escaped separator characters etc.

Which reminds me - we should make sure to properly escape such characters in the ado and odbc strings, since it's not as straightforward as escaping a URL component.

Added GO:

C:\src\go-sqlcmd\cmd\modern>sqlcmd config cs ADO.NET: Server=tcp:localhost,1435;Initial Catalog=master;Persist Security Info=False;User ID=username;Password=REDACTED;MultipleActiveResultSets=False;Encrypt=True;TrustServerCertificate=True;Connection Timeout=30; JDBC: jdbc:sqlserver://localhost:1435;database=master;user=username;password=REDACTED;encrypt=true;trustServerCertificate=true;loginTimeout=30; ODBC: Driver={ODBC Driver 18 for SQL Server};Server=tcp:localhost,1435;Database=master;Uid=username;Pwd=REDACTED;Encrypt=yes;TrustServerCertificate=yes;Connection Timeout=30; GO: sqlserver://username:REDACTED@localhost,1435?database=master;encrypt=true;trustServerCertificate=true;loginTimeout=30 SQLCMD: SET "SQLCMDPASSWORD=REDACTED" & sqlcmd -S localhost,1435 -U username

stuartpa commented 1 year ago

Another suggestion. Can you make the TrustServerCertificate parameter set to false only if connecting to an Azure SQL Database? (you can take a look at the server name. If it ends with .database.windows.net then set that option to false) The goal is to make the experience frustration free.

Done

shueybubbles commented 1 year ago

go-mssqldb doesn't use the same names for a bunch of the parameters. eg "logintimeout" => "dial timeout" There is an adoSynonyms list we can edit to match them up though. https://github.com/microsoft/go-mssqldb/blob/f481f922043187c6b33c43e32683cd03989d9d04/msdsn/conn_str.go#L399

shueybubbles commented 1 year ago

What value do we get from setting trust server cert to false for Azure? Azure connections are always encrypted and always use a root cert that's trusted by most OS distributions. I could understand omitting the parameter but actively setting it to false is not needed.

yorek commented 1 year ago

Yep, I'm fine not adding the TrustServerCertificate property in the connection string for Azure SQL connections. Omitting it means it is false by default.