Re: Review Request 51822: Add Ambari configuration options to support Kerberos token authentication

2016-09-13 Thread Jonathan Hurley 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51822/#review148785
---


Ship it!




Ship It!

- Jonathan Hurley


On Sept. 13, 2016, 4:44 p.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51822/
> ---
> 
> (Updated Sept. 13, 2016, 4:44 p.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Nate 
> Cole.
> 
> 
> Bugs: AMBARI-18365
> https://issues.apache.org/jira/browse/AMBARI-18365
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Add the followng Ambari configuration options to support Kerberos token 
> authentication
> 
> - `authentication.kerberos.enabled`
> -- Determines whether to use Kerberos (SPNEGO) authentication when connecting 
> Ambari:  {{true}} to enable this feature; {{false}}, otherwise
> - `authentication.kerberos.spnego.principal`
> -- The Kerberos principal name to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.spnego.keytab.file`
> -- The Kerberos keytab file to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.user.types`
> -- A comma-delimited (ordered) list of preferred user types to use when 
> finding the Ambari user account for the user-supplied Kerberos identity 
> during authentication via SPNEGO
> - `authentication.kerberos.auth_to_local.rules`
> -- The auth-to-local rules set to use when translating a user's principal 
> name to a local user name during authentication via SPNEGO.
> 
> NOTE: These properties are in the {{ambari.properties}} file since this 
> feature may be enabled whether the rest of the cluster has Kerberos enabled 
> or not. 
> 
> See https://issues.apache.org/jira/browse/AMBARI-18364 for an overview 
> requiring the need for this patch.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
>  ee73b8d 
>   
> ambari-server/src/main/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationProperties.java
>  PRE-CREATION 
>   
> ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
>  f429a36 
>   
> ambari-server/src/test/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationPropertiesTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51822/diff/
> 
> 
> Testing
> ---
> 
> Manually tested...
> 
> # Local test results: 
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 2:51:23.081s
> [INFO] Finished at: Tue Sep 13 00:00:59 EDT 2016
> [INFO] Final Memory: 48M/1792M
> [INFO] 
> 
> 
> # Jenkins test results: 
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 01:34 h
> [INFO] Finished at: 2016-09-13T08:46:23+00:00
> [INFO] Final Memory: 178M/2672M
> [INFO] 
> 
> 
> {color:green}+1 overall{color}.  Here are the results of testing the latest 
> attachment 
>   
> http://issues.apache.org/jira/secure/attachment/12828101/AMBARI-18365_trunk_01.patch
>   against trunk revision .
> 
> {color:green}+1 @author{color}.  The patch does not contain any @author 
> tags.
> 
> {color:green}+1 tests included{color}.  The patch appears to include 2 
> new or modified test files.
> 
> {color:green}+1 javac{color}.  The applied patch does not increase the 
> total number of javac compiler warnings.
> 
> {color:green}+1 release audit{color}.  The applied patch does not 
> increase the total number of release audit warnings.
> 
> {color:green}+1 core tests{color}.  The patch passed unit tests in 
> ambari-server.
> 
> Test results: 
> https://builds.apache.org/job/Ambari-trunk-test-patch/8643//testReport/
> Console output: 
> https://builds.apache.org/job/Ambari-trunk-test-patch/8643//console
> 
> This message is automatically generated.
> 
> 
> Thanks,
> 
> Robert Levas
> 
>

Re: Review Request 51822: Add Ambari configuration options to support Kerberos token authentication

2016-09-13 Thread Jonathan Hurley 


> On Sept. 13, 2016, 8:24 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java,
> >  line 2586
> > 
> >
> > Although I understand the motivation, I still don't like doing this 
> > since getting the backing Properties object for the entire Configuration 
> > would be missing these properties. Can we mirror them in the normal 
> > Properties object as well?
> 
> Robert Levas wrote:
> I am not sure what you mean.  The backing properties object contains the 
> properties from the ambari.properties file and is being used to fill the 
> KerberosAuthenticationProperties instance.   Does this data need to be copied 
> to another map?

Yeah, I missed that it was coming from the backing properties. Sorry.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51822/#review148699
---


On Sept. 13, 2016, 4:44 p.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51822/
> ---
> 
> (Updated Sept. 13, 2016, 4:44 p.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Nate 
> Cole.
> 
> 
> Bugs: AMBARI-18365
> https://issues.apache.org/jira/browse/AMBARI-18365
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Add the followng Ambari configuration options to support Kerberos token 
> authentication
> 
> - `authentication.kerberos.enabled`
> -- Determines whether to use Kerberos (SPNEGO) authentication when connecting 
> Ambari:  {{true}} to enable this feature; {{false}}, otherwise
> - `authentication.kerberos.spnego.principal`
> -- The Kerberos principal name to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.spnego.keytab.file`
> -- The Kerberos keytab file to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.user.types`
> -- A comma-delimited (ordered) list of preferred user types to use when 
> finding the Ambari user account for the user-supplied Kerberos identity 
> during authentication via SPNEGO
> - `authentication.kerberos.auth_to_local.rules`
> -- The auth-to-local rules set to use when translating a user's principal 
> name to a local user name during authentication via SPNEGO.
> 
> NOTE: These properties are in the {{ambari.properties}} file since this 
> feature may be enabled whether the rest of the cluster has Kerberos enabled 
> or not. 
> 
> See https://issues.apache.org/jira/browse/AMBARI-18364 for an overview 
> requiring the need for this patch.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
>  ee73b8d 
>   
> ambari-server/src/main/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationProperties.java
>  PRE-CREATION 
>   
> ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
>  f429a36 
>   
> ambari-server/src/test/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationPropertiesTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51822/diff/
> 
> 
> Testing
> ---
> 
> Manually tested...
> 
> # Local test results: 
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 2:51:23.081s
> [INFO] Finished at: Tue Sep 13 00:00:59 EDT 2016
> [INFO] Final Memory: 48M/1792M
> [INFO] 
> 
> 
> # Jenkins test results: 
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 01:34 h
> [INFO] Finished at: 2016-09-13T08:46:23+00:00
> [INFO] Final Memory: 178M/2672M
> [INFO] 
> 
> 
> {color:green}+1 overall{color}.  Here are the results of testing the latest 
> attachment 
>   
> http://issues.apache.org/jira/secure/attachment/12828101/AMBARI-18365_trunk_01.patch
>   against trunk revision .
> 
> {color:green}+1 @author{color}.  The patch does not contain any @author 
> tags.
> 
> {color:green}+1 tests included{color}.  The patch appears to include 2 
> new or modified test files.
> 
> {color:green}+1 javac{color}.  The applied patch does not increase the 
> total number of javac compiler

Re: Review Request 51822: Add Ambari configuration options to support Kerberos token authentication

2016-09-13 Thread Jonathan Hurley 


> On Sept. 13, 2016, 8:24 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java,
> >  lines 5292-5294
> > 
> >
> > Why not just return here which would prevent you from needing to embed 
> > the rest of the logic in the if-statement
> 
> Robert Levas wrote:
> Then I would have 2 return statements... but I can make the chance since 
> most developers like to short-curcuit their code.

That's true; it's 50/50 on what preferences are for this stuff. I find that 
with Ambari, we have a lot of nested if-statements, so we end up with code like:
```
if(null != cluster)
  if(null != cluster.getServices())
if(null != service.getName())
  // do something
  
```
Where the something is quite complex and has it's own control flow. Because of 
this, I think it's easier and less error prone to quickly return for nulls.


> On Sept. 13, 2016, 8:24 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java,
> >  lines 5298-5301
> > 
> >
> > KERBEROS_AUTH_USER_TYPES has a default value, so `getProperty` will 
> > never return null.
> 
> Robert Levas wrote:
> This is a safety measure in the event someone changes the definition of 
> `KERBEROS_AUTH_USER_TYPES`. Plus I generally feel the need to check for null 
> and empty in these cases. But I can remove this check.

Nope, that's fine - you can keep it then - I was just pointing out that you're 
getting the default for free here so unless somebody sets it to blank in 
`ambari.properties` it should never be null.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51822/#review148699
---


On Sept. 13, 2016, 4:44 p.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51822/
> ---
> 
> (Updated Sept. 13, 2016, 4:44 p.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Nate 
> Cole.
> 
> 
> Bugs: AMBARI-18365
> https://issues.apache.org/jira/browse/AMBARI-18365
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Add the followng Ambari configuration options to support Kerberos token 
> authentication
> 
> - `authentication.kerberos.enabled`
> -- Determines whether to use Kerberos (SPNEGO) authentication when connecting 
> Ambari:  {{true}} to enable this feature; {{false}}, otherwise
> - `authentication.kerberos.spnego.principal`
> -- The Kerberos principal name to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.spnego.keytab.file`
> -- The Kerberos keytab file to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.user.types`
> -- A comma-delimited (ordered) list of preferred user types to use when 
> finding the Ambari user account for the user-supplied Kerberos identity 
> during authentication via SPNEGO
> - `authentication.kerberos.auth_to_local.rules`
> -- The auth-to-local rules set to use when translating a user's principal 
> name to a local user name during authentication via SPNEGO.
> 
> NOTE: These properties are in the {{ambari.properties}} file since this 
> feature may be enabled whether the rest of the cluster has Kerberos enabled 
> or not. 
> 
> See https://issues.apache.org/jira/browse/AMBARI-18364 for an overview 
> requiring the need for this patch.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
>  ee73b8d 
>   
> ambari-server/src/main/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationProperties.java
>  PRE-CREATION 
>   
> ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
>  f429a36 
>   
> ambari-server/src/test/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationPropertiesTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51822/diff/
> 
> 
> Testing
> ---
> 
> Manually tested...
> 
> # Local test results: 
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 2:51:23.081s
> [INFO] Finished at: Tue Sep 13 00:00:59 EDT 2016
> [INFO] Final Memory: 48M/1792M
> [INFO] 
> 
> 
> # Jenkins test results: 
> 
>

Re: Review Request 51822: Add Ambari configuration options to support Kerberos token authentication

2016-09-13 Thread Robert Levas 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51822/
---

(Updated Sept. 13, 2016, 4:44 p.m.)


Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Nate Cole.


Changes
---

Addressed (some) reviewer concerns.


Bugs: AMBARI-18365
https://issues.apache.org/jira/browse/AMBARI-18365


Repository: ambari


Description
---

Add the followng Ambari configuration options to support Kerberos token 
authentication

- `authentication.kerberos.enabled`
-- Determines whether to use Kerberos (SPNEGO) authentication when connecting 
Ambari:  {{true}} to enable this feature; {{false}}, otherwise
- `authentication.kerberos.spnego.principal`
-- The Kerberos principal name to use when verifying user-supplied Kerberos 
tokens for authentication via SPNEGO
- `authentication.kerberos.spnego.keytab.file`
-- The Kerberos keytab file to use when verifying user-supplied Kerberos tokens 
for authentication via SPNEGO
- `authentication.kerberos.user.types`
-- A comma-delimited (ordered) list of preferred user types to use when finding 
the Ambari user account for the user-supplied Kerberos identity during 
authentication via SPNEGO
- `authentication.kerberos.auth_to_local.rules`
-- The auth-to-local rules set to use when translating a user's principal name 
to a local user name during authentication via SPNEGO.

NOTE: These properties are in the {{ambari.properties}} file since this feature 
may be enabled whether the rest of the cluster has Kerberos enabled or not. 

See https://issues.apache.org/jira/browse/AMBARI-18364 for an overview 
requiring the need for this patch.


Diffs (updated)
-

  
ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
 ee73b8d 
  
ambari-server/src/main/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationProperties.java
 PRE-CREATION 
  
ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
 f429a36 
  
ambari-server/src/test/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationPropertiesTest.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/51822/diff/


Testing
---

Manually tested...

# Local test results: 

[INFO] 
[INFO] BUILD SUCCESS
[INFO] 
[INFO] Total time: 2:51:23.081s
[INFO] Finished at: Tue Sep 13 00:00:59 EDT 2016
[INFO] Final Memory: 48M/1792M
[INFO] 

# Jenkins test results: 

[INFO] 
[INFO] BUILD SUCCESS
[INFO] 
[INFO] Total time: 01:34 h
[INFO] Finished at: 2016-09-13T08:46:23+00:00
[INFO] Final Memory: 178M/2672M
[INFO] 

{color:green}+1 overall{color}.  Here are the results of testing the latest 
attachment 
  
http://issues.apache.org/jira/secure/attachment/12828101/AMBARI-18365_trunk_01.patch
  against trunk revision .

{color:green}+1 @author{color}.  The patch does not contain any @author 
tags.

{color:green}+1 tests included{color}.  The patch appears to include 2 new 
or modified test files.

{color:green}+1 javac{color}.  The applied patch does not increase the 
total number of javac compiler warnings.

{color:green}+1 release audit{color}.  The applied patch does not increase 
the total number of release audit warnings.

{color:green}+1 core tests{color}.  The patch passed unit tests in 
ambari-server.

Test results: 
https://builds.apache.org/job/Ambari-trunk-test-patch/8643//testReport/
Console output: 
https://builds.apache.org/job/Ambari-trunk-test-patch/8643//console

This message is automatically generated.


Thanks,

Robert Levas

Re: Review Request 51822: Add Ambari configuration options to support Kerberos token authentication

2016-09-13 Thread Robert Levas 


> On Sept. 13, 2016, 8:29 a.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java,
> >  lines 1334-1336
> > 
> >
> > Is this specifically for SPNEGO, or any Kerberos auth?  If so, then the 
> > property should use the word "spnego" somewhere like you have for other 
> > properties.

These properties are specificly for Kerberos authentication via SPNEGO - 
*S*imple and *P*rotected GSSAPI *Nego*tiation Mechanism.  SPNEGO can be used 
for other authentication protocols, but is most commponly used for Kerberos 
authencation.


- Robert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51822/#review148701
---


On Sept. 13, 2016, 5 a.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51822/
> ---
> 
> (Updated Sept. 13, 2016, 5 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Nate 
> Cole.
> 
> 
> Bugs: AMBARI-18365
> https://issues.apache.org/jira/browse/AMBARI-18365
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Add the followng Ambari configuration options to support Kerberos token 
> authentication
> 
> - `authentication.kerberos.enabled`
> -- Determines whether to use Kerberos (SPNEGO) authentication when connecting 
> Ambari:  {{true}} to enable this feature; {{false}}, otherwise
> - `authentication.kerberos.spnego.principal`
> -- The Kerberos principal name to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.spnego.keytab.file`
> -- The Kerberos keytab file to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.user.types`
> -- A comma-delimited (ordered) list of preferred user types to use when 
> finding the Ambari user account for the user-supplied Kerberos identity 
> during authentication via SPNEGO
> - `authentication.kerberos.auth_to_local.rules`
> -- The auth-to-local rules set to use when translating a user's principal 
> name to a local user name during authentication via SPNEGO.
> 
> NOTE: These properties are in the {{ambari.properties}} file since this 
> feature may be enabled whether the rest of the cluster has Kerberos enabled 
> or not. 
> 
> See https://issues.apache.org/jira/browse/AMBARI-18364 for an overview 
> requiring the need for this patch.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
>  ee73b8d 
>   
> ambari-server/src/main/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationProperties.java
>  PRE-CREATION 
>   
> ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
>  f429a36 
>   
> ambari-server/src/test/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationPropertiesTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51822/diff/
> 
> 
> Testing
> ---
> 
> Manually tested...
> 
> # Local test results: 
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 2:51:23.081s
> [INFO] Finished at: Tue Sep 13 00:00:59 EDT 2016
> [INFO] Final Memory: 48M/1792M
> [INFO] 
> 
> 
> # Jenkins test results: 
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 01:34 h
> [INFO] Finished at: 2016-09-13T08:46:23+00:00
> [INFO] Final Memory: 178M/2672M
> [INFO] 
> 
> 
> {color:green}+1 overall{color}.  Here are the results of testing the latest 
> attachment 
>   
> http://issues.apache.org/jira/secure/attachment/12828101/AMBARI-18365_trunk_01.patch
>   against trunk revision .
> 
> {color:green}+1 @author{color}.  The patch does not contain any @author 
> tags.
> 
> {color:green}+1 tests included{color}.  The patch appears to include 2 
> new or modified test files.
> 
> {color:green}+1 javac{color}.  The applied patch does not increase the 
> total number of javac compiler warnings.
> 
> {color:green}+1 release audit{color}.  The applied patch does not 
> increase the total number of release audit warnings.
> 
> {color:green}+1 core tests{color}.  The patch

Re: Review Request 51822: Add Ambari configuration options to support Kerberos token authentication

2016-09-13 Thread Robert Levas 


> On Sept. 13, 2016, 8:24 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java,
> >  line 2586
> > 
> >
> > Although I understand the motivation, I still don't like doing this 
> > since getting the backing Properties object for the entire Configuration 
> > would be missing these properties. Can we mirror them in the normal 
> > Properties object as well?

I am not sure what you mean.  The backing properties object contains the 
properties from the ambari.properties file and is being used to fill the 
KerberosAuthenticationProperties instance.   Does this data need to be copied 
to another map?


- Robert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51822/#review148699
---


On Sept. 13, 2016, 5 a.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51822/
> ---
> 
> (Updated Sept. 13, 2016, 5 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Nate 
> Cole.
> 
> 
> Bugs: AMBARI-18365
> https://issues.apache.org/jira/browse/AMBARI-18365
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Add the followng Ambari configuration options to support Kerberos token 
> authentication
> 
> - `authentication.kerberos.enabled`
> -- Determines whether to use Kerberos (SPNEGO) authentication when connecting 
> Ambari:  {{true}} to enable this feature; {{false}}, otherwise
> - `authentication.kerberos.spnego.principal`
> -- The Kerberos principal name to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.spnego.keytab.file`
> -- The Kerberos keytab file to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.user.types`
> -- A comma-delimited (ordered) list of preferred user types to use when 
> finding the Ambari user account for the user-supplied Kerberos identity 
> during authentication via SPNEGO
> - `authentication.kerberos.auth_to_local.rules`
> -- The auth-to-local rules set to use when translating a user's principal 
> name to a local user name during authentication via SPNEGO.
> 
> NOTE: These properties are in the {{ambari.properties}} file since this 
> feature may be enabled whether the rest of the cluster has Kerberos enabled 
> or not. 
> 
> See https://issues.apache.org/jira/browse/AMBARI-18364 for an overview 
> requiring the need for this patch.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
>  ee73b8d 
>   
> ambari-server/src/main/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationProperties.java
>  PRE-CREATION 
>   
> ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
>  f429a36 
>   
> ambari-server/src/test/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationPropertiesTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51822/diff/
> 
> 
> Testing
> ---
> 
> Manually tested...
> 
> # Local test results: 
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 2:51:23.081s
> [INFO] Finished at: Tue Sep 13 00:00:59 EDT 2016
> [INFO] Final Memory: 48M/1792M
> [INFO] 
> 
> 
> # Jenkins test results: 
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 01:34 h
> [INFO] Finished at: 2016-09-13T08:46:23+00:00
> [INFO] Final Memory: 178M/2672M
> [INFO] 
> 
> 
> {color:green}+1 overall{color}.  Here are the results of testing the latest 
> attachment 
>   
> http://issues.apache.org/jira/secure/attachment/12828101/AMBARI-18365_trunk_01.patch
>   against trunk revision .
> 
> {color:green}+1 @author{color}.  The patch does not contain any @author 
> tags.
> 
> {color:green}+1 tests included{color}.  The patch appears to include 2 
> new or modified test files.
> 
> {color:green}+1 javac{color}.  The applied patch does not increase the 
> total number of javac compiler warnings.
> 
> {color:green}+1 release audit{color}.  The applied patch does not 
> increase the total number of

Re: Review Request 51822: Add Ambari configuration options to support Kerberos token authentication

2016-09-13 Thread Robert Levas 


> On Sept. 13, 2016, 8:24 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java,
> >  lines 5292-5294
> > 
> >
> > Why not just return here which would prevent you from needing to embed 
> > the rest of the logic in the if-statement

Then I would have 2 return statements... but I can make the chance since most 
developers like to short-curcuit their code.


> On Sept. 13, 2016, 8:24 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java,
> >  lines 5298-5301
> > 
> >
> > KERBEROS_AUTH_USER_TYPES has a default value, so `getProperty` will 
> > never return null.

This is a safety measure in the event someone changes the definition of 
`KERBEROS_AUTH_USER_TYPES`. Plus I generally feel the need to check for null 
and empty in these cases. But I can remove this check.


> On Sept. 13, 2016, 8:24 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java,
> >  lines 5310-5311
> > 
> >
> > Invalid types would seem to be a mis-configuration which could cause 
> > connectivity issues with Ambari; did you want to instead throw a real 
> > exception here?

ok.. that is an option.


> On Sept. 13, 2016, 8:24 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java,
> >  lines 5345-5348
> > 
> >
> > Same question as above - because these configurations involve 
> > connection to Ambari, should we instead throw exceptions and fail startup 
> > on problems like this?

ok.. will fix.


- Robert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51822/#review148699
---


On Sept. 13, 2016, 5 a.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51822/
> ---
> 
> (Updated Sept. 13, 2016, 5 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Nate 
> Cole.
> 
> 
> Bugs: AMBARI-18365
> https://issues.apache.org/jira/browse/AMBARI-18365
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Add the followng Ambari configuration options to support Kerberos token 
> authentication
> 
> - `authentication.kerberos.enabled`
> -- Determines whether to use Kerberos (SPNEGO) authentication when connecting 
> Ambari:  {{true}} to enable this feature; {{false}}, otherwise
> - `authentication.kerberos.spnego.principal`
> -- The Kerberos principal name to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.spnego.keytab.file`
> -- The Kerberos keytab file to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.user.types`
> -- A comma-delimited (ordered) list of preferred user types to use when 
> finding the Ambari user account for the user-supplied Kerberos identity 
> during authentication via SPNEGO
> - `authentication.kerberos.auth_to_local.rules`
> -- The auth-to-local rules set to use when translating a user's principal 
> name to a local user name during authentication via SPNEGO.
> 
> NOTE: These properties are in the {{ambari.properties}} file since this 
> feature may be enabled whether the rest of the cluster has Kerberos enabled 
> or not. 
> 
> See https://issues.apache.org/jira/browse/AMBARI-18364 for an overview 
> requiring the need for this patch.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
>  ee73b8d 
>   
> ambari-server/src/main/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationProperties.java
>  PRE-CREATION 
>   
> ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
>  f429a36 
>   
> ambari-server/src/test/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationPropertiesTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51822/diff/
> 
> 
> Testing
> ---
> 
> Manually tested...
> 
> # Local test results: 
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 2:51:23.081s
> [INFO] Finished at:

Re: Review Request 51822: Add Ambari configuration options to support Kerberos token authentication

2016-09-13 Thread Nate Cole 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51822/#review148701
---


Ship it!




LGTM, just confirming that if these are properties specific for SPNEGO, then I 
think we should say that for property names.


ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
 (lines 1334 - 1336)


Is this specifically for SPNEGO, or any Kerberos auth?  If so, then the 
property should use the word "spnego" somewhere like you have for other 
properties.


- Nate Cole


On Sept. 13, 2016, 5 a.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51822/
> ---
> 
> (Updated Sept. 13, 2016, 5 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Nate 
> Cole.
> 
> 
> Bugs: AMBARI-18365
> https://issues.apache.org/jira/browse/AMBARI-18365
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Add the followng Ambari configuration options to support Kerberos token 
> authentication
> 
> - `authentication.kerberos.enabled`
> -- Determines whether to use Kerberos (SPNEGO) authentication when connecting 
> Ambari:  {{true}} to enable this feature; {{false}}, otherwise
> - `authentication.kerberos.spnego.principal`
> -- The Kerberos principal name to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.spnego.keytab.file`
> -- The Kerberos keytab file to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.user.types`
> -- A comma-delimited (ordered) list of preferred user types to use when 
> finding the Ambari user account for the user-supplied Kerberos identity 
> during authentication via SPNEGO
> - `authentication.kerberos.auth_to_local.rules`
> -- The auth-to-local rules set to use when translating a user's principal 
> name to a local user name during authentication via SPNEGO.
> 
> NOTE: These properties are in the {{ambari.properties}} file since this 
> feature may be enabled whether the rest of the cluster has Kerberos enabled 
> or not. 
> 
> See https://issues.apache.org/jira/browse/AMBARI-18364 for an overview 
> requiring the need for this patch.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
>  ee73b8d 
>   
> ambari-server/src/main/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationProperties.java
>  PRE-CREATION 
>   
> ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
>  f429a36 
>   
> ambari-server/src/test/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationPropertiesTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51822/diff/
> 
> 
> Testing
> ---
> 
> Manually tested...
> 
> # Local test results: 
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 2:51:23.081s
> [INFO] Finished at: Tue Sep 13 00:00:59 EDT 2016
> [INFO] Final Memory: 48M/1792M
> [INFO] 
> 
> 
> # Jenkins test results: 
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 01:34 h
> [INFO] Finished at: 2016-09-13T08:46:23+00:00
> [INFO] Final Memory: 178M/2672M
> [INFO] 
> 
> 
> {color:green}+1 overall{color}.  Here are the results of testing the latest 
> attachment 
>   
> http://issues.apache.org/jira/secure/attachment/12828101/AMBARI-18365_trunk_01.patch
>   against trunk revision .
> 
> {color:green}+1 @author{color}.  The patch does not contain any @author 
> tags.
> 
> {color:green}+1 tests included{color}.  The patch appears to include 2 
> new or modified test files.
> 
> {color:green}+1 javac{color}.  The applied patch does not increase the 
> total number of javac compiler warnings.
> 
> {color:green}+1 release audit{color}.  The applied patch does not 
> increase the total number of release audit warnings.
> 
> {color:green}+1 core tests{color}.  The patch passed unit tests in 
> ambari-server.
> 
> Test results: 
> https://builds.apache.org/job/Ambari-trunk-test-patch/8643//testReport/
> Console output: 
>

Re: Review Request 51822: Add Ambari configuration options to support Kerberos token authentication

2016-09-13 Thread Jonathan Hurley 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51822/#review148699
---




ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
 (line 2376)


convenience



ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
 (line 2585)


authentication



ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
 (line 2586)


Although I understand the motivation, I still don't like doing this since 
getting the backing Properties object for the entire Configuration would be 
missing these properties. Can we mirror them in the normal Properties object as 
well?



ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
 (lines 5292 - 5294)


Why not just return here which would prevent you from needing to embed the 
rest of the logic in the if-statement



ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
 (lines 5298 - 5301)


KERBEROS_AUTH_USER_TYPES has a default value, so `getProperty` will never 
return null.



ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
 (lines 5310 - 5311)


Invalid types would seem to be a mis-configuration which could cause 
connectivity issues with Ambari; did you want to instead throw a real exception 
here?



ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
 (lines 5318 - 5322)


Not sure if this is possible since KERBEROS_AUTH_USER_TYPES has a default 
value.



ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
 (lines 5345 - 5348)


Same question as above - because these configurations involve connection to 
Ambari, should we instead throw exceptions and fail startup on problems like 
this?


- Jonathan Hurley


On Sept. 13, 2016, 5 a.m., Robert Levas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51822/
> ---
> 
> (Updated Sept. 13, 2016, 5 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Nate 
> Cole.
> 
> 
> Bugs: AMBARI-18365
> https://issues.apache.org/jira/browse/AMBARI-18365
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Add the followng Ambari configuration options to support Kerberos token 
> authentication
> 
> - `authentication.kerberos.enabled`
> -- Determines whether to use Kerberos (SPNEGO) authentication when connecting 
> Ambari:  {{true}} to enable this feature; {{false}}, otherwise
> - `authentication.kerberos.spnego.principal`
> -- The Kerberos principal name to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.spnego.keytab.file`
> -- The Kerberos keytab file to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.user.types`
> -- A comma-delimited (ordered) list of preferred user types to use when 
> finding the Ambari user account for the user-supplied Kerberos identity 
> during authentication via SPNEGO
> - `authentication.kerberos.auth_to_local.rules`
> -- The auth-to-local rules set to use when translating a user's principal 
> name to a local user name during authentication via SPNEGO.
> 
> NOTE: These properties are in the {{ambari.properties}} file since this 
> feature may be enabled whether the rest of the cluster has Kerberos enabled 
> or not. 
> 
> See https://issues.apache.org/jira/browse/AMBARI-18364 for an overview 
> requiring the need for this patch.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
>  ee73b8d 
>   
> ambari-server/src/main/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationProperties.java
>  PRE-CREATION 
>   
> ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
>  f429a36 
>   
> ambari-server/src/test/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationPropertiesTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51822/diff/
> 
> 
> Testing
> ---
> 
> Manually tested...
> 
> # Local test results: 
> 
> [INFO] 
>

Re: Review Request 51822: Add Ambari configuration options to support Kerberos token authentication

2016-09-13 Thread Robert Levas 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51822/
---

(Updated Sept. 13, 2016, 5 a.m.)


Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Nate Cole.


Bugs: AMBARI-18365
https://issues.apache.org/jira/browse/AMBARI-18365


Repository: ambari


Description
---

Add the followng Ambari configuration options to support Kerberos token 
authentication

- `authentication.kerberos.enabled`
-- Determines whether to use Kerberos (SPNEGO) authentication when connecting 
Ambari:  {{true}} to enable this feature; {{false}}, otherwise
- `authentication.kerberos.spnego.principal`
-- The Kerberos principal name to use when verifying user-supplied Kerberos 
tokens for authentication via SPNEGO
- `authentication.kerberos.spnego.keytab.file`
-- The Kerberos keytab file to use when verifying user-supplied Kerberos tokens 
for authentication via SPNEGO
- `authentication.kerberos.user.types`
-- A comma-delimited (ordered) list of preferred user types to use when finding 
the Ambari user account for the user-supplied Kerberos identity during 
authentication via SPNEGO
- `authentication.kerberos.auth_to_local.rules`
-- The auth-to-local rules set to use when translating a user's principal name 
to a local user name during authentication via SPNEGO.

NOTE: These properties are in the {{ambari.properties}} file since this feature 
may be enabled whether the rest of the cluster has Kerberos enabled or not. 

See https://issues.apache.org/jira/browse/AMBARI-18364 for an overview 
requiring the need for this patch.


Diffs
-

  
ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
 ee73b8d 
  
ambari-server/src/main/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationProperties.java
 PRE-CREATION 
  
ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
 f429a36 
  
ambari-server/src/test/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationPropertiesTest.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/51822/diff/


Testing (updated)
---

Manually tested...

# Local test results: 

[INFO] 
[INFO] BUILD SUCCESS
[INFO] 
[INFO] Total time: 2:51:23.081s
[INFO] Finished at: Tue Sep 13 00:00:59 EDT 2016
[INFO] Final Memory: 48M/1792M
[INFO] 

# Jenkins test results: 

[INFO] 
[INFO] BUILD SUCCESS
[INFO] 
[INFO] Total time: 01:34 h
[INFO] Finished at: 2016-09-13T08:46:23+00:00
[INFO] Final Memory: 178M/2672M
[INFO] 

{color:green}+1 overall{color}.  Here are the results of testing the latest 
attachment 
  
http://issues.apache.org/jira/secure/attachment/12828101/AMBARI-18365_trunk_01.patch
  against trunk revision .

{color:green}+1 @author{color}.  The patch does not contain any @author 
tags.

{color:green}+1 tests included{color}.  The patch appears to include 2 new 
or modified test files.

{color:green}+1 javac{color}.  The applied patch does not increase the 
total number of javac compiler warnings.

{color:green}+1 release audit{color}.  The applied patch does not increase 
the total number of release audit warnings.

{color:green}+1 core tests{color}.  The patch passed unit tests in 
ambari-server.

Test results: 
https://builds.apache.org/job/Ambari-trunk-test-patch/8643//testReport/
Console output: 
https://builds.apache.org/job/Ambari-trunk-test-patch/8643//console

This message is automatically generated.


Thanks,

Robert Levas

Re: Review Request 51822: Add Ambari configuration options to support Kerberos token authentication

2016-09-13 Thread Robert Levas 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51822/
---

(Updated Sept. 13, 2016, 4:59 a.m.)


Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Nate Cole.


Bugs: AMBARI-18365
https://issues.apache.org/jira/browse/AMBARI-18365


Repository: ambari


Description
---

Add the followng Ambari configuration options to support Kerberos token 
authentication

- `authentication.kerberos.enabled`
-- Determines whether to use Kerberos (SPNEGO) authentication when connecting 
Ambari:  {{true}} to enable this feature; {{false}}, otherwise
- `authentication.kerberos.spnego.principal`
-- The Kerberos principal name to use when verifying user-supplied Kerberos 
tokens for authentication via SPNEGO
- `authentication.kerberos.spnego.keytab.file`
-- The Kerberos keytab file to use when verifying user-supplied Kerberos tokens 
for authentication via SPNEGO
- `authentication.kerberos.user.types`
-- A comma-delimited (ordered) list of preferred user types to use when finding 
the Ambari user account for the user-supplied Kerberos identity during 
authentication via SPNEGO
- `authentication.kerberos.auth_to_local.rules`
-- The auth-to-local rules set to use when translating a user's principal name 
to a local user name during authentication via SPNEGO.

NOTE: These properties are in the {{ambari.properties}} file since this feature 
may be enabled whether the rest of the cluster has Kerberos enabled or not. 

See https://issues.apache.org/jira/browse/AMBARI-18364 for an overview 
requiring the need for this patch.


Diffs
-

  
ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
 ee73b8d 
  
ambari-server/src/main/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationProperties.java
 PRE-CREATION 
  
ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
 f429a36 
  
ambari-server/src/test/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationPropertiesTest.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/51822/diff/


Testing (updated)
---

Manually tested...

# Local test results: 

[INFO] 
[INFO] BUILD SUCCESS
[INFO] 
[INFO] Total time: 2:51:23.081s
[INFO] Finished at: Tue Sep 13 00:00:59 EDT 2016
[INFO] Final Memory: 48M/1792M
[INFO] 

# Jenkins test results: PENDING


Thanks,

Robert Levas