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

Sorry for the spam. The last patch introduced a bug in symmetric key generation using default parameters using the python client. It has been rectified and included in the patch attached to this mail. --Abhishek On Thu, 2014-08-21 at 09:57 -0400, Abhishek Koneru wrote: > Please review the patch 101-2 which addresses all the comments given > by Endi, Jack and Ade. > > This patch deals with adding the feature of generating asymmetric keys > in the KRA. > > Few major changes from the first patch - > - Supported algorithms - RSA and DSA. > - Permitted key sizes - RSA - 256-8192(default, is configurable). DSA > 512, 768, 1024. (Since the PQG params are not yet accepted.) > - Way of storing the private key in the db. Previously, the binary > data is treated as a string, but now the actual Private key object is > wrapped and stored. > > Please see inline comments for any comments that are not addressed. > > On Thu, 2014-08-07 at 16:16 -0400, Ade Lee wrote: > > 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. > > Done > > > > > 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. > > There is a ticket for it. Will fix it separately > > > > > > 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(); > Done > > > > > > 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"); > > > ?? } > > Removed. > > > > > > 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{ > > > > > > Done > > 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. > > > changed the method name to submit_request/submitRequest. > > 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) > > > Done > > 8. Formatting: Can we break up the line for XmlSeeAlso in > > ResourceMessage.java ? > > > Done > > 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. > > > Done > > 10. In KeyClient.java in generateAsymmetricKey, the keySize is > > hardcoded to 1024 bits. > > > Corrected. > > 11. Formatting in line 264 drmtest.py Actually, thats probably a line > > that needs to be removed. > > > Removed. > > 12. Make sure all new files have relevant copyright/licence notices. > > > Done. > > 13. In AsynKeyGenService.java, it looks like you are using Hungarian > > notation for your instance variables. Please avoid this in new code. > > (mKRA, mStorageUnit) > > > Done. > > 14. Formatting - line 92 in AsymKeygenService.java > > > Done > > 15. Why the change in SecurityDataService.java? > > > To not store the algorithm of the session key in the db for the ldap > entry. > > 16. There is no Java test code in DRMTest.java? > > > Done > > 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. > > > > Done. Used the keys to sign and verify some data. > > 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? > > > Done > > Otherwise, looks good in general. > > > > Ade > > > > Some comments from Jack - > > KeyGenerationRequest: > > I see we have only two usages/capabilities defined. > In TPS we support more of these as follows: > > Might want to look into expanding that. > > > op.enroll.soKey.keyGen.encryption.private.keyCapabilities.decrypt=true > op.enroll.soKey.keyGen.encryption.private.keyCapabilities.derive=false > op.enroll.soKey.keyGen.encryption.private.keyCapabilities.encrypt=false > op.enroll.soKey.keyGen.encryption.private.keyCapabilities.private=true > op.enroll.soKey.keyGen.encryption.private.keyCapabilities.sensitive=true > op.enroll.soKey.keyGen.encryption.private.keyCapabilities.sign=false > op.enroll.soKey.keyGen.encryption.private.keyCapabilities.signRecover=false > op.enroll.soKey.keyGen.encryption.private.keyCapabilities.token=true > op.enroll.soKey.keyGen.encryption.private.keyCapabilities.unwrap=true > op.enroll.soKey.keyGen.encryption.private.keyCapabilities.verify=false > op.enroll.soKey.keyGen.encryption.private.keyCapabilities.verifyRecover=false > op.enroll.soKey.keyGen.encryption.private.keyCapabilities.wrap=false > > --Added all the usages except the private, sensitive and token. > > > - This code here: > > + KeyRequestResponse response = null; > + if > (keyAlgorithm.equalsIgnoreCase(KeyRequestResource.RSA_ALGORITHM) || > keyAlgorithm.equalsIgnoreCase(KeyRequestResource.DSA_ALGORITHM)) { > + response = > keyCLI.keyClient.generateAsymmetricKey(clientKeyId, keyAlgorithm, > + Integer.parseInt(keySize), > + usages, null); > + } else { > + response = > keyCLI.keyClient.generateSymmetricKey(clientKeyId, keyAlgorithm, > + Integer.parseInt(keySize), > + usages, null); > + } > MainCLI.printMessage("Key generation request info"); > KeyCLI.printKeyRequestInfo(response.getRequestInfo()); > } > > So the question is, are we sure that if the alg is not RSA or DSA that > we default to a symmetric key? > Are we not passing an alg for that like DES3, etc? What if ECC or some > such gets passed, should we throw an exception? > > --Corrected. > > - This code: > > @Override > + public boolean serviceRequest(IRequest request) throws > EBaseException { > + > + String id = request.getRequestId().toString(); > + String clientKeyId = > request.getExtDataInString(IRequest.SECURITY_DATA_CLIENT_KEY_ID); > + String algorithm = > request.getExtDataInString(IRequest.KEY_GEN_ALGORITHM); > + > + String keySizeStr = > request.getExtDataInString(IRequest.KEY_GEN_SIZE); > + int keySize = Integer.parseInt(keySizeStr); > + > + CMS.debug("AsymKeyGenService.serviceRequest. Request id: " + > id); > + CMS.debug("AsymKeyGenService.serviceRequest algorithm: " + > algorithm); > + > + KeyPairAlgorithm keyPairAlgorithm = > KeyRequestDAO.ASYMKEY_GEN_ALGORITHMS.get(algorithm.toUpperCase()); > + > + String owner = > request.getExtDataInString(IRequest.ATTR_REQUEST_OWNER); > + String auditSubjectID = owner; > + > > Would it make sense here to do some validation and throw exceptions? > If some of this data is bad, the next calls are going > to fail. If we check here we might be able to send a more specific > exception back. For instance that "Integer.parseInt" could > throw up and we won't find out about it unti later. Might check to see > how we handle this in other similar code. I suspect maybe this > validation is done at some higher level? I don't remember :) > > -- Validation added before this statement is processed. (In the > previous methods) > > IN the same method: > > if (kp == null) { > + auditAsymKeyGenRequestProcessed(auditSubjectID, > ILogger.FAILURE, request.getRequestId(), > + clientKeyId, null, "Failed to generate Asymmetric > key"); > + throw new EBaseException("Errors in generating Asymmetric > key"); > + } > > Can kp even be null here? Would not the try / catch block above catch > this? I"m not sure here, just asking. > > -- Just a fail-safe null check. > > Here: > > BigInteger serialNo = storage.getNextSerialNumber(); > + > + if (serialNo == null) { > + mKRA.log(ILogger.LL_FAILURE, > + > CMS.getLogMessage("CMSCORE_KRA_GET_NEXT_SERIAL")); > + auditAsymKeyGenRequestProcessed(auditSubjectID, > ILogger.FAILURE, request.getRequestId(), > + clientKeyId, null, "Failed to get next Key ID"); > + throw new > EBaseException(CMS.getUserMessage("CMS_KRA_INVALID_STATE")); > + } > > > Might check to see if getNextSerialNumber could ever return null. > > -- Just a fail-safe null check. We never know anything with > threads. :) > > Here: > > index > a2d587318efb59f0e1e7ebff0a3533468b558d10..ef353eeab018b9e3cec8f562f25d889892b830dc 100644 > --- a/base/kra/src/com/netscape/kra/SecurityDataRecoveryService.java > +++ b/base/kra/src/com/netscape/kra/SecurityDataRecoveryService.java > @@ -179,7 +179,7 @@ public class SecurityDataRecoveryService > implements IService { > byte[] unwrappedSecData = null; > if (dataType.equals(KeyRequestResource.SYMMETRIC_KEY_TYPE)) { > symKey = recoverSymKey(keyRecord); > - } else if > (dataType.equals(KeyRequestResource.PASS_PHRASE_TYPE)) { > + } else { > unwrappedSecData = recoverSecurityData(keyRecord); > } > > Just wondering the reason for this. I"m sure it's valid, just > curious.. > > -- It was a mistake. Corrected this time. > > Here: > > > > + > + public Response generateAsymKey(AsymKeyGenerationRequest data){ > + if (data == null) { > + throw new BadRequestException("Invalid key generation > request."); > + } > + > + KeyRequestDAO dao = new KeyRequestDAO(); > + KeyRequestResponse response; > + try { > + String owner = > servletRequest.getUserPrincipal().getName(); > + if (owner == null) { > + throw new UnauthorizedException("Key generation must > be performed by an agent"); > + } > + response = dao.submitRequest(data, uriInfo, owner); > + > auditAsymKeyGenRequestMade(response.getRequestInfo().getRequestId(), > ILogger.SUCCESS, > + data.getClientKeyId()); > + > + return createCreatedResponse(response, new > URI(response.getRequestInfo().getRequestURL())); > + > + } catch (EBaseException | URISyntaxException e) { > + e.printStackTrace(); > + auditArchivalRequestMade(null, ILogger.FAILURE, > data.getClientKeyId()); > + throw new PKIException(e.toString()); > + } > + } > } > > > Note the Auth code, with endi's realms and such, is this needed at > this level of code? > > -- Removed > > > --Abhishek > > > > > > _______________________________________________ > Pki-devel mailing list > Pki-devel(a)redhat.com > https://www.redhat.com/mailman/listinfo/pki-devel