<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Endi and Jack,<br>
<br>
thanks for the acks.<br>
<div class="comment searchable">
<p>
pushed to master:
</p>
<p>
commit <a title="trac ticket #888 part2 CA/KRA functions - TPS
rewrite: provide remote ..."
href="https://fedorahosted.org/pki/changeset/a6e67c277f8e7e75bc59659536abfe7797b8f8dc/"
class="changeset">a6e67c277f8e7e75bc59659536abfe7797b8f8dc</a><br>
</p>
<p>Christina<br>
</p>
</div>
<br>
<div class="moz-cite-prefix">On 04/09/2014 10:49 AM, Christina Fu
wrote:<br>
</div>
<blockquote cite="mid:53458835.20905@redhat.com" type="cite">Thanks
to Endi and Jack for the review comments. Also special thanks to
Adam for his git assistance for producing proper patch name.
<br>
Please see the attachment for the updated patch that addressed the
comments.
<br>
<br>
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.
<br>
Yes, it is a good idea to have that separate ticket to discuss
backward compatibility. We will leave that discussion until then.
<br>
<br>
thanks,
<br>
Christina
<br>
<br>
On 04/08/2014 04:48 PM, Endi Sukma Dewata wrote:
<br>
<blockquote type="cite">On 4/7/2014 11:19 PM, Christina Fu wrote:
<br>
<blockquote type="cite">Attached please find patch to #888 TPS
rewrite: provide remote authority
<br>
functions
<br>
- part 2: CA and KRA functions
<br>
<br>
In this patch, most all of the remaining remote (CA and KRA
<br>
specifically) functions are converted from the old tps c++
code to Java.
<br>
Including:
<br>
CA: Enrollment, Renewal, Revocation, Unrevocation
<br>
For revocation/unrevocation specifically, CA discovery
for
<br>
revocation routing support
<br>
KRA: Server-Side Key Generation/key archival, Key Recovery
<br>
<br>
One caveat is that since the Secure Channel is not yet ready,
many of
<br>
the functionalities (pretty much anything other than
<br>
revocation/unrevocation) can only be tested minimally The
major "TODO"
<br>
item is mainly figuring out the proper data/structure
conversion. For
<br>
example, the ECC curve to oid mappings in the original TPS C++
code is
<br>
most likely not necessary as JSS code and existing CS java
code most
<br>
likely provide that, so I am not going to write that until we
can
<br>
actually test out those affected remote functions and find out
what
<br>
exactly we need (or not).
<br>
<br>
A separate ticket was filed to capture the remaining processor
functions -
<br>
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/pki/ticket/941">https://fedorahosted.org/pki/ticket/941</a>-
<br>
Rewrite: Enrollment, Recovery, KeyRecovery,
<br>
revoke/unrevoke processor
<br>
The final data/structure conversion will be finalized at that
time when
<br>
end-to-end testing is available
<br>
<br>
You will also find some changes in the tks (submitted in part
1) area.
<br>
They are just some improvements to conform with the new CA and
KRA code.
<br>
<br>
thanks,
<br>
Christina
<br>
</blockquote>
<br>
Some comments:
<br>
<br>
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:
<a class="moz-txt-link-freetext" href="http://pki.fedoraproject.org/wiki/Patches">http://pki.fedoraproject.org/wiki/Patches</a>
<br>
<br>
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.
<br>
<br>
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.
<br>
<br>
4. In CARemoteRequestHandler enrollCertificate() and
renewCertificate() and the content is parsed before checking if
it's null/empty:
<br>
<br>
XMLObject xmlResponse = getXMLparser(content);
<br>
<br>
if (content != null && !content.equals("")) {
<br>
...
<br>
}
<br>
<br>
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.
<br>
<br>
5. Some null initializations are redundant, so the following
lines can be combined:
<br>
<br>
String value = null;
<br>
value = xmlResponse.getValue(...);
<br>
<br>
String value = null;
<br>
value = (String) response.get(...);
<br>
<br>
6. The following lines can be simplified:
<br>
<br>
Iterator<String> iterator = caList.iterator();
<br>
while (iterator.hasNext()) {
<br>
try {
<br>
String caSkiString = getCaSki(iterator.next());
<br>
<br>
using for-each:
<br>
<br>
for (String ca : caList) {
<br>
try {
<br>
String caSkiString = getCaSki(ca);
<br>
<br>
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.
<br>
<br>
Exception exception = null;
<br>
for (String ca : caList) {
<br>
try {
<br>
...
<br>
} catch (Exception e) {
<br>
exception = e;
<br>
}
<br>
}
<br>
<br>
if (exception == null) {
<br>
throw new EBaseException("Signing CA not found");
<br>
} else {
<br>
throw exception;
<br>
}
<br>
<br>
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.
<br>
<br>
try {
<br>
String configName = "tps.connector." + conn + ".caSKI";
<br>
return conf.getString(configName);
<br>
<br>
} catch (EPropertyNotFound e) {
<br>
// caSKI not found, continue below
<br>
<br>
} catch (EBaseException e) {
<br>
throw e; // other error, rethrow
<br>
}
<br>
<br>
// calculate caSKI
<br>
<br>
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.
<br>
<br>
try {
<br>
...
<br>
<br>
} catch (EBaseException e) {
<br>
throw e;
<br>
<br>
} catch (NotInitializedException e) {
<br>
throw new EBaseException(...);
<br>
}
<br>
<br>
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.
<br>
<br>
try {
<br>
...
<br>
<br>
} catch (IOException e) {
<br>
throw e;
<br>
<br>
} catch (Exception e) {
<br>
throw new EBaseException(...);
<br>
}
<br>
<br>
11. There are redundant parentheses in the following lines:
<br>
<br>
if ((pubKeybuf == null) || (uid == null) || (cuid == null)) {
<br>
if ((serialno == null) || (reason == null)) {
<br>
if ((revoke == true) && (reason == null)) {
<br>
if ((cuid == null) || (userid == null) || (sDesKey == null)) {
<br>
if ((cuid == null) || (userid == null) || (sDesKey == null)
<br>
|| (b64cert == null)) {
<br>
if ((cuid == null) || (keyInfo == null) || (card_challenge ==
null)
<br>
|| (card_cryptogram == null) || (host_challenge == null))
{
<br>
if ((cuid == null) || (NewMasterVer == null) || (version ==
null)) {
<br>
if ((cuid == null) || (version == null) || (inData == null)) {
<br>
return (CMS.BtoA(kid.getIdentifier()).trim());
<br>
<br>
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?
<br>
<br>
try {
<br>
caSkiString = getCaSki(connid);
<br>
certAkiString = getCertAkiString(cert);
<br>
<br>
} catch (Exception e) {
<br>
skipMatch = true; // skip match on any error
<br>
}
<br>
<br>
13. In ConnectionManager the CA routing list can be initialized
as follows:
<br>
<br>
List<String> caList;
<br>
caList = Arrays.asList(caListString.split(","));
<br>
<br>
<br>
</blockquote>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
Pki-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:Pki-devel@redhat.com">Pki-devel@redhat.com</a>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/pki-devel">https://www.redhat.com/mailman/listinfo/pki-devel</a></pre>
</blockquote>
<br>
</body>
</html>