9:29 a.m.
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
11:28 a.m.
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.
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.
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();
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");
}
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{
--
Endi S. Dewata
3:16 p.m.
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.
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.
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();
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");
}
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{
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.
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)
8. Formatting: Can we break up the line for XmlSeeAlso in
ResourceMessage.java ?
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.
10. In KeyClient.java in generateAsymmetricKey, the keySize is
hardcoded to 1024 bits.
11. Formatting in line 264 drmtest.py Actually, thats probably a line
that needs to be removed.
12. Make sure all new files have relevant copyright/licence notices.
13. In AsynKeyGenService.java, it looks like you are using Hungarian
notation for your instance variables. Please avoid this in new code.
(mKRA, mStorageUnit)
14. Formatting - line 92 in AsymKeygenService.java
15. Why the change in SecurityDataService.java?
16. There is no Java test code in DRMTest.java?
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.
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?
Otherwise, looks good in general.
Ade
8:57 a.m.
New subject: [PATCH] 101-2 Generation of asymmetric keys in the DRM
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
4:21 p.m.
New subject: [PATCH] 101-2 Generation of asymmetric keys in the DRM
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
1:59 p.m.
New subject: [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
3:46 p.m.
New subject: [PATCH] 101-2 Generation of asymmetric keys in the DRM
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(a)redhat.com
> > https://www.redhat.com/mailman/listinfo/pki-devel
>
3533
days inactive
3556
days old
6 comments
3 participants
participants (3)
-
Abhishek Koneru -
Ade Lee -
Endi Sukma Dewata