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

ACK. Looks good overall. Some small nits/questions: Just a couple of questions: 1. Typo in commit message (Uisng ...) 2. tests in drmtest.py need to be beefed up -- in particular, we want to retrieve and verify the keys generated. You can do this in a separate patch. 3. SecurityDataRecoveryService -- fix the TODO (for the exception). 4. SecurityDataService -- explain the change from algStr -> null? 5, AsymKeyGenService.java line 144 - can usageList be null?

Ade On Thu, 2014-08-21 at 17:21 -0400, Abhishek Koneru wrote: > 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 >