9:48 a.m.
Hi,
The attached patches add Symmetric Key generation service to the DRM and
refactor the DRM REST interface. Its worthwhile to look at each patch
individually, but there will be many cases where I changed my mind on
how to represent something - for instance, Request -> KeyRequest ->
ResourceMessage. So, the patches should be viewed as a whole.
Summary of changes:
1) Added new REST service to generate symmetric keys.
2) Refactor API to use POST /keyrequests for all request types and using
a generic RequestMessage object.
3) Refactor PKIException to use RequestMessage object.
4) Rename some objects in Key and KeyRequest resources.
I tested all this using the DRMTest code. I needed to comment out a
couple of tests because they were causing problems (including a core
dump on the client side), and I need to investigate why that happened.
Those tests will be restored once I figure out whats going on.
I'd like to get several eyes on this, please.
Thanks,
Ade
1:02 p.m.
On 1/30/2014 9:48 AM, Ade Lee wrote:
Hi,
The attached patches add Symmetric Key generation service to the DRM and
refactor the DRM REST interface. Its worthwhile to look at each patch
individually, but there will be many cases where I changed my mind on
how to represent something - for instance, Request -> KeyRequest ->
ResourceMessage. So, the patches should be viewed as a whole.
Summary of changes:
1) Added new REST service to generate symmetric keys.
2) Refactor API to use POST /keyrequests for all request types and using
a generic RequestMessage object.
3) Refactor PKIException to use RequestMessage object.
4) Rename some objects in Key and KeyRequest resources.
I tested all this using the DRMTest code. I needed to comment out a
couple of tests because they were causing problems (including a core
dump on the client side), and I need to investigate why that happened.
Those tests will be restored once I figure out whats going on.
I'd like to get several eyes on this, please.
Thanks,
Ade
Some comments:
1. Minor issue. Please put a space before the curly bracket:
public static class Data extends ResourceMessage{
2. I'm not sure if the ResourceMessage should have a Link attribute. The
PKIException doesn't need it. Probably many other request/resource
objects won't need it either.
3. The PKIException previously has <Attributes>. Now that it uses
<Properties> should we start implementing API versioning?
4. This is actually an existing issue in the current code. The
marshall/unmarshal code currently swallows the exception. We probably
should have thrown the original exception, or if not possible we should
wrap it with a RuntimeException.
5. This is also an existing issue. The KeyDataInfo name is probably
redundant. If it's an Info that means it doesn't have the Data, so the
name probably should be KeyInfo. Similarly, the KeyDataInfoCollection
probably can be renamed to KeyInfoCollection. The XmlRootElement should
match too, but this probably requires versioning.
@XmlRootElement(name = "KeyDataInfos")
public class KeyDataInfoCollection extends DataCollection<KeyDataInfo> {
6. The KEYGEN_ALGORITHMS is defined in SymKeyGenerationRequest but it's
only used by the server only. Will the client need this too? Otherwise
we should move it to the server. Maybe the client just needs the list of
alg names instead of the actual objects?
7. The DRMTest is using constants in PKIService. While this works in dev
env, a real client will not be able to use the server class. We should
move the constants and maybe provide a method in the client library to
strip the header & trailer. Or maybe strip them on the server.
transportCert = transportCert.substring(PKIService.HEADER.length(),
transportCert.indexOf(PKIService.TRAILER));
I may have some more comments later.
--
Endi S. Dewata
9:55 p.m.
New patch attached to address comments as below.
Please apply 189 on top of rest of patches.
On Thu, 2014-01-30 at 13:02 -0600, Endi Sukma Dewata wrote:
On 1/30/2014 9:48 AM, Ade Lee wrote:
> Hi,
>
> The attached patches add Symmetric Key generation service to the DRM and
> refactor the DRM REST interface. Its worthwhile to look at each patch
> individually, but there will be many cases where I changed my mind on
> how to represent something - for instance, Request -> KeyRequest ->
> ResourceMessage. So, the patches should be viewed as a whole.
>
> Summary of changes:
> 1) Added new REST service to generate symmetric keys.
> 2) Refactor API to use POST /keyrequests for all request types and using
> a generic RequestMessage object.
> 3) Refactor PKIException to use RequestMessage object.
> 4) Rename some objects in Key and KeyRequest resources.
>
> I tested all this using the DRMTest code. I needed to comment out a
> couple of tests because they were causing problems (including a core
> dump on the client side), and I need to investigate why that happened.
> Those tests will be restored once I figure out whats going on.
>
> I'd like to get several eyes on this, please.
>
> Thanks,
> Ade
Some comments:
1. Minor issue. Please put a space before the curly bracket:
public static class Data extends ResourceMessage{
Fixed.
2. I'm not sure if the ResourceMessage should have a Link
attribute. The
PKIException doesn't need it. Probably many other request/resource
objects won't need it either.
Removed Link
3. The PKIException previously has <Attributes>. Now that it
uses
<Properties> should we start implementing API versioning?
Revert to Attributes
4. This is actually an existing issue in the current code. The
marshall/unmarshal code currently swallows the exception. We probably
should have thrown the original exception, or if not possible we should
wrap it with a RuntimeException.
Deferring to later patch.
5. This is also an existing issue. The KeyDataInfo name is probably
redundant. If it's an Info that means it doesn't have the Data, so the
name probably should be KeyInfo. Similarly, the KeyDataInfoCollection
probably can be renamed to KeyInfoCollection. The XmlRootElement should
match too, but this probably requires versioning.
@XmlRootElement(name = "KeyDataInfos")
public class KeyDataInfoCollection extends DataCollection<KeyDataInfo> {
Renamed classes.
6. The KEYGEN_ALGORITHMS is defined in SymKeyGenerationRequest but
it's
only used by the server only. Will the client need this too? Otherwise
we should move it to the server. Maybe the client just needs the list of
alg names instead of the actual objects?
Fixed.
7. The DRMTest is using constants in PKIService. While this works in
dev
env, a real client will not be able to use the server class. We should
move the constants and maybe provide a method in the client library to
strip the header & trailer. Or maybe strip them on the server.
transportCert = transportCert.substring(PKIService.HEADER.length(),
transportCert.indexOf(PKIService.TRAILER));
Defer to later patch.
I may have some more comments later.
Also added code to address Christina's comments as below:
SymKeyGenService.java:
* does IRequest.RESULT not need to be set in request for
SymKeyGenService? and also
mKRA.getRequestQueue().updateRequest(request); ?
Added code to SymKeyGenService etc.
* I am actually kind of surprised that the SymKeyGenService doesn't do
both key gen and recovery in one shot. That's what asym key
server-side-key-generation does --- generating keys, archive, and then
return the keys to the caller, all in one shot.
If not needed now, we can at least put down as "ToDo" as an option, if
the client supplies the kra-transport-wrapped session key.
Defer to later patch.
12:43 p.m.
Final ack by Endi.
Pushed to master:
Exit status 0
To ssh://vakwetu@git.fedorahosted.org/git/pki.git
78031e7..cf425df master -> master
On Mon, 2014-02-03 at 22:55 -0500, Ade Lee wrote:
New patch attached to address comments as below.
Please apply 189 on top of rest of patches.
On Thu, 2014-01-30 at 13:02 -0600, Endi Sukma Dewata wrote:
> On 1/30/2014 9:48 AM, Ade Lee wrote:
> > Hi,
> >
> > The attached patches add Symmetric Key generation service to the DRM and
> > refactor the DRM REST interface. Its worthwhile to look at each patch
> > individually, but there will be many cases where I changed my mind on
> > how to represent something - for instance, Request -> KeyRequest ->
> > ResourceMessage. So, the patches should be viewed as a whole.
> >
> > Summary of changes:
> > 1) Added new REST service to generate symmetric keys.
> > 2) Refactor API to use POST /keyrequests for all request types and using
> > a generic RequestMessage object.
> > 3) Refactor PKIException to use RequestMessage object.
> > 4) Rename some objects in Key and KeyRequest resources.
> >
> > I tested all this using the DRMTest code. I needed to comment out a
> > couple of tests because they were causing problems (including a core
> > dump on the client side), and I need to investigate why that happened.
> > Those tests will be restored once I figure out whats going on.
> >
> > I'd like to get several eyes on this, please.
> >
> > Thanks,
> > Ade
>
> Some comments:
>
> 1. Minor issue. Please put a space before the curly bracket:
>
> public static class Data extends ResourceMessage{
>
Fixed.
> 2. I'm not sure if the ResourceMessage should have a Link attribute. The
> PKIException doesn't need it. Probably many other request/resource
> objects won't need it either.
>
Removed Link
> 3. The PKIException previously has <Attributes>. Now that it uses
> <Properties> should we start implementing API versioning?
>
Revert to Attributes
> 4. This is actually an existing issue in the current code. The
> marshall/unmarshal code currently swallows the exception. We probably
> should have thrown the original exception, or if not possible we should
> wrap it with a RuntimeException.
>
Deferring to later patch.
> 5. This is also an existing issue. The KeyDataInfo name is probably
> redundant. If it's an Info that means it doesn't have the Data, so the
> name probably should be KeyInfo. Similarly, the KeyDataInfoCollection
> probably can be renamed to KeyInfoCollection. The XmlRootElement should
> match too, but this probably requires versioning.
>
> @XmlRootElement(name = "KeyDataInfos")
> public class KeyDataInfoCollection extends DataCollection<KeyDataInfo> {
>
Renamed classes.
> 6. The KEYGEN_ALGORITHMS is defined in SymKeyGenerationRequest but it's
> only used by the server only. Will the client need this too? Otherwise
> we should move it to the server. Maybe the client just needs the list of
> alg names instead of the actual objects?
>
Fixed.
> 7. The DRMTest is using constants in PKIService. While this works in dev
> env, a real client will not be able to use the server class. We should
> move the constants and maybe provide a method in the client library to
> strip the header & trailer. Or maybe strip them on the server.
>
> transportCert = transportCert.substring(PKIService.HEADER.length(),
> transportCert.indexOf(PKIService.TRAILER));
>
Defer to later patch.
> I may have some more comments later.
>
Also added code to address Christina's comments as below:
SymKeyGenService.java:
* does IRequest.RESULT not need to be set in request for
SymKeyGenService? and also
mKRA.getRequestQueue().updateRequest(request); ?
Added code to SymKeyGenService etc.
* I am actually kind of surprised that the SymKeyGenService doesn't do
both key gen and recovery in one shot. That's what asym key
server-side-key-generation does --- generating keys, archive, and then
return the keys to the caller, all in one shot.
If not needed now, we can at least put down as "ToDo" as an option, if
the client supplies the kra-transport-wrapped session key.
Defer to later patch.
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
1:40 p.m.
Ade,
This is my first cut. focus of this review is more on the server side
sym key generation part.
SymKeyGenService.java:
* does IRequest.RESULT not need to be set in request for
SymKeyGenService? and also mKRA.getRequestQueue().updateRequest(request); ?
* I am actually kind of surprised that the SymKeyGenService doesn't do
both key gen and recovery in one shot. That's what asym key
server-side-key-generation does --- generating keys, archive, and then
return the keys to the caller, all in one shot.
If not needed now, we can at least put down as "ToDo" as an option, if
the client supplies the kra-transport-wrapped session key.
Christina
On 01/30/2014 07:48 AM, Ade Lee wrote:
Hi,
The attached patches add Symmetric Key generation service to the DRM and
refactor the DRM REST interface. Its worthwhile to look at each patch
individually, but there will be many cases where I changed my mind on
how to represent something - for instance, Request -> KeyRequest ->
ResourceMessage. So, the patches should be viewed as a whole.
Summary of changes:
1) Added new REST service to generate symmetric keys.
2) Refactor API to use POST /keyrequests for all request types and using
a generic RequestMessage object.
3) Refactor PKIException to use RequestMessage object.
4) Rename some objects in Key and KeyRequest resources.
I tested all this using the DRMTest code. I needed to comment out a
couple of tests because they were causing problems (including a core
dump on the client side), and I need to investigate why that happened.
Those tests will be restored once I figure out whats going on.
I'd like to get several eyes on this, please.
Thanks,
Ade
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
3732
days inactive
3737
days old
4 comments
3 participants
participants (3)
-
Ade Lee -
Christina Fu -
Endi Sukma Dewata