SNOW-595230: Keypair authentication broken

[varun-dc]

Please answer these questions before submitting your issue. Thanks!

1. What version of NodeJS are you using (node --version and npm --version)? Node: 16.13.1 Npm: 8.1.2

2. What operating system and processor architecture are you using? Arch Linux, x86_64

3. What are the component versions in the environment (npm list)? snowflake-sdk 1.6.6

4. What did you do?

    ...
    const options: snowflake.ConnectionOptions = {
      account: parseAccountIdFromHost(connection.host),
      username: connection.credentials.user,
      authenticator: 'SNOWFLAKE_JWT',
      privateKeyPass: connection.credentials.private_key_passphrase,
      privateKeyPath: connection.credentials.private_key_path,
      database: queryConnection.database,
      role,
      schema,
      warehouse,
    };
    const client: snowflake.Connection = snowflake.createConnection(options);
    const connectFn =
      connection.credentials.password !== undefined
        ? client.connect.bind(client)
        : client.connectAsync.bind(client);

    const connectPromise = new Promise<snowflake.Connection>(
      (resolve, reject) => {
        // Try to connect to Snowflake, and check whether the connection was
        // successful.
        connectFn(
          async (
            err: snowflake.SnowflakeError | undefined
          ): Promise<void | Error> => {
            if (err) {
              ...
              reject(...);
            }
            resolve(client);
          }
        );
      }
    );
  ...

6. What did you expect to see? The connection should establish without error

7. What did you see instead? This was giving me,

Unhandled worker exception TypeError: Cannot read properties of undefined (reading 'then')                                                                                                                                                                                        
    at Connection.connectAsync (/home/work-dc/code/dc/ide-wt-2/node_modules/snowflake-sdk/lib/connection/connection.js:267:9)                                                                                                                                                     
    at /home/work-dc/code/dc/ide-wt-2/app/backend/dist/webpack:/deep-channel/app/backend/worker/connections/drivers/snowflake.ts:220:9                                                                                                                                            
    at new Promise (<anonymous>)                                                                                                                                                                                                                                                  
    at getClient (/home/work-dc/code/dc/ide-wt-2/app/backend/dist/webpack:/deep-channel/app/backend/worker/connections/drivers/snowflake.ts:215:28)                                                                                                                               
    at /home/work-dc/code/dc/ide-wt-2/app/backend/dist/webpack:/deep-channel/app/backend/worker/connections/drivers/snowflake.ts:547:26                                                                                                                                           
    at Module.<anonymous> (/home/work-dc/code/dc/ide-wt-2/app/backend/dist/webpack:/deep-channel/app/lib/log/util.ts:84:24)                                                                                                                                                       
    at Module.sync (/home/work-dc/code/dc/ide-wt-2/app/backend/dist/webpack:/deep-channel/app/backend/worker/connections/index.ts:49:36)                                                                                                                                          
    at CONNECTION_SYNC (/home/work-dc/code/dc/ide-wt-2/app/backend/dist/webpack:/deep-channel/app/backend/worker/ipc-backend.ts:176:12)                                                                                                                                           
    at DcIpc.onIncomingSendMessage (/home/work-dc/code/dc/ide-wt-2/app/backend/dist/webpack:/deep-channel/app/lib/ipc.ts:321:37)                                                                                                                                                  
    at DcIpc.onIncomingMessage (/home/work-dc/code/dc/ide-wt-2/app/backend/dist/webpack:/deep-channel/app/lib/ipc.ts:281:14)

9. Additional information I believe this is due to a function incorrectly returning void where as the call site expects a Promise<void>

lib/authentication/auth_keypair.js:156,

  this.authenticate = async function (authenticator, serviceName, account, username)
                      ^^^^^
                  missing async

The call site is, lib/connection/connection.js:263,

      await auth.authenticate(connectionConfig.getAuthenticator(),
        connectionConfig.getServiceName(),
        connectionConfig.account,
        connectionConfig.username)
        .then(() =>
        {

So auth.authenticate returning void would obviously cause a null pointer exception as we're seeing.

Simplest solution I could think of was to add a async to make the function return Promise<void> instead and it was then proceeding without error. I don't know if this is the right solution, be advise otherwise.

[safareli]

issue is not the await keyword: we have expression like:

await auth.authenticate(...).then(() =>...)

which is same as:

await (auth.authenticate(...).then(() =>...))

Thus if authenticate returns undefined it will have no that property on it:

this can be observed here:

function noop() { }

async function f() {
  await noop()
}

async function g() {
  await noop().then(_ => {})
}

f().then(() => console.log("done"));
g().then(null, () => console.log("failed"));

making authenticate async might be a breaking change, non breaking solution could be to just remove then call and expand it's callback after await. see https://github.com/snowflakedb/snowflake-connector-nodejs/pull/311

[varun-dc]

The auth.authenticate(...) method appears to actually be an interface with the keypair implementation being one implementation of many. I don't believe it would be exposed publicly, so no risk of a breaking change in that sense.

Additionally, all the other implementations already appear to be Promise<void> returning, so I thought making this also async so that it also returns a Promise<void> would be the least risky and most minimal change we could do; it also makes this function adhere to the implicit interface this caller is expecting (a function/method that is returning Promise<void>)

[sfc-gh-dszmolka]

issue should be now fixed with https://github.com/snowflakedb/snowflake-connector-nodejs/pull/522 and is readily available for use with npm i https://github.com/snowflakedb/snowflake-connector-nodejs.git & co. and I'll update this issue again when it's out with the next official release

[sfc-gh-dszmolka]

fix is out with release 1.6.23