tvrprasad / sspi-client Goto Github PK

Need to setup some continuous integration tests, travis-ci or whatever is the flavor of the day.

Ref: https://github.com/tvrprasad/sspi-client/blob/master/src_native/sspi_impl.cpp#L197

The code uses APIs like InitializeSecurityContextA and AcquireCredentialsHandleA
These are ANSI APIs and should be replaced with Unicode APIs

Ref: https://msdn.microsoft.com/en-us/library/ff381407.aspx

These changes will also allow support for IDN connectivity to Sql Server.

node-gyp rebuild should succeed on Linux so that npm install succeeds on all platforms with Tedious.

• Simple exclude all sources in binding.gyp for non-Windows platforms.

Throw not supported platform exception in SspiClient constructor for non-Windows platforms.

• In case someone tries to use it.

Connecting to database on a different machine is working with Tedious hack integration - https://github.com/tvrprasad/tedious/tree/windows-integrated-auth-draft

Connecting to the same machine seems to fail.

NTLM Login7: data
NTLM Login7: message
NTLM Login7: receivedChallenge
NTLM Reseponse: data
buffer.js:816
throw new RangeError('Index out of range');
^

RangeError: Index out of range
at checkOffset (buffer.js:816:11)
at Buffer.readInt16LE (buffer.js:951:5)
at parseChallenge (E:\src\tedious\src\token\sspi-token-parser.js:13:32)
at parser.readUsVarByte (E:\src\tedious\src\token\sspi-token-parser.js:27:19)
at awaitData (E:\src\tedious\src\token\stream-parser.js:316:7)
at Parser.awaitData (E:\src\tedious\src\token\stream-parser.js:100:7)
at Parser.readBuffer (E:\src\tedious\src\token\stream-parser.js:313:10)
at readUInt16LE (E:\src\tedious\src\token\stream-parser.js:348:12)
at awaitData (E:\src\tedious\src\token\stream-parser.js:144:7)
at Parser.awaitData (E:\src\tedious\src\token\stream-parser.js:100:7)

var config= {
  userName: 'venktam',
  domain: 'REDMOND',
  password: '',
  server: 'prasadtammana',
  options: { encrypt: false, database: 'AdventureWorks2016CTP3' }
};

var connection = new Connection(config);
connection.on('connect', function (err) {
  if (err) {
    console.log("Connection failed. Error details:");
    console.log(err);
  }
  else {
    console.log("Connected");
    connection.close();
  }
});

At https://github.com/tvrprasad/sspi-client/blob/master/src_js/index.js#L85 you can pass in a parameter to the function instead of using the variable that declared before the function call.

const invokeGetNextBlob = function (sspiClient) {... }
and
invokeGetNextBlob(this);

There are a couple of places where we're not freeing resources allocated by SSPI. Clean them up.

In a couple places in src_js/fdqn.js, including the file name. :)

Initialization code only gets run once. Tests related to Initialization need to be run a separate process to ensure code execution.

Need to investigate nodeunit support for running a given test in a separate process.

Hello,

I'm running the below

npm install sspi-client

Which fails to build due to the use of snprintf.

I believe this was deprecated in MS VC++ in favour of a more secure behaviour.

I need to build sspi-client to be able to use windows auth in the tedious driver, however I can't build it in windows.

This is the output, if you need anymore info let me know.

..\src_native\sspi_impl.cpp(72): error C3861: 'snprintf': identifier not found [C:\Users\USER\App\node_modules\sspi-client\build\sspi-client.vcxproj] ..\src_native\sspi_impl.cpp(106): error C3861: 'snprintf': identifier not found [C:\Users\USER\App\node_modules\sspi-client\build\sspi-client.vcxproj] ..\src_native\sspi_impl.cpp(118): error C3861: 'snprintf': identifier not found [C:\Users\USER\App\node_modules\sspi-client\build\sspi-client.vcxproj] ..\src_native\sspi_impl.cpp(214): error C3861: 'snprintf': identifier not found [C:\Users\USER\App\node_modules\sspi-client\build\sspi-client.vcxproj] ..\src_native\sspi_impl.cpp(267): error C3861: 'snprintf': identifier not found [C:\Users\USER\App\node_modules\sspi-client\build\sspi-client.vcxproj] ..\src_native\sspi_impl.cpp(287): error C3861: 'snprintf': identifier not found [C:\Users\USER\App\node_modules\sspi-client\build\sspi-client.vcxproj] ..\src_native\sspi_impl.cpp(328): error C3861: 'snprintf': identifier not found [C:\Users\USER\App\node_modules\sspi-client\build\sspi-client.vcxproj] ..\src_native\sspi_client.cpp(76): warning C4996: 'Nan::Callback::Call': was declared deprecated [C:\Users\USER\App\node_modules\sspi-client\build\sspi-client.vcxproj] C:\Users\USER\App\node_modules\nan\nan.h(1618) : see declaration of 'Nan::Callback::Call' ..\src_native\sspi_impl.cpp(351): error C3861: 'snprintf': identifier not found [C:\Users\USER\App\node_modules\sspi-client\build\sspi-client.vcxproj] ..\src_native\sspi_client.cpp(161): warning C4996: 'Nan::Callback::Call': was declared deprecated [C:\Users\USER\App\node_modules\sspi-client\build\sspi-client.vcxproj] C:\Users\USER\App\node_modules\nan\nan.h(1618) : see declaration of 'Nan::Callback::Call' ..\src_native\sspi_client.cpp(205): error C3861: 'AsyncQueueWorker': identifier not found [C:\Users\USER\App\node_modules\sspi-client\build\sspi-client.vcxproj] ..\src_native\sspi_client.cpp(300): error C3861: 'AsyncQueueWorker': identifier not found [C:\Users\USER\App\node_modules\sspi-client\build\sspi-client.vcxproj]

Figure out what cleanup work needs to be done and publish npm package.

src_native\sspi_client.cpp(271): warning C4996: 'v8::Function::NewInstance': was declared deprecated [E:\src\sspi-cl
ient\build\sspi-client.vcxproj]

Hey @tvrprasad!

I took a short, not very in depth, look over the code here. I'm not an expert on writing native Node.JS modules, so take everything I suggest here with a grain of salt. 😄

• Why is SSPI initialization performed asynchronously? Wouldn't it be simpler to make it a synchronous operation (both from implementation as well as usage perspective)?

const SSPIClient = require('sspi-client');

// SSPIClient([securityPackage])
// This allows a user to either specify one or multiple SecurityPackage to use (like 'kerberos', 'ntlm', etc).
// If at least one of the given packages is available, this returns a new client, otherwise it
// throws some sort of exception.
const client = new SSPIClient('kerbereos');

or you could even go as far as making the securityPackage option mandatory and expose the list of available securityPackages via SSPIClient.availableSecurityPackages. This would shift the handling of which security package to use onto the client code.

Also, it looks like currently it's not possible to have multiple SSPIClient instances that use different security packages? SSPIClient.getSspiPackageName seems to be shared across all instances?

• SspiImpl::GetNextBlob takes an inBlob argument that is not used. Is that still missing?

• Is there some specific reason why all these methods are async? If the calls to the secur32 API are reasonable fast, going through the thread pool might actually cause worse performance than just calling these functions in a blocking style. That's also the reason why the Node.js crypto APIs are all implemented in a blocking fashion - making them non-blocking has awful performance.

• Overall, the SSPI API feels very much like some sort of DuplexStream. I don't know whether this actually fits or not. A potential API for this would be:

const client = new SSPIClient('kerbereos');
client.on('data', (chunk) => {
  // Do something with the data in `chunk`
});
client.on('end', () => {
  // Authentication completed?
});
client.on('error', () => {
  // Fired if authentication failed?
});

// If there there is multiple roundtrips for authentication, we could use write to 
chunk.write(input);

The nice thing about a DuplexStream would be that we could simply use .pipe when performing the authentication:

const client = new SSPIClient('kerbereos');
client.on('end', () => {
  // Authentication was successful, we can stop the pipes
  realSocket.unpipe(client);
  client.unpipe(realSocket);
});

client.on('error', (error) => {
  // Close `realSocket`;
  realSocket.destroy();

  // Handle `error`
  ...
});
realSocket.pipe(client).pipe(realSocket);

Overall great work so far! ❤️ I bet many of the current tedious users will be looking forward to this getting integrated!

The code throws Error in many places, change that to TypeError where the error is about the Type.

Currently the tests are hard-coded to a particular machine name, user name etc. Make the tests configuration driven.

Is there any plans to include server-side methods? Node is really missing a native module that does this properly.

Thanks
Simon

Hello,

I figured I would post this issue here. I was running into this problem when working on an application for work which uses node, mssql and tedious driver which depends on sspi-client I believe. Apologize in advance for my lack of knowledge on the subject - I only dug around in the debugger and google as much as I can but I'm not too familiar with this stuff. I have a hypothesis about the problem, and I managed to hack-solve this issue for myself but I don't know what the wider solution would be.

os.hostname() on my computer returns my computer name e.g. michael-pc. My PC is domain-joined to my work network and the FQDN of my PC is michael-pc.ad.company.org. It seems to me that the problem is around fqdn.js in the getFqdnForHostname and getFqdnForIpAddress methods. dns.lookup on hostname michael-pc results in a variety of ip addresses which I find when running ipconfig. The first tried is the ipv4 address on my wifi adapter. However, running dns.reverse on that ip address does not return the fqdn michael-pc.ad.company.org. Instead, it returns some-other-host.other-domain.company.org.

Running ping -a 10.0.0.138 (my ipv4 address) resolves to the other domain when I'm connected via VPN and my computer's FQDN when I'm not.

However, some-other-host.other-domain.company.org is not the same domain as ad.company.org which is why AD authentication does not work (my hypothesis). That leads to Login failed. The login is from an untrusted domain and cannot be used with Windows authentication. [CLIENT: 127.0.0.1] when attempting to connect to SQL Server using sspi-client.

If I modify the function getFqdnForIpAddress in fqdn.js to return my computer's FQDN instead of the VPN FQDN, things work fine.

I've spent a while debugging this issue so I don't have more time to look into it right now, but I may come back at a later date to debug further if I can.

It's a it inconsistent to have the length but not the begin offset into the buffer. It may be simplest to just get rid of the serverResponseLength parameter and expect the entire buffer to just be serverResponse.

Alternately add a begin offset parameter.

Lots of connections and queries to multiple servers using different securityPackages.

The native code expects the JavaScript layer to do all the parameter validation. There may be some places in native where we should add some retail asserts. This issue will track that.

Validate:

• npm install works.
• node-gyp configure and node-gyp build work.
• Unit tests pass - there should be no Windows dependencies for Unit tests.
• Tedious tests pass.