[
https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127674#comment-13127674
]
[email protected] 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
