[Pki-devel] [PATCH] 101-2 Generation of asymmetric keys in the DRM
Abhishek Koneru
akoneru at redhat.com
Wed Aug 27 20:46:21 UTC 2014
Addressed the comments from Ade and Jack and pushed to master.
-- Abhishek
On Tue, 2014-08-26 at 14:59 -0400, Ade Lee wrote:
> 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?
Yes. The generated keys passed the verification test.
>
> 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 at redhat.com
> > > https://www.redhat.com/mailman/listinfo/pki-devel
> >
>
>
More information about the Pki-devel
mailing list