[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