<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 TRANSITIONAL//EN"> <HTML> <HEAD> <META HTTP-EQUIV="Content-Type" CONTENT="text/html; CHARSET=UTF-8"> <META NAME="GENERATOR" CONTENT="GtkHTML/4.6.6"> </HEAD> <BODY> 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: <BLOCKQUOTE TYPE=CITE> <PRE> 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. </PRE> </BLOCKQUOTE> Done <BLOCKQUOTE TYPE=CITE> <PRE> > 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. </PRE> </BLOCKQUOTE> There is a ticket for it. Will fix it separately <BLOCKQUOTE TYPE=CITE> <PRE> > > 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(); </PRE> </BLOCKQUOTE> Done <BLOCKQUOTE TYPE=CITE> <PRE> > > 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"); > &#-1;&#-1; } </PRE> </BLOCKQUOTE> Removed. <BLOCKQUOTE TYPE=CITE> <PRE> > > 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{ > </PRE> </BLOCKQUOTE> Done <BLOCKQUOTE TYPE=CITE> <PRE> 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. </PRE> </BLOCKQUOTE> changed the method name to submit_request/submitRequest. <BLOCKQUOTE TYPE=CITE> <PRE> 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) </PRE> </BLOCKQUOTE> Done <BLOCKQUOTE TYPE=CITE> <PRE> 8. Formatting: Can we break up the line for XmlSeeAlso in ResourceMessage.java ? </PRE> </BLOCKQUOTE> Done <BLOCKQUOTE TYPE=CITE> <PRE> 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. </PRE> </BLOCKQUOTE> Done <BLOCKQUOTE TYPE=CITE> <PRE> 10. In KeyClient.java in generateAsymmetricKey, the keySize is hardcoded to 1024 bits. </PRE> </BLOCKQUOTE> Corrected. <BLOCKQUOTE TYPE=CITE> <PRE> 11. Formatting in line 264 drmtest.py Actually, thats probably a line that needs to be removed. </PRE> </BLOCKQUOTE> Removed. <BLOCKQUOTE TYPE=CITE> <PRE> 12. Make sure all new files have relevant copyright/licence notices. </PRE> </BLOCKQUOTE> Done. <BLOCKQUOTE TYPE=CITE> <PRE> 13. In AsynKeyGenService.java, it looks like you are using Hungarian notation for your instance variables. Please avoid this in new code. (mKRA, mStorageUnit) </PRE> </BLOCKQUOTE> Done. <BLOCKQUOTE TYPE=CITE> <PRE> 14. Formatting - line 92 in AsymKeygenService.java </PRE> </BLOCKQUOTE> Done <BLOCKQUOTE TYPE=CITE> <PRE> 15. Why the change in SecurityDataService.java? </PRE> </BLOCKQUOTE> To not store the algorithm of the session key in the db for the ldap entry. <BLOCKQUOTE TYPE=CITE> <PRE> 16. There is no Java test code in DRMTest.java? </PRE> </BLOCKQUOTE> Done <BLOCKQUOTE TYPE=CITE> <PRE> 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. </PRE> </BLOCKQUOTE> Done. Used the keys to sign and verify some data. <BLOCKQUOTE TYPE=CITE> <PRE> 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? </PRE> </BLOCKQUOTE> Done <BLOCKQUOTE TYPE=CITE> <PRE> Otherwise, looks good in general. Ade </PRE> </BLOCKQUOTE> 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 </BODY> </HTML>