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

Added support for AAD authentication via connection string #1436

Merged

Conversation

Shin-Aska
Copy link
Contributor

What this does:

This PR adds several fields related to AAD authentication. The ones added on this PR are:

  1. Authentication - dictating the authentication type
  2. Client ID/ClientID - used when authenticating via AAD service principal
  3. Client Secret/ClientSecret - used when authenticating via AAD service principal
  4. Tenant ID/TenantID - used when authenticating via AAD service principal
  5. Token - used when authenticating via AAD access token

I am completely open in renaming these fields to whichever feels more appropriate but feedback is more than welcome.

This PR is going to accomodate issue #1400

Copy link
Collaborator

@dhensby dhensby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this - there's a small linting fix that that is required.

So I'm clear (as I can't really find any documentation on this), this would be used in conjunction with Authentication=Active Directory Integrated; in the connection string?

Are the properties you've added (tenantid, token, etc) part of a documented spec? They are not listed here: https://learn.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlconnection.connectionstring?view=dotnet-plat-ext-6.0

If they don't form part of a documented spec, then I think we need to add some info to the readme on this.

@Shin-Aska
Copy link
Contributor Author

Hi,

So I'm clear (as I can't really find any documentation on this), this would be used in conjunction with Authentication=Active Directory Integrated; in the connection string?

Yes but I only have tested Azure Active Directory Access Token and Azure Active Directory Service Principal Secret on the Authentication value.

Are the properties you've added (tenantid, token, etc) part of a documented spec? They are not listed here: https://learn.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlconnection.connectionstring?view=dotnet-plat-ext-6.0

Yes, they are part of the active directory identity: https://learn.microsoft.com/en-us/azure/databricks/dev-tools/api/latest/aad/service-prin-aad-token

@dhensby
Copy link
Collaborator

dhensby commented Oct 27, 2022

Yes, they are part of the active directory identity: https://learn.microsoft.com/en-us/azure/databricks/dev-tools/api/latest/aad/service-prin-aad-token

Thanks - I meant more a documented connection string spec. I think it's not a connection string spec, so we should also add some info to the docs.

Copy link
Collaborator

@dhensby dhensby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix linting issues
  • Only use one key name for each input (eg: just client id)
  • Add info to docs for this

lib/base/connection-pool.js Show resolved Hide resolved
@Shin-Aska
Copy link
Contributor Author

I went and started fixing the code linting and redundant key. I will work on the documentation tomorrow. Where do you suggest we put the information? After the SQL Injection under the Other category? or should be it after the Connection Pools under the Example category of readme.md?

@dhensby
Copy link
Collaborator

dhensby commented Oct 27, 2022

Under Formats > classic connection string ?

Add a section about authentication config?

Thanks for the effort!

@Shin-Aska
Copy link
Contributor Author

I added some samples on how to configure node-mssql using connection string via different authentication types. I have also added labels for the classic examples since apparently it contained two configurations (one for classic using default tedious driver and the other being nodemssqlv8 driver).

@dhensby
Copy link
Collaborator

dhensby commented Oct 28, 2022

Thanks! Though it looks like there's been an accidental duplication of the readme

@Shin-Aska
Copy link
Contributor Author

Oops, my bad. I removed it now

dhensby
dhensby previously approved these changes Oct 28, 2022
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 this pull request may close these issues.

2 participants