Windows integrated auth draft #486
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Bug fix: When clientResponse length from getNextBlob() is 0, that indicates server is done with authentication and start sending data. The old code was switching to SENT_NTLM_RESPONSE asynchronously, this was causing problems when the server data comes too soon. Fix is to switch to SENT_NTLM_RESPONSE synchronously. This is safe as server should not be sending data unless it's done with authentication on it's end. There is an extra round trip involved in authentication process with NTLM vs Kerberos.
Must be squashed before pushing to github.
through relative paths.
npm install for these versions is failing due to a bug that causes installation of native modules to fail with the error: C:\Program Files (x86)\nodejs\node_modules\npm\node_modules\node-gyp\src\win_delay_load_hook.c(34): error C2373: '__pfnDliNotifyHook2': redefinition; different type modifiers [C:\projects\tedious\node_modules\sspi-client\build\sspi-client.vcxproj] This seems to be a known issue with certain npm/node-gyp versions: nodejs/node-gyp#972 nodejs/node#7286 Fixing this by installing the latest version of npm before running the tests.
tvrprasad
added a commit
to tvrprasad/tedious
that referenced
this pull request
Feb 1, 2017
Status of various versions of node may be seen at: https://github.com/nodejs/LTS Per that, versions v0.10, v0.12 and v5 fell out of maintenance mode. It might make sense drop support for these versions for future releases of Tedious. Specific motivation for this is the Windows Integrated Authentication work which has test failures due to bugs in those versions, and forcing a hacky work-around that requires installing latest version of npm before running the tests for each version of nodejs. See: tediousjs@15a52b7 This part of this PR: tediousjs#486
…s in a few places, where they can't be caught by application code, with emitting 'error' events.
tvrprasad
added a commit
to tvrprasad/tedious
that referenced
this pull request
Feb 2, 2017
- This is currently implemented for Windows only. - No username/password needed when connecting as domain user. - Leverages current implementation of NTLM authentication that requires username/password. - Adds a dependency on https://www.npmjs.com/package/sspi-client package for implementation of SSPI protocol. - sspi-client has native code which means the module will be built on client machines at Tedious installation time. Address issue - tediousjs#415 This is a squashed version of the work done under tediousjs#486
|
Opened a PR without the 'draft' at the end, which makes it real :) |
|
will it support window based auth without password on Linux |
|
@sanjeev2315 This only works on Windows. I plan to add Linux support after this gets merged. I closed this PR in favor of #497, please track that. |
tvrprasad
added a commit
to tvrprasad/tedious
that referenced
this pull request
May 17, 2017
- This is currently implemented for Windows only. - No username/password needed when connecting as domain user. - Leverages current implementation of NTLM authentication that requires username/password. - Adds a dependency on https://www.npmjs.com/package/sspi-client package for implementation of SSPI protocol. - sspi-client has native code which means the module will be built on client machines at Tedious installation time. Address issue - tediousjs#415 This is a squashed version of the work done under tediousjs#486
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@arthurschreiber I believe this is close to final form. Can I get some early feedback please? I just want to make sure I'm on track here. CI tests will fail as I don't have sspi-client package published yet.
I tested this locally, opened 1600 simultaneous connections and sent queries to two different machines. Works just fine. You can see the simple test at - https://github.com/tvrprasad/sspi-client/blob/master/integration/sqlconnect-stress.js
If you got time to make another pass at https://github.com/tvrprasad/sspi-client, that'd be great. Got some more work before I can publish the npm package.
@arobson FYI. I'm able to take advantage of all the work done for password based NTLM support. I welcome any feedback.
I'm very much looking forward to getting this in :-)