Closed
Bug 966820
Opened 10 years ago
Closed 10 years ago
Add tests for that trust bits are interpreted correctly by CertVerifier
Categories
(Core :: Security: PSM, defect, P3)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: briansmith, Assigned: cviecco)
References
Details
Attachments
(1 file, 4 obsolete files)
|
21.48 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
1. Test that a root cert must have the SSL CA trust bit in order to be a trust anchor for SSL certificates, and that Email or Object Signer trust bits do not work as a substitute. 2. Test that a root cert with multiple trust bits (including SSL) works. 3. Test that a root cert with no trust bits works. 4. Test the same for intermediates 5. Test the same for end-entities.
|
Reporter | |
Updated•10 years ago
|
Priority: -- → P3
|
Assignee | |
Updated•10 years ago
|
Assignee: nobody → cviecco
|
|
Assignee | |
Comment 1•10 years ago
|
||
|
|
Assignee | |
Updated•10 years ago
|
Attachment #8388832 -
Flags: review?(dkeeler)
Comment on attachment 8388832 [details] [diff] [review] test-trust-bits Review of attachment 8388832 [details] [diff] [review]: ----------------------------------------------------------------- Overall, this looks good - everything is nicely organized. I think some more comments/documentation would go a long way. For example, explain a bit more what each trust string means, what each usage means, and why we're expecting the results we get. In particular, when insanity::pkix differs from classic, it's not clear to me if there's a bug we need to address. ::: security/manager/ssl/tests/unit/test_cert_trust.js @@ +9,5 @@ > +const certdb = Cc["@mozilla.org/security/x509certdb;1"] > + .getService(Ci.nsIX509CertDB); > + > +// This is the list of certificates needed for the test > +// The certificates prefixed by 'int-' are intermediates nit: this comment doesn't quite describe the list of certificates for this test @@ +22,5 @@ > + addCertFromFile(certdb, "test_cert_trust/" + cert_filename, trust_string); > +} > + > +function setup_basic_trusts(ca_cert, int_cert) { > + const nsIX509Cert = Components.interfaces.nsIX509Cert; This probably isn't necessary - you can just use Ci.nsIX509Cert, right? @@ +32,5 @@ > + certdb.setCertTrust(int_cert, nsIX509Cert.CA_CERT, 0); > +} > + > +function check_cert_err_generic(cert, expected_error, usage) { > + const nsIX509Cert3 = Components.interfaces.nsIX509Cert3; doesn't look like this is used @@ +34,5 @@ > + > +function check_cert_err_generic(cert, expected_error, usage) { > + const nsIX509Cert3 = Components.interfaces.nsIX509Cert3; > + do_print("cert cn=" + cert.commonName); > + do_print("cert issuer cn=" + cert.issuerCommonName); nit: probably don't need these print statements, since any error should give you a backtrace that indicates which test failed @@ +43,5 @@ > + do_check_eq(error, expected_error); > + > +}; > + > +function test_ca_distrust(ee_cert, cert_to_distrust, isRootCA, useInsanity) { maybe "cert_to_modify_trust" @@ +44,5 @@ > + > +}; > + > +function test_ca_distrust(ee_cert, cert_to_distrust, isRootCA, useInsanity) { > + //check_cert_err_generic(cert, expected_error, certificateUsageSSLServer) probably don't need this line @@ +65,5 @@ > + check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_UNTRUSTED_ISSUER > + : SEC_ERROR_INADEQUATE_CERT_TYPE, > + certificateUsageObjectSigner); > + check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_CA_CERT_INVALID > + : SEC_ERROR_INVALID_ARGS, SEC_ERROR_INVALID_ARGS seems like a bug - what's going on here? @@ +71,5 @@ > + check_cert_err_generic(ee_cert, SEC_ERROR_INADEQUATE_CERT_TYPE, > + certificateUsageStatusResponder); > + > + > + // Trust set to T - trusted CA to issue client certs These tests don't make sense to me - why are we getting errors if the CA is trusted to issue client certs? @@ +79,5 @@ > + : 0, > + certificateUsageSSLServer); > + > + check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER > + : 0 Why the classic/insanity::pkix discrepancy? This seems like a bug. @@ +84,5 @@ > + : 0, > + certificateUsageSSLClient); > + > + check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_CA_CERT_INVALID > + : SEC_ERROR_INADEQUATE_CERT_TYPE, I would expect this to be INADEQUATE_CERT_TYPE in both cases... what's going on here? @@ +98,5 @@ > + : 0, > + certificateUsageEmailRecipient); > + > + check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER > + : SEC_ERROR_INADEQUATE_CERT_TYPE nit: trailing whitespace @@ +100,5 @@ > + > + check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER > + : SEC_ERROR_INADEQUATE_CERT_TYPE > + : useInsanity ? 0 > + : SEC_ERROR_INADEQUATE_CERT_TYPE, classic/insanity::pkix are different - is this a bug? @@ +104,5 @@ > + : SEC_ERROR_INADEQUATE_CERT_TYPE, > + certificateUsageObjectSigner); > + > + check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_CA_CERT_INVALID > + : SEC_ERROR_INVALID_ARGS, another discrepancy - bug? @@ +114,5 @@ > + // Now tests on the SSL trust bit > + setCertTrust(cert_to_distrust, 'p,C,C'); > + check_cert_err_generic(ee_cert, SEC_ERROR_UNTRUSTED_ISSUER, > + certificateUsageSSLServer); > + check_cert_err_generic(ee_cert, useInsanity ? 0 this seems like a bug? @@ +120,5 @@ > + certificateUsageSSLClient); > + > + check_cert_err_generic(ee_cert, 0, certificateUsageEmailSigner); > + check_cert_err_generic(ee_cert, 0, certificateUsageEmailRecipient); > + check_cert_err_generic(ee_cert, useInsanity ? 0 bug? @@ +127,5 @@ > + > + > + // Inherited trust SSL > + setCertTrust(cert_to_distrust, ',C,C'); > + check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER nit: trailing whitespace @@ +131,5 @@ > + check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER > + : SEC_ERROR_UNTRUSTED_ISSUER > + : 0, > + certificateUsageSSLServer); > + check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? 0 // XXX Insanity Bug please file a new bug, have it block the insanity::pkix tracking bug, and then reference it here @@ +145,5 @@ > + > + // Now tests on the EMAIL trust bit > + setCertTrust(cert_to_distrust, 'C,p,C'); > + check_cert_err_generic(ee_cert, 0, certificateUsageSSLServer); > + check_cert_err_generic(ee_cert, isRootCA ? SEC_ERROR_UNTRUSTED_ISSUER nit: trailing space @@ +170,5 @@ > + certificateUsageSSLClient); > + check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER > + : SEC_ERROR_UNTRUSTED_ISSUER > + : 0, > + certificateUsageEmailSigner); /// Bug on both NSS and pkix? file a bug, please @@ +176,5 @@ > + > + check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER > + : SEC_ERROR_UNTRUSTED_ISSUER > + : 0, > + certificateUsageEmailRecipient); /// Bug on both NSS and pkix? bug please @@ +178,5 @@ > + : SEC_ERROR_UNTRUSTED_ISSUER > + : 0, > + certificateUsageEmailRecipient); /// Bug on both NSS and pkix? > + check_cert_err_generic(ee_cert, useInsanity ? 0 > + : SEC_ERROR_INADEQUATE_CERT_TYPE, is this a bug? @@ +180,5 @@ > + certificateUsageEmailRecipient); /// Bug on both NSS and pkix? > + check_cert_err_generic(ee_cert, useInsanity ? 0 > + : SEC_ERROR_INADEQUATE_CERT_TYPE, > + certificateUsageObjectSigner); > + What about object signing trust? @@ +190,5 @@ > + clearOCSPCache(); > + clearSessionCache(); > + > + let ca_cert = certdb.findCertByNickname(null, 'ca'); > + do_check_false(!ca_cert) do_check_true(ca_cert) should work ::: security/manager/ssl/tests/unit/test_cert_trust/generate.py @@ +1,1 @@ > +#!/usr/bin/python nit: python mode line before this, no empty line after @@ +31,5 @@ > +authority_key_ident = "authorityKeyIdentifier = keyid, issuer\n" > +subject_key_ident = "subjectKeyIdentifier = hash\n" > + > + > +def self_sign_csr(db_dir, dst_dir, csr_name, key_file, serial_num, ext_text, is this still necessary? (the call to it is commented out, below) @@ +57,5 @@ > + key_type, > + 'ca', > + ca_ext) > + > + name = 'int' I'm not sure this is necessary - just pass in 'int' like you do with the ca and ee. @@ +80,5 @@ > + > + > + #shutil.copy(ca_cert, srcdir + "/" + "ca-1.der") > + #self_sign_csr(db, srcdir, db + "/ca.csr", ca_key, 2, ca_ext, "ca-2") > + #os.remove(ca_cert); nit: remove unused lines
Attachment #8388832 -
Flags: review?(dkeeler) → review-
|
|
Assignee | |
Comment 3•10 years ago
|
||
Comment on attachment 8388832 [details] [diff] [review] test-trust-bits Review of attachment 8388832 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the fast review. Agreed on all the nits, and will file bugs for the misbehaving issues. ::: security/manager/ssl/tests/unit/test_cert_trust.js @@ +65,5 @@ > + check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_UNTRUSTED_ISSUER > + : SEC_ERROR_INADEQUATE_CERT_TYPE, > + certificateUsageObjectSigner); > + check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_CA_CERT_INVALID > + : SEC_ERROR_INVALID_ARGS, It is from https://mxr.mozilla.org/mozilla-central/source/security/certverifier/CertVerifier.cpp#463 on classic we do not allow UsageVerifyCA (which looks like a bug now) @@ +71,5 @@ > + check_cert_err_generic(ee_cert, SEC_ERROR_INADEQUATE_CERT_TYPE, > + certificateUsageStatusResponder); > + > + > + // Trust set to T - trusted CA to issue client certs Client cert/server cert (is clear on the SSLServer/SSL client case) but obscure in the emailsigner/emailrecipient case. @@ +79,5 @@ > + : 0, > + certificateUsageSSLServer); > + > + check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER > + : 0 Yes it is @@ +84,5 @@ > + : 0, > + certificateUsageSSLClient); > + > + check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_CA_CERT_INVALID > + : SEC_ERROR_INADEQUATE_CERT_TYPE, Need to look. @@ +100,5 @@ > + > + check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER > + : SEC_ERROR_INADEQUATE_CERT_TYPE > + : useInsanity ? 0 > + : SEC_ERROR_INADEQUATE_CERT_TYPE, No, is is expected given the different properties of the objectsigner in classic vs insanity @@ +146,5 @@ > + // Now tests on the EMAIL trust bit > + setCertTrust(cert_to_distrust, 'C,p,C'); > + check_cert_err_generic(ee_cert, 0, certificateUsageSSLServer); > + check_cert_err_generic(ee_cert, isRootCA ? SEC_ERROR_UNTRUSTED_ISSUER > + : useInsanity ? SEC_ERROR_UNTRUSTED_ISSUER This is the actual bug! (insanity claims is not valid, when is clear it is). @@ +170,5 @@ > + certificateUsageSSLClient); > + check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER > + : SEC_ERROR_UNTRUSTED_ISSUER > + : 0, > + certificateUsageEmailSigner); /// Bug on both NSS and pkix? actually this is correct. @@ +180,5 @@ > + certificateUsageEmailRecipient); /// Bug on both NSS and pkix? > + check_cert_err_generic(ee_cert, useInsanity ? 0 > + : SEC_ERROR_INADEQUATE_CERT_TYPE, > + certificateUsageObjectSigner); > + I tought about it, but since the behaviours are so different, I decided not to. (new certs would be needed with deprecated NSS object signer OIDS in them). @@ +190,5 @@ > + clearOCSPCache(); > + clearSessionCache(); > + > + let ca_cert = certdb.findCertByNickname(null, 'ca'); > + do_check_false(!ca_cert) It did not :o
|
|
Assignee | |
Comment 4•10 years ago
|
||
Comment on attachment 8388832 [details] [diff] [review] test-trust-bits Review of attachment 8388832 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_cert_trust.js @@ +146,5 @@ > + // Now tests on the EMAIL trust bit > + setCertTrust(cert_to_distrust, 'C,p,C'); > + check_cert_err_generic(ee_cert, 0, certificateUsageSSLServer); > + check_cert_err_generic(ee_cert, isRootCA ? SEC_ERROR_UNTRUSTED_ISSUER > + : useInsanity ? SEC_ERROR_UNTRUSTED_ISSUER arrg. NSS has the bug, not insanity. The itnermediate is expliciyly distrusted here,
|
|
Assignee | |
Comment 5•10 years ago
|
||
Attachment #8388832 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 6•10 years ago
|
||
Attachment #8389471 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 7•10 years ago
|
||
Comment on attachment 8389491 [details] [diff] [review] test-trust-bits (v 2.1) Review of attachment 8389491 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_cert_trust.js @@ +71,5 @@ > + certificateUsageStatusResponder); > + > + > + // Trust set to T - trusted CA to issue client certs, where client cert is > + // usageSSLClient. nit: missed this one.
Attachment #8389491 -
Flags: review?(dkeeler)
Comment on attachment 8389491 [details] [diff] [review] test-trust-bits (v 2.1) Review of attachment 8389491 [details] [diff] [review]: ----------------------------------------------------------------- This is looking really good. My two main concerns are: 1. It doesn't look like my comments on generate.py from the previous review were addressed 2. There are 8 usages we handle in CertVerifier::InsanityVerifyCert. I think we should test that each usage returns the results we're expecting every time we change the trust on these certificates (so, for example, certificateUsageSSLCA should always fail). ::: security/manager/ssl/tests/unit/test_cert_trust.js @@ +5,5 @@ > + > +"use strict"; > + > +do_get_profile(); // must be called before getting nsIX509CertDB > +const certdb = Cc["@mozilla.org/security/x509certdb;1"] nit: maybe 'gCertDB' or something? @@ +15,5 @@ > + 'ca', > +] > + > +function load_cert(cert_name, trust_string) { > + var cert_filename = cert_name + ".der"; nit: let, not var @@ +40,5 @@ > + > +}; > + > +function test_ca_distrust(ee_cert, cert_to_modify_trust, isRootCA, useInsanity) { > + // On reset everyting is successful Well, but there are some usages we're always expecting to fail, right? Let's include them here. @@ +64,5 @@ > + check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_UNTRUSTED_ISSUER > + : SEC_ERROR_INADEQUATE_CERT_TYPE, > + certificateUsageObjectSigner); > + check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_CA_CERT_INVALID > + : SEC_ERROR_INVALID_ARGS, nit: alignment @@ +79,5 @@ > + : 0, > + certificateUsageSSLServer); > + > + check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER > + : 0 Did you file a bug on this? (In any case, include the bug number here, please.) @@ +99,5 @@ > + certificateUsageEmailRecipient); > + > + check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER > + : SEC_ERROR_INADEQUATE_CERT_TYPE > + : useInsanity ? 0 A comment explaining why this is the case would be good. @@ +114,5 @@ > + // Now tests on the SSL trust bit > + setCertTrust(cert_to_modify_trust, 'p,C,C'); > + check_cert_err_generic(ee_cert, SEC_ERROR_UNTRUSTED_ISSUER, > + certificateUsageSSLServer); > + check_cert_err_generic(ee_cert, useInsanity ? 0 //XXX Bug Bug 982340 nit: probably only need one "bug" :) @@ +147,5 @@ > + setCertTrust(cert_to_modify_trust, 'C,p,C'); > + check_cert_err_generic(ee_cert, 0, certificateUsageSSLServer); > + check_cert_err_generic(ee_cert, isRootCA ? SEC_ERROR_UNTRUSTED_ISSUER > + : useInsanity ? SEC_ERROR_UNTRUSTED_ISSUER > + : 0, // Insanity is OK, NSS bug well, we should probably at least file a bug... @@ +179,5 @@ > + certificateUsageEmailRecipient); > + check_cert_err_generic(ee_cert, useInsanity ? 0 > + : SEC_ERROR_INADEQUATE_CERT_TYPE, > + certificateUsageObjectSigner); > + nit: unnecessary blank line @@ +181,5 @@ > + : SEC_ERROR_INADEQUATE_CERT_TYPE, > + certificateUsageObjectSigner); > + > +} > + nit: unnecessary blank line @@ +186,5 @@ > + > +function run_test_in_mode(useInsanity) { > + Services.prefs.setBoolPref("security.use_insanity_verification", useInsanity); > + clearOCSPCache(); > + clearSessionCache(); I don't think either of these are necessary - we aren't doing OCSP fetching, and we're not connecting to any servers, right? @@ +189,5 @@ > + clearOCSPCache(); > + clearSessionCache(); > + > + let ca_cert = certdb.findCertByNickname(null, 'ca'); > + do_check_false(!ca_cert) nit: missing ';' @@ +191,5 @@ > + > + let ca_cert = certdb.findCertByNickname(null, 'ca'); > + do_check_false(!ca_cert) > + let int_cert = certdb.findCertByNickname(null, 'int'); > + do_check_false(!int_cert) nit: missing ';' ::: security/manager/ssl/tests/unit/test_cert_trust/generate.py @@ +80,5 @@ > + > + > + #shutil.copy(ca_cert, srcdir + "/" + "ca-1.der") > + #self_sign_csr(db, srcdir, db + "/ca.csr", ca_key, 2, ca_ext, "ca-2") > + #os.remove(ca_cert); Doesn't look like my comments were addressed from the previous review of this file.
Attachment #8389491 -
Flags: review?(dkeeler) → review-
|
|
Assignee | |
Comment 9•10 years ago
|
||
Comment on attachment 8389491 [details] [diff] [review] test-trust-bits (v 2.1) Review of attachment 8389491 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_cert_trust.js @@ +5,5 @@ > + > +"use strict"; > + > +do_get_profile(); // must be called before getting nsIX509CertDB > +const certdb = Cc["@mozilla.org/security/x509certdb;1"] all the xpcshell tests that I have written use this convetion. Lets keep it or mass migrate on another bug. @@ +186,5 @@ > + > +function run_test_in_mode(useInsanity) { > + Services.prefs.setBoolPref("security.use_insanity_verification", useInsanity); > + clearOCSPCache(); > + clearSessionCache(); agreed ::: security/manager/ssl/tests/unit/test_cert_trust/generate.py @@ +80,5 @@ > + > + > + #shutil.copy(ca_cert, srcdir + "/" + "ca-1.der") > + #self_sign_csr(db, srcdir, db + "/ca.csr", ca_key, 2, ca_ext, "ca-2") > + #os.remove(ca_cert); mea culpa
|
|
Assignee | |
Comment 10•10 years ago
|
||
Attachment #8389491 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 11•10 years ago
|
||
Attachment #8390647 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•10 years ago
|
Attachment #8390648 -
Flags: review?(dkeeler)
Comment on attachment 8390648 [details] [diff] [review] test-trust-bits (v3.1) Review of attachment 8390648 [details] [diff] [review]: ----------------------------------------------------------------- Great! ::: security/manager/ssl/tests/unit/test_cert_trust.js @@ +212,5 @@ > + certificateUsageVerifyCA); > + check_cert_err_generic(ee_cert, SEC_ERROR_INADEQUATE_CERT_TYPE, > + certificateUsageStatusResponder); > +} > + nit: unnecessary blank line ::: security/manager/ssl/tests/unit/test_cert_trust/generate.py @@ +32,5 @@ > + > +authority_key_ident = "authorityKeyIdentifier = keyid, issuer\n" > +subject_key_ident = "subjectKeyIdentifier = hash\n" > + > + nit: not sure we need all this vertical space here @@ +65,5 @@ > + int_key, > + int_cert) > + > + > + nit: again, we probably don't need so much vertical space - one line should do it.
Attachment #8390648 -
Flags: review?(dkeeler) → review+
|
|
Assignee | |
Comment 13•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ce7df2c86fa1
|
||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8c23b2efbe3c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•