Support AAD authentication via connection string

[zijchen]

Expected behaviour:

Successful connection when "Active Directory Password" is specified in the connection string Example:

const connectionString = "Server=test.database.windows.net;Database=master;Authentication=Active Directory Password;User [email protected];Password=abcd";
mssql.connect(connectionString);

See https://learn.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlconnection.connectionstring?view=dotnet-plat-ext-7.0 for allowed values in authentication prop ("Active Directory Integrated, Active Directory Password, Sql Password")

Actual behaviour:

Connection fails because the authentication is ignored when creating the connection config, defaulting to SQL login with user and password https://github.com/tediousjs/node-mssql/blob/567c2def9408dee6ca3139c9b8103d9a68ba3dd4/lib/base/connection-pool.js#L107-L108

Software versions

• NodeJS: 16.13.2
• node-mssql: 8.1.1
• SQL Server: Azure

[dhensby]

This definitely would be a worthwhile enhancement - would you be prepared to open a PR?

[lukesammy]

@dhensby would it be possible to get a new release with latest (including the above)?

[dhensby]

Oh yes! sorry, this shouldn't have been left unreleased so long. v9.1.0 is out

[lukesammy]

👍 perfect thanks @dhensby

[dhensby]

Unfortunately the release broke standard connections strings. The changes hadn't been tested to ensure standard connection strings worked and, as such, the PR was no adequate to work in a generic way. It provided AD support at the expense of standard authentication.

[Shin-Aska]

Upon further investigation, I found two issues

The first one, the breakage of standard authentication was caused by this code.

https://github.com/tediousjs/node-mssql/blob/9e1fc421a4ae5568e6393f6888d764de8d1914f7/lib/base/connection-pool.js#L302-L305

Where tedious still recognizes an empty authentication object. However if we selectively initialize the authentication under certain conditions like something like this for example:

return Object.entries(parsed).reduce((config, [key, value]) => {
      if (["client id", "client secret", "tenant id", "tenant id", "token", "authentication"].includes(key)) {
        if (typeof(config.authentication) === "undefined") {
          config.authentication = { options: {} }
        }
      }

It would work with basic authentication and aad authentication. I tested this with basic authentication:

var conn_str = `Server=${config.server};Database=${config.database};User Id=${config.username};Password=${config.password};`;

and AAD authentication using client secrets

var conn_str = `Server=${config.server};Database=${config.database};Authentication=azure-active-directory-service-principal-secret;client id=${process.env.AZURE_CLIENT_ID};client secret=${process.env.AZURE_CLIENT_SECRET};tenant id=${process.env.AZURE_TENANT_ID};`;

Another problem I found was with parsing UUIDs. If the value starts with a numeric value, the method parseSqlConnectionString() under require('@tediousjs/connection-string') parses them into an integer. Example 35f6b9e3-92d3-4eb4-999d-1d83ab7d6cde gets parsed into 35, which is a problem because AZURE_TENANT_ID and AZURE_CLIENT_ID are in UUIDs.

So if the UUID on Azure was generated with a numeric character, then our code wouldn't work eitherway.

Should we rewrite _parseConnectionString? or should we file this as a bug under tedious?

[dhensby]

Thanks for that feedback. I had come to the same conclusion about the fact that an empty authentication object will cause standard auth to fail.

We have two choices:

1. Coerce the standard auth into an authentication object (my preferred approach)
2. Only use the authentication object if we are using some other auth type

As for the issue with parsing UUIDs, we must get that sorted in the parsing library.

Other feedback on the PR is that it is not using the correct strings for the authentication types as dictated by the docs so that also needs to be re-implemented.

[Shin-Aska]

Alright, I'll try to sort these out (except for the parsing library one) later this weekend, should I submit a new PR?

[dhensby]

connection string fix here: https://github.com/tediousjs/connection-string/pull/27

[Shin-Aska]

Got the PR up tonight. Went with approach 1. I tried to follow how the authentication object is handle in the connection-pool.js under tedious, where the parameters such as client id, tenant and such are assigned on the config object and then reassigned to config.authentication later.

Also done updating the values of authentication to align with the docs, was a bit tricky to do since what is in the docs isn't exactly a one-to-one match with what is available on tedious so I had to create a method called _parseAuthenticationType where it changes based on the other parameters given. Such as if the type is Active Directory Integrated and only the token parameter is supplied, internally, it becomes azure-active-directory-access-token and so-on.