feat: Add dynamic retrieval for client password #1926

sehrope · 2019-07-17T17:17:04Z

PR to add support for dynamic password retrieval. There was already something quite similar to this to support reading passwords from a PGPASS file. This PR expands that section a bit to support invoking an arbitrary function.

Rather than supporting a callback parameter, this PR allows for either a synchronous function that returns a string or one that returns a Promise that resolves to a string. As there's Promise support on every supported runtime I figure it's a better choice for anything new being added.

Here's some examples of how to use it:

// Existing hard coded password:
const pool = new pg.Pool({
    password: 'test',
});

// Sync return a static string
const pool = new pg.Pool({
    password: () => {
        return 'test';
    }
});

// Async API
const pool = new pg.Pool({
    password: async () => {
        await new Promise(resolve => setTimeout(resolve, 1000));
        const value = await someAsyncFunc();
        return value;
    }
});

This PR does not have any new tests yet, I just tested it with some local examples. If the concept itself seems workable then should be easy to add a few that use a function and check for its side effects. Let me know.

Adds option to specify a function for a client password. When the client is connected, if the value of password is a function then it is invoked to get the password to use for that connection. The function must return either a string or a Promise that resolves to a string. If the function throws or rejects with an error then it will be bubbled up to the client.

Add testAsync() helper function to Suite to simplify running tests that return a Promise. The test action is executed and if a syncronous error is thrown then it is immediately considered failed. If the Promise resolves successfully then the test is considered successful. If the Promise rejects with an Error then the test is considered failed.

sehrope · 2019-07-18T19:27:30Z

I force pushed an update to this that changes:

Explicitly sets the password field to null if the password function does not return a usable value
Changes the error for a non-string return value to be a TypeError

Also added some tests for retrieving a sync, async, and errant passwords via functions. For the tests I added a new helper suite.testAsync(...) to simplify writing them using promises.

brianc

This is fantastic. 20 line code change, 150 lines of tests. 😍

brianc · 2019-07-19T03:29:18Z

lib/client.js

-      if (self.password !== null) {
+      if (typeof(self.password) === 'function') {
+        try {
+          self._Promise.resolve(self.password())


ohhh nice use on supporting BYOP - not sure how popular BYOP is now that promises have shipped natively in node for years, but still nice for backwards compatibility 👌

brianc · 2019-07-19T03:30:12Z

lib/client.js

+            })
+        } catch (err) {
+          con.emit('error', err)
+        }


wow - impressive error handling

Apparently not as impressive as @charmander ;-)

brianc · 2019-07-19T03:34:00Z

test/suite.js

@@ -72,6 +72,19 @@ class Suite {
    const test = new Test(name, cb)
    this._queue.push(test)
  }
+
+  testAsync (name, action) {


i apologize the tests here aren't written w/ a modern node testing framework. There's this cost-benefit thing I keep running into where it's like "Should I re-write all these tests into jest/mocha/etc? They already work and I know they work...but maybe rewriting will introduce some breakage." The original tests in the repo were written before mocha or jest existed so it's kinda like...well....if it ain't broke...otoh I feel like it's probably more confusing to contribute to. I also at the time, coming from ruby, thought i'd be 'clever' and attach a bunch of stuff to the global namespace like assert and other things. That was a mistake. :(

No worries. I've thought about cleaning up some of them more than once and it's always the same conclusion. Maybe having a separate harness for things going forward as it's definitely easier to write async tests with an async harness. Doesn't even have to be be full on mocha/jest/whatever ... just having something to call out to with async actions should be enough.

I like how you can easily run any one of the tests in this project via "node path/to/test.js".

charmander

Some formatting consistency options.

lib/client.js

test/integration/connection/dynamic-password.js

charmander · 2019-07-19T10:53:50Z

test/integration/connection/dynamic-password.js

+const helper = require('./../test-helper')
+const suite = new helper.Suite()
+const pg = require('../../../lib/index');
+const Client = pg.Client;


Suggested change

const Client = pg.Client;

const Client = pg.Client

(This one still exists)

yeah i need to turn on linting on the test folder

Would also help if we turned back on the semi rule. This PR passed "make lint" as the semi rule got removed when the eslint config was last cleaned up (by me...).

test/integration/connection/dynamic-password.js

test/suite.js

sehrope · 2019-07-19T12:49:40Z

I pushed some updates to address the comments (thanks @charmander!). As part of this I also cleaned up the error handling in client.js to use the same style of Promise.resove() chaining as it removes the need for separate sync error handling.

I left the error handling for that piece on L131 of client.js as a separate .catch(...) to handle anything being thrown by the supplied cb rather than a single .then(onSuccess, onError). Anything that errors out would then bubble up as an error event on the client.

FYI, taking a peek at the callers of that checkPgPass(...) function, I think I may have found a potential future issue with the SCRAM auth, I'm going to open a separate issue to track it.

lib/client.js

charmander · 2019-07-19T12:53:43Z

test/integration/connection/dynamic-password.js

+const helper = require('./../test-helper')
+const suite = new helper.Suite()
+const pg = require('../../../lib/index');
+const Client = pg.Client;


(This one still exists)

sehrope · 2019-07-19T13:26:11Z

No clue why it failed on node v12 in the PR build but passed for me (https://travis-ci.org/sehrope/node-postgres/builds/561008527). There's no specific error in the travis logs either. I think there was some transient error building Postgres.

I'm going to push a dummy commit to see if it happens again.

charmander

Nice :)

alekstr · 2019-07-23T11:48:07Z

Btw, checked and tested this change in Prod with RDS Postgres 11.4, works fine! When you plan merge this into master?

brianc · 2019-07-25T17:48:28Z

Right now! & I'll document it and release a new minor version. 😄

brianc · 2019-07-25T18:07:43Z

Published pg@7.12.0 - thanks!

benkeil · 2020-10-30T18:37:21Z

You missed the types...

sehrope · 2020-10-30T18:57:06Z

@benkeil Yes the typings are maintained externally in DefinitelyTyped (i.e. @types/pg) so it can't be updated in tandem. I submitted a PR for that so hopefully that'll get synced soon: DefinitelyTyped/DefinitelyTyped#49257

benkeil · 2020-10-30T19:41:30Z

Naive question: why not migrate the project to typescript?

sehrope · 2020-10-31T11:49:05Z

I think it will eventually and some of the internal components have already been migrated to Typescript (e.g. see the packet parser).

brianc · 2020-11-02T16:42:55Z

Naive question: why not migrate the project to typescript?

yeah i'm very slowly working on that

vitaly-t · 2021-04-08T12:16:14Z

This PR and the change are confusing. I thought at first this would actually allow dynamic passwords support, but all it does - makes only one call in the beginning. After that, if the password changes, the callback is never called again. So it is not very useful then.

I had to use a connection object, and then do pool.options.password = 'new-password', or if connection string used, then set new one for pool.options.connectionString, in order to start using a new password.

Feature introduced in brianc/node-postgres#1926.

sehrope mentioned this pull request Jul 17, 2019

Support retrieving db password via callback #1873

Closed

sehrope force-pushed the feat-add-dynamic-password branch from 354b19f to c3aa08d Compare July 17, 2019 17:34

sehrope added 3 commits July 18, 2019 15:20

test: Add tests for dynamic password

2d7f9fd

sehrope force-pushed the feat-add-dynamic-password branch from c3aa08d to 2d7f9fd Compare July 18, 2019 19:20

brianc approved these changes Jul 19, 2019

View reviewed changes

charmander reviewed Jul 19, 2019

View reviewed changes

sehrope added 4 commits July 19, 2019 08:18

test: Simplify testAsync error handling

bafdad5

fix: Clean up dynamic password error handling and misc style

afc7388

test: Remove extra semicolons

14e4fb5

test: Change testAsync(...) calls to use arrow functions

fe5a246

charmander reviewed Jul 19, 2019

View reviewed changes

fix: Wrap self.password() invocation in an arrow function

3b1c2e4

test: Add a comment to testAsync(...)

e1d71f8

charmander approved these changes Jul 19, 2019

View reviewed changes

brianc merged commit 0894a3c into brianc:master Jul 25, 2019

vitaly-t mentioned this pull request Aug 15, 2019

Add dynamic password support vitaly-t/pg-promise#643

Closed

This was referenced Mar 12, 2020

Postgres: Allow providing a function for password typeorm/typeorm#5673

Merged

[QUESTION]: Connecting to Amazon RDS with IAM credentials using TypeORM typeorm/typeorm#4724

Closed

sehrope mentioned this pull request Aug 4, 2020

Dynamic authentication info #2301

Closed

biern mentioned this pull request Oct 9, 2020

Pass a custom pg connection object gajus/slonik#115

Closed

sehrope mentioned this pull request Oct 30, 2020

Support a password function for the client configuration #2396

Closed

sehrope mentioned this pull request Oct 30, 2020

fix: Add pg dynamic password support to ClientOptions DefinitelyTyped/DefinitelyTyped#49257

Merged

10 tasks

vitaly-t mentioned this pull request Apr 8, 2021

Dynamic password change #2513

Closed

charmander pushed a commit to brianc/node-postgres-docs that referenced this pull request Apr 9, 2021

Add example use for password function (#126)

eabf9c3

Feature introduced in brianc/node-postgres#1926.

thijs mentioned this pull request Jan 6, 2022

AWS RDS password rotation with changing username #2679

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add dynamic retrieval for client password #1926

feat: Add dynamic retrieval for client password #1926

sehrope commented Jul 17, 2019

sehrope commented Jul 18, 2019

brianc left a comment

brianc Jul 19, 2019

brianc Jul 19, 2019

sehrope Jul 19, 2019

brianc Jul 19, 2019

sehrope Jul 19, 2019

charmander left a comment

charmander Jul 19, 2019

charmander Jul 19, 2019

brianc Jul 19, 2019

sehrope Jul 19, 2019

sehrope commented Jul 19, 2019

charmander Jul 19, 2019

sehrope commented Jul 19, 2019

charmander left a comment

alekstr commented Jul 23, 2019 •

edited

Loading

brianc commented Jul 25, 2019

brianc commented Jul 25, 2019

benkeil commented Oct 30, 2020

sehrope commented Oct 30, 2020

benkeil commented Oct 30, 2020

sehrope commented Oct 31, 2020

brianc commented Nov 2, 2020

vitaly-t commented Apr 8, 2021

feat: Add dynamic retrieval for client password #1926

feat: Add dynamic retrieval for client password #1926

Conversation

sehrope commented Jul 17, 2019

sehrope commented Jul 18, 2019

brianc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charmander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sehrope commented Jul 19, 2019

Choose a reason for hiding this comment

sehrope commented Jul 19, 2019

charmander left a comment

Choose a reason for hiding this comment

alekstr commented Jul 23, 2019 • edited Loading

brianc commented Jul 25, 2019

brianc commented Jul 25, 2019

benkeil commented Oct 30, 2020

sehrope commented Oct 30, 2020

benkeil commented Oct 30, 2020

sehrope commented Oct 31, 2020

brianc commented Nov 2, 2020

vitaly-t commented Apr 8, 2021

alekstr commented Jul 23, 2019 •

edited

Loading