aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kevin Sweeney" <kevi...@apache.org>
Subject Re: Review Request 32559: Add Kerberos support to the scheduler
Date Fri, 27 Mar 2015 19:38:40 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message