Support multiple password authentication plugins #7151

skrzypo987 · 2021-03-04T10:03:39Z

Fixes: #1791

skrzypo987 · 2021-03-04T10:19:11Z

docs still missing

kokosing

Skimmed

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticator.java

kokosing · 2021-03-04T11:38:01Z

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticatorManager.java

 import static java.util.Objects.requireNonNull;

 public class PasswordAuthenticatorManager
 {
    private static final Logger log = Logger.get(PasswordAuthenticatorManager.class);

-    private static final File CONFIG_FILE = new File("etc/password-authenticator.properties");
+    private static final File DEFAULT_CONFIG_FILE = new File("etc/password-authenticator.properties");


Can you move to as default value of password-authenticator.config-files?

This is how we do it for similar plugins. If we move it, we need to check here that the list is not empty. (maybe can use bean validation for that)

It appears to be screwing up testing pretty badly as the validation of the existence of the file is done always.

sopel39 · 2021-03-09T10:51:02Z

I agree with @kokosing that we should try to flatten password authenticatiors instead of nesting them.
It seems the biggest magic would be in ServerSecurityModule to allow binding for same type of authenticator (io.trino.server.security.ServerSecurityModule#authenticatorBinder). This could be done similar as in https://stackoverflow.com/questions/49107121/guice-mapbinder-with-list-values

skrzypo987 · 2021-03-17T13:39:21Z

@kokosing @sopel39
I tried to "flatten password authenticatiors" instead of nesting them. However there is one problem:
The authenticator list is created in the Guice context. However the PasswordAuthenticatorManager is populated with data (authenticator factories) on the later stage in PluginManager, so the information about the types of password authenticators is unknown when the list is made. Do you have any idea how to get around that?

kokosing · 2021-03-18T08:37:27Z

I was thinking that each file should generate one PasswordAuthenticator binding. Notice that it is already a set multibinder. However I see it is not an easy thing to do... but still I think it is the way to go (or at least to explore).

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticatorManager.java

core/trino-main/src/main/java/io/trino/server/security/AuthenticationFilter.java

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticatorManager.java

sopel39 · 2021-03-18T22:00:06Z

core/trino-main/src/main/java/io/trino/server/security/ServerSecurityModule.java

@@ -62,6 +62,7 @@ protected void setup(Binder binder)
                .internalOnlyResource(StoreResource.class);

        binder.bind(PasswordAuthenticatorManager.class).in(Scopes.SINGLETON);
+        configBinder(binder).bindConfig(PasswordAuthenticatorConfig.class);


It should not be possible to specify password config properties when password authentication is not used.

Maybe PasswordAuthenticatorManager (along with config) can be binded conditionally? Add PasswordAuthenticatorSupportModule as for Jwt and Oauth2.

It is not that easy.
PasswordAuthenticatorManager is used by PluginManager and needs to be present even if it is not used. And the config is needed by Guice to create it

You can use https://google.github.io/guice/api-docs/4.1/javadoc/com/google/inject/multibindings/OptionalBinder.html

core/trino-main/src/main/java/io/trino/server/ui/FormUiAuthenticatorModule.java

core/trino-main/src/test/java/io/trino/server/security/TestPasswordAuthenticatorManager.java

core/trino-main/src/main/java/io/trino/server/security/AuthenticationFilter.java

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticator.java

electrum · 2021-03-04T18:15:14Z

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticatorManager.java

 import static java.util.Objects.requireNonNull;

 public class PasswordAuthenticatorManager
 {
    private static final Logger log = Logger.get(PasswordAuthenticatorManager.class);

-    private static final File CONFIG_FILE = new File("etc/password-authenticator.properties");
+    private static final File DEFAULT_CONFIG_FILE = new File("etc/password-authenticator.properties");


This is how we do it for similar plugins. If we move it, we need to check here that the list is not empty. (maybe can use bean validation for that)

skrzypo987

Comments addressed

skrzypo987 · 2021-03-19T08:23:30Z

core/trino-main/src/main/java/io/trino/server/security/ServerSecurityModule.java

@@ -62,6 +62,7 @@ protected void setup(Binder binder)
                .internalOnlyResource(StoreResource.class);

        binder.bind(PasswordAuthenticatorManager.class).in(Scopes.SINGLETON);
+        configBinder(binder).bindConfig(PasswordAuthenticatorConfig.class);


It is not that easy.
PasswordAuthenticatorManager is used by PluginManager and needs to be present even if it is not used. And the config is needed by Guice to create it

core/trino-main/src/test/java/io/trino/server/security/TestPasswordAuthenticatorManager.java

core/trino-main/src/main/java/io/trino/server/ui/FormUiAuthenticatorModule.java

skrzypo987 · 2021-03-22T09:04:42Z

Commits reordered, tests fixed.
I will update docs in a separate PR

core/trino-main/src/test/java/io/trino/server/ui/TestWebUi.java

core/trino-main/src/main/java/io/trino/server/ui/FormUiAuthenticatorModule.java

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticator.java

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticatorConfig.java

core/trino-main/src/test/java/io/trino/server/ui/TestWebUi.java

core/trino-main/src/main/java/io/trino/server/ui/FormUiAuthenticatorModule.java

sopel39

lgtm % small comments

core/trino-main/src/test/java/io/trino/server/ui/TestWebUi.java

core/trino-main/src/main/java/io/trino/server/security/ServerSecurityModule.java

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticator.java

core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java

skrzypo987 · 2021-03-24T12:51:51Z

Squashed the two "optional binding" commits.
Made validation for password-authenticator.config-files to be non-empty

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticatorConfig.java

skrzypo987 · 2021-03-24T13:57:58Z

@sopel39 Config description added

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticatorConfig.java

core/trino-main/src/main/java/io/trino/server/PluginManager.java

core/trino-main/src/main/java/io/trino/server/security/AuthenticationFilter.java

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticator.java

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticatorManager.java

core/trino-main/src/main/java/io/trino/server/ui/FormUiAuthenticatorModule.java

core/trino-main/src/main/java/io/trino/server/Server.java

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticatorManager.java

electrum · 2021-03-29T19:22:38Z

client/trino-jdbc/src/test/java/io/trino/jdbc/TestTrinoDriverImpersonateUser.java

    {
+        Path passwordConfigDummy = Files.createTempFile("passwordConfigDummy", "");


Why not use (null, null) like TestPasswordAuthenticatorConfig? Seems like they should be consistent

The prefix is defensive. If you happen to find 5k of files in /tmp you know where to look for a bug. Suffix changed to null

core/trino-main/src/main/java/io/trino/server/Server.java

core/trino-main/src/test/java/io/trino/server/security/TestPasswordAuthenticatorManager.java

core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java

kokosing

Just skimmed FMI.

I think you want to add a product tests here where you could use actual two password authenticators.

client/trino-jdbc/src/test/java/io/trino/jdbc/TestTrinoDriverImpersonateUser.java

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticatorManager.java

core/trino-main/src/main/java/io/trino/server/security/ServerSecurityModule.java

skrzypo987 · 2021-04-01T17:55:08Z

@kokosing added a simple product test. PTAL

kokosing

Reviewed product tests only.

testing/trino-product-tests/src/main/java/io/trino/tests/cli/TestTrinoLdapCli.java

kokosing · 2021-04-01T18:43:46Z

testing/trino-product-tests/src/main/java/io/trino/tests/cli/TestTrinoLdapCli.java

+        launchTrinoCliWithServerArgument();
+        trino.waitForPrompt();
+        trino.getProcessInput().println("select * from hive.default.nation;");
+        assertThat(trimLines(trino.readLinesUntilPrompt())).containsAll(nationTableInteractiveLines);


Please use JDBC for testing. Please make sure you test two authenticators. Please also add some negative tests where user was not able to authenticate.

Please next time make 3 remarks instead of one.
2.This test is for the file authenticator, shouldRunQueryWithLdap is for LDAP. I don't see reason to repeat any of them.
3.Every failing test in this class doe this, e.g. shouldFailQueryForWrongLdapPassword

Using TrinoLdapCLI for your use case is not a best fit. These tests are targeted for something else. I think need a dedicated set of tests.

I use both CLI and JDBC now.

...er/src/main/java/io/trino/tests/product/launcher/env/environment/AbstractSinglenodeLdap.java

kokosing · 2021-04-01T18:49:04Z

.../resources/docker/presto-product-tests/conf/environment/singlenode-ldap-and-file/password.db

@@ -0,0 +1 @@
+DefaultGroupUser:$2y$10$xA36wzOAnGWJmukr/zItyOrOyXPD6prgszOCN93MyFUpMAbGKcklm


Please do not use same user here as in ldap. That way we don't know which authenticator was actually used.

Part of the whole idea is to use two authenticators for one user.
I will another one and test it separately

So how does it work then? You will one authenticator any way. It is not much interesting case.

Multiple password authenticators works exactly the same as standard authenticators.

kokosing · 2021-04-01T18:50:20Z

Thanks for product tests! 🍰

skrzypo987

@kokosing PTAL

skrzypo987 · 2021-04-06T10:45:23Z

@kokosing added offline-requested upper-case SQL keywords.

kokosing

product tests LGTM

...her/src/main/java/io/trino/tests/product/launcher/env/environment/SinglenodeLdapAndFile.java

testing/trino-product-tests/src/main/java/io/trino/tests/jdbc/TestLdapTrinoJdbc.java

This way password config is not read when password authentication type is not set

LDAP + file based authentication is used.

sopel39 · 2021-04-06T20:59:31Z

please add documentation

cla-bot bot added the cla-signed label Mar 4, 2021

skrzypo987 requested review from sopel39 and electrum March 4, 2021 10:03

kokosing reviewed Mar 4, 2021

View reviewed changes

skrzypo987 added the WIP label Mar 9, 2021

findepi requested a review from dain March 17, 2021 16:50

sopel39 reviewed Mar 18, 2021

View reviewed changes

electrum reviewed Mar 18, 2021

View reviewed changes

skrzypo987 force-pushed the skrzypo/022-multiple-password-authenticators branch from 3787dab to 53f041e Compare March 19, 2021 09:31

skrzypo987 commented Mar 19, 2021

View reviewed changes

skrzypo987 removed the WIP label Mar 19, 2021

sopel39 reviewed Mar 19, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/server/ui/FormUiAuthenticatorModule.java Show resolved Hide resolved

skrzypo987 force-pushed the skrzypo/022-multiple-password-authenticators branch from 41bde08 to 2ab1da2 Compare March 22, 2021 09:03

skrzypo987 force-pushed the skrzypo/022-multiple-password-authenticators branch from 2ab1da2 to cae545b Compare March 22, 2021 15:09

sopel39 reviewed Mar 22, 2021

View reviewed changes

skrzypo987 force-pushed the skrzypo/022-multiple-password-authenticators branch 3 times, most recently from 1395f19 to 60fa2d5 Compare March 24, 2021 08:59

sopel39 approved these changes Mar 24, 2021

View reviewed changes

core/trino-main/src/test/java/io/trino/server/ui/TestWebUi.java Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/server/security/ServerSecurityModule.java Show resolved Hide resolved

sopel39 reviewed Mar 24, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticator.java Outdated Show resolved Hide resolved

core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java Outdated Show resolved Hide resolved

skrzypo987 force-pushed the skrzypo/022-multiple-password-authenticators branch from 60fa2d5 to c8d715c Compare March 24, 2021 12:50

sopel39 approved these changes Mar 24, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticatorConfig.java Outdated Show resolved Hide resolved

skrzypo987 force-pushed the skrzypo/022-multiple-password-authenticators branch from c8d715c to 1e609a2 Compare March 24, 2021 13:53

sopel39 reviewed Mar 24, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/server/security/PasswordAuthenticatorConfig.java Outdated Show resolved Hide resolved

skrzypo987 force-pushed the skrzypo/022-multiple-password-authenticators branch from 1e609a2 to 2b4aa7e Compare March 24, 2021 15:45

electrum reviewed Mar 24, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/server/PluginManager.java Show resolved Hide resolved

electrum reviewed Mar 29, 2021

View reviewed changes

electrum approved these changes Mar 29, 2021

View reviewed changes

kokosing reviewed Mar 30, 2021

View reviewed changes

skrzypo987 force-pushed the skrzypo/022-multiple-password-authenticators branch 2 times, most recently from e8490dd to f97fdba Compare March 30, 2021 12:57

sopel39 approved these changes Mar 31, 2021

View reviewed changes

kokosing reviewed Apr 1, 2021

View reviewed changes

skrzypo987 force-pushed the skrzypo/022-multiple-password-authenticators branch from aa63487 to e518249 Compare April 2, 2021 09:38

skrzypo987 commented Apr 2, 2021

View reviewed changes

skrzypo987 force-pushed the skrzypo/022-multiple-password-authenticators branch from e518249 to b0184aa Compare April 6, 2021 10:44

kokosing reviewed Apr 6, 2021

View reviewed changes

...her/src/main/java/io/trino/tests/product/launcher/env/environment/SinglenodeLdapAndFile.java Outdated Show resolved Hide resolved

testing/trino-product-tests/src/main/java/io/trino/tests/jdbc/TestLdapTrinoJdbc.java Outdated Show resolved Hide resolved

skrzypo987 force-pushed the skrzypo/022-multiple-password-authenticators branch from b0184aa to 00ed323 Compare April 6, 2021 13:17

skrzypo987 added 6 commits April 6, 2021 16:45

Throw unchecked exception in place of IOException

0a29d5a

Bind PasswordAuthenticatorManager if required

99efc7b

This way password config is not read when password authentication type is not set

Support multiple password authentication plugins

1fbea1b

Extract constant

fe3f209

Make SQL keywords uppercase

169747f

Product tests for multiple password authenticators

d83246c

LDAP + file based authentication is used.

skrzypo987 force-pushed the skrzypo/022-multiple-password-authenticators branch from 00ed323 to d83246c Compare April 6, 2021 14:45

sopel39 merged commit 10bae4c into trinodb:master Apr 6, 2021

sopel39 mentioned this pull request Apr 6, 2021

Release notes for 355 #7366

Closed

7 tasks

martint added this to the 355 milestone Apr 6, 2021

electrum mentioned this pull request Sep 13, 2021

Add header authenticator SPI #8895

Merged

		{
		Path passwordConfigDummy = Files.createTempFile("passwordConfigDummy", "");

		@@ -0,0 +1 @@
		DefaultGroupUser:$2y$10$xA36wzOAnGWJmukr/zItyOrOyXPD6prgszOCN93MyFUpMAbGKcklm

Support multiple password authentication plugins #7151

Support multiple password authentication plugins #7151

Conversation

skrzypo987 commented Mar 4, 2021 • edited by sopel39

skrzypo987 commented Mar 4, 2021

kokosing 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

sopel39 commented Mar 9, 2021

skrzypo987 commented Mar 17, 2021

kokosing commented Mar 18, 2021

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

skrzypo987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skrzypo987 commented Mar 22, 2021

sopel39 left a comment

Choose a reason for hiding this comment

skrzypo987 commented Mar 24, 2021

skrzypo987 commented Mar 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

skrzypo987 commented Apr 1, 2021

kokosing 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing commented Apr 1, 2021

skrzypo987 left a comment

Choose a reason for hiding this comment

skrzypo987 commented Apr 6, 2021

kokosing left a comment

Choose a reason for hiding this comment

sopel39 commented Apr 6, 2021

skrzypo987 commented Mar 4, 2021 •

edited by sopel39