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&gt; 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 > >

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: > https://github.com/dogtagpki/pki/pull/440 > 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: > https://github.com/dogtagpki/jss/pull/575 > > 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 <cfu(a)redhat.com&gt; 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 <ftweedal(a)redhat.com&gt; > > 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 > >> > >>

Thanks for the testing notes, Christina. Today I set up a local test CT log server using a container image. I plan to document more thoroughly but rough notes at [1]. Now to the issue I found - I have a commit[2] in a private branch. Hopefully the commit message and comments explain it well enough. Is it the last issue? Not sure yet, I finally got it compiled but haven't run the code yet. Tomorrow's adventure. [1] https://github.com/frasertweedale/notes-redhat/blob/master/ct.rst [2] https://github.com/frasertweedale/pki/tree/fix/ct-verify Cheers, Fraser

On Mon, Jun 15, 2020 at 10:45:53AM -0700, Christina Fu wrote: > Hi Fraser, > That sounds good! I just added the following page to document my "quick > test" procedure which I use during development: > https://www.dogtagpki.org/wiki/PKI_10.9_Certificate_Transparency > 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. > > I'll ask Dinesh to add his more comprehensive testing procedure to the page. > thanks!! > Christina > > On Thu, Jun 11, 2020 at 5:58 PM Fraser Tweedale <ftweedal(a)redhat.com&gt; wrote: > > > 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: > > > https://github.com/dogtagpki/pki/pull/440 > > > 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: > > > https://github.com/dogtagpki/jss/pull/575 > > > > > > 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 <cfu(a)redhat.com&gt; 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 <ftweedal(a)redhat.com&gt; > > > > 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 > > > >> > > > >> > > > >