[
https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127529#comment-13127529
]
[email protected] commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2581
-----------------------------------------------------------
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.
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. :-)
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java
<https://reviews.apache.org/r/2362/#comment5789>
The current impl does all of the work of creating crypters in the
constructor or during configuration change events, but your new impl seems to
introduce the possibility of crypters being created on request threads. Is
there any reason you really want to do that? Also the old impl used to remove
crypters when their associated container was removed -- your new impl doesn't
seem to do this. The changes to the way crypters are created/managed dont seem
like they are needed to me -- is there something I'm missing?
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java
<https://reviews.apache.org/r/2362/#comment5790>
Do we really want to be loading crypters on request threads as opposed to
just pulling them from the crypter map?
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java
<https://reviews.apache.org/r/2362/#comment5791>
Do we really want to be loading crypters on request threads as opposed to
just pulling them from the crypter map?
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java
<https://reviews.apache.org/r/2362/#comment5786>
There are a bunch of potential concurrency issues in this class -- I've
noted a few of them but probably haven't covered them all. You could probably
fix them by making many of the methods synchronized, which is probably fine
since those methods would likely not be used very often (only when things
change) and the method that might get used often (getKey) could be left
unsynchronized as long as its underlying map was threadsafe.
Or you could leave it as is and note in the API docs that it is not
threadsafe -- although I think I'd try to make it safe.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java
<https://reviews.apache.org/r/2362/#comment5785>
This should probably be a ConcurrentHashMap since it could be read
from/written to by different threads.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java
<https://reviews.apache.org/r/2362/#comment5782>
This isn't thread safe as is currently implemented - two callers trying to
add a listener for container "foo" at the same time could result in a race
condition where a new listener list is created for each call and the last call
would win. Probably not a likely scenario, but since this is part of a public
API who's really to say... So I think I'd make the entire method synchronized
just to be safe.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java
<https://reviews.apache.org/r/2362/#comment5783>
I think this is supposed to be a break, not a return.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java
<https://reviews.apache.org/r/2362/#comment5784>
I think there could be concurrency issues here too since we're iterating
over a list that could be added to on another thread elsewhere.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyProvider.java
<https://reviews.apache.org/r/2362/#comment5787>
This interface looks suspiciously akin to the ContainerConfig interface to
me. This interface allows callers to load a per-container string of
configuration data and supports notifying callers when it changes. That is
*exactly* the same thing that ContainerConfig does.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java
<https://reviews.apache.org/r/2362/#comment5788>
Calling init on a currently-in-use crypter instance (which will in turn
regenerate the cipher and hmac keys) probably isn't threadsafe. This would
probably only break encrypt/decrypt calls for a very brief period of time, but
could break them nonetheless. I think the previous implementation managed the
key changes higher up the stack (in the BlobCrypterSecurityTokenCodec class)
and would create entirely new crypters and swap them out rather than try to
change an existing one which seems like a safer strategy.
- Jesse
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
