New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce a plugin manager interface to allow different mechanisms to acquire a password. #2321
Changes from 2 commits
d20dc08
3075b04
dd17da2
22130dd
51dcb8c
b75a9c2
f80666c
d39c8f1
2687f2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
/* | ||
* Copyright (c) 2021, PostgreSQL Global Development Group | ||
* See the LICENSE file in the project root for more information. | ||
*/ | ||
|
||
package org.postgresql.core; | ||
|
||
public interface AuthenticationPlugin { | ||
byte[] getEncodedPassword(String userName, String password); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The intended use is that if you want a different authentication mechanism you load it via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case server determines the method should we have something like this?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does there need to be consideration about what classloader to use? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding args to the plugin is interesting. We would have to pass the arg into getAuthenticationPlugin There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we just follow the same pattern as the SSL factories (and use the ObjectFactory logic)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.getMessage() ); | ||
davecramer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
throw ex; | ||
} | ||
} | ||
} | ||
} |
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 java.nio.charset.StandardCharsets; | ||
|
||
public class PasswordAuthentication implements AuthenticationPlugin { | ||
|
||
@Override | ||
public byte[] getEncodedPassword(String userName, String password) { | ||
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.
probably need some javadoc here.
for example, this asks for encoded password, but what encoding?
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.
fair enough. This is really a work in progress.