Support Kerberos authentication between nodes by anusudarsan · Pull Request #9683 · prestodb/presto

anusudarsan · 2018-01-05T21:08:46Z

Supersedes #8859.

kokosing · 2018-01-06T19:43:46Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

it is not invalid method argument but invalid configuration. Should you throw configuration related exception here?

hmm..looks like we valid configs in other places the same way.

Another way to do this is to put the Kerberos configs in a separate class. This allows the properties to be @NotNull and thus they will be validated by the config system (which has good error messages). The class is only bound/requested if the isKerberosEnabled flag is set.

This is similar to how it works in the Hive connector where HdfsKerberosConfig has required properties but the config class is only bound when Kerberos is enabled.

kokosing · 2018-01-06T19:51:09Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationConfig.java

is this needed? I see that here com/facebook/presto/server/security/KerberosAuthenticator.java:68 it is dynamically evaluated from serviceName

ya, but this is for setting io.airlift.http.client.HttpClientConfig which is by the http client between nodes. See https://github.com/airlift/airlift/blob/master/http-client/src/main/java/io/airlift/http/client/spnego/SpnegoAuthentication.java#L84

Wouldn't this need to match http.server.authentication.krb5.service-name from KerberosConfig? Or will this be different after the next commit because it contains a _HOST placeholder?

kokosing · 2018-01-06T19:58:06Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

In com.facebook.presto.server.security.KerberosConfig#getKeytab it is nullable, does it have to be non null here?

right. that assumption is good here as well. removed

kokosing · 2018-01-06T20:07:29Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

how does it relate to KerberosConfig configured in com/facebook/presto/server/security/ServerSecurityModule.java:50

What will happen if internal kerberos communication does not match server kerboros communication? WIll one overwrite the other?

This one is different and is setting io.airlift.http.client.spnego.KerberosConfig which is used by the http client between nodes. Also see io.airlift.http.client.spnego.SpnegoAuthentication similar to KerberosAuthenticator.

They are setting different things, but they have to match, right?

kokosing · 2018-01-06T20:12:50Z

presto-main/src/main/java/com/facebook/presto/util/KerberosPrincipal.java

Maybe you could have internal static class Parser and store parsing related code there. What do you think? Having this class defined at the bottom would not polute this file and would make the most important things to be at the top of the class.

ya looks cleaner. done.

kokosing · 2018-01-06T20:22:48Z

presto-product-tests/conf/docker/multinode-tls-kerberos/docker-compose.yml

why no impersonation?

I dont think there is any specific reason. just chose one of the simple kerberos configs.

kokosing · 2018-01-06T20:23:25Z

.travis.yml

add description of this environment to README file

kokosing · 2018-01-06T20:26:12Z

presto-product-tests/conf/presto/etc/multinode-tls-kerberos-worker.properties

double licence

kokosing · 2018-01-06T20:27:16Z

...duct-tests/src/main/java/com/facebook/presto/tests/hive/TestTablePartitioningInsertInto.java

why it fails?

This is using the HttpQueryStatsClient and hence fail with encrypted connection. Previously we never ran smoke group with multinode-tls tests, but with this profile we do.

kokosing · 2018-01-06T20:31:30Z

presto-product-tests/conf/tempto/tempto-configuration-for-docker-tls-kerberos.yaml

do you need to define new tempto configuration file, I thought you reuse file which is used other kerberized environment

The only difference from the other kerberos config is that, this one defines http_port (line 22) which is needed by TLS test group (there is an assertion to test the http port is actually closed). I added this property to the other file and deleted this one.

anusudarsan

@kokosing addressed comments

anusudarsan · 2018-01-09T18:02:21Z

presto-product-tests/conf/tempto/tempto-configuration-for-docker-tls-kerberos.yaml

The only difference from the other kerberos config is that, this one defines http_port (line 22) which is needed by TLS test group (there is an assertion to test the http port is actually closed). I added this property to the other file and deleted this one.

anusudarsan · 2018-01-09T18:07:31Z

...duct-tests/src/main/java/com/facebook/presto/tests/hive/TestTablePartitioningInsertInto.java

This is using the HttpQueryStatsClient and hence fail with encrypted connection. Previously we never ran smoke group with multinode-tls tests, but with this profile we do.

anusudarsan · 2018-01-09T18:09:27Z

presto-product-tests/conf/docker/multinode-tls-kerberos/docker-compose.yml

I dont think there is any specific reason. just chose one of the simple kerberos configs.

anusudarsan · 2018-01-09T18:09:45Z

presto-main/src/test/java/com/facebook/presto/util/TestKerberosPrincipal.java

its already tested below at line 101?

anusudarsan · 2018-01-09T18:29:37Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

right. that assumption is good here as well. removed

anusudarsan · 2018-01-09T18:40:32Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

This one is different and is setting io.airlift.http.client.spnego.KerberosConfig which is used by the http client between nodes. Also see io.airlift.http.client.spnego.SpnegoAuthentication similar to KerberosAuthenticator.

anusudarsan · 2018-01-09T18:46:54Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationConfig.java

ya, but this is for setting io.airlift.http.client.HttpClientConfig which is by the http client between nodes. See https://github.com/airlift/airlift/blob/master/http-client/src/main/java/io/airlift/http/client/spnego/SpnegoAuthentication.java#L84

anusudarsan · 2018-01-09T18:50:12Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

probably. but that is not part of this PR.

anusudarsan · 2018-01-09T20:06:23Z

presto-main/src/main/java/com/facebook/presto/util/KerberosPrincipal.java

ya looks cleaner. done.

anusudarsan · 2018-01-09T20:10:59Z

presto-main/src/main/java/com/facebook/presto/util/KerberosPrincipal.java

this will just be appending the SEPARATORs to the username/hostname/realm, right? the actual escaping happens in parse and the escape characters will be retained in the variables here.

anusudarsan · 2018-01-16T13:40:31Z

@electrum I have also addressed your comments from here-
#8859 (review) . Please take a look.

electrum

Overall this looks good. I have some concerns about the duplicated config parameters that must be the same value (as far as I can tell).

electrum · 2018-02-08T19:33:49Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationConfig.java

Wouldn't this need to match http.server.authentication.krb5.service-name from KerberosConfig? Or will this be different after the next commit because it contains a _HOST placeholder?

electrum · 2018-02-08T19:35:02Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationConfig.java

Same with these, shouldn't it be the same as KerberosConfig?

Added a check to verify both matches.

electrum · 2018-02-08T19:38:56Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

Another way to do this is to put the Kerberos configs in a separate class. This allows the properties to be @NotNull and thus they will be validated by the config system (which has good error messages). The class is only bound/requested if the isKerberosEnabled flag is set.

This is similar to how it works in the Hive connector where HdfsKerberosConfig has required properties but the config class is only bound when Kerberos is enabled.

electrum · 2018-02-08T19:40:07Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

They are setting different things, but they have to match, right?

electrum · 2018-02-08T19:41:19Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

This module is already quite long. How about we move the internal communication stuff into a separate InternalCommunicationModule?

done. created a new module

electrum · 2018-02-08T19:57:23Z

presto-main/src/main/java/com/facebook/presto/util/KerberosPrincipal.java

Can the class be private?

electrum · 2018-02-08T19:57:38Z

presto-main/src/main/java/com/facebook/presto/util/KerberosPrincipal.java

Make this public since it's part of the API

electrum · 2018-02-08T19:57:47Z

presto-main/src/main/java/com/facebook/presto/util/KerberosPrincipal.java

Make public and move to the top

electrum · 2018-02-08T21:10:03Z

presto-main/src/main/java/com/facebook/presto/util/KerberosPrincipal.java

No need for break here

electrum · 2018-02-08T21:12:33Z

presto-docs/src/main/sphinx/security/internal-communication.rst

Should this document _HOST?

anusudarsan

@electrum addressed your comments. please take another look.

anusudarsan · 2018-03-29T19:37:59Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationConfig.java

Added a check to verify both matches.

anusudarsan · 2018-03-29T19:38:14Z

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

done. created a new module

anusudarsan · 2018-04-03T12:14:09Z

related - #10317

panditsurabhi · 2018-07-10T20:56:20Z

We are awaiting this change. Is it going to be merged soon? What's the ETA?

kokosing · 2018-07-11T06:10:37Z

@anusudarsan Could you please rebase?
@arhimondr Could you please review?

arhimondr

Please let me have an another look before you merge. Also please fix the commit history (git rebase "$(git merge-base HEAD master)" --ignore-date -x 'git commit --amend -C HEAD --date="$(date -R)" && sleep 1.05') (Thanks @findepi , sorry, didn't find the original paste)

arhimondr · 2018-07-11T17:08:25Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationModule.java

+            KerberosInternalCommunicationConfig internalKerberosConfig = buildConfigObject(KerberosInternalCommunicationConfig.class);
+            com.facebook.presto.server.security.KerberosConfig serverKerberosConfig = buildConfigObject(com.facebook.presto.server.security.KerberosConfig.class);
+
+            checkState(serverKerberosConfig.getKeytab() != null && serverKerberosConfig.getKeytab().getAbsolutePath().equals(internalKerberosConfig.getKerberosKeytab().getAbsolutePath()), "kerberos keytab values must match");


Since we are doing these checks, do we actually need 2 type of properties (for the client and for the server). Can we just use these properties from the server config?

arhimondr · 2018-07-11T17:08:56Z

presto-main/src/main/java/com/facebook/presto/server/KerberosInternalCommunicationConfig.java

+
+public class KerberosInternalCommunicationConfig
+{
+    private String kerberosPrincipal;


You can build it based on the service name. kerberosPrincipal=<kerberosServiceName>/_HOST, and it should get the realm from the config. See https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/server/security/KerberosAuthenticator.java#L68

We are using this convention to build the service principal for the server. I thing we can safely follow the approach to build a client principal as well.

arhimondr · 2018-07-11T17:09:16Z

presto-main/src/main/java/com/facebook/presto/server/KerberosInternalCommunicationConfig.java

+public class KerberosInternalCommunicationConfig
+{
+    private String kerberosPrincipal;
+    private String kerberosServiceName;


Use the one from the server config, remove this property

arhimondr · 2018-07-11T17:09:20Z

presto-main/src/main/java/com/facebook/presto/server/KerberosInternalCommunicationConfig.java

+{
+    private String kerberosPrincipal;
+    private String kerberosServiceName;
+    private File kerberosKeytab;


Use the one from the server config, remove this property

arhimondr · 2018-07-11T17:09:42Z

presto-main/src/main/java/com/facebook/presto/server/KerberosInternalCommunicationConfig.java

+    private String kerberosPrincipal;
+    private String kerberosServiceName;
+    private File kerberosKeytab;
+    private File kerberosConfig;


Use the one from the server config, remove this property. (this one is a static one property, JVM wide)

arhimondr · 2018-07-11T17:15:27Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationConfig.java

+        return kerberosEnabled;
+    }
+
+    @Config("internal-communication.authentication.kerberos.enabled")


internal-communication.kerberos.enabled

arhimondr · 2018-07-11T17:16:13Z

presto-main/src/main/java/com/facebook/presto/server/KerberosInternalCommunicationConfig.java

+        return kerberosUseCanonicalHostname;
+    }
+
+    @Config("internal-communication.authentication.krb5.use-canonical-hostname")


internal-communication.kerberos.use-canonical-hostname

arhimondr · 2018-07-11T17:31:18Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationModule.java

 */
 package com.facebook.presto.server;

+import com.facebook.presto.util.KerberosPrincipal;


If you build the principal name based on the service name you will no longer need this commit. Feel free to drop it.

arhimondr · 2018-07-11T17:33:56Z

.travis.yml

    fi
+    if [[ -v PRODUCT_TESTS_SPECIFIC_ENVIRONMENT ]]; then
+      presto-product-tests/bin/run_on_docker.sh \
+        multinode-tls-kerberos -g smoke,cli,group-by,join,tls -x stats_client


Personally i would just drop the smoke group here, so you don't have do exclude the stats_client group.

The idea with group-by and join here is to make the Presto do the exchanges.

arhimondr · 2018-07-11T17:34:25Z

presto-product-tests/conf/tempto/tempto-configuration-for-docker-kerberos.yaml

  presto:
    host: presto-master.docker.cluster
    port: 7778
+    http_port: 8080


Why is it needed? http is disabled

ya. but we run TLS test group (TlsTests) now in this profile. This property is used there to test if http_port is not actually being used.

anusudarsan

@arhimondr addressed all comments

anusudarsan · 2018-07-16T17:13:42Z

presto-product-tests/conf/tempto/tempto-configuration-for-docker-kerberos.yaml

  presto:
    host: presto-master.docker.cluster
    port: 7778
+    http_port: 8080


ya. but we run TLS test group (TlsTests) now in this profile. This property is used there to test if http_port is not actually being used.

arhimondr · 2018-07-17T16:46:48Z

presto-docs/src/main/sphinx/security/internal-communication.rst

s/Coordinator/server

arhimondr

LGTM % few comments

arhimondr · 2018-07-17T16:48:05Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationModule.java

clientKerberosConfig?

arhimondr · 2018-07-17T16:51:51Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationModule.java

UncheckedIOException

arhimondr · 2018-07-17T16:56:18Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationModule.java

getServiceName and getKerberosConfig are marked as @NotNull, and being validated during the KerberosConfig configuration creation

arhimondr · 2018-07-17T16:56:49Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationModule.java

I wonder if there is better exception than NullPointerException. NPE for the end user looks like a bug in the code. How about io.airlift.configuration.InvalidConfigurationException?

anusudarsan · 2018-07-17T22:49:57Z

@arhimondr addressed comments

anusudarsan · 2018-07-20T17:47:16Z

@arhimondr ping :)

findepi · 2018-07-20T18:28:21Z

@arhimondr i think i will just merge this

arhimondr · 2018-07-20T20:24:40Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationModule.java

Let's just add a message here

@findepi I can add the message and merge. Lets just figure out the message. How about

"http.server.authentication.krb5.keytab must be set when kerberos authentication for internal communication is enabled"

We can extract http.server.authentication.krb5.keytab into a constant ofcourse.

sure. that sounds good to me

maybe also point to internal-communication.kerberos.enabled in the message?

Yup, that would work

anusudarsan mentioned this pull request Jan 5, 2018

Support kerberos authentication between Presto nodes #8859

Closed

facebook-github-bot added the CLA Signed label Jan 5, 2018

anusudarsan requested review from electrum and kokosing January 5, 2018 21:09

kokosing reviewed Jan 6, 2018

View reviewed changes

anusudarsan force-pushed the anu/kerberos_auth_between_nodes branch from 7b5538d to 45fe2bd Compare January 9, 2018 21:20

anusudarsan commented Jan 9, 2018

View reviewed changes

kokosing approved these changes Jan 16, 2018

View reviewed changes

electrum reviewed Feb 9, 2018

View reviewed changes

anusudarsan force-pushed the anu/kerberos_auth_between_nodes branch 2 times, most recently from a13745e to 9c9f1e1 Compare March 29, 2018 21:09

anusudarsan commented Mar 29, 2018

View reviewed changes

anusudarsan force-pushed the anu/kerberos_auth_between_nodes branch 2 times, most recently from f2a6a3a to 2801eb0 Compare April 3, 2018 15:19

anusudarsan force-pushed the anu/kerberos_auth_between_nodes branch 2 times, most recently from 271a310 to d8ebc56 Compare June 15, 2018 15:49

arhimondr reviewed Jul 11, 2018

View reviewed changes

anusudarsan force-pushed the anu/kerberos_auth_between_nodes branch from d8ebc56 to e2dbb6e Compare July 16, 2018 20:11

anusudarsan commented Jul 16, 2018

View reviewed changes

arhimondr reviewed Jul 17, 2018

View reviewed changes

presto-docs/src/main/sphinx/security/internal-communication.rst Outdated

Copy link

Member

arhimondr Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Coordinator/server

arhimondr approved these changes Jul 17, 2018

View reviewed changes

anusudarsan force-pushed the anu/kerberos_auth_between_nodes branch from e2dbb6e to 723d5c2 Compare July 17, 2018 22:49

anusudarsan force-pushed the anu/kerberos_auth_between_nodes branch from 723d5c2 to dda62ca Compare July 17, 2018 22:59

arhimondr approved these changes Jul 20, 2018

View reviewed changes

anusudarsan force-pushed the anu/kerberos_auth_between_nodes branch 2 times, most recently from 927c755 to 28ed737 Compare July 20, 2018 21:14

arhimondr force-pushed the anu/kerberos_auth_between_nodes branch from 28ed737 to f08e10b Compare July 20, 2018 21:22

anusudarsan and others added 4 commits July 20, 2018 17:25

Move secured internal communication injection to new module

0eb2282

Support for Kerberos secured internal communication

4317f7c

Document Kerberos secured internal communication

286efd3

Product tests for Kerberos secured internal communication

3b50e03

arhimondr force-pushed the anu/kerberos_auth_between_nodes branch from f08e10b to 3b50e03 Compare July 20, 2018 21:27

arhimondr merged commit 3b50e03 into prestodb:master Jul 20, 2018

findepi mentioned this pull request Oct 29, 2018

Ignore SpnegoFilter during inner https(secure) request #8973

Closed

Conversation

anusudarsan commented Jan 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anusudarsan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anusudarsan commented Jan 16, 2018

Uh oh!

electrum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment