[Pki-devel] [PATCH] 888 part2 CA/KRA functions - TPS rewrite : provide remote authority functions

Christina Fu cfu at redhat.com
Wed Apr 9 23:58:08 UTC 2014


Endi and Jack,

thanks for the acks.

pushed to master:

commit a6e67c277f8e7e75bc59659536abfe7797b8f8dc 
<https://fedorahosted.org/pki/changeset/a6e67c277f8e7e75bc59659536abfe7797b8f8dc/>

Christina


On 04/09/2014 10:49 AM, Christina Fu wrote:
> Thanks to Endi and Jack for the review comments.  Also special thanks 
> to Adam for his git assistance for producing proper patch name.
> Please see the attachment for the updated patch that addressed the 
> comments.
>
> Also, just for the record, for Endi's comment #2 and #3.  The changes 
> were intentional and I actually initiated the discussion with jack at 
> the time when I made the changes and we agreed that it was most likely 
> not going to affect anyone, based on questions we have seen in the 
> dogtag community (never heard anyone setting up TMS), and if so, the 
> changes could be reverted easily.
> Yes, it is a good idea to have that separate ticket to discuss 
> backward compatibility.  We will leave that discussion until then.
>
> thanks,
> Christina
>
> On 04/08/2014 04:48 PM, Endi Sukma Dewata wrote:
>> On 4/7/2014 11:19 PM, Christina Fu wrote:
>>> Attached please find patch to #888 TPS rewrite: provide remote 
>>> authority
>>> functions
>>>                                 - part 2: CA and KRA functions
>>>
>>> In this patch, most all of the remaining remote (CA and KRA
>>> specifically) functions are converted from the old tps c++ code to 
>>> Java.
>>> Including:
>>> CA: Enrollment, Renewal, Revocation, Unrevocation
>>>       For revocation/unrevocation specifically, CA discovery for
>>> revocation routing support
>>> KRA: Server-Side Key Generation/key archival, Key Recovery
>>>
>>> One caveat is that since the Secure Channel is not yet ready, many of
>>> the functionalities (pretty much anything other than
>>> revocation/unrevocation) can only be tested minimally  The major "TODO"
>>> item is mainly figuring out the proper data/structure conversion.  For
>>> example, the ECC curve to oid mappings in the original TPS C++ code is
>>> most likely not necessary as JSS code and existing CS java code most
>>> likely provide that, so I am not going to write that until we can
>>> actually test out those affected remote functions and find out what
>>> exactly we need (or not).
>>>
>>> A separate ticket was filed to capture the remaining processor 
>>> functions -
>>> https://fedorahosted.org/pki/ticket/941-
>>>                Rewrite: Enrollment, Recovery, KeyRecovery,
>>> revoke/unrevoke processor
>>> The final data/structure conversion will be finalized at that time when
>>> end-to-end testing is available
>>>
>>> You will also find some changes in the tks (submitted in part 1) area.
>>> They are just some improvements to conform with the new CA and KRA 
>>> code.
>>>
>>> thanks,
>>> Christina
>>
>> Some comments:
>>
>> 1. Please use a numbering system for the patches. It would be more 
>> difficult to find a particular patch among other patch files if they 
>> are not numbered. Here's a proposed system: 
>> http://pki.fedoraproject.org/wiki/Patches
>>
>> 2. As discussed on IRC, the DoRevokeTPS and DoUnrevokeTPS now uses 
>> "&" for name-value pair separator. It would make sense to change the 
>> content type of the response to application/form-url-encoded as well 
>> to formalize the format.
>>
>> 3. As discussed on IRC, the changes done in #2 and in 
>> GenerateKeyPairServlet (CUID -> tokencuid) probably breaks backward 
>> compatibility with pre-10.2 systems. Ticket #957 will determine if 
>> backward compatibility is necessary. If it is, we may need to revert 
>> these changes.
>>
>> 4. In CARemoteRequestHandler enrollCertificate() and 
>> renewCertificate() and the content is parsed before checking if it's 
>> null/empty:
>>
>>   XMLObject xmlResponse = getXMLparser(content);
>>
>>   if (content != null && !content.equals("")) {
>>       ...
>>   }
>>
>> The code works fine because getXMLparser() checks if the parameter is 
>> null, but it would make more sense to call getXMLparser() after 
>> making sure that the content is not null/empty.
>>
>> 5. Some null initializations are redundant, so the following lines 
>> can be combined:
>>
>>   String value = null;
>>   value = xmlResponse.getValue(...);
>>
>>   String value = null;
>>   value = (String) response.get(...);
>>
>> 6. The following lines can be simplified:
>>
>>   Iterator<String> iterator = caList.iterator();
>>   while (iterator.hasNext()) {
>>       try {
>>           String caSkiString = getCaSki(iterator.next());
>>
>> using for-each:
>>
>>   for (String ca : caList) {
>>       try {
>>           String caSkiString = getCaSki(ca);
>>
>> 7. The final exception in revokeFromOtherCA() could be misleading. 
>> Suppose a signing CA is found, but there's an error while 
>> revoking/unrevoking the certificate, the exception will be swallowed 
>> by the catch clause inside the loop. Later the code will incorrectly 
>> throw a new exception saying signing CA not found. It might be better 
>> to throw the last exception caught in the loop.
>>
>>   Exception exception = null;
>>   for (String ca : caList) {
>>       try {
>>           ...
>>       } catch (Exception e) {
>>           exception = e;
>>       }
>>   }
>>
>>   if (exception == null) {
>>       throw new EBaseException("Signing CA not found");
>>   } else {
>>       throw exception;
>>   }
>>
>> 8. The first try-catch clause in getCaSki() swallows all exceptions. 
>> If a property is missing, the getString() will throw 
>> EPropertyNotFound. This is probably the exception that should be 
>> detected. Other errors should not be ignored.
>>
>>   try {
>>       String configName = "tps.connector." + conn + ".caSKI";
>>       return conf.getString(configName);
>>
>>   } catch (EPropertyNotFound e) {
>>       // caSKI not found, continue below
>>
>>   } catch (EBaseException e) {
>>       throw e; // other error, rethrow
>>   }
>>
>>   // calculate caSKI
>>
>> 9. The second try-catch clause in getCaSki() replaces the original 
>> EBaseException with a new EBaseException that's missing the original 
>> info. It should just rethrow the original EBaseException.
>>
>>   try {
>>       ...
>>
>>   } catch (EBaseException e) {
>>       throw e;
>>
>>   } catch (NotInitializedException e) {
>>       throw new EBaseException(...);
>>   }
>>
>> 10. The third try-catch clause in getCaSki() replaces the original 
>> IOException with a new IOException that's missing the original info. 
>> It should just rethrow the original IOException.
>>
>>   try {
>>       ...
>>
>>   } catch (IOException e) {
>>       throw e;
>>
>>   } catch (Exception e) {
>>       throw new EBaseException(...);
>>   }
>>
>> 11. There are redundant parentheses in the following lines:
>>
>>   if ((pubKeybuf == null) || (uid == null) || (cuid == null)) {
>>   if ((serialno == null) || (reason == null)) {
>>   if ((revoke == true) && (reason == null)) {
>>   if ((cuid == null) || (userid == null) || (sDesKey == null)) {
>>   if ((cuid == null) || (userid == null) || (sDesKey == null)
>>       || (b64cert == null)) {
>>   if ((cuid == null) || (keyInfo == null) || (card_challenge == null)
>>       || (card_cryptogram == null) || (host_challenge == null)) {
>>   if ((cuid == null) || (NewMasterVer == null) || (version == null)) {
>>   if ((cuid == null) || (version == null) || (inData == null)) {
>>   return (CMS.BtoA(kid.getIdentifier()).trim());
>>
>> 12. In revokeCertificate() the code relies on IOException to 
>> determine whether to skip matching. This is uncommon because 
>> IOException is usually used to detect system error. Is there a more 
>> specific exception that it should catch instead? Or maybe it should 
>> catch all exceptions in general?
>>
>>   try {
>>       caSkiString = getCaSki(connid);
>>       certAkiString = getCertAkiString(cert);
>>
>>   } catch (Exception e) {
>>       skipMatch = true; // skip match on any error
>>   }
>>
>> 13. In ConnectionManager the CA routing list can be initialized as 
>> follows:
>>
>>   List<String> caList;
>>   caList = Arrays.asList(caListString.split(","));
>>
>>
>
>
>
> _______________________________________________
> 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/20140409/edb91f2c/attachment.htm>


More information about the Pki-devel mailing list