shindig-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide an encryption key to the SecurityToken workflow
Date Fri, 14 Oct 2011 16:32:12 GMT

    [ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127674#comment-13127674
] 

jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------



bq.  On 2011-10-14 13:20:28, Jesse Ciancetta wrote:
bq.  > I think I'm likely doing a poor job of conveying my ideas around the ContainerConfig
stuff because I think we're in agreement on the key points.  You had said that your really
trying to decouple the security code from the location of the key, and I'm in 100% agreement
with that, and what I had proposed with the ContainerConfig stuff would absolutely do that
(the key piece being that callers to ContainerConfig to get the value for the token key would
now get back the actual key value as opposed to a pointer to a key file or classpath resource).
 If I'm missing something though and you can see that it really wouldn't please feel free
to point it out.
bq.  > 
bq.  > For the review as is -- I've taken a pass through the code and noted the issues
that I've seen.  I hope this stuff isn't nitpicky -- I'm honestly not trying to be a pain
or just having sour grapes about the difference in opinions on the best strategy (I think
discussions like this are perfectly fine and healthy to have) -- I'm just calling what I see.
 Hopefully its useful.  Please feel free to tell me it's nitpicky.  :-)

Not nit-picky at all.  All of the concurrency issues are valid and I'm glad you brought them
up.

I think most of the problems here can be solved by removing the KeyProvider reference from
BasicBlobCrypter.  Basically, BasicBlobCrypter would exist as it did pre-patch.  BlobCrypterSecurityTokenCodec
would manage the BlobCrypters as it did before by listening for key changes instead of container
config changes.  And that sort of proves your point. :)  Why not just stick with ContainerConfig
in this case?  The only change would then be to introduce the changes for backwards compatibility
into BlobCrypterSecurityTokenCodec so it can read both a key and keyfile from the config.

The "hybrid" ContainerConfig still feels weird to me.  And maybe it's just because of the
need for backwards compatbility and probably doing it with some conditional logic in BCSTC.
 The KeyProvider removes the need for that sort of awkward logic.

How did you envision the ContainerConfig being implemented?  How does it know what values
to read from another source and which ones to read from the container.js?

And now that I think more about it, the key file container configuration param was probably
introduced to move the key out of the container.js.  Now we're sticking it back in there but
"not really"?  Interesting how code evolves. :)


- Stanton


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


On 2011-10-13 14:19:32, Stanton Sievers wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/2362/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2011-10-13 14:19:32)
bq.  
bq.  
bq.  Review request for Ryan Baxter, Dan Dumont and Jesse Ciancetta.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  Currently, org.apache.shindig.auth.BlobCrypterSecurityTokenCodec.loadContainers(ContainerConfig,
Collection<String>, Map<String, BlobCrypter>, Map<String, String>) reads
an encryption key from a keyfile to instantiate the BlobCrypter. The keyfile is defined in
the container.js. An improvement to this behavior would be to provide an injectable KeyProvider
class that can return the key. This would allow the key to reside anywhere instead of in a
static keyfile. 
bq.  
bq.  Initial review to Dan, Ryan, and Jesse.  Once we've decided that this seems like a rational
approach, I'll add the dev list.
bq.  
bq.  
bq.  This addresses bug SHINDIG-1636.
bq.      https://issues.apache.org/jira/browse/SHINDIG-1636
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java
1182570 
bq.    http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java
1182570 
bq.    http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java
PRE-CREATION 
bq.    http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyProvider.java
PRE-CREATION 
bq.    http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java
1182570 
bq.    http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java
1182570 
bq.    http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java
1182570 
bq.    http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/KeyFileKeyProviderTest.java
PRE-CREATION 
bq.  
bq.  Diff: https://reviews.apache.org/r/2362/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  Updated and ran existing JUnits.  
bq.  Created new JUnits for the new KeyFileKeyProvider.  
bq.  Performed manual functional tests with encrypted security tokens in the sample common
container.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Stanton
bq.  
bq.


                
> Create a KeyProvider to provide an encryption key to the SecurityToken workflow
> -------------------------------------------------------------------------------
>
>                 Key: SHINDIG-1636
>                 URL: https://issues.apache.org/jira/browse/SHINDIG-1636
>             Project: Shindig
>          Issue Type: Improvement
>          Components: Java
>            Reporter: Stanton Sievers
>   Original Estimate: 48h
>  Remaining Estimate: 48h
>
> Currently, org.apache.shindig.auth.BlobCrypterSecurityTokenCodec.loadContainers(ContainerConfig,
Collection<String>, Map<String, BlobCrypter>, Map<String, String>) reads
an encryption key from a keyfile to instantiate the BlobCrypter.  The keyfile is defined in
the container.js.  An improvement to this behavior would be to provide an injectable KeyProvider
class that can return the key.  This would allow the key to reside anywhere instead of in
a static keyfile.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message