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

Support AAD authentication via connection string #1400

Open
zijchen opened this issue Jun 1, 2022 · 10 comments
Open

Support AAD authentication via connection string #1400

zijchen opened this issue Jun 1, 2022 · 10 comments

Comments

@zijchen
Copy link

zijchen commented Jun 1, 2022

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

case 'authentication':
break

Software versions

  • NodeJS: 16.13.2
  • node-mssql: 8.1.1
  • SQL Server: Azure
@dhensby
Copy link
Collaborator

dhensby commented Jun 1, 2022

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

@lukesammy
Copy link

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

@dhensby
Copy link
Collaborator

dhensby commented Jan 17, 2023

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

@dhensby dhensby closed this as completed Jan 17, 2023
@lukesammy
Copy link

👍 perfect thanks @dhensby

@dhensby
Copy link
Collaborator

dhensby commented Jan 18, 2023

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.

@dhensby dhensby reopened this Jan 18, 2023
@Shin-Aska
Copy link
Contributor

Shin-Aska commented Jan 19, 2023

Upon further investigation, I found two issues

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

}
return config
}, { authentication: { options: {} }, options: {}, pool: {} })
}

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
Copy link
Collaborator

dhensby commented Jan 19, 2023

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
Copy link
Contributor

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

@dhensby
Copy link
Collaborator

dhensby commented Jan 19, 2023

connection string fix here: tediousjs/connection-string#27

@Shin-Aska
Copy link
Contributor

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.

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

No branches or pull requests

4 participants