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");
> &#-1;&#-1; }
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