SQL Server usernames and passwords with certain characters fail

[tartale]

SQL Server allows for most characters in the username and password. However, if a user changes their username and/or password to any of the following cases:

• Contains a semi-colon
• Starts with a space
• Ends with a space

Then any access from a Go program using this library will fail with this error: Login error: mssql: Login failed for user 'xyz'.

[tartale]

My colleague and I brainstormed a few solutions to this - we're attempting to come up with non-breaking (backward-compatible) solutions.

Solution 1:

• Parse the connection string by locating the start of all the 'key=' strings (where "key" is one of the documented, supported keys in the README).
• Consider the 'value' to be everything between the '=' and either:
  • the end of the string, OR
  • the next location of a 'key=' string (minus the semicolon immediately prior).
• Do not trim the values for 'user id' and 'password'

Solution 2:

• Use an alternate separator character (such as <tab>, or a non-printable character like the "record separator" (ascii code 30)). If the connection string contains this alternate separator, it is assumed to be the separator to be used.
• Do not trim the values for 'user id' and 'password'

Solution 3:

• Allow for escaped values. For example, if the 'escape' character is '\', then the following escape sequences would be supported:
  • '\' - literal '\'
  • '\;' - literal ';'
  • '\ ' - literal ' '
• This solution would need a version stamp (see below)

Solution 4:

• Allow for quoted values (see MS references below). For every quote character supported, there must also be an 'escape' to allow for a literal of that character to be specified as well.
• This solution would need a version stamp (see below)

In any of the above solutions, we could (and indeed may need to) also ensure it is backwards-compatible by introducing a new "key" called "version"; in connection strings with "version=2", we'd engage the alternate code.

Opinions? I'm planning on submitting a patch with some unit tests, but would like input on the direction.

[dimdin]

Miscrosoft uses two ways to solve the problem, the ODBC way is to enclose in {} https://msdn.microsoft.com/en-us/library/ee224215(v=sql.105).aspx and the OLEDB way is to enclose in quotes https://msdn.microsoft.com/en-us/library/ee223950(v=sql.105).aspx

[tartale]

@dimdin Does this library support enclosing passwords in {} or quotes? If not, I can file a separate issue, or should this issue be re-opened until a solution is available?

I suspect this may have been closed by accident?

I don't think I'll get many comments/discussion on a closed issue... If you or @denisenkom can re-open, that'd be great so I don't need to re-initiate the discussion

[dimdin]

sorry, I did not intent to close this

[dimdin]

The library does not support enclosing user names or password in {} or in quotes.

[tartale]

Thanks for re-opening, I added a solution to my original post...

[denisenkom]

I don't like solutions 1-3, they are non-standard. I think we should go with ODBC or OLEDB approach. I personally lean toward OLEDB; it seems to be more flexible.

Also I don't think we should add version attribute, it would make connection string uglier. I think if connection string parsing changes in non-backward compatible way it is easy enough for users to update their connection strings to conform to new syntax.

[ArtnerC]

Since Microsoft is deprecating OLEDB it might make more sense to go with ODBC standards? Though I too prefer quotes.

[denisenkom]

Agree, ODBC is probably better. By reading MS ODBC docs it is not clear how to escape password which contains both semicolon and curly bracket, e.g.: 'abc};def'

[dimdin]

you double the closing brace i.e. {abc}};def} https://msdn.microsoft.com/en-us/library/ee209078(v=sql.105).aspx

[tartale]

Depending on how a surround-by-bracket solution was implemented, the calling code could get somewhat tedious - imagine the requirement to escape-and-quote was leveraged to the caller - a library user's code would look something like this:

var userId string = ... // obtain user Id
var password string = ... // obtain password

userId = strings.Replace(userId, "{", "{{", -1)
userId = strings.Replace(userId, "}", "}}", -1)
password = strings.Replace(password, "{", "{{", -1)
password = strings.Replace(password, "}", "}}", -1)
connectionString := fmt.Sprintf("user id={%s}; password={%s}; ...")
db, err := sql.Open("mssql", connectionString)
...

Unless I'm missing something?

[melignus]

CIFS urlencodes the special characters in usernames and passwords e.g:

Testing{;password becomes Testing%7B%3Bpassword

Adding a line of documentation requiring special characters to be encoded and then decoding on the back sounds easier than reworking the parser.

e.g.

Usernames or Passwords with special characters should be url encoded first to avoid parsing errors.

[arsing]

We're interested in getting this fixed since we're using this driver at work, and would like to have passwords with semi-colons, etc work.

Will you be interested in accepting a PR that adds support for parsing the ODBC format (values wrapped in {}) ?

Switching to ODBC parsing mode completely will be a breaking change since it'll change how {} are interpreted. How would you like that resolved? If the breaking change is undesirable, one idea is to prepend "odbc://" to the connection string to opt in to ODBC parsing mode.

Regarding the difficulty of constructing such an escaped string in user code - it could be possible to expose an API that takes in a ConnectionParameters struct and generates a connection string for use with sql.Open ?

[denisenkom]

Here is another option:

Introduce a function to set connection string style: SetConnectionStringStyle, this function would take a enum which can have values: connStrStyleSimple, connStrStyleODBC. connStrStyleSimple would be a default.

Change Open method to use connection string style which was set by previous call to SetConnectionStringStyle.

Than said I actually like prefix better for it's simplicity, but I think it should instead be "odbc:", without // at the end.

@dimdin and @kardianos can you chime in?

[arsing]

(Reposting from correct account. Sorry for spam :) )

SetConnectionStringStyle will set it on the global driver instance, yes? That will be a problem if the application has multiple connection strings in mixed format. Each call to sql.Open will need to be preceded by a call to SetConnectionStringStyle, and even then it won't be concurrent-safe.

[kardianos]

I would be in favor of supporting a URL style DSN. Code would look like:

switch {
default:
    return parseCurrentDSN(dsn)
case strings.HasPrefix(dsn, "sqlserver://")
    return parseUrlDsn(dsn)
}

This would be ambiguous in detection and because URL can generally encode most vars with url encoding, it should support the various password chars or re-implementing something different.

[denisenkom]

Right, URL format would be great. You don't have to implement encoder for it. But there are only two common ways to specify connection strings for MSSQL server today and they are ODBC style connection strings or OLEDB style connection strings, so that would be a downside of URL format because it won't be a common format.

But we could possibly support multiple formats, e.g.:

switch {
default:
    return parseCurrentDSN(dsn)
case strings.HasPrefix(dsn, "sqlserver_odbc:")
    return parseOdbcDsn(dsn)
case strings.HasPrefix(dsn, "sqlserver_oledb:")
    return parseOledbDsn(dsn)
case strings.HasPrefix(dsn, "sqlserver://")
    return parseUrlDsn(dsn)
}

[kardianos]

Sounds good to me. But I would omit the sqlserver_ prefix as it should still be clear without them.

On Sun, Dec 11, 2016, 09:19 denisenkom notifications@github.com wrote:

Right, URL format would be great. You don't have to implement encoder for it. But there are only two common ways to specify connection strings for MSSQL server today and they are ODBC style connection strings or OLEDB style connection strings, so that would be a downside of URL format because it won't be a common format.

But we could possibly support multiple formats, e.g.:

switch { default: return parseCurrentDSN(dsn) case strings.HasPrefix(dsn, "sqlserver_odbc:") return parseOdbcDsn(dsn) case strings.HasPrefix(dsn, "sqlserver_oledb:") return parseOledbDsn(dsn) case strings.HasPrefix(dsn, "sqlserver://") return parseUrlDsn(dsn) }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/denisenkom/go-mssqldb/issues/169#issuecomment-266294286, or mute the thread https://github.com/notifications/unsubscribe-auth/AAuFsZcX83hsOXJjKPOtZNE16EH20uQhks5rHDChgaJpZM4KCPs3 .

[arsing]

Just to clarify, is this the decision for the formats?

• "server=localhost;user id=someuser;password=foobar" No prefix - parsed by splitting on semicolons
• "sqlserver://server=localhost&user+id=someuser&password=foo%3Bbar" Parsed using url.ParseQuery to get a map [string] []string. Also easy to create for users via url.Values.Encode()
• "odbc:server=localhost;user id=someuser;password={foo;bar}" Parsed using custom parser.
• "oledb:server=localhost;user id=someuser;password='foo;bar'" Parsed using custom parser.

[kardianos]

@arsing The odbc and oledb look fine to me.

For the url parser, I would perhaps do: sqlserver://user:pass@localhost/Instance?op1=foo&op2=fii

[arsing]

Got it. Thanks.

I'm waiting for some administrative work to finish up before I can contribute. Once that's done I'll send out a PR for the odbc: format and the URL format.