<div dir="ltr"><div>Hi Fraser,</div><div>That sounds good! I just added the following page to document my "quick test" procedure which I use during development:</div><div><a href="https://www.dogtagpki.org/wiki/PKI_10.9_Certificate_Transparency">https://www.dogtagpki.org/wiki/PKI_10.9_Certificate_Transparency</a></div><div>btw, the verifySCT is currently enabled, but the failure is ignored. However, you could look in the debug log for "verifySCT" to see relevant debug messages. </div><div> </div><div>I'll ask Dinesh to add his more comprehensive testing procedure to the page.</div><div>thanks!!</div><div>Christina </div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jun 11, 2020 at 5:58 PM Fraser Tweedale <<a href="mailto:ftweedal@redhat.com">ftweedal@redhat.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Christina, I will find a day next week to have a close look. Probably Tuesday or Wednesday. It will help to have test environment setup documentation, i.e. how to set up a log server to test with, how to configure Dogtag, etc. If this stuff is already written then you just need to tell me where to look :) Cheers, Fraser On Thu, Jun 11, 2020 at 05:08:25PM -0700, Christina Fu wrote: > HI Fraser, > verifySCT still fails. I still think the fact the rfc does not require the > signed object to accompany the signature presents undue challenge to the > party that needs to verify the signature. Although I understand that this > is v1, and the issue would not be present in v2 since there will not be > poison extension ;-/. > I'd appreciate it if you could find time to take a closer look. > > Here is my latest attempt: > <a href="https://github.com/dogtagpki/pki/pull/440" rel="noreferrer" target="_blank">https://github.com/dogtagpki/pki/pull/440</a> > Since it's a patch against the latest code, for a full view, it would be > easier if you just apply the patch and read from "(Certificate > Transparency)" in > base/ca/src/com/netscape/ca/CAService.java > This patch would require JSS change at: > <a href="https://github.com/dogtagpki/jss/pull/575" rel="noreferrer" target="_blank">https://github.com/dogtagpki/jss/pull/575</a> > > Code still requires some refinement but I wish to address the verification > issue before cleaning things up. Of course I still let verifySCT returns > success for now just so people could still play with CT. > Much appreciated! > Christina > > On Tue, Jun 2, 2020 at 3:05 PM Christina Fu <<a href="mailto:cfu@redhat.com" target="_blank">cfu@redhat.com</a>> wrote: > > > 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 <<a href="mailto:ftweedal@redhat.com" target="_blank">ftweedal@redhat.com</a>> > > 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". > >> > > >> <a href="https://github.com/dogtagpki/pki/blob/master/base/ca/src/com/netscape/ca/CAService.java#L1209" rel="noreferrer" target="_blank">https://github.com/dogtagpki/pki/blob/master/base/ca/src/com/netscape/ca/CAService.java#L1209</a> > >> > 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? > >> > > >> <a href="https://github.com/dogtagpki/pki/blob/master/base/ca/src/com/netscape/ca/CAService.java#L951" rel="noreferrer" target="_blank">https://github.com/dogtagpki/pki/blob/master/base/ca/src/com/netscape/ca/CAService.java#L951</a> > >> > > >> 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 > >> > >> </blockquote></div>