Add plugin manager to allow dynamically supplying passwords #2369

sehrope · 2021-12-20T20:59:06Z

This is a revised version of #2321. I'll close that one out in favor of this.

The first two commits do some refactoring to consolidate where username and passwords are pulled from the properties map and removing those args from the various internal creation methods. This simplifies the changeset for actual feature which adds a plugin hook for dynamically providing the password. The plugin callback also provides the authentication type so the caller has a bit more information to make a decision (e.g. the caller can ensure they are using SCRAM and not sending the password over plaintext).

Because of the message flow of the driver, if the server is configured with trust authentication then no callback will be invoked.

@davecramer While reviewing, in particular take a peek at the public interfaces to make sure it makes sense as we'll be stuck with them.

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

Does your submission pass tests?
Does ./gradlew autostyleCheck checkstyleAll pass ?

…ties Changes internal constructors for PgConnection and related classes to only accept the connection properties object and remove the user and password arguments. Any locations that required those fields can retrieve them from the properties map.

davecramer · 2021-12-21T12:08:46Z

pgjdbc/src/main/java/org/postgresql/core/AuthenticationPluginManager.java

+
+    AuthenticationPlugin authPlugin;
+    try {
+      authPlugin = (AuthenticationPlugin) ObjectFactory.instantiate(authPluginClassName, info,


Does it make sense to cache the instance ? Is it possible that different connections would use different authentication plugins ?

No it should not be cached as connections should be isolated from each other. If a user needs to maintain state across multiple runtime instantiations then it would need to be done in the user's implementation through either static fields or some kind of proxying.

Thinking about that a bit more, perhaps we need a new opaque "arg" property that users can use to customize their implementation. The plugin constructor takes the connection properties but there's no specific property that user's could use for their own purposes. Something like authPluginArg (e.g. the plugin equivalent of socketFactoryArg).

Fairly good chance that the user will only use one auth plugin. I wonder what the cost of instantiating a class really is (in other words is it worth worrying about)

Yes I doubt it's an issue in practice and connections tend to be long lived anyway. The time spent opening the underlying socket and TLS handshake is going to be significantly more than any object creation.

Fair enough.

davecramer · 2021-12-21T13:16:53Z

I wonder why this is failing on windows ?

sehrope · 2021-12-21T13:29:46Z

Looks like two separate issues failing the AppVeyor build but neither of them is actually Windows related.

The v9.6 environment is failing because the testing isn't correctly being skipped on older platforms. The v10 environment is random failures from ~~gremlins~~ race conditions that query timeout test.

I'm going to push a fix for the test skipping.

…t runtime Adds authenticationPluginClassName connection property that allows end users to specify a class that will provide the connection passwords at runtime. This allows configuring a connection with a password that must be generated on the fly or periodically changes. Custom implementations have access to the full connection properties and the type of authentication that is being requested by the server, e.g. MD5 or SCRAM.

davecramer · 2021-12-28T13:33:17Z

@sehrope I don't see any reason not to merge this.

sehrope · 2021-12-28T14:28:38Z

Sounds good. I'll merge this in a bit.

vlsi · 2021-12-29T06:10:58Z

pgjdbc/src/main/java/org/postgresql/plugin/AuthenticationPlugin.java

+  @Nullable
+  String getPassword(AuthenticationRequestType type) throws PSQLException;


Security best practices suggest that passwords should be used as char[] so they can be nullified after use: https://www.baeldung.com/java-storing-passwords

vlsi · 2021-12-29T06:15:42Z

README.md

@@ -142,6 +142,7 @@ In addition to the standard connection parameters the driver supports a number o
 | adaptiveFetchMaximum          | Integer | -1      | Specifies maximum number of rows, which can be calculated by adaptiveFetch. Number of rows used by adaptiveFetch cannot go above this value. Any negative number set as adaptiveFetchMaximum is used by adaptiveFetch as infinity number of rows.
 | localSocketAddress            | String  | null    | Hostname or IP address given to explicitly configure the interface that the driver will bind the client side of the TCP/IP connection to when connecting.
 | quoteReturningIdentifiers     | Boolean | true    | By default we double quote returning identifiers. Some ORM's already quote them. Switch allows them to turn this off
+| authenticationPluginClassName | String  | null    | Fully qualified class name of the class implementing the AuthenticationPlugin interface. If this is null, the password value in the connection properties will be used.


Frankly speaking, I believe the naming is misleading. What the thing does it supplies passwords, so it is not clear why do we call it authenticationPlugin rather than passwordCallback or something like that.

There's a similar interface in Java: javax.security.auth.callback.PasswordCallback

davecramer · 2021-12-29T13:34:15Z

@sehrope I think these suggestions make sense. Thoughts ?

vlsi · 2021-12-29T13:36:49Z

pgjdbc/src/test/java/org/postgresql/test/plugin/PluginTestSuite.java

+@RunWith(Suite.class)
+@SuiteClasses({
+    AuthenticationPluginTest.class,
+})
+public class PluginTestSuite {


Do we need suites? I think we no longer use them.

sehrope · 2021-12-30T12:44:41Z

Security best practices suggest that passwords should be used as char[] so they can be nullified after use: https://www.baeldung.com/java-storing-passwords

I considered having the interface be char[] when I put this together but stuck with String as the rest of the driver deals with String passwords directly (e.g. receiving them via java.util.Properties, passing them as a String into the SASL functions). It'd have been a much larger change to cascade it through all the downstream call sites. I'm not against it, just concerned that the overall changes required to meaningfully support that are larger and more likely to break something.

Do we need suites? I think we no longer use them.

In that case we should get rid of all of the suites. It was added to be consistent with the existing code.

Frankly speaking, I believe the naming is misleading. What the thing does it supplies passwords, so it is not clear why do we call it authenticationPlugin rather than passwordCallback or something like that.

There's a similar interface in Java: javax.security.auth.callback.PasswordCallback

Well to be fair, I did specifically request feedback on the public interface naming...

I picked "Auth..." over simply "Password..." because it's meant to handle the authentication stage of the protocol. In most cases that involves a password but it might be extended to handle a non-password step in the future. The interface also includes the additional parameter for the authentication type (e.g. plain, MD5, or SASL) to allow the client further customize it's handling (e.g. reject sending things over in plaintext).

I'm fine with changing the name to something more appropriate but it's more than just a password callback.

vlsi · 2021-12-30T12:49:39Z

I considered having the interface be char[] when I put this together but stuck with String as the rest of the driver deals with String passwords directly

I think we should fix that as much as we can.
I believe we just missed it.

I don't know what to do with Properties, however, we could probably refactor internals to char[] for better security.

Well to be fair, I did specifically request feedback on the public interface naming

Could you please clarify the use-case?
What would the end-to-end use of the feature?

sehrope · 2021-12-30T12:59:17Z

The original user issue that lead to this and #2321 (replaced by this PR) is this: #2102

It's for connecting to systems like RDS that allow for passwords that are really short lived digital signatures. They can't be static values as they expire in a matter of minutes so a connection pool that would be periodically creating new connections would need to generate them on the fly. Rather than supporting N different mechanisms directly, the idea is to have the user supply a class that provides the password on the fly at connect time. That way the digital signature or whatever else is involved to generate the password can happen at the last possible moment.

sehrope added 2 commits December 20, 2021 13:12

refactor: Use multi-catch for exceptions in ConnectionFactoryImpl

3ddb55a

sehrope mentioned this pull request Dec 20, 2021

Introduce a plugin manager interface to allow different mechanisms to acquire a password. #2321

Closed

davecramer reviewed Dec 21, 2021

View reviewed changes

sehrope added 2 commits December 21, 2021 08:54

test: Add TestUtil.assumeHaveMinimumServerVersion(...) helper

5e69015

sehrope force-pushed the pr-2321-add-plugin-manager-v4 branch from e8ec934 to b95ef95 Compare December 21, 2021 13:55

sehrope merged commit 473091a into pgjdbc:master Dec 28, 2021

vlsi reviewed Dec 29, 2021

View reviewed changes

stephanprantl mentioned this pull request Feb 2, 2022

Bump postgresql from 42.3.1 to 42.3.2 in /backend wrk-tafel/admin#28

Merged

theaspect mentioned this pull request Feb 25, 2022

Bump postgresql from 42.3.1 to 42.3.3 theaspect/kotlin-public-api#10

Merged

backwind1233 mentioned this pull request Aug 9, 2022

[FEATURE REQ] Make sure credential-free JDBC plugin is compatible with multiple MySQL/PostgreSQL versions Azure/azure-sdk-for-java#29775

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plugin manager to allow dynamically supplying passwords #2369

Add plugin manager to allow dynamically supplying passwords #2369

sehrope commented Dec 20, 2021

davecramer Dec 21, 2021

sehrope Dec 21, 2021

sehrope Dec 21, 2021

davecramer Dec 21, 2021

sehrope Dec 21, 2021

davecramer Dec 21, 2021

davecramer commented Dec 21, 2021

sehrope commented Dec 21, 2021

davecramer commented Dec 28, 2021

sehrope commented Dec 28, 2021

vlsi Dec 29, 2021

vlsi Dec 29, 2021

davecramer commented Dec 29, 2021

vlsi Dec 29, 2021

sehrope commented Dec 30, 2021

vlsi commented Dec 30, 2021

sehrope commented Dec 30, 2021

		@Nullable
		String getPassword(AuthenticationRequestType type) throws PSQLException;

Add plugin manager to allow dynamically supplying passwords #2369

Add plugin manager to allow dynamically supplying passwords #2369

Conversation

sehrope commented Dec 20, 2021

All Submissions:

New Feature Submissions:

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

davecramer commented Dec 21, 2021

sehrope commented Dec 21, 2021

davecramer commented Dec 28, 2021

sehrope commented Dec 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecramer commented Dec 29, 2021

Choose a reason for hiding this comment

sehrope commented Dec 30, 2021

vlsi commented Dec 30, 2021

sehrope commented Dec 30, 2021