You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@shindig.apache.org by "Stanton Sievers (Created) (JIRA)" <ji...@apache.org> on 2011/10/06 22:43:29 UTC
[jira] [Created] (SHINDIG-1636) Create a KeyProvider to provide an
encryption key to the SecurityToken workflow
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
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13132988#comment-13132988 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2750
-----------------------------------------------------------
I've reviewed the code and provided feedback on a few items, but other than those I think the code is ok. Although I will say that the interactions between the ValueProvider and its collaborators feel a bit complicated to me and I'm a little concerned that I'm not fully understanding whats supposed to be happening where/when -- but as long as Dan and Stanton understand it that's good enough for me.
However I still feel like this is really complicated and heavy-weight for doing pretty much what ContainerConfig already does today. The only things I see that are added over what ContainerConfig does now are providing the old value during the change event and providing strong types -- but everything else appears to be essentially just re-implementing a bunch of the existing functionality. And we still have a full standalone interface (KeyProvider) and implementation (KeyFileKeyProvider) for something that is only ever going to be used in one place. And it doesn't cleanly replace ContainerConfig -- so now classes that want to use this strategy for configuration need to take ContainerConfig and an arbitrary configuration-type interface (in this case KeyProvider but in other cases it would be different). I'm not saying I'd actually want to abstract ContainerConfig away though -- I'm just pointing out that now we have to provide two configuration-providing components to those who want to use this new strategy which seems sub-optimal to me.
ContainerConfig is already in fairly wide usage throughout the codebase. If we wanted to reuse this new strategy in all the places ContainerConfig is used we'd end up with something like 30ish more KeyProvider and KeyFileKeyProvider-like (but not exactly the same) interfaces and implementations.
Is the strong typing and old value being passed on change a dealbreaker for you guys? It seems configuration of a component is usually a handful of primitive values, or at most a structured set of primitive values (JSON object?) so it doesn't seem that strong typing would really be critical. And it seems the configuration-consuming impls themselves would have the old values available to them internally if they really needed them. IMO I still think doing something like the prototype I had put together where we're passing configuration contributors to the ContainerConfig is a simpler/cleaner/more generic way to go. I know there were still some questions about being able to contribute multiple configuration values but I think we could solve those pretty easily in a number of different ways. Are there other reasons why you guys don't want to go down that path?
Sorry for being the squeaky wheel on this set of changes. I was really hoping that I'd open up these change sets and it would all just make sense to me and be the right thing so I could just say "ship it!" and move on... It's really really hard for me to continue saying "I don't get it" (cause I'm afraid I'm missing something obvious and I don't want to hold you guys up), but, unfortunately, I still don't get it.
If you guys feel strongly though that this is the right way to go please feel free to go ahead and open it up to the community to get additional feedback on your new strategy.
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/#comment6191>
Arrays.asList is a varargs method -- no need to wrap the parameters in an array
- Jesse
On 2011-10-21 16:23:00, 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-21 16:23:00)
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/BasicSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1187375
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1187375
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
[jira] [Commented] (SHINDIG-1636) Create a mechanism to provide an
encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13142302#comment-13142302 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
bq. On 2011-11-02 00:28:45, Ryan Baxter wrote:
bq. > One small nit, otherwise LGTM
Committed revision 1196683.
- Ryan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2648/#review3003
-----------------------------------------------------------
On 2011-11-01 18:12:28, Stanton Sievers wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2648/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-11-01 18:12:28)
bq.
bq.
bq. Review request for shindig, Ryan Baxter and Dan Dumont.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. This patch is to illustrate the minimal set of changes needed to allow the BlobCrypterSecurityTokenCodec to have a key provided by the config instead of needing a key file to be provided.
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1195457
bq. http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1195457
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1195457
bq.
bq. Diff: https://reviews.apache.org/r/2648/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Updated existing JUnits and added one more.
bq.
bq.
bq. Thanks,
bq.
bq. Stanton
bq.
bq.
> Create a mechanism 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
> Affects Versions: 3.0.0
> Reporter: Stanton Sievers
> Attachments: MinimalSecurityChangesPatchv2.txt
>
> 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.
> Update:
> The old approach was to provide a KeyProvider class but that turned out to be a little too heavy and there was some contention over the best implementation. Until there is a consensus on the best way to implement that abstraction, we can simply add another config value to the container.js that is the key itself and have the codec read and use that value if it exists.
--
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
[jira] [Commented] (SHINDIG-1636) Create a mechanism to provide an
encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13141514#comment-13141514 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
bq. On 2011-11-01 19:00:41, Jesse Ciancetta wrote:
bq. > This looks good to me, however if we're going to go this route I wouldn't mind seeing the BlobCrypterSecurityTokenCodec changed to always expect to get the actual token key from ContainerConfig -- and then making the code that parses the container.js file smart enough to be able to resolve res://key-file or file:///path/to/key-file automatically (or if not prefixed to take the value from container.js as is). I think this would work well for this particular case and would be a nice addition to the ContainerConfig code.
bq. >
bq. > It's pretty straight forward to add that support to ContainerConfig and I've got a patch that does that which I could post if people are interested -- I'd need to add some additional unit tests around the enhancements to the ContainerConfig loader but other than that it should be good to go.
bq. >
bq. > Or you could commit this as is and I could post that as a follow on patch at a later date and if people like it we could move to it. Either way is fine with me.
That was something I was thinking we could tackle later. I'm just trying to identify the minimal amount of changes that will unblock my use case and then we can focus on making it better.
- Stanton
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2648/#review2993
-----------------------------------------------------------
On 2011-11-01 18:12:28, Stanton Sievers wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2648/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-11-01 18:12:28)
bq.
bq.
bq. Review request for shindig, Ryan Baxter and Dan Dumont.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. This patch is to illustrate the minimal set of changes needed to allow the BlobCrypterSecurityTokenCodec to have a key provided by the config instead of needing a key file to be provided.
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1195457
bq. http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1195457
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1195457
bq.
bq. Diff: https://reviews.apache.org/r/2648/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Updated existing JUnits and added one more.
bq.
bq.
bq. Thanks,
bq.
bq. Stanton
bq.
bq.
> Create a mechanism 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.
> Update:
> The old approach was to provide a KeyProvider class but that turned out to be a little too heavy and there was some contention over the best implementation. Until there is a consensus on the best way to implement that abstraction, we can simply add another config value to the container.js that is the key itself and have the codec read and use that value if it exists.
--
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
[jira] [Commented] (SHINDIG-1636) Create a mechanism to provide an
encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13141343#comment-13141343 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2648/
-----------------------------------------------------------
(Updated 2011-11-01 17:14:11.549968)
Review request for Ryan Baxter and Dan Dumont.
Summary
-------
This patch is to illustrate the minimal set of changes needed to allow the BlobCrypterSecurityTokenCodec to have a key provided by the config instead of needing a key file to be provided.
This addresses bug SHINDIG-1636.
https://issues.apache.org/jira/browse/SHINDIG-1636
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1195457
http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1195457
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1195457
Diff: https://reviews.apache.org/r/2648/diff
Testing
-------
None right now. Updated JUnits so they'd pass
Thanks,
Stanton
> Create a mechanism 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.
> Update:
> The old approach was to provide a KeyProvider class but that turned out to be a little too heavy and there was some contention over the best implementation. Until there is a consensus on the best way to implement that abstraction, we can simply add another config value to the container.js that is the key itself and have the codec read and use that value if it exists.
--
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
[jira] [Commented] (SHINDIG-1636) Create a mechanism to provide an
encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13141417#comment-13141417 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2648/
-----------------------------------------------------------
(Updated 2011-11-01 18:12:28.326279)
Review request for shindig, Ryan Baxter and Dan Dumont.
Summary
-------
This patch is to illustrate the minimal set of changes needed to allow the BlobCrypterSecurityTokenCodec to have a key provided by the config instead of needing a key file to be provided.
This addresses bug SHINDIG-1636.
https://issues.apache.org/jira/browse/SHINDIG-1636
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1195457
http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1195457
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1195457
Diff: https://reviews.apache.org/r/2648/diff
Testing (updated)
-------
Updated existing JUnits and added one more.
Thanks,
Stanton
> Create a mechanism 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.
> Update:
> The old approach was to provide a KeyProvider class but that turned out to be a little too heavy and there was some contention over the best implementation. Until there is a consensus on the best way to implement that abstraction, we can simply add another config value to the container.js that is the key itself and have the codec read and use that value if it exists.
--
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
[jira] [Commented] (SHINDIG-1636) Create a mechanism to provide an
encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13142107#comment-13142107 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
bq. On 2011-11-01 19:00:41, Jesse Ciancetta wrote:
bq. > This looks good to me, however if we're going to go this route I wouldn't mind seeing the BlobCrypterSecurityTokenCodec changed to always expect to get the actual token key from ContainerConfig -- and then making the code that parses the container.js file smart enough to be able to resolve res://key-file or file:///path/to/key-file automatically (or if not prefixed to take the value from container.js as is). I think this would work well for this particular case and would be a nice addition to the ContainerConfig code.
bq. >
bq. > It's pretty straight forward to add that support to ContainerConfig and I've got a patch that does that which I could post if people are interested -- I'd need to add some additional unit tests around the enhancements to the ContainerConfig loader but other than that it should be good to go.
bq. >
bq. > Or you could commit this as is and I could post that as a follow on patch at a later date and if people like it we could move to it. Either way is fine with me.
bq.
bq. Stanton Sievers wrote:
bq. That was something I was thinking we could tackle later. I'm just trying to identify the minimal amount of changes that will unblock my use case and then we can focus on making it better.
Sounds good -- I'll try to post a follow on patch to implement the enhancements to ContainerConfig shortly after this one goes in.
- Jesse
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2648/#review2993
-----------------------------------------------------------
On 2011-11-01 18:12:28, Stanton Sievers wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2648/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-11-01 18:12:28)
bq.
bq.
bq. Review request for shindig, Ryan Baxter and Dan Dumont.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. This patch is to illustrate the minimal set of changes needed to allow the BlobCrypterSecurityTokenCodec to have a key provided by the config instead of needing a key file to be provided.
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1195457
bq. http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1195457
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1195457
bq.
bq. Diff: https://reviews.apache.org/r/2648/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Updated existing JUnits and added one more.
bq.
bq.
bq. Thanks,
bq.
bq. Stanton
bq.
bq.
> Create a mechanism 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.
> Update:
> The old approach was to provide a KeyProvider class but that turned out to be a little too heavy and there was some contention over the best implementation. Until there is a consensus on the best way to implement that abstraction, we can simply add another config value to the container.js that is the key itself and have the codec read and use that value if it exists.
--
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134443#comment-13134443 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/
-----------------------------------------------------------
(Updated 2011-10-24 20:26:34.157165)
Review request for shindig, Ryan Baxter, Dan Dumont, and Jesse Ciancetta.
Changes
-------
Adding the dev list. Pardon the large backlog of discussion. I wanted to make sure this approach was sane before getting everyone involved.
Summary
-------
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.
Initial review to Dan, Ryan, and Jesse. Once we've decided that this seems like a rational approach, I'll add the dev list.
This patch depends on https://reviews.apache.org/r/2467/
This addresses bug SHINDIG-1636.
https://issues.apache.org/jira/browse/SHINDIG-1636
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BasicSecurityTokenCodec.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/KeyFileKeyProviderTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/2362/diff
Testing
-------
Updated and ran existing JUnits.
Created new JUnits for the new KeyFileKeyProvider.
Performed manual functional tests with encrypted security tokens in the sample common container.
Thanks,
Stanton
> 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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13126733#comment-13126733 ]
jiraposter@reviews.apache.org 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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127102#comment-13127102 ]
jiraposter@reviews.apache.org 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.
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.
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... :)
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.
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.
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.
- Jesse
-----------------------------------------------------------
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13126072#comment-13126072 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/
-----------------------------------------------------------
Review request for Ryan Baxter, Dan Dumont and Jesse Ciancetta.
Summary
-------
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.
Initial review to Dan, Ryan, and Jesse. Once we've decided that this seems like a rational approach, I'll add the dev list.
This addresses bug SHINDIG-1636.
https://issues.apache.org/jira/browse/SHINDIG-1636
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1182524
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1182524
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java 1182524
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1182524
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1182524
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/KeyFileKeyProviderTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/2362/diff
Testing
-------
Updated and ran existing JUnits.
Created new JUnits for the new KeyFileKeyProvider.
Performed manual functional tests with encrypted security tokens in the sample common container.
Thanks,
Stanton
> 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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134297#comment-13134297 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/
-----------------------------------------------------------
(Updated 2011-10-24 17:58:20.289469)
Review request for Ryan Baxter, Dan Dumont and Jesse Ciancetta.
Changes
-------
Adding dependent patch.
Summary (updated)
-------
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.
Initial review to Dan, Ryan, and Jesse. Once we've decided that this seems like a rational approach, I'll add the dev list.
This patch depends on https://reviews.apache.org/r/2467/
This addresses bug SHINDIG-1636.
https://issues.apache.org/jira/browse/SHINDIG-1636
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BasicSecurityTokenCodec.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/KeyFileKeyProviderTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/2362/diff
Testing
-------
Updated and ran existing JUnits.
Created new JUnits for the new KeyFileKeyProvider.
Performed manual functional tests with encrypted security tokens in the sample common container.
Thanks,
Stanton
> 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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ 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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134329#comment-13134329 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
bq. On 2011-10-24 18:09:46, Henry Saputra wrote:
bq. > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java, line 93
bq. > <https://reviews.apache.org/r/2362/diff/6/?file=52120#file52120line93>
bq. >
bq. > Since we are trying to be flexible, I was trying to find useable injection points for the BlobSecurityTokenCodec so SHindig implementors dont have to override this file to extend it.
bq. >
bq. > So any internal "new" is bad. For now it looks ok bc default key and domain listeners do exactly what it needs to do to update the keys but since the purpose of this patch also to make it extendible might as well inject while you can.
I would rather see inline anonymous creation of the generic ValueChangeListener used here rather than doing a new KeyProvidedValueListener.
The ProvidedValueListener generic interface is provided by the abstraction and shouldn't need to be injected.
I think that it's easier to follow if we just add anonymous implementations inline. But that's just me.
Anyway, the focus of these patches is to move the logic of building derived values from the config (key to use) into these ValueProvider classes so that they can be more easily overridden by injection. It's possible that we could also move the derived keys into the provider also, we weren't sure how generic that task would be for different kinds of crypto.
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2797
-----------------------------------------------------------
On 2011-10-24 17:58:20, 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-24 17:58:20)
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. This patch depends on https://reviews.apache.org/r/2467/
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/BasicSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1187375
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1187375
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127159#comment-13127159 ]
jiraposter@reviews.apache.org 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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134310#comment-13134310 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2797
-----------------------------------------------------------
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/#comment6264>
Since we are trying to be flexible, I was trying to find useable injection points for the BlobSecurityTokenCodec so SHindig implementors dont have to override this file to extend it.
So any internal "new" is bad. For now it looks ok bc default key and domain listeners do exactly what it needs to do to update the keys but since the purpose of this patch also to make it extendible might as well inject while you can.
- Henry
On 2011-10-24 17:58:20, 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-24 17:58:20)
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. This patch depends on https://reviews.apache.org/r/2467/
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/BasicSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1187375
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1187375
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127580#comment-13127580 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
bq. On 2011-10-14 13:20:28, Jesse Ciancetta wrote:
bq. > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java, line 137
bq. > <https://reviews.apache.org/r/2362/diff/3/?file=49952#file49952line137>
bq. >
bq. > I think this is supposed to be a break, not a return.
What I meant to say is -- I think is supposed to be a continue, not a return.
- Jesse
-----------------------------------------------------------
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127489#comment-13127489 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2582
-----------------------------------------------------------
Ship it!
LGTM
- Dan
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
[jira] [Commented] (SHINDIG-1636) Create a mechanism to provide an
encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13141466#comment-13141466 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2648/#review2993
-----------------------------------------------------------
Ship it!
This looks good to me, however if we're going to go this route I wouldn't mind seeing the BlobCrypterSecurityTokenCodec changed to always expect to get the actual token key from ContainerConfig -- and then making the code that parses the container.js file smart enough to be able to resolve res://key-file or file:///path/to/key-file automatically (or if not prefixed to take the value from container.js as is). I think this would work well for this particular case and would be a nice addition to the ContainerConfig code.
It's pretty straight forward to add that support to ContainerConfig and I've got a patch that does that which I could post if people are interested -- I'd need to add some additional unit tests around the enhancements to the ContainerConfig loader but other than that it should be good to go.
Or you could commit this as is and I could post that as a follow on patch at a later date and if people like it we could move to it. Either way is fine with me.
- Jesse
On 2011-11-01 18:12:28, Stanton Sievers wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2648/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-11-01 18:12:28)
bq.
bq.
bq. Review request for shindig, Ryan Baxter and Dan Dumont.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. This patch is to illustrate the minimal set of changes needed to allow the BlobCrypterSecurityTokenCodec to have a key provided by the config instead of needing a key file to be provided.
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1195457
bq. http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1195457
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1195457
bq.
bq. Diff: https://reviews.apache.org/r/2648/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Updated existing JUnits and added one more.
bq.
bq.
bq. Thanks,
bq.
bq. Stanton
bq.
bq.
> Create a mechanism 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.
> Update:
> The old approach was to provide a KeyProvider class but that turned out to be a little too heavy and there was some contention over the best implementation. Until there is a consensus on the best way to implement that abstraction, we can simply add another config value to the container.js that is the key itself and have the codec read and use that value if it exists.
--
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
[jira] [Commented] (SHINDIG-1636) Create a mechanism to provide an
encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13141802#comment-13141802 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2648/#review3003
-----------------------------------------------------------
Ship it!
One small nit, otherwise LGTM
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java
<https://reviews.apache.org/r/2648/#comment6724>
small nit, your could put the cypters.put call outside of the if else block
- Ryan
On 2011-11-01 18:12:28, Stanton Sievers wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2648/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-11-01 18:12:28)
bq.
bq.
bq. Review request for shindig, Ryan Baxter and Dan Dumont.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. This patch is to illustrate the minimal set of changes needed to allow the BlobCrypterSecurityTokenCodec to have a key provided by the config instead of needing a key file to be provided.
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1195457
bq. http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1195457
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1195457
bq.
bq. Diff: https://reviews.apache.org/r/2648/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Updated existing JUnits and added one more.
bq.
bq.
bq. Thanks,
bq.
bq. Stanton
bq.
bq.
> Create a mechanism 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.
> Update:
> The old approach was to provide a KeyProvider class but that turned out to be a little too heavy and there was some contention over the best implementation. Until there is a consensus on the best way to implement that abstraction, we can simply add another config value to the container.js that is the key itself and have the codec read and use that value if it exists.
--
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13131963#comment-13131963 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/
-----------------------------------------------------------
(Updated 2011-10-20 19:58:54.238925)
Review request for Ryan Baxter, Dan Dumont and Jesse Ciancetta.
Changes
-------
Updates based on Dan's review.
Summary
-------
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.
Initial review to Dan, Ryan, and Jesse. Once we've decided that this seems like a rational approach, I'll add the dev list.
This addresses bug SHINDIG-1636.
https://issues.apache.org/jira/browse/SHINDIG-1636
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BasicSecurityTokenCodec.java 1186847
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1186847
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1186847
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1186847
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1186847
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/KeyFileKeyProviderTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/2362/diff
Testing
-------
Updated and ran existing JUnits.
Created new JUnits for the new KeyFileKeyProvider.
Performed manual functional tests with encrypted security tokens in the sample common container.
Thanks,
Stanton
> 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
[jira] [Updated] (SHINDIG-1636) Create a KeyProvider to provide an
encryption key to the SecurityToken workflow
Posted by "Stanton Sievers (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Stanton Sievers updated SHINDIG-1636:
-------------------------------------
Description:
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.
Update:
The old approach was to provide a KeyProvider class but that turned out to be a little too heavy and there was some contention over the best implementation. Until there is a consensus on the best way to implement that abstraction, we can simply add another config value to the container.js that is the key itself and have the codec read and use that value if it exists.
was: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.
> 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.
> Update:
> The old approach was to provide a KeyProvider class but that turned out to be a little too heavy and there was some contention over the best implementation. Until there is a consensus on the best way to implement that abstraction, we can simply add another config value to the container.js that is the key itself and have the codec read and use that value if it exists.
--
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134194#comment-13134194 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
bq. On 2011-10-21 19:46:09, Jesse Ciancetta wrote:
bq. > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java, line 93
bq. > <https://reviews.apache.org/r/2362/diff/6/?file=52122#file52122line93>
bq. >
bq. > Arrays.asList is a varargs method -- no need to wrap the parameters in an array
We need this for type coercion. Without it, the types were ambiguous and Eclipse was sad.
- Stanton
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2750
-----------------------------------------------------------
On 2011-10-21 16:23:00, 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-21 16:23:00)
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/BasicSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1187375
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1187375
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134279#comment-13134279 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2794
-----------------------------------------------------------
Please do include Shindig as reviewer. The more open the process is the better.
Is the ValueProvider class new for this patch?
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/#comment6260>
Not sure how this can ever be null?
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/#comment6261>
Can we inject these too?
- Henry
On 2011-10-21 16:23:00, 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-21 16:23:00)
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/BasicSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1187375
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1187375
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134332#comment-13134332 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
bq. On 2011-10-24 18:09:46, Henry Saputra wrote:
bq. > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java, line 93
bq. > <https://reviews.apache.org/r/2362/diff/6/?file=52120#file52120line93>
bq. >
bq. > Since we are trying to be flexible, I was trying to find useable injection points for the BlobSecurityTokenCodec so SHindig implementors dont have to override this file to extend it.
bq. >
bq. > So any internal "new" is bad. For now it looks ok bc default key and domain listeners do exactly what it needs to do to update the keys but since the purpose of this patch also to make it extendible might as well inject while you can.
bq.
bq. Dan Dumont wrote:
bq. I would rather see inline anonymous creation of the generic ValueChangeListener used here rather than doing a new KeyProvidedValueListener.
bq.
bq. The ProvidedValueListener generic interface is provided by the abstraction and shouldn't need to be injected.
bq. I think that it's easier to follow if we just add anonymous implementations inline. But that's just me.
bq.
bq. Anyway, the focus of these patches is to move the logic of building derived values from the config (key to use) into these ValueProvider classes so that they can be more easily overridden by injection. It's possible that we could also move the derived keys into the provider also, we weren't sure how generic that task would be for different kinds of crypto.
ProvidedValueListener -> ValueChangeListener
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2797
-----------------------------------------------------------
On 2011-10-24 17:58:20, 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-24 17:58:20)
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. This patch depends on https://reviews.apache.org/r/2467/
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/BasicSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1187375
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1187375
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
[jira] [Commented] (SHINDIG-1636) Create a mechanism to provide an
encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13142124#comment-13142124 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2648/#review3012
-----------------------------------------------------------
Ship it!
LGTM!
- Dan
On 2011-11-01 18:12:28, Stanton Sievers wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2648/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-11-01 18:12:28)
bq.
bq.
bq. Review request for shindig, Ryan Baxter and Dan Dumont.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. This patch is to illustrate the minimal set of changes needed to allow the BlobCrypterSecurityTokenCodec to have a key provided by the config instead of needing a key file to be provided.
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1195457
bq. http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1195457
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1195457
bq.
bq. Diff: https://reviews.apache.org/r/2648/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Updated existing JUnits and added one more.
bq.
bq.
bq. Thanks,
bq.
bq. Stanton
bq.
bq.
> Create a mechanism 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.
> Update:
> The old approach was to provide a KeyProvider class but that turned out to be a little too heavy and there was some contention over the best implementation. Until there is a consensus on the best way to implement that abstraction, we can simply add another config value to the container.js that is the key itself and have the codec read and use that value if it exists.
--
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13131899#comment-13131899 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2715
-----------------------------------------------------------
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/#comment6120>
Was this carried over from existing impl or is this a new FIXME?
- Dan
On 2011-10-20 15:51:00, 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-20 15:51:00)
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/BasicSecurityTokenCodec.java 1186847
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1186847
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1186847
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1186847
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1186847
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13131706#comment-13131706 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/
-----------------------------------------------------------
(Updated 2011-10-20 15:51:00.333160)
Review request for Ryan Baxter, Dan Dumont and Jesse Ciancetta.
Changes
-------
Updating to use the new ValueProvider abstraction. https://reviews.apache.org/r/2467/
Trying to illustrate all of the stuff Dan and I have been talking about.
Summary
-------
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.
Initial review to Dan, Ryan, and Jesse. Once we've decided that this seems like a rational approach, I'll add the dev list.
This addresses bug SHINDIG-1636.
https://issues.apache.org/jira/browse/SHINDIG-1636
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BasicSecurityTokenCodec.java 1186847
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1186847
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1186847
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1186847
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1186847
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/KeyFileKeyProviderTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/2362/diff
Testing
-------
Updated and ran existing JUnits.
Created new JUnits for the new KeyFileKeyProvider.
Performed manual functional tests with encrypted security tokens in the sample common container.
Thanks,
Stanton
> 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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13132790#comment-13132790 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/
-----------------------------------------------------------
(Updated 2011-10-21 16:23:00.455060)
Review request for Ryan Baxter, Dan Dumont and Jesse Ciancetta.
Changes
-------
Merged in Dan's security token refactoring. Updated JUnits and everything passes.
Summary
-------
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.
Initial review to Dan, Ryan, and Jesse. Once we've decided that this seems like a rational approach, I'll add the dev list.
This addresses bug SHINDIG-1636.
https://issues.apache.org/jira/browse/SHINDIG-1636
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BasicSecurityTokenCodec.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1187375
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/KeyFileKeyProviderTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/2362/diff
Testing
-------
Updated and ran existing JUnits.
Created new JUnits for the new KeyFileKeyProvider.
Performed manual functional tests with encrypted security tokens in the sample common container.
Thanks,
Stanton
> 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
[jira] [Commented] (SHINDIG-1636) Create a mechanism to provide an
encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13141345#comment-13141345 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2648/
-----------------------------------------------------------
(Updated 2011-11-01 17:16:16.418233)
Review request for shindig, Ryan Baxter and Dan Dumont.
Changes
-------
Adding the dev list.
Summary (updated)
-------
This patch is to illustrate the minimal set of changes needed to allow the BlobCrypterSecurityTokenCodec to have a key provided by the config instead of needing a key file to be provided.
This addresses bug SHINDIG-1636.
https://issues.apache.org/jira/browse/SHINDIG-1636
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1195457
http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1195457
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1195457
Diff: https://reviews.apache.org/r/2648/diff
Testing
-------
None right now. Updated JUnits so they'd pass
Thanks,
Stanton
> Create a mechanism 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.
> Update:
> The old approach was to provide a KeyProvider class but that turned out to be a little too heavy and there was some contention over the best implementation. Until there is a consensus on the best way to implement that abstraction, we can simply add another config value to the container.js that is the key itself and have the codec read and use that value if it exists.
--
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134298#comment-13134298 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
bq. On 2011-10-24 17:41:14, Henry Saputra wrote:
bq. > Please do include Shindig as reviewer. The more open the process is the better.
bq. >
bq. > Is the ValueProvider class new for this patch?
The ValueProvider is in another patch that this one depends on. All of this work was done at the same time but splitting it apart seemed natural.
bq. On 2011-10-24 17:41:14, Henry Saputra wrote:
bq. > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java, line 84
bq. > <https://reviews.apache.org/r/2362/diff/6/?file=52120#file52120line84>
bq. >
bq. > Not sure how this can ever be null?
This is mainly a safeguard against future changes in DefaultSecurityTokenCodec not passing along the provider since it's not directly injected into this constructor.
bq. On 2011-10-24 17:41:14, Henry Saputra wrote:
bq. > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java, line 93
bq. > <https://reviews.apache.org/r/2362/diff/6/?file=52120#file52120line93>
bq. >
bq. > Can we inject these too?
That's an interesting idea. We could but I'm not sure I see the value of doing so. Can you elaborate?
- Stanton
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2794
-----------------------------------------------------------
On 2011-10-21 16:23:00, 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-21 16:23:00)
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/BasicSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1187375
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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1187375
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1187375
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127529#comment-13127529 ]
jiraposter@reviews.apache.org 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
[jira] [Updated] (SHINDIG-1636) Create a mechanism to provide an
encryption key to the SecurityToken workflow
Posted by "Stanton Sievers (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Stanton Sievers updated SHINDIG-1636:
-------------------------------------
Attachment: MinimalSecurityChangesPatchv2.txt
> Create a mechanism 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
> Attachments: MinimalSecurityChangesPatchv2.txt
>
> 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.
> Update:
> The old approach was to provide a KeyProvider class but that turned out to be a little too heavy and there was some contention over the best implementation. Until there is a consensus on the best way to implement that abstraction, we can simply add another config value to the container.js that is the key itself and have the codec read and use that value if it exists.
--
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
[jira] [Updated] (SHINDIG-1636) Create a mechanism to provide an
encryption key to the SecurityToken workflow
Posted by "Stanton Sievers (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Stanton Sievers updated SHINDIG-1636:
-------------------------------------
Summary: Create a mechanism to provide an encryption key to the SecurityToken workflow (was: Create a KeyProvider to provide an encryption key to the SecurityToken workflow)
> Create a mechanism 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.
> Update:
> The old approach was to provide a KeyProvider class but that turned out to be a little too heavy and there was some contention over the best implementation. Until there is a consensus on the best way to implement that abstraction, we can simply add another config value to the container.js that is the key itself and have the codec read and use that value if it exists.
--
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13126705#comment-13126705 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2552
-----------------------------------------------------------
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.
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.
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.
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.
So putting that all together I'm thinking we could change the token key configuration in container.js from this:
"gadgets.securityTokenKeyFile" : "/path/to/key/file.txt",
to something like this:
"gadgets.securityTokenKey" : "file://path/to/key/file.txt",
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.
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.
What do you think?
- 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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13126607#comment-13126607 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
bq. On 2011-10-13 13:54:33, Ryan Baxter wrote:
bq. > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/KeyFileKeyProviderTest.java, line 128
bq. > <https://reviews.apache.org/r/2362/diff/2/?file=49767#file49767line128>
bq. >
bq. > Small nit, you can use @Test(expected = TheExceptionYourExpecting.class) instead of the try catch...its a little less code
I like catching it in the test itself so that I can compare the messages to ensure that it is the exact exception I expect and not just the same type of exception. One could argue it couples the tests too tightly to the exact wording of the exception, but it seems like it's worth it.
- Stanton
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2546
-----------------------------------------------------------
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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13126606#comment-13126606 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/
-----------------------------------------------------------
(Updated 2011-10-13 14:19:32.710261)
Review request for Ryan Baxter, Dan Dumont and Jesse Ciancetta.
Changes
-------
Updates based on Ryan's comments and some small nits I found while reviewing the code again.
Summary
-------
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.
Initial review to Dan, Ryan, and Jesse. Once we've decided that this seems like a rational approach, I'll add the dev list.
This addresses bug SHINDIG-1636.
https://issues.apache.org/jira/browse/SHINDIG-1636
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1182570
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1182570
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyProvider.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java 1182570
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1182570
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1182570
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/KeyFileKeyProviderTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/2362/diff
Testing
-------
Updated and ran existing JUnits.
Created new JUnits for the new KeyFileKeyProvider.
Performed manual functional tests with encrypted security tokens in the sample common container.
Thanks,
Stanton
> 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
[jira] [Commented] (SHINDIG-1636) Create a KeyProvider to provide
an encryption key to the SecurityToken workflow
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13126589#comment-13126589 ]
jiraposter@reviews.apache.org commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2546
-----------------------------------------------------------
Ship it!
Love the idea. Code looks good. One small comment, Henry made me remove the @author tags from the new classes I added in my last patch, so you may want to do that as well.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/KeyFileKeyProviderTest.java
<https://reviews.apache.org/r/2362/#comment5717>
Small nit, you can use @Test(expected = TheExceptionYourExpecting.class) instead of the try catch...its a little less code
- Ryan
On 2011-10-12 19:27:30, 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-12 19:27:30)
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 1182524
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java 1182524
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 1182524
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1182524
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java 1182524
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