5:05 p.m.
Hi Fraser,
Thanks for the response!
Regarding the poison extension, yes I was aware that it needed to be
removed so the code already had it removed. It was the order of things
left inside tbsCert that I was concerned about since I used the existing
delete method provided for the Extension class, which I wasn't sure if it'd
preserve the order of the remaining extensions. Thanks for confirming my
suspicion. I will double-check the order.
Also thanks for the input on how to handle failed CT log communication v.s.
response verification failure. I will address them separately as
suggested.
Finally, nice catch with the missing data length!! I'll add that and go
from there.
thanks again!
Christina
On Mon, Jun 1, 2020 at 7:31 PM Fraser Tweedale <ftweedal(a)redhat.com> wrote:
Hi Christina,
Adding pki-devel@ for wider audience. Comments below.
On Mon, Jun 01, 2020 at 06:28:42PM -0700, Christina Fu wrote:
> Hi Fraser,
> Do you know how the signature returned in the SCT response could be
> verified by the CA?
> My thought is that the CA should somehow verify the CT response after
> sending the add-pre-chain request and before signing the cert. Since log
> inclusion verification would not be feasible right after the request (the
> SCT response is supposed to be just a "promise, according to the rfc), I
> ruled that out and intend to stay with just the following two
verifications
> on the response itself:
>
> - checking if log id (CT log signer public key hash) returned in the
CT
> response is correct
> - this I have no problem verifying
> - Verifying if the signature returned in the CT response is correct
> - this I can't seem to get it working.
>
> I put the verification work above in the method "verifySCT".
>
https://github.com/dogtagpki/pki/blob/master/base/ca/src/com/netscape/ca/...
> What I am wondering is how this can be done properly. Since the tbsCert
is
> not to contain the poison extension, the poison extension needs to be
> removed by the CT server before it signs. What if the order of the
> extensions contained in the tbsCert gets changed in the process?
>
The poison extension must be removed, and care must be taken to keep
everything else in the same order, and reserialise the parts in
exactly the same way.
> It seems that the response should contain the tbsCert that it signs
(which
> isn't per the rfc) or I am not sure how the CA could verify the
signature.
>
The response does not contain the TBCCertificate. Both sides (log
and client) remove the poison extension (and change nothing else),
then both sides can sign the same data.
> So the question now is if I should just leave out the check, unless you
> have other suggestions.
>
I do think we should verify the signature, to ensure the message was
actually received by the log server we wanted and not an impostor.
> Of course, I also could have missed something in my code.
>
The binary format is complex and it's easy to miss something. After
you implement removal of the poison extension, if it is still not
working I will go over the code with a fine-tooth comb.
I copied some of the code and left comments below, too. Comments
begin with `!!'. I think I found one bug and a couple of possible
improvements.
> One last question, currently in the code, if verifySCT fails, I just
> "continue" to process for next CT log. Should this just bail out all
> together or it's fine to continue? Or could this be a choice by the
admin.
> What do you think and why?
>
https://github.com/dogtagpki/pki/blob/master/base/ca/src/com/netscape/ca/...
>
My line of thinking is this:
- we should tolerate communication errors with log (perhaps
enqueuing the cert for a retry later)
- but (assuming we implement it correctly) verifySCT failure is
indicative of something wrong with the log (e.g. key changed); it
is not a communication error and can be treated differently.
- I think it's OK to fail hard. Admins will likely want to know if
something is wrong with CT logging.
- But in case we make a mistake, or an org needs issuance to
continue despite CT log misbehaviour, there should be a config
knob to allow this condition to be ignored. "In case of
emergency..."
>
> thanks,
> Christina
boolean verifySCT(CTResponse response, byte[] tbsCert, String
logPublicKey)
throws Exception {
/* ... SNIP ... */
byte[] extensions = new byte[] {0, 0};
!! although no extensions have been defined I think we should we take
extensions from the CT response to future-proof this code. i.e.
decode the base64 data from the "extensions" field, and prepend the
length.
// piece them together
int data_len = version.length + signature_type.length +
timestamp.length + entry_type.length +
issuer_key_hash.length + tbsCert.length +
extensions.length;
logger.debug(method + " data_len = "+ data_len);
ByteArrayOutputStream ostream = new ByteArrayOutputStream();
ostream.write(version);
ostream.write(signature_type);
ostream.write(timestamp);
ostream.write(entry_type);
ostream.write(issuer_key_hash);
ostream.write(tbsCert);
!! I believe you need to prepend the length of tbsCert as a
THREE-byte length field, because its definition is
`opaque TBSCertificate<1..2^24-1>;'
ostream.write(extensions);
byte[] data = ostream.toByteArray();
// Now, verify the signature
// Note: this part currently does not work; see method comment
above
// cfu ToDo: interpret the alg bytes later; hardcode for now
Signature signer = Signature.getInstance("SHA256withEC",
"Mozilla-JSS");
signer.initVerify(log_pubKey);
signer.update(data);
!! We could call signer.update() multiple times instead of making an
intermediate ByteArrayOutputStream. I do not care about the
performance, just whatever might simplify the routine.
if (!signer.verify(signature)) {
logger.error(method + "failed to verify SCT signature; pass
for now");
// this method is not yet working; Let this pass for now
// return false;
} else {
logger.debug(method + "SCT signature verified successfully");
}
logger.debug("verifySCT ends");
return true;
}
Cheers,
Fraser