Introduce a plugin manager interface to allow different mechanisms to acquire a password. #2321
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| /* | ||
| * Copyright (c) 2021, PostgreSQL Global Development Group | ||
| * See the LICENSE file in the project root for more information. | ||
| */ | ||
|
|
||
| package org.postgresql.core; | ||
|
|
||
| import org.postgresql.util.PSQLException; | ||
|
|
||
| import java.util.Properties; | ||
|
|
||
| public interface AuthenticationPlugin { | ||
|
|
||
| byte[] getEncodedPassword(Properties info) throws PSQLException, PSQLException; | ||
|
|
||
| } | ||
|
There was a problem hiding this comment. 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 ;) ) There was a problem hiding this comment. Why would you need to know the auth method? I would think at the end of the day all we need is the password? There was a problem hiding this comment. 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. There was a problem hiding this comment. What would There was a problem hiding this comment. The intended use is that if you want a different authentication mechanism you load it via There was a problem hiding this comment. In case server determines the method should we have something like this? There was a problem hiding this comment. 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. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| /* | ||
| * Copyright (c) 2021, PostgreSQL Global Development Group | ||
| * See the LICENSE file in the project root for more information. | ||
| */ | ||
|
|
||
| package org.postgresql.core; | ||
|
|
||
| import org.postgresql.PGProperty; | ||
|
|
||
| import java.util.Properties; | ||
| import java.util.logging.Level; | ||
| import java.util.logging.Logger; | ||
|
|
||
| public class AuthenticationPluginManager { | ||
|
|
||
| private static final Logger LOGGER = Logger.getLogger(AuthenticationPluginManager.class.getName()); | ||
|
|
||
| public static AuthenticationPlugin getAuthenticationPlugin(Properties info) throws Exception { | ||
| String authenticationClassName = PGProperty.AUTHENTICATION_PLUGIN_CLASS_NAME.getSetString(info); | ||
|
|
||
| if ( authenticationClassName == null ) { | ||
| return new PasswordAuthentication(); | ||
| } else { | ||
davecramer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| try { | ||
| return (AuthenticationPlugin) Class.forName(authenticationClassName).getDeclaredConstructor().newInstance(); | ||
|
There was a problem hiding this comment. does there need to be consideration about what classloader to use? There was a problem hiding this comment. 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. 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. There was a problem hiding this comment. Adding args to the plugin is interesting. We would have to pass the arg into getAuthenticationPlugin There was a problem hiding this comment. No the Properties alone is fine as the user can retrieve the value if they want it. They just need a factory name, like We could add a separate constructor that takes a String (again similar to the SSL factories) but it's not strictly necessary. There was a problem hiding this comment. Should we just follow the same pattern as the SSL factories (and use the ObjectFactory logic)? There was a problem hiding this comment. Yes that's a good idea. All this class instantiation should happen in one place so the ClassLoader et al are handled consistently. |
||
| } catch (Exception ex ) { | ||
| LOGGER.log(Level.FINE, "Unable to load Authentication Plugin " + ex.toString() ); | ||
| throw ex; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| * Copyright (c) 2021, PostgreSQL Global Development Group | ||
| * See the LICENSE file in the project root for more information. | ||
| */ | ||
|
|
||
| package org.postgresql.core; | ||
|
|
||
| import org.postgresql.PGProperty; | ||
| import org.postgresql.util.GT; | ||
| import org.postgresql.util.PSQLException; | ||
| import org.postgresql.util.PSQLState; | ||
|
|
||
| import java.nio.charset.StandardCharsets; | ||
| import java.util.Properties; | ||
|
|
||
| public class PasswordAuthentication implements AuthenticationPlugin { | ||
|
|
||
| @Override | ||
| public byte[] getEncodedPassword(Properties info) throws PSQLException { | ||
| String password = PGProperty.PASSWORD.getSetString(info); | ||
|
|
||
| if (password == null) { | ||
| throw new PSQLException( | ||
| GT.tr( | ||
| "The server requested password-based authentication, but no password was provided."), | ||
| PSQLState.CONNECTION_REJECTED); | ||
| } | ||
|
|
||
| return password.getBytes(StandardCharsets.UTF_8); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there risk in providing (i.e. leaking) reference to the mutable
Propertiesinstance?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
Function<PGProperty, String>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the method name
getEncodedPassword()should beencodedPassword()in that this really isn't a "getter" per say.