[Pki-devel] [PATCH] 101-2 Generation of asymmetric keys in the DRM
Ade Lee
alee at redhat.com
Tue Aug 26 18:59:02 UTC 2014
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 at redhat.com
> > https://www.redhat.com/mailman/listinfo/pki-devel
>
More information about the Pki-devel
mailing list