> On March 27, 2015, 11:09 a.m., Joshua Cohen wrote:
> > examples/vagrant/provision-dev-cluster.sh, line 20
> > <https://reviews.apache.org/r/32559/diff/2/?file=907384#file907384line20>
> >
> >     Is this needed to build kerberos in the test script? Do we need to 
> > build it from source? Can we just apt-get install kerb directly?

We build from source to get access to the "make testrealm" target. Installing 
the OS one would require more configuration (as it's intended for a production 
deployment). I'll drop a TODO.


> On March 27, 2015, 11:09 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizeHeaderToken.java,
> >  lines 31-32
> > <https://reviews.apache.org/r/32559/diff/2/?file=907387#file907387line31>
> >
> >     These don't appear to be used elsewhere, make them private?

Good catch, done.


> On March 27, 2015, 11:09 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizeHeaderToken.java,
> >  line 38
> > <https://reviews.apache.org/r/32559/diff/2/?file=907387#file907387line38>
> >
> >     This exception is caught in the servlet filter and its message is 
> > logged, so it would probably be good to include a message here when we 
> > throw it?

Added.


> On March 27, 2015, 11:09 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5Realm.java,
> >  lines 82-83
> > <https://reviews.apache.org/r/32559/diff/2/?file=907389#file907389line82>
> >
> >     Fits on a single line.

Nope, semicolon is in column 101.


> On March 27, 2015, 11:09 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java,
> >  line 47
> > <https://reviews.apache.org/r/32559/diff/2/?file=907390#file907390line47>
> >
> >     Fix.

Oops (fixed).


> On March 27, 2015, 11:09 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java,
> >  line 113
> > <https://reviews.apache.org/r/32559/diff/2/?file=907390#file907390line113>
> >
> >     I feel like it'd be better to just let people specify a path to their 
> > jaas config on the command line rather than specifying the individual 
> > pieces so that we can generate a config for them. That way they'd have 
> > control over the pieces we don't expose as command line args (e.g. debug 
> > on/off, etc.).

I buy debug (I'll add a debug=$debug$ line), but I feel like JAAS is 
sufficiently complex that we want to insulate our users from its intricasies. I 
*really* wish there was a `GssCredential.fromKeyTab(byte[])` method that would 
let us avoid this dance altogether.


> On March 27, 2015, 11:09 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java,
> >  line 132
> > <https://reviews.apache.org/r/32559/diff/2/?file=907390#file907390line132>
> >
> >     If we do decide to stick with generating the jaas conf it might be 
> > useful to either write out the path to the generated file or write out the 
> > contents of the file to the log so that operators can inspect the config?

This line writes the generated config.


> On March 27, 2015, 11:09 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroUtils.java,
> >  line 34
> > <https://reviews.apache.org/r/32559/diff/2/?file=907393#file907393line34>
> >
> >     Fill in.

Doc'd


> On March 27, 2015, 11:09 a.m., Joshua Cohen wrote:
> > src/test/java/org/apache/aurora/scheduler/http/api/security/KerberosPrincipalParserTest.java,
> >  line 20
> > <https://reviews.apache.org/r/32559/diff/2/?file=907397#file907397line20>
> >
> >     Add a negative test case?

Done.


> On March 27, 2015, 11:09 a.m., Joshua Cohen wrote:
> > src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilterTest.java,
> >  line 42
> > <https://reviews.apache.org/r/32559/diff/2/?file=907398#file907398line42>
> >
> >     It feels like most of this stuff can be tested directly with a mock 
> > request/response/chain rather than starting up an http server.
> >     
> >     I think it's good to have integration tests for this as well, but we 
> > can probably simplify those to test the happy/sad path for integration 
> > tests and save testing all the corners for the unit test with mocks.

As written the class under test has 100% coverage with this integration test. 
(I actually found the IT easier to write since I have access to ClientBuilder, 
rather than mocking HttpServletRequest/HttpServletResponse methods)


> On March 27, 2015, 11:09 a.m., Joshua Cohen wrote:
> > src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilterTest.java,
> >  line 54
> > <https://reviews.apache.org/r/32559/diff/2/?file=907398#file907398line54>
> >
> >     kill

done.


> On March 27, 2015, 11:09 a.m., Joshua Cohen wrote:
> > src/test/sh/org/apache/aurora/e2e/test_kerberos_end_to_end.sh, lines 71-72
> > <https://reviews.apache.org/r/32559/diff/2/?file=907399#file907399line71>
> >
> >     We should undo this change after running this test, otherwise 
> > subsequent end to end test runs will fail?
> >     
> >     Or alternately just set up a separate service for scheduler w/ kerberos 
> > and stop the normal scheduler, start the kerb one, run tests, then 
> > stop/start in reverse?

Good suggestion, I created a separate upstart job.


> On March 27, 2015, 11:09 a.m., Joshua Cohen wrote:
> > src/test/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModuleTest.java,
> >  line 36
> > <https://reviews.apache.org/r/32559/diff/2/?file=907396#file907396line36>
> >
> >     It feels like we're missing a few test cases here. If we keep it, we 
> > should test the jaas config generation, we should test that the system 
> > property is restored, etc.

Most of the stuff in this class is glue that's really difficult to test without 
PowerMock (static method and constructor mocking). Since an error in this 
module will cause a failure immediately at application startup I think we're 
okay sacrificing a bit of test coverage here. One test I'd want to write, for 
example, is that gssManager.createCredential is called within a 
PrivilegedAction (where there's a threadlocal that makes it work), but not 
seeing a straightforward way to test that given the way the API works.


- Kevin


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


On March 26, 2015, 6:23 p.m., Kevin Sweeney wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32559/
> -----------------------------------------------------------
> 
> (Updated March 26, 2015, 6:23 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-812
>     https://issues.apache.org/jira/browse/AURORA-812
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Support authenticating to the scheduler API with Kerberos.
> 
> 
> Diffs
> -----
> 
>   config/findbugs/excludeFilter.xml 5ff5f87d6bb7d8bf3d9ecb1bc6c6b1a524d1bf15 
>   examples/vagrant/provision-dev-cluster.sh 
> ae500436e703140065e5c16fc0e38dbe3214e69f 
>   examples/vagrant/upstart/aurora-scheduler-kerberos.conf PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java
>  ec6a02c4086ee0d5a7529083030d978ea889f677 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizeHeaderToken.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/IniShiroRealmModule.java
>  58b559e96402ef282dc0e5ac2de0930906256487 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5Realm.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/KerberosPrincipalParser.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilter.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroUtils.java 
> PRE-CREATION 
>   
> src/main/resources/org/apache/aurora/scheduler/http/api/security/jaas.conf.st 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/AuthorizeHeaderTokenTest.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModuleTest.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/KerberosPrincipalParserTest.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilterTest.java
>  PRE-CREATION 
>   src/test/sh/org/apache/aurora/e2e/test_kerberos_end_to_end.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/32559/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/test_kerberos_end_to_end.sh
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Reply via email to