[Pki-devel] [PATCH] 101-2 Generation of asymmetric keys in the DRM
Abhishek Koneru
akoneru at redhat.com
Thu Aug 21 21:21:39 UTC 2014
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-akoneru-101-2-Generate-asymmetric-keys-in-the-DRM.patch
Type: text/x-patch
Size: 93887 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140821/7dccc857/attachment.bin>
More information about the Pki-devel
mailing list