1255153 - convert most of the xpcshell name constraints tests to mozilla::pkix gtests

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) [:keeler]]

There are about 150 certificates generated for xpcshell name constraint tests. Many of these testcases are redundant considering the mozilla::pkix name constraint gtests. We should just remove the unnecessary ones and add a few more gtests.

Comment 1

[Dana Keeler (she/her) [:keeler]]

Attachment: MozReview Request: bug 1255153 - (re)move redundant xpcshell name constraint tests to gtests r?Cykesiopka r?jcj

Review commit: https://reviewboard.mozilla.org/r/39035/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39035/

Comment 2

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8728688 [details]
MozReview Request: bug 1255153 - (re)move redundant xpcshell name constraint tests to gtests r?Cykesiopka r?jcj

https://reviewboard.mozilla.org/r/39035/#review36007

Test code looks fine to me. It was a fun run down how name constraints work in PKIX. The removals I'll leave in Treeherder's hands.

Comment 3

[:Cykesiopka]

Comment on attachment 8728688 [details]
MozReview Request: bug 1255153 - (re)move redundant xpcshell name constraint tests to gtests r?Cykesiopka r?jcj

https://reviewboard.mozilla.org/r/39035/#review36359

LGTM.

::: security/manager/ssl/tests/unit/test_name_constraints.js:7
(Diff revision 1)
>  // This Source Code Form is subject to the terms of the Mozilla Public
>  // License, v. 2.0. If a copy of the MPL was not distributed with this
>  // file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  "use strict";
>  

Optional: Would be nice to add a brief description on what this file tests relative to the GTests.

::: security/manager/ssl/tests/unit/test_name_constraints.js:42
(Diff revision 1)
> +  checkCertInNameSpace(certFromFile('NameConstraints.dcissallowed'));
> +  checkCertNotInNameSpace(certFromFile('NameConstraints.dcissblocked'));

Nit: Use double quotes.

::: security/pkix/test/gtest/pkixnames_tests.cpp:1550
(Diff revision 1)
>    if (subjectAltName != NO_SAN) {
>      extensions[0] = CreateEncodedSubjectAltName(subjectAltName);
>      EXPECT_FALSE(ENCODING_FAILED(extensions[0]));
>    }
> +  if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
> +    EXPECT_EQ(subjectAltName, NO_SAN);

Nit: Maybe ASSERT_EQ() here? This shouldn't happen if the GTest is written correctly, and it feels like a waste of time doing anything else if the test file is wrong.

::: security/pkix/test/gtest/pkixnames_tests.cpp:2630
(Diff revision 1)
>  
>  INSTANTIATE_TEST_CASE_P(pkixnames_CheckNameConstraints,
>                          pkixnames_CheckNameConstraints,
>                          testing::ValuesIn(NAME_CONSTRAINT_PARAMS));
> +
> +static const NameConstraintParams NO_FALLBACK_NAME_CONSTRAINT_PARAMS[] =

Optional: Might be helpful to add a comment here that the |subjectAltName| param is ignored for these entries, to avoid future frustration.

Comment 4

[Dana Keeler (she/her) [:keeler]]

https://reviewboard.mozilla.org/r/39035/#review36359

> Optional: Would be nice to add a brief description on what this file tests relative to the GTests.

Sounds good.

> Nit: Use double quotes.

Fixed.

> Nit: Maybe ASSERT_EQ() here? This shouldn't happen if the GTest is written correctly, and it feels like a waste of time doing anything else if the test file is wrong.

Good call.

> Optional: Might be helpful to add a comment here that the |subjectAltName| param is ignored for these entries, to avoid future frustration.

Sounds good.

Comment 5

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8728688 [details]
MozReview Request: bug 1255153 - (re)move redundant xpcshell name constraint tests to gtests r?Cykesiopka r?jcj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39035/diff/1-2/

Comment 6

[Dana Keeler (she/her) [:keeler]]

https://reviewboard.mozilla.org/r/39035/#review36421

::: security/pkix/test/gtest/pkixnames_tests.cpp:1550
(Diff revision 2)
>    if (subjectAltName != NO_SAN) {
>      extensions[0] = CreateEncodedSubjectAltName(subjectAltName);
>      EXPECT_FALSE(ENCODING_FAILED(extensions[0]));
>    }
> +  if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
> +    ASSERT_EQ(subjectAltName, NO_SAN);

Actually, looks like this can't be ASSERT_EQ, since that eventually expands to a 'return;' statement, which doesn't match the function's return type. CreateCert could be reworked to return void and instead pass the created certificate out by reference, but I think that's a fair bit of work for not a lot of gain. Instead I'll just add a comment that we would like to use ASSERT_EQ but can't.

Comment 7

[Dana Keeler (she/her) [:keeler]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #6)
> https://reviewboard.mozilla.org/r/39035/#review36421
> 
> ::: security/pkix/test/gtest/pkixnames_tests.cpp:1550
> (Diff revision 2)
> >    if (subjectAltName != NO_SAN) {
> >      extensions[0] = CreateEncodedSubjectAltName(subjectAltName);
> >      EXPECT_FALSE(ENCODING_FAILED(extensions[0]));
> >    }
> > +  if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
> > +    ASSERT_EQ(subjectAltName, NO_SAN);
> 
> Actually, looks like this can't be ASSERT_EQ, since that eventually expands
> to a 'return;' statement, which doesn't match the function's return type.
> CreateCert could be reworked to return void and instead pass the created
> certificate out by reference, but I think that's a fair bit of work for not
> a lot of gain. Instead I'll just add a comment that we would like to use
> ASSERT_EQ but can't.

(See https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-8e079db6ceb56e046c28f153021af739f49eafce/try-linux/try-linux-bm75-try1-build1427.txt.gz )

Comment 8

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8728688 [details]
MozReview Request: bug 1255153 - (re)move redundant xpcshell name constraint tests to gtests r?Cykesiopka r?jcj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39035/diff/2-3/

Comment 9

[Dana Keeler (she/her) [:keeler]]

Thanks for the reviews! Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebd6be1d65f8

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6092e439db9

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e6092e439db9