[
https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127159#comment-13127159
]
[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?
bq.
bq. Stanton Sievers wrote:
bq. Thanks for the review Jesse.
bq.
bq. 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.
bq.
bq. 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?
bq.
bq. 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.
bq.
bq. 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.
bq.
bq. I hope all of that makes sense. :) Let me know if you have any
questions or concerns.
bq.
bq. Jesse Ciancetta wrote:
bq. I guess what I meant by heavyweight was more about the extra code
kicking around Shindig as opposed to what implementers would have to do to
override it with a custom implementation. I think for many (most?) deployments
the token key is just a static value that maybe changes on some preset interval
or as needed in an emergency situation (like a key compromise) -- so having a
specific provider for managing that (likely) static value just seems
heavyweight to me.
bq.
bq. So then I started thinking about how your new provider could be made
more generically useful so it wasn't specific to just token keys and started
looking at it as more of a generic configuration provider -- which led me back
to ContainerConfig since that what it already does... :)
bq.
bq. I'm not sure how clunky it would actually be to go the ContainerConfig
approach -- I'd envision a ContainerConfig implementation that loaded default
values from the container.js file and then called on some component of your own
that does whatever your KeyProvider does now to load the token key -- and then
whenever you need to change the key that component would just alert your custom
ContainerConfig through some kind of callback and in turn the ContainerConfig
could alert all the observers to the change via the existing ConfigObserver API.
bq.
bq. The only bit I see that wouldn’t work exactly as you have things now
is the key changed event -- you'd only be able to get access to the new key in
your ConfigObserver's, but it seems you'd already have a reference to the old
key in a local variable somewhere anyway so it doesn’t seem like you'd need it.
bq.
bq. If you still think this isn’t the right direction for your needs
that's fine though -- just let me know and I'll look over the code and give you
some feedback on it as is.
Honestly, the generic ContainerConfig approach just doesn't seem right to me.
The config should be there to support the implementation, not the other way
around.
I also don't like how tightly coupled the SecurityToken code is to the
configuration and this KeyProvider would remedy that. Someone could also put
the key in a database (or anywhere else) but they would have to Inject their
own implemenations of the SecurityTokenCodec and maybe even the Crypters.
Again, I'm not against the ContainerConfig approach in general. I could store
all of my config in a database and inject my own ContainerConfig to enable
that. Great. I just don't think it works in this case since the BlobCrypter*
code is so tightly coupled to the fact that the key is in a file. I'm not
trying to change the way we load the config, per se, I'm just trying to
decouple the security code from the location of the key.
If you can look through the code and give some feedback that would be great. I
can then add the dev list and get the larger audience's feedback.
- 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
