[
https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13126733#comment-13126733
]
[email protected] commented on SHINDIG-1636:
--------------------------------------------------------
bq. On 2011-10-13 16:30:18, Jesse Ciancetta wrote:
bq. > Thinking through this a bit I'm feeling like this strategy might be a
little too specific and heavyweight IMO to achieve the higher level goal of
generically loading a bit of container configuration from an arbitrary
location. I'm wondering if you can't take a slightly higher level approach and
come out with a more generically applicable solution in the end.
bq. >
bq. > For the sake of simplicity (so I don't have to continue to refer to "an
arbitrary location") let's assume the arbitrary location is a database -- so
right now you're just looking to load the security token key from a database,
but what if in the future you want to load 10 other container config properties
from the database too? One way you might accomplish that now would be to
provide a custom implementation of the ContainerConfig interface which extends
the default JsonContainerConfig implementation -- and in your custom
implementation maybe you load those 10 properties from your database at
initialization time and use those values to overwrite the values pulled from
container.js.
bq. >
bq. > Of course that doesn't help you now with the token encryption key
because the config currently returns the location of the key file -- but if the
config was changed to return the actual key value instead then you could
override that value with the approach I outlined above.
bq. >
bq. > You wouldn't want to break existing users who expect to be able to load
from a file or the classpath though or make them hard code their key into
container.js -- so I think to work around that we make the machinery that loads
the data from container.js smart enough to recognize the res:// and file://
prefixes, and for those cases it could use the Shindig ResourceLoader to load
the actual value from a file.
bq. >
bq. > So putting that all together I'm thinking we could change the token key
configuration in container.js from this:
bq. >
bq. > "gadgets.securityTokenKeyFile" : "/path/to/key/file.txt",
bq. >
bq. > to something like this:
bq. >
bq. > "gadgets.securityTokenKey" : "file://path/to/key/file.txt",
bq. >
bq. > make the loader smart enough to resolve file:// and res:// into actual
values, change the security token Java code to expect to pull the actual key
value from ContainerConfig instead of a key file location, and finally, you
could implement and Guice inject the custom ContainerConfig I was talking about
which would load the key value from the database to override the value read
from container.js.
bq. >
bq. > Thinking through what would be required to make all this work really
doesn't seem like it would be too difficult and I think you end up with
something cleaner and simpler to deal with -- plus you're already all setup
then to pull other container config values from the database when you need to.
bq. >
bq. > What do you think?
Thanks for the review Jesse.
I'm not convinced that your approach is really any more lightweight than
implementing your own KeyProvider. You're still requiring the developer to
inject their own ContainerConfig implementation, which I think would be just as
much work as implementing a KeyProvider.
I do see your point that someone may want to load other container config
properties from a database, and that's not really something I had thought about
when proposing these changes. Do you think this is common requirement?
One advantage the KeyProvider approach has is that you could make a KeyProvider
that generates a new key automatically on startup or even periodically. That
way the administrator doesn't need to worry about managing keys. You might
also imagine a scenario where a key is compromised or a security token is
compromised (how you would determine this, I'll leave to the imagination :) and
your KeyProvider implementation has a built in mechanism to purge keys and
create a new one. I think this type of key management would be a little
clunkier to implement with a ContainerConfig implementation.
I'm not opposed to the idea of loading arbitrary container config properties in
a general way (as you described), but it's just not the problem I'm trying to
solve here. It might be worth asking the dev list and getting a larger
audience's eyes on that idea.
I hope all of that makes sense. :) Let me know if you have any questions or
concerns.
- Stanton
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2552
-----------------------------------------------------------
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
