Some notes

[arthurschreiber]

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!

[tvrprasad]

Why is SSPI initialization performed asynchronously?

I'm making calls into SSPI layer whose implementation I have no insight into. SSPI is a provider model, so different client machines may have entirely different providers for implementation of different security protocols. I didn't want to make any assumptions as to whether there is I/O involved in the operations.

I'm not particularly concerned about performance penalty from going through thread pool as these are not high frequency calls. Just low single digit number of calls per each SQL connection.

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?

Currently neither ADO.net nor ODBC driver support client application specifying which security package to use with Windows Integrated Auth. Since those are by far the most used client drivers, we can reasonably assume there isn't a need to allow the client application to specify which package to use. Let me know if you think it should be different for Tedious for some reason.

That said, there is no reason to not support specifying security package name in SspiClient() constructor, presumably this could be used by other applications than Tedious that might want to specify the specific security package to use. I opened an issue - #3 - to track that for later. First I want to see Tedious working with Windows Integrated Auth :-)

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?

This is again following the model from other drivers as explained above. The issue #3 will address that also.

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

Ah. I'm sure I'd have caught that in integration testing :-) I'll take care of that. Issue #4.

Overall, the SSPI API feels very much like some sort of DuplexStream.

SSPI is not a streaming API. You get the full blob when the function to get the blob returns.

Thanks a lot for the feedback. Very much appreciate it!

[tvrprasad]

Address the issue of being able to specify different securityPackages packages for different connections - #3.