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

davecramer · 2021-10-25T11:39:00Z

No description provided.

… acquire a password.

sehrope · 2021-10-25T12:09:32Z

+1 to the overall idea.

We should do this in a way that does not require even a dummy password to be supplied. Skimming through this patch, it looks like it'd fail prior to invoking the handler if a value is not provided:

                if (password == null) {
                  throw new PSQLException(
                      GT.tr(
                          "The server requested password-based authentication, but no password was provided."),
                      PSQLState.CONNECTION_REJECTED);
                }

                byte[] encodedPassword = authenticationPlugin.getEncodedPassword(user, password);

Do we also want to be able to dynamically generate the username or other properties?

How about providing additional connection properties to the supplier method so that it has more context for the request?

One requested use case for this feature is RDS's IAM authentication. If the same username is being used in two different databases (e.g. "app_user"), there'd be no way for a generic plugin class to know which access key to use to generate the auth token. We should either pass the entire properties object or an additional "arg" parameter the way the SSL factories work.

davecramer · 2021-10-25T12:42:37Z

+1 to the overall idea.

We should do this in a way that does not require even a dummy password to be supplied. Skimming through this patch, it looks like it'd fail prior to invoking the handler if a value is not provided:
                if (password == null) {
                  throw new PSQLException(
                      GT.tr(
                          "The server requested password-based authentication, but no password was provided."),
                      PSQLState.CONNECTION_REJECTED);
                }

                byte[] encodedPassword = authenticationPlugin.getEncodedPassword(user, password);
Do we also want to be able to dynamically generate the username or other properties?

How about providing additional connection properties to the supplier method so that it has more context for the request?

One requested use case for this feature is RDS's IAM authentication. If the same username is being used in two different databases (e.g. "app_user"), there'd be no way for a generic plugin class to know which access key to use to generate the auth token. We should either pass the entire properties object or an additional "arg" parameter the way the SSL factories work.

Good catch. Thanks! Yes, we need to do this in such a way that they do not need to provide a dummy password.

bokken · 2021-10-25T13:29:05Z

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

+package org.postgresql.core;
+
+public interface AuthenticationPlugin {
+  byte[] getEncodedPassword(String userName, String password);


probably need some javadoc here.
for example, this asks for encoded password, but what encoding?

fair enough. This is really a work in progress.

davecramer · 2021-10-25T19:55:57Z

@sehrope I've reworked this, please have a look.

srdan-bozovic-msft · 2021-10-25T23:18:59Z

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

+
+  byte[] getEncodedPassword(Properties info) throws PSQLException, PSQLException;
+
+}


I would prefer somewhat more generic interface that would include different auth-methods that PG supports today and some future looking like oauth2 access token that are currently commonly implemented through overloaded password field (though they are nothing like a passwords), and in future would hopefully get it's own auth-method (but keep the existing plugin ;) )

public interface AuthenticationPlugin { AuthenticationInfo getAuthenticationInfo(Properties info) throws PSQLException, PSQLException; } public class AuthenticationInfo { final String authenticationType; final byte[] credential; AuthenticationInfo (String authenticationType, byte[] credential) { this.authenticationType= authenticationType; this.credential= credential; } }

Why would you need to know the auth method? I would think at the end of the day all we need is the password?
How would I use the AuthenticationInfo ?

If you scope AuthenticationPlugin only to Password auth-method then you would not need that info (and you may call plugin PasswordAuthenticationPlugin).

My proposal is to make plugin applicable to the other auth-methods as well. Depending on authenticationType the driver would put credential (password) in use differently.

That said, below may be a better interface.

public interface AuthenticationPlugin { String getAuthenticationType(Properties info); byte[] getCredential(Properties info) throws PSQLException, PSQLException; }

What would getAuthenticationType(...) return? Is that supposed to be the server auth method? If so, that would not work as the server decides how it wants the client to authenticate.

The intended use is that if you want a different authentication mechanism you load it via Class.forName() and acquire your password any way you wish. Ultimately the server only needs the password bytes. As @sehrope mentioned there is no real way to determine many of the auth methods ahead of time.

In case server determines the method should we have something like this?

public interface AuthenticationPlugin { boolean isSupported(String authenticationType, Properties info); byte[] getCredential(String authenticationType, Properties info) throws PSQLException, PSQLException; }

There is currently one place where the server determines that we would use GSS encryption, but not authentication. I don't see anywhere else in the protocol where this happens.

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

bokken · 2021-10-26T12:35:52Z

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

+      return new PasswordAuthentication();
+    } else {
+      try {
+        return (AuthenticationPlugin) Class.forName(authenticationClassName).getDeclaredConstructor().newInstance();


does there need to be consideration about what classloader to use?

It should match however we handle other class parameters like socket factories.

We should also either add an "authManagerPluginArg" parameter to allow the caller to customize the instantiated class, e.g. new FooAuthPlugin(String arg).

Otherwise we'd be relying on them passing extra properties into the Properties object that we would ignore in the Driver itself. But if there's no standard for naming those, there's no guarantee we don't eventually add them as "real" properties. Maybe a prefixed name like "ext." to signify end user properties that we'll allow to exist in the Properties map but otherwise ignore.

Adding args to the plugin is interesting. We would have to pass the arg into getAuthenticationPlugin
So one option is to provide a second getAuthenticationPlugin(Properties props, Object arg) ?

No the Properties alone is fine as the user can retrieve the value if they want it. They just need a factory name, like sslfactoryarg but specifically for this.

We could add a separate constructor that takes a String (again similar to the SSL factories) but it's not strictly necessary.

Should we just follow the same pattern as the SSL factories (and use the ObjectFactory logic)?

Yes that's a good idea. All this class instantiation should happen in one place so the ClassLoader et al are handled consistently.

pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java

bokken · 2021-10-26T12:45:07Z

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

+
+public interface AuthenticationPlugin {
+
+  byte[] getEncodedPassword(Properties info) throws PSQLException, PSQLException;


Is there risk in providing (i.e. leaking) reference to the mutable Properties instance?

Maybe Function<PGProperty, String>

It'd be nice if it's read-only but I don't think it's an issue. We already do it like this for anything created by ObjectFactory:

pgjdbc/pgjdbc/src/main/java/org/postgresql/util/ObjectFactory.java

Lines 39 to 62 in f61fbfe

public static Object instantiate(String classname, Properties info, boolean tryString,

@Nullable String stringarg)

throws ClassNotFoundException, SecurityException, NoSuchMethodException,

IllegalArgumentException, InstantiationException, IllegalAccessException,

InvocationTargetException {

@Nullable Object[] args = {info};

Constructor<?> ctor = null;

Class<?> cls = Class.forName(classname);

try {

ctor = cls.getConstructor(Properties.class);

} catch (NoSuchMethodException ignored) {

}

if (tryString && ctor == null) {

try {

ctor = cls.getConstructor(String.class);

args = new String[]{stringarg};

} catch (NoSuchMethodException ignored) {

}

}

if (ctor == null) {

ctor = cls.getConstructor();

args = new Object[0];

}

return ctor.newInstance(args);

Plus we're blindly running whatever user code they supply to us via the class so in theory they could do whatever they want anyway.

I was not tracking on that. Do we have it documented anywhere that mutating the Properties instance could lead to indeterminate behavior (presumably not just for this connection but other connections, including future)?

Yes, I'm not sure it's listed anywhere but it should be.

Modifying one should not impact any other connections though. Each one has their own copy even if it starts from the same common Properties conf:

pgjdbc/pgjdbc/src/main/java/org/postgresql/Driver.java

Line 231 in d78059a

Properties props = new Properties(defaults);

IMO the method name getEncodedPassword() should be encodedPassword() in that this really isn't a "getter" per say.

srdan-bozovic-msft · 2021-10-27T07:10:19Z

Should we add password expiration time as well?

IAM and AAD tokens have limited validity period. Expiration time would be relevant for connection poolers ...

davecramer · 2021-10-27T09:07:33Z

As this would undoubtedly come from some property, I would think it would be enough to pass the properties into the plugin as proposed above.

…in constructor

davecramer · 2021-11-05T11:26:23Z

@sehrope @bokken @jorsol any reason not to merge this ?

sehrope · 2021-11-05T12:27:18Z

@davecramer I'd like to review this a bit more and am planning on getting to it either today or tomorrow.

jorsol · 2021-11-05T12:36:02Z

I haven't checked this and the use case, but having a Test setting an additional implementation of AuthenticationPlugin would be nice.

sehrope · 2021-11-07T12:57:53Z

I started going through this PR. Definitely not ready to merge yet as it needs tests and I'm pretty sure there's at least one issue with supplying passwords to GSS auth.

I'll follow up in a bit when I push some changes to a branch I'm testing out.

davecramer · 2021-11-07T13:38:53Z

@sehrope I left that out on purpose as anyone that is implementing their own authentication mechanism is not going to use GSS authentication. I guess I could be mistaken there...

sehrope · 2021-11-07T15:44:10Z

Yes I think it'd make sense for it to be consistent regardless of auth mechanism. That way it could be used for something like end user .pgpass handling regardless of auth mechanism.

I pushed a branch here that started off as this PR rebased atop master: master...sehrope:pr-2321-add-plugin-manager-v3

First commit splits out some unrelated exception handling clean up. Second commit pulls out the user / password args from the internal methods and has any usage of them pull the values directly from Properties. This makes the "feature" commit much smaller and easier to review.

The meat of the PR is the third commit which adds the new plugin feature. Couple changes / additions:

Changed the auth plugin interface to return a String as some of the internal code paths require a String and not an encoded password.
Added an enum arg to the auth plugin getPassword(...) for the type of authentication type: PLAIN, MD5, SCRAM or GSS. This can be used by the plugin to decide the password or even throw an exception if it doesn't like the mechanism, e.g. refuse to authentication if PLAIN or MD5 is requested. Might even be useful in the SCRAM tests to ensure we're actually using SCRAM.
Removed the "default plugin" and changed the auth plugin manager to not instantiate an object for the common case where no auth plugin is specified. Instead it directly returns back the value from the connection properties.
Changed the code paths in ConnectionFactoryImpl that need a password to request it at the last possible moment that it is needed.

It passes the existing tests (https://github.com/sehrope/pgjdbc/runs/4131435657?check_suite_focus=true) but still needs some new ones for the new functionality. If this approach and API changes make sense, I can add those this week.

davecramer · 2021-11-10T19:07:09Z

seems like a good idea, lets continue down this path. Thanks!

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

Closing this in favor of #2369

Introduce a plugin manager interface to allow different mechanisms to…

d20dc08

… acquire a password.

fix checkstyle

3075b04

bokken reviewed Oct 25, 2021

View reviewed changes

davecramer added 4 commits October 25, 2021 10:58

remove need for passing user and password all over the place

dd17da2

appease checker framework

22130dd

appease checker framework

51dcb8c

checkstyle

b75a9c2

davecramer requested a review from sehrope October 25, 2021 19:56

srdan-bozovic-msft reviewed Oct 25, 2021

View reviewed changes

bokken reviewed Oct 26, 2021

View reviewed changes

pgjdbc/src/main/java/org/postgresql/core/AuthenticationPluginManager.java Outdated Show resolved Hide resolved

bokken reviewed Oct 26, 2021

View reviewed changes

pgjdbc/src/main/java/org/postgresql/core/AuthenticationPluginManager.java Show resolved Hide resolved

bokken reviewed Oct 26, 2021

View reviewed changes

pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java Show resolved Hide resolved

bokken reviewed Oct 26, 2021

View reviewed changes

fix formatting for error message

f80666c

davecramer added 2 commits October 27, 2021 08:50

Use ObjectFactory to instantiate Plugin. Also pass properties to plug…

d39c8f1

…in constructor

checkstyle

2687f2b

sehrope mentioned this pull request Dec 20, 2021

Add plugin manager to allow dynamically supplying passwords #2369

Merged

4 tasks

sehrope closed this Dec 20, 2021

vlsi mentioned this pull request May 11, 2022

proposal: Plugin Manager #2509

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

davecramer commented Oct 25, 2021

sehrope commented Oct 25, 2021

davecramer commented Oct 25, 2021

bokken Oct 25, 2021

davecramer Oct 25, 2021

davecramer commented Oct 25, 2021

srdan-bozovic-msft Oct 25, 2021 •

edited

davecramer Oct 25, 2021

srdan-bozovic-msft Oct 26, 2021 •

edited

sehrope Oct 26, 2021

davecramer Oct 26, 2021

srdan-bozovic-msft Oct 26, 2021 •

edited

davecramer Oct 26, 2021

bokken Oct 26, 2021

sehrope Oct 26, 2021

davecramer Oct 26, 2021

sehrope Oct 26, 2021

bokken Oct 26, 2021

sehrope Oct 26, 2021

bokken Oct 26, 2021

bokken Oct 26, 2021

sehrope Oct 26, 2021

bokken Oct 26, 2021

sehrope Oct 26, 2021

rbygrave Oct 26, 2021

srdan-bozovic-msft commented Oct 27, 2021

davecramer commented Oct 27, 2021

davecramer commented Nov 5, 2021

sehrope commented Nov 5, 2021

jorsol commented Nov 5, 2021

sehrope commented Nov 7, 2021

davecramer commented Nov 7, 2021

sehrope commented Nov 7, 2021

davecramer commented Nov 10, 2021

sehrope commented Dec 20, 2021


		byte[] getEncodedPassword(Properties info) throws PSQLException, PSQLException;

		}


		public interface AuthenticationPlugin {

		byte[] getEncodedPassword(Properties info) throws PSQLException, PSQLException;

	public static Object instantiate(String classname, Properties info, boolean tryString,
	@Nullable String stringarg)
	throws ClassNotFoundException, SecurityException, NoSuchMethodException,
	IllegalArgumentException, InstantiationException, IllegalAccessException,
	InvocationTargetException {
	@Nullable Object[] args = {info};
	Constructor<?> ctor = null;
	Class<?> cls = Class.forName(classname);
	try {
	ctor = cls.getConstructor(Properties.class);
	} catch (NoSuchMethodException ignored) {
	}
	if (tryString && ctor == null) {
	try {
	ctor = cls.getConstructor(String.class);
	args = new String[]{stringarg};
	} catch (NoSuchMethodException ignored) {
	}
	}
	if (ctor == null) {
	ctor = cls.getConstructor();
	args = new Object[0];
	}
	return ctor.newInstance(args);

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

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

Conversation

davecramer commented Oct 25, 2021

sehrope commented Oct 25, 2021

davecramer commented Oct 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecramer commented Oct 25, 2021

srdan-bozovic-msft Oct 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdan-bozovic-msft Oct 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdan-bozovic-msft Oct 26, 2021 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdan-bozovic-msft commented Oct 27, 2021

davecramer commented Oct 27, 2021

davecramer commented Nov 5, 2021

sehrope commented Nov 5, 2021

jorsol commented Nov 5, 2021

sehrope commented Nov 7, 2021

davecramer commented Nov 7, 2021

sehrope commented Nov 7, 2021

davecramer commented Nov 10, 2021

sehrope commented Dec 20, 2021

srdan-bozovic-msft Oct 25, 2021 •

edited

srdan-bozovic-msft Oct 26, 2021 •

edited

srdan-bozovic-msft Oct 26, 2021 •

edited