Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1414923007:
Add initial code for verifying a certificate chain. (Closed)
DescriptionAdd initial code for verifying a certificate chain.
The code handles many of RFC 5280's requirements: BasicConstraints, KeyUsage, Signature, Validity.
But is lacking Revocation checks, subjectAltName, ExtendedKeyUsage and name constraints.
The unit-tests depend on data that was reviewed separately in https://codereview.chromium.org/1414393008/
BUG=410574
Committed: https://crrev.com/c1aac5aad208df58fc0c53af88b6c2ca6ab99bfc
Cr-Commit-Position: refs/heads/master@{#365987}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : remove unused todo #Patch Set 4 : rebase #Patch Set 5 : rebase (ParseExtensions() changed, leading to different design here) #
Total comments: 4
Patch Set 6 : rebase #Patch Set 7 : Add missing headers #
Total comments: 14
Patch Set 8 : Start tackling Matt's comments #
Total comments: 21
Patch Set 9 : first pass at addressing feedback #
Total comments: 20
Patch Set 10 : More direclty mirror the structure of RFC 5280 section 6.1 to address review comments #Patch Set 11 : rebase (and fix a comment) #
Total comments: 20
Patch Set 12 : address latest comments #Patch Set 13 : fix compile #Patch Set 14 : add data dependency copy rule for ios #Patch Set 15 : ifdef out the tests on ios #
Messages
Total messages: 38 (15 generated)
Description was changed from ========== Add initial code for verifying a certificate chain. The code handles many of the normative requirements of RFC 5280 including: BasicConstraints KeyUsage Signature Validity But is also incomplete, most notably missing: Revocation checks subjectAltName ExtendedKeyUsage BUG=410574 ========== to ========== Add initial code for verifying a certificate chain. The code handles many of the normative requirements of RFC 5280 including: BasicConstraints KeyUsage Signature Validity But is also incomplete, most notably missing: Revocation checks subjectAltName ExtendedKeyUsage Note that this change depends on test data that is added by https://codereview.chromium.org/1414393008/ BUG=410574 ==========
eroman@chromium.org changed reviewers: + rsleevi@chromium.org
eroman@chromium.org changed reviewers: + davidben@chromium.org, mattm@chromium.org - rsleevi@chromium.org
Changing reviewer to Matt/David, since Ryan is OOO.
ping
https://codereview.chromium.org/1414923007/diff/80001/net/cert/internal/verif... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/80001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain.cc:32: // issuerAltName. Did you mean "consider issuerAltName" like "should also check issuerAltName" or "should consider whether checking issuerAltName is needed"? (Since 5280 section 4.2.1.7 says "Issuer alternative names are not processed as part of the certification path validation algorithm in Section 6. (That is, issuer alternative names are not used in name chaining and name constraints are not enforced.)", I guess it's the second?) https://codereview.chromium.org/1414923007/diff/80001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain.cc:76: // Extract values of interested from the parsed Certificate. interest https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:149: // TODO(eroman): Should account for normalization (taht work is part of a that https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:327: // CAs MUST NOT include the pathLenConstraint field unless the cA I wonder if it would be more clear to just do the check explicitly on each cert? It's explicitly checked here but just implicitly checked for intermediates. I found it a little hard to follow through the various code paths to ensure the condition is met for all cases. (Also see second part of comment below.) https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:341: // Constraints was omitted, should keyCertSign be allowed even though it I would interpret it as "basic constraints not present" = "cA is not asserted", based on "If the basic constraints extension is not present in a version 3 certificate, or the extension is present but the cA boolean is not asserted, then the certified public key MUST NOT be used to verify certificate signatures." (At least for v3 certs.) https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:344: cert.key_usage.AssertsBit(KEY_USAGE_BIT_KEY_CERT_SIGN)) { An alternate question is, if Key Usage extension is not present, does that count as asserting keyCertSign, since we allow all uses if the key usage extension isn't present? This check says no, but the implicit check for "CAs MUST NOT include the pathLenConstraint field unless the cA boolean is asserted and the key usage extension asserts the keyCertSign bit." in the issuer path says yes. https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:375: // TODO(eroman): Relax this and allow the caller to decide. This could also be an argument for moving some of the correctness checks out of VerifyTargetCertificateIsEndEntity.
https://codereview.chromium.org/1414923007/diff/80001/net/cert/internal/verif... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/80001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain.cc:32: // issuerAltName. On 2015/11/11 23:55:44, mattm wrote: > Did you mean "consider issuerAltName" like "should also check issuerAltName" or > "should consider whether checking issuerAltName is needed"? > > (Since 5280 section 4.2.1.7 says "Issuer alternative names are not processed as > part of the certification path validation algorithm in Section 6. (That is, > issuer alternative names are not used in name chaining and name constraints are > not enforced.)", I guess it's the second?) > Thanks, Done. The TODO was basically to figure out what is needed for issuerAltName. From the text you pointed me to sounds like no processing is actually needed for it, so I have removed the TODO. https://codereview.chromium.org/1414923007/diff/80001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain.cc:76: // Extract values of interested from the parsed Certificate. On 2015/11/11 23:55:44, mattm wrote: > interest Done. https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:149: // TODO(eroman): Should account for normalization (taht work is part of a On 2015/11/11 23:55:44, mattm wrote: > that Done. (The name matching functionality is now checked in, but I think I will still leave wiring that up to a follow-up CL). https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:327: // CAs MUST NOT include the pathLenConstraint field unless the cA On 2015/11/11 23:55:44, mattm wrote: > I wonder if it would be more clear to just do the check explicitly on each cert? > It's explicitly checked here but just implicitly checked for intermediates. I > found it a little hard to follow through the various code paths to ensure the > condition is met for all cases. (Also see second part of comment below.) Let me try some refactorings out and get back to you... https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:341: // Constraints was omitted, should keyCertSign be allowed even though it On 2015/11/11 23:55:44, mattm wrote: > I would interpret it as "basic constraints not present" = "cA is not asserted", > based on "If the basic constraints extension is not present in a version 3 > certificate, or the extension is present but the cA boolean is not asserted, > then the certified public key MUST NOT be used to verify certificate > signatures." (At least for v3 certs.) On a related note, Firefox does not enforce the constraint below. Here is their comment: // RFC 5280 says "The keyCertSign bit is asserted when the subject public // key is used for verifying signatures on public key certificates. If the // keyCertSign bit is asserted, then the cA bit in the basic constraints // extension (Section 4.2.1.9) MUST also be asserted." // However, we allow end-entity certificates (i.e. certificates without // basicConstraints.cA set to TRUE) to claim keyCertSign for compatibility // reasons. This does not compromise security because we only allow // certificates with basicConstraints.cA set to TRUE to act as CAs. https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:344: cert.key_usage.AssertsBit(KEY_USAGE_BIT_KEY_CERT_SIGN)) { On 2015/11/11 23:55:44, mattm wrote: > An alternate question is, if Key Usage extension is not present, does that count > as asserting keyCertSign, since we allow all uses if the key usage extension > isn't present? I would say no, because that would effectively mean that the KeyUsage extension is mandatory on end-entity certificates. (Since by that interpretation, lack of the KeyUsage means the certSign bit is asserted) > > This check says no, but the implicit check for "CAs MUST NOT include the > pathLenConstraint field unless the cA boolean is asserted and the key usage > extension asserts the keyCertSign bit." in the issuer path says yes. I am not sure I see the contradiction, but I will read through these again and see about at the very least re-organizing for better sanity. https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:375: // TODO(eroman): Relax this and allow the caller to decide. On 2015/11/11 23:55:44, mattm wrote: > This could also be an argument for moving some of the correctness checks out of > VerifyTargetCertificateIsEndEntity. Yeah, VerifyTargetCertificateIsEndEntity() is definitely a temporary code organization. This function should probably take parameters for EKU and KeyUsage, or leave the target certificate verification elsewhere. I had mostly put this in as a quick-and-dirty since my tests all assumed verification of end-entity certs, but maybe now is the time to clean it up some more. Do you have suggestions?
https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:344: cert.key_usage.AssertsBit(KEY_USAGE_BIT_KEY_CERT_SIGN)) { On 2015/11/12 01:58:43, eroman wrote: > On 2015/11/11 23:55:44, mattm wrote: > > An alternate question is, if Key Usage extension is not present, does that > count > > as asserting keyCertSign, since we allow all uses if the key usage extension > > isn't present? > > I would say no, because that would effectively mean that the KeyUsage extension > is mandatory on end-entity certificates. (Since by that interpretation, lack of > the KeyUsage means the certSign bit is asserted) Good point. > > > > > This check says no, but the implicit check for "CAs MUST NOT include the > > pathLenConstraint field unless the cA boolean is asserted and the key usage > > extension asserts the keyCertSign bit." in the issuer path says yes. > > I am not sure I see the contradiction, but I will read through these again and > see about at the very least re-organizing for better sanity. VerifyKeyUsageForIssuer first checks "if (!cert.has_key_usage) return true", and then checks for KEY_USAGE_BIT_KEY_CERT_SIGN. So a cert that has cA=true and has a pathLenConstraint, but does not have a keyUsage extension would be accepted, even though keyCertSign bit is not asserted.
https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:148: // different change). Link to a bug or something? "that work is part of a different change" is an odd thing to see in the source code after a change has landed. https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:199: // The mechanism through which equality is determined is unspecified. It's an ASN.1 structure, so I think the natural interpretation is ASN.1's notion of equality, which should be the same as byte-for-byte equality in DER. Rather than have to route through an Equals, perhaps we should do byte-for-byte equality and, if they fail, just check for sha1WithRSAEncryption and sha1WithRSASignature? (If you want to be super-lazy, we could even just write out their DER serializations somewhere and memcmp the two.) That we we don't need to have code for comparing funny kinds of parameters or implement it for future ones. If we end up having to grow a looseness in SignatureAlgorithm parsing later or they have two ways to express the same thing, we can keep this line in the sand and prevent it from getting worse. https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:236: // issuance of other certificates. Might be worth a comment that |current_cert_index| is counting from the end-entity, starting from zero. |cert| must be an issuer, so |current_cert_index| may not be zero. Or something like that. https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:241: DCHECK_GT(current_cert_index, 0u); DCHECK_LT(num_prev_self_issued_certs, current_cert_index); https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:291: // TODO(eroman): subjectAltName and issuerAltName ? This TODO should also be removed, right? https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:340: // doesn't make sense?) My interpretation is {cA boolean is asserted} means {has key usage AND key usage has cA bit}, so {cA boolean is not asserted} should include the missing key usage xtn case. https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:384: for (size_t i = 0; i < certs_der.size(); ++i) { It seems RFC 5280 6.1 actually specifies a different algorithm which walks in the other order. It then has a somewhat inverted algorithm for checking max_path_len. Initialize to n, decrement on non-self-issued cert (fail if you can't), take minimum every time you see a path length constraint. I'm wondering why you choose to do it in this direction. I haven't thought about this very hard so I don't know which order is cleaner for the more complex checks. The RFC's algorithm does have to intersect sets of permitted subtrees for name constraints (or do some O(N^2) thing), which is rather a bother. But I don't if the other direction makes it any easier to avoid that. https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:389: if (!VerifyValidity(cert, time)) At no point do we verify that the the certificate isn't valid on February 31s or some such. Not worth bothering? (Probably not.) https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.h (right): https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.h:27: struct NET_EXPORT TrustedRoot { Optional: The spec seems to say "trust anchor" everywhere, so maybe TrustAnchor? Though we do say "known root". *shrug* Your call. https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain_unittest.cc (right): https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain_unittest.cc:76: pem_headers.push_back(kResultHeader); (We should try to get cxx@chromium.org to allow initializer_list...) https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain_unittest.cc:97: *verify_result = block_data == "SUCCESS"; Nit: Probably good to explode if block_data isn't one of "SUCCESS" or "WHATEVER_THE_OTHER_ONE_IS".
https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:327: // CAs MUST NOT include the pathLenConstraint field unless the cA On 2015/11/12 01:58:43, eroman wrote: > On 2015/11/11 23:55:44, mattm wrote: > > I wonder if it would be more clear to just do the check explicitly on each > cert? > > It's explicitly checked here but just implicitly checked for intermediates. I > > found it a little hard to follow through the various code paths to ensure the > > condition is met for all cases. (Also see second part of comment below.) > > Let me try some refactorings out and get back to you... I believe I have improved this with the new code organization. https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:344: cert.key_usage.AssertsBit(KEY_USAGE_BIT_KEY_CERT_SIGN)) { On 2015/11/12 03:00:09, mattm wrote: > On 2015/11/12 01:58:43, eroman wrote: > > On 2015/11/11 23:55:44, mattm wrote: > > > An alternate question is, if Key Usage extension is not present, does that > > count > > > as asserting keyCertSign, since we allow all uses if the key usage extension > > > isn't present? > > > > I would say no, because that would effectively mean that the KeyUsage > extension > > is mandatory on end-entity certificates. (Since by that interpretation, lack > of > > the KeyUsage means the certSign bit is asserted) > > Good point. > > > > > > > > > This check says no, but the implicit check for "CAs MUST NOT include the > > > pathLenConstraint field unless the cA boolean is asserted and the key usage > > > extension asserts the keyCertSign bit." in the issuer path says yes. > > > > I am not sure I see the contradiction, but I will read through these again and > > see about at the very least re-organizing for better sanity. > > VerifyKeyUsageForIssuer first checks "if (!cert.has_key_usage) return true", > and then checks for KEY_USAGE_BIT_KEY_CERT_SIGN. So a cert that has cA=true and > has a pathLenConstraint, but does not have a keyUsage extension would be > accepted, even though keyCertSign bit is not asserted. There is alternate (simpler) wording of these constraints in 6.1., such as: .... (n) If a key usage extension is present, verify that the keyCertSign bit is set. ... I have pored over the language for a while, and I am fairly confident that the intended meaning is: * A certificate shall not be considered a CA except if it has basic constraints, and CA=true (you are free to reject any v1 and v2 certificate). * The pathLenConstraint must only be set if it is a CA. * The keyCertSign bit must only be set if it is a CA. From this I interpret: - if the pathLenConstraint is set on a cert which has CA=false we reject it - If the keyUsages extension is missing, we are good (both for CA=true and CA=false). - If the keyUsages extension is specified, and keyCertSign is asserted, but CA=false, then we reject. This sounds reasonable since we cannot truly consider the certificate as having the keyCertSign privilege, given that it isn't actually a CA. So it does represent a contradiction. https://codereview.chromium.org/1414923007/diff/120001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:375: // TODO(eroman): Relax this and allow the caller to decide. On 2015/11/12 01:58:43, eroman wrote: > On 2015/11/11 23:55:44, mattm wrote: > > This could also be an argument for moving some of the correctness checks out > of > > VerifyTargetCertificateIsEndEntity. > > Yeah, VerifyTargetCertificateIsEndEntity() is definitely a temporary code > organization. This function should probably take parameters for EKU and > KeyUsage, or leave the target certificate verification elsewhere. > > I had mostly put this in as a quick-and-dirty since my tests all assumed > verification of end-entity certs, but maybe now is the time to clean it up some > more. Do you have suggestions? I have re-organized the code to avoid this problem. (In fact I removed the requirement that the target certificate be an end-entity). https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:148: // different change). On 2015/11/19 22:24:02, davidben (behind on reviews) wrote: > Link to a bug or something? "that work is part of a different change" is an odd > thing to see in the source code after a change has landed. I re-worded the TODO, and will address as follow-up. VerifyNameMatches has since landed and I should be able to use that, but it has a different API - expects the sequence tag already stripped - so will need to resolve some stuff first. https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:199: // The mechanism through which equality is determined is unspecified. On 2015/11/19 22:24:02, davidben (behind on reviews) wrote: > It's an ASN.1 structure, so I think the natural interpretation is ASN.1's notion > of equality, which should be the same as byte-for-byte equality in DER. > > Rather than have to route through an Equals, perhaps we should do byte-for-byte > equality and, if they fail, just check for sha1WithRSAEncryption and > sha1WithRSASignature? (If you want to be super-lazy, we could even just write > out their DER serializations somewhere and memcmp the two.) > > That we we don't need to have code for comparing funny kinds of parameters or > implement it for future ones. If we end up having to grow a looseness in > SignatureAlgorithm parsing later or they have two ways to express the same > thing, we can keep this line in the sand and prevent it from getting worse. Done. That sounds like a very sensible approach. I have sent out a separate CL to remove the existing code for doing SignatureAlgorithm::Equals(). https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:236: // issuance of other certificates. On 2015/11/19 22:24:02, davidben (behind on reviews) wrote: > Might be worth a comment that |current_cert_index| is counting from the > end-entity, starting from zero. |cert| must be an issuer, so > |current_cert_index| may not be zero. Or something like that. I went ahead and changed the ordering so it matches RFC 5280. https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:241: DCHECK_GT(current_cert_index, 0u); On 2015/11/19 22:24:02, davidben (behind on reviews) wrote: > DCHECK_LT(num_prev_self_issued_certs, current_cert_index); This line is no longer applicable. https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:340: // doesn't make sense?) On 2015/11/19 22:24:02, davidben (behind on reviews) wrote: > My interpretation is {cA boolean is asserted} means {has key usage AND key usage > has cA bit}, so {cA boolean is not asserted} should include the missing key > usage xtn case. Removed TODO and am enforcing this along that interpretation https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:384: for (size_t i = 0; i < certs_der.size(); ++i) { On 2015/11/19 22:24:02, davidben (behind on reviews) wrote: > It seems RFC 5280 6.1 actually specifies a different algorithm which walks in > the other order. It then has a somewhat inverted algorithm for checking > max_path_len. Initialize to n, decrement on non-self-issued cert (fail if you > can't), take minimum every time you see a path length constraint. > > I'm wondering why you choose to do it in this direction. I haven't thought about > this very hard so I don't know which order is cleaner for the more complex > checks. The RFC's algorithm does have to intersect sets of permitted subtrees > for name constraints (or do some O(N^2) thing), which is rather a bother. But I > don't if the other direction makes it any easier to avoid that. I have switched it to the other direction. (Doesn't make much difference until adding policy and name constraints anyway). https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:389: if (!VerifyValidity(cert, time)) On 2015/11/19 22:24:02, davidben (behind on reviews) wrote: > At no point do we verify that the the certificate isn't valid on February 31s or > some such. Not worth bothering? (Probably not.) I think we are already good here, but worth adding a test tomorrow. Day ranges for months are currently enforced during parsing of UTCTime/GeneralizedTime: https://code.google.com/p/chromium/codesearch#chromium/src/net/der/parse_valu... https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.h (right): https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.h:27: struct NET_EXPORT TrustedRoot { On 2015/11/19 22:24:02, davidben (behind on reviews) wrote: > Optional: The spec seems to say "trust anchor" everywhere, so maybe TrustAnchor? > Though we do say "known root". *shrug* Your call. Done. https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain_unittest.cc (right): https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain_unittest.cc:76: pem_headers.push_back(kResultHeader); On 2015/11/19 22:24:02, davidben (behind on reviews) wrote: > (We should try to get mailto:cxx@chromium.org to allow initializer_list...) Word. https://codereview.chromium.org/1414923007/diff/140001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain_unittest.cc:97: *verify_result = block_data == "SUCCESS"; On 2015/11/19 22:24:03, davidben (behind on reviews) wrote: > Nit: Probably good to explode if block_data isn't one of "SUCCESS" or > "WHATEVER_THE_OTHER_ONE_IS". Done.
https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:31: // Extensions Nit: period https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:56: // deleting it, although a bit less clear. *shrug* Deleting it seems probably right. I doubt that kind of micro-optimization matters. https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:68: // Parse the outter Certificate. outter -> outer https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:153: !(cert.tbs.validity_not_after < time)); Nit: I think the outermost parens here are unnecessary. ! < can also just be >= https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:208: // (RFC 5280 6.1.3 step a.1) Nit: period https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:237: // verify that it is a CA. These extra checks follow implicitly from: [I'm guessing we do this so that we check stuff while verifying chains which target at a CA? I'm a little puzzled since the RFC neither says to do this nor to assert that the target is *not* a CA. I guess you could consider either this logic or that hypothetical logic to fall under 6.1.5.f or leave it to the caller? What's the intended interface here?] https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:268: // From RFC 5280 6.1.4: Nit: Above you cite the letter of the step. May as well do so here too. Ditto below. https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:339: // anchor. Nit: I would probably omit the "as it does [...]" bit (and save line :-) ). It also, I dunno, is invalid because it lacks a target. And is insane for other reasons. https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:364: // the root certificate (the one signed by a trust anchor) and progressing This is a little confusing. I would think the root is the trust anchor. People often use "root" to refer to a self-signed certificate form of the trust anchor. Though I suppose if it's self-signed that's true... (still, it the first certificate were an intermediate, that would work too). Maybe just drop the "(the one signed by a trust anchor)" or say "starting from the trust anchor". This would be in the "start from the direction of the trust anchor" sense rather than the "starting from the trust anchor itself" sense. https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:390: previous_cert->tbs.subject_tlv = InputFromString(&trust_anchor->name); Might be worth adding a comment: // This is the first iteration, so |previous_cert| is otherwise default-initialized. To be clear we're assuming this. Or, better, maybe pull this code up outside the loop as part of initializing it, so that's obvious. EDIT: Actually, I guess you're either relying on this or relying on ProcessCertificateInPath only reading these two fields. I wonder if it's better to just call them previous_spki and previous_subject. Or working_issuer_name and working_spki. I intentionally mismatched the key name because the spec splits this out into working_public_key{,algorithm,parameters}. I believe this is to support DSA in X.509's horrible parameter inheritance thing. We don't support DSA and I do not believe ECDSA or RSA in X.509 allow this. This would also save your std::swap current_cert/previous_cert stuff. Though it means that, should more state from the previous certificate propagate forward, it would need to be explicitly pulled out. I dunno. I'll defer to you as to which is better. My default thought is to mimic the RFC's algorithm wherever relevant, but I haven't thought ahead any, so there may be good reasons not to.
Description was changed from ========== Add initial code for verifying a certificate chain. The code handles many of the normative requirements of RFC 5280 including: BasicConstraints KeyUsage Signature Validity But is also incomplete, most notably missing: Revocation checks subjectAltName ExtendedKeyUsage Note that this change depends on test data that is added by https://codereview.chromium.org/1414393008/ BUG=410574 ========== to ========== Add initial code for verifying a certificate chain. The code handles many of RFC 5280's requirements: BasicConstraints, KeyUsage, Signature, Validity/ But is lacking Revocation checks, subjectAltName, ExtendedKeyUsage and name constraints. The unit-tests depend on data that was reviewed separately in https://codereview.chromium.org/1414393008/ BUG=410574 ==========
Response to David's comments below. A patchset that implements the proposed resolutions will follow shortly... https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:31: // Extensions On 2015/12/04 22:36:09, davidben (behind on reviews) wrote: > Nit: period Done. I reworded this to: // Standard extensions that were parsed. and changed the comment for unconsumed_extensions to: // The remaining extensions (excludes the standard ones above). https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:56: // deleting it, although a bit less clear. On 2015/12/04 22:36:10, davidben (behind on reviews) wrote: > *shrug* Deleting it seems probably right. I doubt that kind of > micro-optimization matters. Removed comment. https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:68: // Parse the outter Certificate. On 2015/12/04 22:36:09, davidben (behind on reviews) wrote: > outter -> outer Done. Noticed the codebase has some others too: crbug.com/567351 https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:153: !(cert.tbs.validity_not_after < time)); On 2015/12/04 22:36:10, davidben (behind on reviews) wrote: > Nit: I think the outermost parens here are unnecessary. ! < can also just be >= Removed the paren. As far as using >=, der::GeneralizedTime only supports operator< right now, so will express it in terms of <. https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:208: // (RFC 5280 6.1.3 step a.1) On 2015/12/04 22:36:09, davidben (behind on reviews) wrote: > Nit: period Done. https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:237: // verify that it is a CA. These extra checks follow implicitly from: On 2015/12/04 22:36:09, davidben (behind on reviews) wrote: > [I'm guessing we do this so that we check stuff while verifying chains which > target at a CA? I'm a little puzzled since the RFC neither says to do this nor > to assert that the target is *not* a CA. I guess you could consider either this > logic or that hypothetical logic to fall under 6.1.5.f or leave it to the > caller? What's the intended interface here?] tl;dr: I will post an update where I rework this to have a more 1:1 mapping to 6.1.{4, 5}, so a length explanation is no longer needed (this may come at the cost of some code duplication). Thanks David, this may be a misunderstanding on my part. Let me explain in many words, what my thinking was in this patchset (regardless, I am re-working the code so it is hopefully easier to follow) RFC 5280 section 6.1.x breaks tests into two sections: (a) Preparation for Certificate i+1 (6.1.4) (b) Wrap-Up Procedure (6.1.5) The "preparation" processing is done for all certificates except the final one (target). It is basically the tests to ensure that certificate i is an intermediary. This structure was awkward to translate directly as it includes some duplication, and seemingly omitted a test described in 4.2.1.9. As far as duplication goes, "6.1.4.o" and "6.1.5.f" are the same thing. In this patchset I chose to extract them to a single place rather than have two separate branches that process remaining critical extensions. This leaves nothing left for 6.1.5 since this CL is not yet implementing policy checks. As such, the content in the "if (current_cert_must_be_ca) {" corresponds with the remaining tests of 6.1.4 (minus 6.1.4.o which we already extracted). The difference then is the boolean condition when I enter this code. In 6.1.4 it would only be run when !is_target_cert. However in this patchset I chose to additionally run it when the certificate is a target cert AND "looks" like a CA. This ends up enforcing the consistency checks of 4.2.1.9 on the target certificate as well (but it does duplicate step "L", which is subtle), Some concrete examples where this will make a difference is: https://codereview.chromium.org/1414393008/diff/140001/net/data/verify_certif... https://codereview.chromium.org/1414393008/diff/140001/net/data/verify_certif... The current API allows verifying both CA and end-entity certificates (same as 6.1) I believe that the above examples should fail, since the target certificates violate 4.2.1.9. An alternative code structure would be to stick to 6.1.x directly, and then run extra tests on the target certificate to enforce 4.2.1.9 (which I am working on for the next patchset update). Or am I wrong and the intent is really to not do any consistency checks on the target certificate? Another possibility is to restrict the current API to verifying only end-entity certificates, in which we would automatically reject any CA certificate. But it still leaves the open question of what to do when CA=false but the certificate sets keyCertSign. Or CA=false but the certificate species a pathLenConstraint https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:268: // From RFC 5280 6.1.4: On 2015/12/04 22:36:09, davidben (behind on reviews) wrote: > Nit: Above you cite the letter of the step. May as well do so here too. Ditto > below. This embroiled with the issue described above. (Because I changed the code between 6.1.4 and 6.1.5 this doesn't have a 1:1 mapping, as it may be run on the target certificate). Will resolve this by more directly translating 6.1.{4,5}, and then adding the corresponding comments. https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:339: // anchor. On 2015/12/04 22:36:09, davidben (behind on reviews) wrote: > Nit: I would probably omit the "as it does [...]" bit (and save line :-) ). It > also, I dunno, is invalid because it lacks a target. And is insane for other > reasons. Done. https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:364: // the root certificate (the one signed by a trust anchor) and progressing On 2015/12/04 22:36:09, davidben (behind on reviews) wrote: > This is a little confusing. I would think the root is the trust anchor. People > often use "root" to refer to a self-signed certificate form of the trust anchor. > Though I suppose if it's self-signed that's true... (still, it the first > certificate were an intermediate, that would work too). > > Maybe just drop the "(the one signed by a trust anchor)" or say "starting from > the trust anchor". This would be in the "start from the direction of the trust > anchor" sense rather than the "starting from the trust anchor itself" sense. I wiped away any references to "root" (both in comment and variable name). Note my concern with saying "starting from the trust anchor" is that the trust anchor is not actually present in "certs_der" so in a literal sense is not the first certificate iterated over. But I agree that comment does give a more useful orientation. https://codereview.chromium.org/1414923007/diff/160001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:390: previous_cert->tbs.subject_tlv = InputFromString(&trust_anchor->name); On 2015/12/04 22:36:09, davidben (behind on reviews) wrote: > Might be worth adding a comment: > // This is the first iteration, so |previous_cert| is otherwise > default-initialized. > To be clear we're assuming this. Or, better, maybe pull this code up outside the > loop as part of initializing it, so that's obvious. > > EDIT: Actually, I guess you're either relying on this or relying on > ProcessCertificateInPath only reading these two fields. I wonder if it's better > to just call them previous_spki and previous_subject. Or working_issuer_name and > working_spki. I intentionally mismatched the key name because the spec splits > this out into working_public_key{,algorithm,parameters}. I believe this is to > support DSA in X.509's horrible parameter inheritance thing. We don't support > DSA and I do not believe ECDSA or RSA in X.509 allow this. > > This would also save your std::swap current_cert/previous_cert stuff. Though it > means that, should more state from the previous certificate propagate forward, > it would need to be explicitly pulled out. I dunno. I'll defer to you as to > which is better. My default thought is to mimic the RFC's algorithm wherever > relevant, but I haven't thought ahead any, so there may be good reasons not to. I considered those various options too, here were my thoughts: (1) Pulling the initialization of previous_cert out of the loop: This causes some ugliness around having to parse the first certificate twice (once to extract the anchor information, and another as part of the regular processing). This duplicate parsing can be solved by either caching the parsed information for first certificate, or parsing all the certificates in the chain to an array upfront. (2) Extracting state variables: This has the same concern with avoiding duplicating the parsing of first certificate. I also personally found the "working_*" variables to be less clear on what comparisons are being made, but can certainly buy the argument for consistency with RFC 5280. I'll go ahead and change to option (2) per your recommendation. I will probably leave the initialization inside of the loop body though to avoid the duplicate-parsing issue for the first certificate. Lastly there is an inefficiency in that the issuer name is being compared against the working subject name for the first certificate. But based on how we did the lookup in the certificate store this is necessarily true. I don't think this is worth "fixing" though.
I have posted a new patchset that addresses the review feedback, please take another look. The structure of the code now matches that of RFC 5280 section 6.1 much more directly, down to having WrapUp() and PrepareForNextCertificate() functions, which should make it easier to review for equivalence.
Description was changed from ========== Add initial code for verifying a certificate chain. The code handles many of RFC 5280's requirements: BasicConstraints, KeyUsage, Signature, Validity/ But is lacking Revocation checks, subjectAltName, ExtendedKeyUsage and name constraints. The unit-tests depend on data that was reviewed separately in https://codereview.chromium.org/1414393008/ BUG=410574 ========== to ========== Add initial code for verifying a certificate chain. The code handles many of RFC 5280's requirements: BasicConstraints, KeyUsage, Signature, Validity. But is lacking Revocation checks, subjectAltName, ExtendedKeyUsage and name constraints. The unit-tests depend on data that was reviewed separately in https://codereview.chromium.org/1414393008/ BUG=410574 ==========
lgtm. Just a few small comments. Though I also don't have a clear sense of the roadmap, so I don't know if you want a review at that level or not. https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:208: // compatibility sake. [Incidentally, I have a guess as to what happened. OpenSSL only caches the TBSCertificate parse (otherwise the signature would break), and re-encodes all the outer bits. If it, or another implementation which behaves similarly, converts from one to the other when re-encoding, badness will ensue. To that end, if we ever felt like it, we could actually drive this out of the system by providing a tool for people to rewrite their certificates and make the outer sigalg match the inner one. The outer sigalg isn't signed and can be freely re-encoded. When we get there, it may be worth gathering UMA on this.] https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:230: // match. This isn't part of RFC section 6.1.3, but is mandated by sections Nit: RFC section -> section or RFC 5280 section? https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:271: // From RFC 5280 section 6.1.4 step d: Strictly speaking, this is steps d-f combined, with the crazy parameter inheritance bit of step e dropped because we don't support key types which require it. https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:330: Nit: Add TODO about policies and name constraints and such where you've skipped steps? (Or not. Whatever's convenient.) https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:387: // direclty match the procedures in RFC 5280's section 6.1. directly https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:423: der::Input working_public_key; It's actually a combination of working_public_key, working_public_key_algorithm, and working_public_key_parameters. The three are split in the RFC to allow for implementing the parameter inheritance thing. (I'd call this working_spki or working_public_key_info or something to distinguish from those names.) https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:425: // |working_public_key| corresponds with the same named variable in RFC 5280 working_issuer_name https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:448: // certificate is i=N and the certificate signed by trust anchor is i=N. (I probably wouldn't have bothered, but whatever. :-) I suppose teeeeecccchhhhnnniiiicccaaaallllyyyy a 1-indexed might overflow and a 0-indexed can't. But sizeof(der::Input) > 1, so not actually.) https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:496: // certification path. It's certainly possible in so far as I could construct such a thing easily and pass it into this function. Although I dunno what you'd check here. You could do byte-for-byte equality, but then there's the two ways to encode the sig-alg. Or maybe I sign two sliiiight variants on the same certificate. I suppose you could go by serial number?? I dunno, this seems more of a concern for the path builder avoiding infinite loop than anything else.
Oh, Windows hates you because SignatureAlgorithm's destructor is private. (Actually, I'm kind of confused how that works at all...)
https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:330: On 2015/12/10 23:23:35, davidben wrote: > Nit: Add TODO about policies and name constraints and such where you've skipped > steps? (Or not. Whatever's convenient.) I think it would make sense. It's kind of obvious if you skip from step d to k, but less obvious like in BasicCertificateProcessing where it's just later steps that are missing.
https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:208: // compatibility sake. On 2015/12/10 23:23:35, davidben wrote: > [Incidentally, I have a guess as to what happened. OpenSSL only caches the > TBSCertificate parse (otherwise the signature would break), and re-encodes all > the outer bits. If it, or another implementation which behaves similarly, > converts from one to the other when re-encoding, badness will ensue. > > To that end, if we ever felt like it, we could actually drive this out of the > system by providing a tool for people to rewrite their certificates and make the > outer sigalg match the inner one. The outer sigalg isn't signed and can be > freely re-encoded. When we get there, it may be worth gathering UMA on this.] Good to know, thanks https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:230: // match. This isn't part of RFC section 6.1.3, but is mandated by sections On 2015/12/10 23:23:35, davidben wrote: > Nit: RFC section -> section or RFC 5280 section? Done. https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:271: // From RFC 5280 section 6.1.4 step d: On 2015/12/10 23:23:35, davidben wrote: > Strictly speaking, this is steps d-f combined, with the crazy parameter > inheritance bit of step e dropped because we don't support key types which > require it. Done. (added comment) https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:330: On 2015/12/17 02:37:35, mattm wrote: > On 2015/12/10 23:23:35, davidben wrote: > > Nit: Add TODO about policies and name constraints and such where you've > skipped > > steps? (Or not. Whatever's convenient.) > > I think it would make sense. It's kind of obvious if you skip from step d to k, > but less obvious like in BasicCertificateProcessing where it's just later steps > that are missing. Done. I have added numerous comments so each step is covered. Either I have a TODO (policy/name constraints), or I have a note explaining why the step was omitted https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:387: // direclty match the procedures in RFC 5280's section 6.1. On 2015/12/10 23:23:35, davidben wrote: > directly Done. https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:423: der::Input working_public_key; On 2015/12/10 23:23:35, davidben wrote: > It's actually a combination of working_public_key, working_public_key_algorithm, > and working_public_key_parameters. The three are split in the RFC to allow for > implementing the parameter inheritance thing. > > (I'd call this working_spki or working_public_key_info or something to > distinguish from those names.) Good idea, done (also added a comment block trying to explain the why). https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:425: // |working_public_key| corresponds with the same named variable in RFC 5280 On 2015/12/10 23:23:35, davidben wrote: > working_issuer_name Done. https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:448: // certificate is i=N and the certificate signed by trust anchor is i=N. On 2015/12/10 23:23:35, davidben wrote: > (I probably wouldn't have bothered, but whatever. :-) I suppose > teeeeecccchhhhnnniiiicccaaaallllyyyy a 1-indexed might overflow and a 0-indexed > can't. But sizeof(der::Input) > 1, so not actually.) Changed to 0-based indexing. Agreed, perhaps this was overkill with matching RFC 5280. https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:496: // certification path. On 2015/12/10 23:23:35, davidben wrote: > It's certainly possible in so far as I could construct such a thing easily and > pass it into this function. Although I dunno what you'd check here. You could do > byte-for-byte equality, but then there's the two ways to encode the sig-alg. Or > maybe I sign two sliiiight variants on the same certificate. I suppose you could > go by serial number?? > > I dunno, this seems more of a concern for the path builder avoiding infinite > loop than anything else. Perhaps not worth handling this at the verification layer. Are there any security problems you can think of by allowing duplicates, other than being crappy paths?
https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1414923007/diff/200001/net/cert/internal/veri... net/cert/internal/verify_certificate_chain.cc:496: // certification path. On 2015/12/17 19:43:52, eroman wrote: > On 2015/12/10 23:23:35, davidben wrote: > > It's certainly possible in so far as I could construct such a thing easily and > > pass it into this function. Although I dunno what you'd check here. You could > do > > byte-for-byte equality, but then there's the two ways to encode the sig-alg. > Or > > maybe I sign two sliiiight variants on the same certificate. I suppose you > could > > go by serial number?? > > > > I dunno, this seems more of a concern for the path builder avoiding infinite > > loop than anything else. > > Perhaps not worth handling this at the verification layer. Are there any > security problems you can think of by allowing duplicates, other than being > crappy paths? None that I can think of.
> Oh, Windows hates you because SignatureAlgorithm's destructor is private. > (Actually, I'm kind of confused how that works at all...) I believe the problem was: *out = FullyParsedCert(); FullyParsedCert however contains a scoped_ptr<> member, for which operator= is not a thing. Interesting that this was working on non-MSVC compiler. Perhaps they were doing some sort of move optimization since the RHS is a temporary? Meh. I changed the code to instead reset the needed members directly, sent to try jobs to see if that fixes...
The CQ bit was checked by eroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1414923007/#ps240001 (title: "fix compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414923007/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414923007/240001
The CQ bit was unchecked by eroman@chromium.org
The CQ bit was checked by eroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1414923007/#ps260001 (title: "add data dependency copy rule for ios")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414923007/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414923007/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by eroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1414923007/#ps280001 (title: "ifdef out the tests on ios")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414923007/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414923007/280001
Message was sent while issue was closed.
Description was changed from ========== Add initial code for verifying a certificate chain. The code handles many of RFC 5280's requirements: BasicConstraints, KeyUsage, Signature, Validity. But is lacking Revocation checks, subjectAltName, ExtendedKeyUsage and name constraints. The unit-tests depend on data that was reviewed separately in https://codereview.chromium.org/1414393008/ BUG=410574 ========== to ========== Add initial code for verifying a certificate chain. The code handles many of RFC 5280's requirements: BasicConstraints, KeyUsage, Signature, Validity. But is lacking Revocation checks, subjectAltName, ExtendedKeyUsage and name constraints. The unit-tests depend on data that was reviewed separately in https://codereview.chromium.org/1414393008/ BUG=410574 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add initial code for verifying a certificate chain. The code handles many of RFC 5280's requirements: BasicConstraints, KeyUsage, Signature, Validity. But is lacking Revocation checks, subjectAltName, ExtendedKeyUsage and name constraints. The unit-tests depend on data that was reviewed separately in https://codereview.chromium.org/1414393008/ BUG=410574 ========== to ========== Add initial code for verifying a certificate chain. The code handles many of RFC 5280's requirements: BasicConstraints, KeyUsage, Signature, Validity. But is lacking Revocation checks, subjectAltName, ExtendedKeyUsage and name constraints. The unit-tests depend on data that was reviewed separately in https://codereview.chromium.org/1414393008/ BUG=410574 Committed: https://crrev.com/c1aac5aad208df58fc0c53af88b6c2ca6ab99bfc Cr-Commit-Position: refs/heads/master@{#365987} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/c1aac5aad208df58fc0c53af88b6c2ca6ab99bfc Cr-Commit-Position: refs/heads/master@{#365987} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
