[Pki-devel] [PATCH] 101 Generation of asymmetric keys in the DRM

[Ade Lee]

On Tue, 2014-08-05 at 11:28 -0500, Endi Sukma Dewata wrote:
> On 8/4/2014 9:29 AM, Abhishek Koneru wrote:
> > Please find the attached patch which generates the asymmetric keys using
> > algorithms RSA and DSA (EC to be added) for a valid key sizes of 512,
> > 1024, 2048, 4096.
> >
> > Key Changes in the patch -
> >
> >     - Adding methods for generation of Asymmetric keys in the DRM.
> >     - Allowing the key-generate CLI command to accept algorithms RSA and
> > DSA.
> >     - Returning the base64 encoded public key in the KeyInfo object
> > (key-show CLI command).
> >     - Retrieving the private key using the retrieveKeyData method in the
> > KeyClient.
> >
> > -- Abhishek
> 
> I've opened some tickets related to key management. Please take a look 
> at them.
> 
> The patch seems to be working just fine, so it's ACKed. Some comments below:
> 
> 1. Not sure about the "b64" prefix in b64PublicKey and b64_public_key 
> field names. We have some other fields that contain base-64 encoded 
> values but they use regular field names.
> 
Agreed.  Lets remove the b64 parts.  What we can (and should be doing
though) is documenting both in the Java class and the python class that
base 64 encoded data is required/provided.

> 2. Existing issue. The KeyGenerationRequest.getKeySize() swallows 
> NumberFormatException and returns null. I think the method should let 
> the exception be handled by the caller. It's a RuntimeException so it 
> doesn't need to be declared.
> 
> 3. In AsymKeyGenService.serviceRequest() the request ID doesn't really 
> need to be converted into string. The string concatenation later will do 
> that automatically.
> 
>    String id = request.getRequestId().toString();
> 
> 4. The following code in KeyRequestService might not be necessary 
> because access to this service is already controlled by ACL, so owner is 
> never null.
> 
>    if (owner == null) {
>        throw new UnauthorizedException(
>            "Key generation must be performed by an agent");
>    }
> 
> In general we shouldn't hard-code authorization logic in the code unless 
> it's something can't be expressed via ACL.
> 
> 5. Some formatting issues:
> 
> Formatting issue in KeyCLI.java:
> 
>    for(i=0;i<publicKey.length()/64;i++){
> 
> KeyRequestService.java:
> 
>    } else if (request instanceof AsymKeyGenerationRequest){
>    public Response generateAsymKey(AsymKeyGenerationRequest data){

> KeyService.java:
> 
>    if(rec.getPublicKeyData() != null && getPublicKey){
> 
> AsymKeyGenerationRequest.java:
> 
>    public class AsymKeyGenerationRequest extends KeyGenerationRequest{
> 
> KeyGenerationRequest.java:
> 
>    public class KeyGenerationRequest extends ResourceMessage{
> 

6. In key.py, create_request() seems to be confusingly named for me.
How about submit_request() or submit_creation_request() instead?
Same comment on createRequest() in the Java code.

7. generate_asymmetric_key() is unnecessarily wordy ..  How about
something like this (which is more likely what the final output will
look like?)

def generate_asymmetric_key(self, client_key_id, algorithm=None, size=None,
                               usages=None,
                               trans_wrapped_session_key=None):
        """ Generate and archive asymmetric keys in the DRM.

            Return a KeyRequestResponse which contains a KeyRequestInfo
            object that describes the URL for the request and generated keys.

        """
        if client_key_id is None:
            raise TypeError("Must specify Client Key ID")

	twsk = None
        if trans_wrapped_session_key is not None:
            twsk = base64.encodestring(trans_wrapped_session_key)

	request = AsymKeyGenerationRequest(
            client_key_id=client_key_id,
            key_size=size,
            key_algorithm=algorithm,
            key_usages=usages,
            trans_wrapped_session_key=twsk)
        
        if twsk is not None:
            raise NotImplementedError(
                "Returning the asymmetric keys in the same call is not yet "
                "implemented.")

        return self.create_request(request)

8.  Formatting: Can we break up the line for XmlSeeAlso in
ResourceMessage.java ?

9. In AsymKeyGenerationRequest.java, its useful at the end to have some
unit test code so that we can see the format of the requests.

10.  In KeyClient.java in generateAsymmetricKey, the keySize is
hardcoded to 1024 bits.

11.  Formatting in line 264 drmtest.py  Actually, thats probably a line
that needs to be removed.

12. Make sure all new files have relevant copyright/licence notices.

13. In AsynKeyGenService.java, it looks like you are using Hungarian
notation for your instance variables.  Please avoid this in new code.
(mKRA, mStorageUnit)

14. Formatting - line 92 in AsymKeygenService.java

15. Why the change in SecurityDataService.java?

16. There is no Java test code in DRMTest.java?

17.  It would be useful to have some kind of test for the validity of
the keys generated, either in drmtest.py or DRMTest.java.  Maybe some
thing that validates a public against the private key.  Something that
tells us that what is being generated is valid.

18. Where does the list of valid key sizes come from?  We probably
should support whatever key sizes JSS/NSS supports - or at least some
reasonable subset thereof.  Furthermore, that set should be make
configurable in CS.cfg.  Are there any other parameters that are
important to key generation?  Usage/ Mode?  Or is key size (at least for
RSA and DSA the only factor needed in the constructor of the
KeyGenerator object in JSS?

Otherwise, looks good in general.

Ade