-
Notifications
You must be signed in to change notification settings - Fork 438
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
feat: Introduce pluggable authentication providers. #624
base: master
Are you sure you want to change the base?
Conversation
This merges the `SENT_LOGIN7_WITH_STANDARD_LOGIN`, `SENT_LOGIN7_WITH_NTLM` and `SENT_NTLM_RESPONSE` states into a single `SENT_LOGIN7` state.
Both domain and workstation are optional according to the MS-NLMP specification, so we should not require a domain to be given. This should improve/ease the use of NTLM auth with Azure SQL Server.
SSPI is the interface name / API that's used for all authentication providers.. "native" is a more fitting name because it depends on the operating system's native APIs to provide security support.
@arthurschreiber sound like a good idea! Later we can also bring these |
@arthurschreiber Couple of quick high level comments:
|
@tvrprasad The idea would be to have at least 3 separate packages:
I think that'd cover all SSPI types SQL Server supports. I've seen that there is also some sort of Federated Authentication support documented in the latest TDS specifications, but I've not really looked into how it works - but it seems different enough from the SSPI authentication mechanisms that there's probably no sane way to reconcile the APIs.
|
@v-suhame Thanks! I'd love to have tests for NTLM and Kerberos auth as part of the CI builds (first inside tedious itself, later as part of the individual packages). Is there some "straightforward" way to set up a CI system for this? Maybe via Azure SQL Server and Azure AD? 🤔 I do have an Azure account and could set everything up, but some pointers would be great. (/cc @tvrprasad - I know you might not have time, but maybe you have a good idea here.) |
Federated Authentication was the former name for Azure AD, TDS docs still seems to use the old name. We have long term plan to add Azure AD support to Tedious 😉 |
src/connection.js
Outdated
@@ -396,6 +399,8 @@ class Connection extends EventEmitter { | |||
REDIRECT: 1, | |||
RETRY: 2 | |||
}; | |||
|
|||
this.authProvider = new DefaultAuthProvider(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.authProvider
should be equal to the local authProvider
here.
src/connection.js
Outdated
authProvider = config.authProvider; | ||
} else if (config.domain) { | ||
authProvider = new NTLMAuthProvider(this, { domain: config.domain }); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NTLMAuthProvider
needs username, password, workstation along with domain.
@v-suhame that seems like the best idea right now. @Hadis-Fard any thoughts on this? How do we do this for PHP? |
src/connection.js
Outdated
return this.sendDataToTokenStreamParser(data); | ||
// Use SSPI blob if received | ||
if (this.ntlmpacketBuffer) { | ||
return this.authProvider.processChallenge(this.ntlmpacketBuffer, (error, responseBuffer) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function call should be this.authProvider.parseChallengeMessage( )
, authentication provider implementation uses the name parseChallengeMessage
😄
@meet-bhagdev +1 to @v-suhame's idea. As she said Tedious doesn't support AzureAD yet and is in our future plans to support it. |
There are tests inside Tedious for NTLM, both for sspi-client and the non-sspi-client version. I think they run as part of CI but not 100% sure. sspi-client is applicable only for Windows and that's where the tests run. sspi-client is not just NTLM, it's NTLM, Kerberos and Negotiate protocols. But since it uses Microsoft proprietary API, it'll only work on Windows. Kerberos auth code path that @v-suhame has in PR, I think, should only be enabled for Linux. |
--Updated-- And yes, Kerberos auth code path is reachable only for non-Windows based system. |
@v-suhame how woulad Azure integrated auth work with the SQL Server docker image? I am not sure if that is a supported scenario. Am I missing something? |
Sorry! I meant SQL Server in Azure VM. |
@arthurschreiber I've committed changes, based on my review comments. |
@v-suhame Could you install box version of SQL Server on Azure VM that's added to an AD domain to test for AD authentication, right? Or does that also only work with Azure AD? |
@arthurschreiber I have finished AAD (fedauth) authentication usign username password based on current master, and able to connect. I was wondering when would you merge this PR, so I can adapt this design, or should i just send the AAD PR as is? 😕 |
@Hadis-Fard Would be great if you could send the current version of your PR. 👍 This would allow us to take a look and see whether we can integrate the different auth approaches somehow. |
@arthurschreiber Is there anything you need assistance with to wrap up this PR? 😄 Looks like it is almost ready to merge. |
@arthurschreiber Is there anything we can do to help move this PR forward? We'd really like to be able to add additional authentication methods in tedious using this method. A lot of customers are migrating to Azure hosted services and supporting them is a high priority for us. 😄 |
…ggable-auth-providers
👋 Hey @David-Engel, I've picked up work on this pull request and #707 - I'm trying right now to come up with an authentication provider interface that will support both integrated and federated authentication schemes. 👍 |
Hey @arthurschreiber, any update on this? Is there anything I can help with to get this going? |
When will this be worked on? |
@arthurschreiber This feature sounds perfect, do you know if this will completed? |
I am mainly looking for kerberos authentication support in tedious. Ticket #612 says it would be fixed as part of this ticket. Is there any update ? Thanks. |
In preparation of a new
tedious
release, I was going through all changes sincev2.0.1
. One thing I want to cleanup before the release is the integration withsspi-client
.I'm not super happy about the current situation where
sspi-client
is a hard dependency oftedious
, and seeing that SQL Server actually supports more authentication methods and that there is another authentication related pull request in flight (#612) that addsnode-kerberos
as another hard dependency, I feel that the current approach for authentication methods is not scalable.In this pull request, I explore the addition of what I call "authentication" providers (they're called "Security Support Providers" in the MS-TDS specification). The whole pull request is still very much in flux and a work in progress, but I believe the direction proposed here to be sound.
Authentication providers are simply objects that implement a
.handshake(input, callback)
method that is responsible for taking in authentication handshake data coming from SQL Server, and calling the passedcallback
with the handshake data that should be sent back to the SQL Server.The long-term goal is to ship tedious with only a "default" dummy provider that does not actually do anything. Other providers like for NTLM, Kerberos or the native SSPI support will be extracted into separate npm modules organization. This way, we can extract both the implementation of these providers out of
tedious
, and keeptedious
free from dependencies that are not required for all users.The authentication provider to use for connecting to SQL Server will be specifiable via a new option when establishing a connection, and each provider will be able to handle options that are specific to it in it's own way.
For example, when using NTLM (domain) authentication I imagine connection creation could look like this:
While native SSPI could look like this:
@tvrprasad @v-suhame What do you think?