Support kerberos authentication between Presto nodes#8859
Support kerberos authentication between Presto nodes#8859arhimondr wants to merge 4 commits intoprestodb:masterfrom
Conversation
electrum
left a comment
electrum
left a comment
There was a problem hiding this comment.
A few minor comments. I did a quick pass and it seems good, will let someone more familiar with this double check and approve.
| File kerberosKeytab = internalCommunicationConfig.getKerberosKeytab(); | ||
| String kerberosPrincipal = internalCommunicationConfig.getKerberosPrincipal(); | ||
| String kerberosServiceName = internalCommunicationConfig.getKerberosServiceName(); | ||
| checkArgument(kerberosConfig != null, "kerberos config must be set"); |
There was a problem hiding this comment.
Add these as @NotNull validation constraints to the config class. This will provide appropriate error messages when the values are not set. Then you can remove the checks here since we know the config system will only return a validated bean.
There was a problem hiding this comment.
Ah, I see that this validation needs to be conditional. You can put these new config properties (except for the enabled flag) in a separate class (KerberosInternalCommunicationConfig or KerberosCommunicationConfig?) that only gets created if Kerberos is enabled.
We do a similar trick in HiveAuthenticationModule where the various modules with their configs get bound conditionally if they are enabled.
|
|
||
| import static org.testng.Assert.assertEquals; | ||
|
|
||
| public class TestKerberosPrincipal |
There was a problem hiding this comment.
I'd add some tests for invalid principals to make sure the error handling works and we don't get an IndexOutOfBoundsException or similar.
| assertEquals(KerberosPrincipal.valueOf("user/_HOST@REALM.COM"), new KerberosPrincipal("user", Optional.of("_HOST"), Optional.of("REALM.COM"))); | ||
| assertEquals(KerberosPrincipal.valueOf("user/part1/part2/_HOST@REALM.COM"), new KerberosPrincipal("user/part1/part2", Optional.of("_HOST"), Optional.of("REALM.COM"))); | ||
| assertEquals(KerberosPrincipal.valueOf("user\\/part1\\/part2\\/_HOST@REALM.COM"), new KerberosPrincipal("user\\/part1\\/part2\\/_HOST", Optional.empty(), Optional.of("REALM.COM"))); | ||
| assertEquals(KerberosPrincipal.valueOf("user\\/part1\\/part2\\/_HOST\\@REALM.COM"), new KerberosPrincipal("user\\/part1\\/part2\\/_HOST\\@REALM.COM", Optional.empty(), Optional.empty())); |
There was a problem hiding this comment.
These longer ones would be easier to read if wrapped like in the method below.
| Internal SSL/TLS communication with Kerberos | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| If the :doc:`Kerberos</security/server>` authentication is enabled, specify valid Kerberos |
There was a problem hiding this comment.
Remove "the" so it reads as "If Kerberos authentication is enabled..."
|
Not sure why GitHub shows the comments on |
| }); | ||
| configBinder(binder).bindConfigGlobalDefaults(HttpClientConfig.class, config -> { | ||
| config.setAuthenticationEnabled(true); | ||
| config.setKerberosPrincipal(principal.substituteHostnamePlaceholder().toString()); |
There was a problem hiding this comment.
what was the reason to do this _HOST transformation, is it related with how the keytab/principle generated. Shall we make an optional logic against directly using "internalCommunicationConfig.getKerberosPrincipal()" ?
|
when i use presto 0.187 with kerberos in coordinator and internal-communication,but I get error : Can you help me ?Thks |
|
addressed comments and rebased here - #9683. Closing this. |
6 participants
No description provided.