[Pki-devel] patches for review - ECC key archival / recovery feature implementation - Bug 745278 - [RFE] ECC encryption keys cannot be archived
Christina Fu
cfu at redhat.com
Tue Mar 20 22:08:33 UTC 2012
JSS part rolled in comments, please review again:
https://bugzilla.redhat.com/attachment.cgi?id=571545&action=diff&context=patch&collapsed=&headers=1&format=raw
Note: all other CS components have dependency on the availability of
this patch, so this one needs to be ack'd separately ahead of time.
thanks,
Christina
On 03/19/2012 06:00 PM, Christina Fu wrote:
> On 03/19/2012 05:41 PM, John Magne wrote:
>>
>> Took a look at this patch for ECC support.
>>
>> Comments and questions below.
>>
>>
>> 1.
>>
>> KeyRecord.java line 273
>>
>> public MetaInfo getMetaInfo() throws EBaseException {
>>
>> return mMetaInfo;
>>
>> }
>>
>>
>> Why does this throw the EBaseException?
>> All it does is return a value.
>>
>
> yes, it does not need to declare such throw of exception, even though
> it is the trend in that file and I was just copying them. Will change.
>
>> 2.
>>
>>
>> CryptUtil.java line 186
>>
>> if (sensitive == 1 )
>>
>> keygen.sensitivePairs(true);
>>
>> else if (sensitive == 0)
>>
>> keygen.sensitivePairs(false);
>>
>> if (extractable == 1 )
>>
>> keygen.extractablePairs(true);
>>
>> else if (extractable == 0)
>>
>> keygen.extractablePairs(false);
>>
>>
>> keygen.initialize(keysize);
>>
>>
>>
>>
>> The values extractable and sensitive are sent in as integer?
>> Should they be a boolean? I see the function calls have -1,-1
>> for those two final params. How does not setting sensitive and
>> extractable different from setting them to false?
>>
>> I can see this being normal.
>
> As explained in the comment, the definitions are defined in JSS
> pkcs11/PK11KeyPairGenerator.java, so I just followed the definition.
> For your information, here is what it says in JSS:
> private boolean temporaryPairMode = false;
> // 1: sensitive
> // 0: insensitive
> // -1: sensitive if temporaryPairMode is false,
> // insensitive if temporaryPairMode is true
> // (the default depends on temporaryPairMode for backward
> // compatibility)
> private int sensitivePairMode = -1;
> // 1: extractable
> // 0: unextractable
> // -1: unspecified (token dependent)
> private int extractablePairMode = -1;
>
>>
>> 3.
>>
>> RecoveryService.java line 363
>>
>>
>> if (!isRSA) {
>> CMS.debug("RecoverService: recoverKey: key to recover is
>> RSA");
>> } else {
>>
>> CMS.debug("RecoverService: recoverKey: key to recover is
>> not RSA");
>> }
>>
>> Would it not be simpler to print out the value of isRSA?
>
> It really does not matter. A smarter question would be, why did I even
> bother to have "isRSA" parameter for this method? The reason is that
> there is an existing recoverKey for RSA, and the function signature does
> not take return type into account so it doesn't allow polyinstantiation
> of the function unless I add one more parameter to it. So I added this
> isRSA param which really serves no purpose other than getting the JAVA
> compiler to comply.
> That said, I'll change it so that it prints isRSA instead.
>
>>
>> 4.
>>
>> EnrollmentService.java line 451
>>
>> metaInfo.set("EllipticCurve", oidDescription);
>>
>> I think in KeyRecord.java you have a static public member variable
>> for "EllipticCurve"
>> Probably should use that here for clarity.
>
> Will do
>
>>
>> 5.
>>
>> ASN1Util.c line 58
>>
>> SECItem *oid;
>>
>> Initialize to null?
>
> will do
>
>>
>> 6.
>>
>>
>> ASN1Util.c line 78
>>
>> oidTag = SECOID_FindOIDTag(oid);
>>
>> if (oidTag == SEC_OID_UNKNOWN) {
>>
>> JSS_throwMsg(env, INVALID_PARAMETER_EXCEPTION,
>>
>> "JSS getTagDescriptionByOid: OID UNKNOWN");
>>
>> goto finish;
>>
>> }
>>
>> What if oidTag is null? Perhaps the call can never return this but
>> some well known constant?
>
> It can't be. It was initialized to SEC_OID_UNKNOWN, and NSS returns
> SEC_OID_UNKNOWN to you as well if it can't find it.
> I will add comment to explain.
>
>
>>
>> 7.
>>
>>
>> ASN1Uti.java line 126
>>
>> Question, it looks like the routine takes in an entire public key
>> blob as input.
>> Would there be some simpler input that could end up giving us the
>> same answer? I do not know.
> There isn't any. You would have to require callers to parse down the
> pubkey if we take a smaller byte array for this function.
>
>>
>> 8.
>>
>> Also, the routine throws a InvalidBERException exception or some such.
>>
>> There are a bunch of calls to methods such as : Arrays.copyOfRange
>>
>> That method appears to throw the following exceptions:
>>
>> ArrayIndexOutOfBoundsException -
>> IllegalArgumentException -
>> NullPointerException -
>>
>> Instead of catching only an "Exception" and forcing a
>> "InvalidBERException", would it make sense to
>> declare the function to also throw those exceptions?
>>
> will do
>> 8.
>>
>>
>> Should we check for X509PubKeyBytes input parameter for null?
> will do
>>
>> 9.
>>
>>
>> File displayBySerial.template line 103
>>
>> if ((result.header.keyLength == null) || (result.header.keyLength<=
>> 0)) {
>>
>> It looks like we use the above check to assume an ECC key. Is this
>> the best way to make
>> this determination? Is there any info that actually tells us we have
>> an ECC key instead of this
>> lack of information?
>>
>> Actually it looks like this check is used everywhere in this part of
>> the patch when a decision is needed.
>>
>>
> I wonder about that myself. I use rec.EllipticCurve != null at certain
> places which worked well, but at some specific places, I couldn't for
> some reason. That's why I used<=0 check.
> I can try again.
>>
>>
>>
>>
>> ----- Original Message -----
>> From: "Christina"<cfu at redhat.com>
>> To: pki-devel at redhat.com
>> Sent: Wednesday, March 14, 2012 2:49:37 PM
>> Subject: [Pki-devel] patches for review - ECC key archival / recovery
>> feature implementation - Bug 745278 - [RFE] ECC encryption keys
>> cannot be archived
>>
>>
>>
>> This is the ECC phase 2 implementation (ECC key archival / recovery
>> feature) in the JSS and DRM (KRA)
>>
>> Bug: Bug 745278 - [RFE] ECC encryption keys cannot be archived
>>
>> Please review the following patches (see "BEFORE you review" at later
>> part of this email):
>>
>> *
>> https://bugzilla.redhat.com/attachment.cgi?id=570109&action=diff&context=patch&collapsed=&headers=1&format=raw
>> *
>> https://bugzilla.redhat.com/attachment.cgi?id=570110&action=diff&context=patch&collapsed=&headers=1&format=raw
>> *
>> https://bugzilla.redhat.com/attachment.cgi?id=570112&action=diff&context=patch&collapsed=&headers=1&format=raw
>>
>> thanks,
>> Christina
>> ==============
>> BEFORE you review:
>>
>> For the ECC plan and design for the different phases, please refer to
>> http://pki.fedoraproject.org/wiki/ECC_in_Dogtag
>>
>> Note: the designs beyond phase 2 were more like a brain dump.
>> Although I said "Do Not Review," you are free to take a peak at
>> what's intended down the road. I will go back and take a closer look
>> and refine/adjust the designs when I begin implementation for each
>> new phase.
>>
>> This patch contains code for the following packages:
>> JSS, pki-kra, pki-common, pki-util, and pki-kra-ui
>>
>> What you need to know:
>>
>> * Problem 1 - nethsm issue:
>> On the server side, if you turn on FIPS mode, in addition to nethsm,
>> you need to attach certicom as well to have ECC SSL working on the
>> server side. This problem has already been reported to Thales last
>> year and they said they'd look into putting the item on their next
>> release. Recently through a different contact, we learned there might
>> be a way to "turn it on" (still waiting for their further instruction)
>>
>> * Problem 2- Certicom issue:
>> This is a show-stopper. Initially, on the client side, I used Kai's
>> special version of Xulrunner/Firefox, attached to Certicom token, so
>> that the CRMF requests can be generated with key archival option.
>> However, I encountered (or, re-encountered) an issue with certicom
>> token. Certicom generates ECC keys with the wrong format (not PKCS7
>> conforming), which makes ECC key archival impossible on the server
>> side if you use non-certicom token with DRM (but we expect an HSM in
>> most product deployment). I have contacted Certicom for this issue,
>> and they confirmed that they indeed have such issue. We are waiting
>> for their fix.
>>
>> But then you might ask, "I thought I saw some ECC enrollment
>> profiles/javascripts being checked in? How were the tests done?" The
>> tests for those profiles were done against this ECC key
>> archival/recovery DRM prototype I implemented last year (needs to be
>> turned on manually in 8.1), where I "cheated" (yeah, that's why it's
>> called a prototype) by decrypting the private key in the CRMF on DRM,
>> and then manipulating the byte array to strip off the offending bytes
>> before archival.
>> In the real, non-prototype implementation, which is what's in this
>> patch, for security reasons, private keys are unwrapped directly onto
>> the token during key archival, so there is no way to manipulate the
>> keys in memory and bypass the Certicom issue.
>>
>> A word about Kai's special version of Xulrunner/Firefox. It is not
>> yet publicly available.
>>
>> * Problem 3- Firefox with nethsm issue:
>> Another option was to connect Kai's special version firefox with an
>> HSM to test my DRM/JSS code. However, for whatever reason, I could
>> not get SSL going between such Firefox and ECC CA ( I did not try
>> very hard though, as I have one other option -- writing my own ECC
>> CRMF generation tool. I might come back to try the nethsm Firefox
>> idea later)
>>
>> My solution (how I work on this official implementation):
>> * I hacked up a ECC CRMF tool by taking the CRMFPopClient (existing
>> in current releases), gutting out the RSA part of the code, and
>> replacing it with ECC code. I call it CRMFPopClientEC. Two types of
>> ECC key pairs could be generated: ECDSA or ECDH (That's another
>> benefit of writing my own tool -- I don't know if you can select
>> which type to generate in the Javascript... maybe you can, I just
>> don't know). I'm in no way condoning archival of signing keys!! This
>> is just a test tool.
>> This tool takes a curve name as option (along with others), generates
>> an ECC key pair, crafts up an CRMF request with key archival option,
>> and sends request directly to the specified CA. You will see a
>> "Deferred" message in the HTML response (see attachment for example)
>> Once CA agent approves the request, the archival request goes to DRM
>> and the user private key is archived.
>> For recovery, DRM agent selects key recovery, etc, and you get your
>> pkcs12.
>>
>> I did some sanity test with the pkcs12 recovered:
>> * Import the recovered pkcs12 into a certicom library:
>> pk12util -d . -h "Certicom FIPS Cert/Key Services" -i userEC.p12
>>
>> I also tested by retrieving a p12, importing it into a browser, and
>> adding the user as an agent and the user could act as agent via ssl
>> client auth to the CA.
>>
>> Finally, much of the RSA-centric code had been cleared out of the way
>> at the time when I worked on the DRM ECC prototype, so you don't see
>> much of that in this round.
>>
>> About SELinux:
>> I have a set of rules generated on my system to run in enforcing
>> mode. I do a writeup.
>>
>> For QE:
>> How to set up the servers? The internal wiki is at
>> https://wiki.idm.lab.bos.redhat.com/export/idmwiki/Working_with_ECC .
>> I might have given someone a copy of how to set up ECC CA to publish
>> on fedora.org when I worked on phase1 back a couple years ago. I will
>> put an updated copy to cover both CA and DRM when I am checking in to
>> dogtag.
>>
>> How do you test? Well, unless you want to use my CRMFPopClientEC tool
>> hooked up with a nethsm (like I did), or write your own tool, you
>> can't really test it until Certicom fixes their issue. (BTW
>> CRMFPopClientEC can also be changed to work with ceriticom, although
>> you would run into the same issue I mentioned above)
>> It is not on my schedule to work on this tool; It is certainly not in
>> production quality to be released as a regular tool. However, if you
>> are interested in it, if we get enough request maybe we can think
>> about doing something with it.
>> Other test suggestion: I did not try to clone the ECC DRM. It's a
>> good idea to test it out.
>>
>> For techpub:
>> TBD
>>
>> _______________________________________________
>> Pki-devel mailing list
>> Pki-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/pki-devel
>
>
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20120320/c393c9f4/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5130 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20120320/c393c9f4/attachment.p7s>
More information about the Pki-devel
mailing list