11:19 p.m.
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
6:48 p.m.
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(","));
--
Endi S. Dewata
12:49 p.m.
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(","));
6:58 p.m.
Endi and Jack,
thanks for the acks.
pushed to master:
commit a6e67c277f8e7e75bc59659536abfe7797b8f8dc
<https://fedorahosted.org/pki/changeset/a6e67c277f8e7e75bc59659536abfe7797...
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
3680
days inactive
3681
days old
3 comments
2 participants
participants (2)
-
Christina Fu -
Endi Sukma Dewata