1349312 - add debug-only test logs for integration testing of certificate transparency

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

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

As we found in bug 1348713, we don't have a way to do integration testing of our certificate transparency information. One way to do this would be to add (debug-only) test logs with known keys and the ability to generate SCTs for our test certificates. This would be similar to our EV testing implementation.

Comment 1

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

Attachment: bug 1349312 - part 1/2: patch CT implementation to include debug-only test logs

This modifies getCTKnownLogs.py to inject 3 debug-only Certificate Transparency
log keys and 2 organizations ("Mozilla Test Org 1" and "2") for use with
integration tests. Also updates CTKnownLogs.h as generated by the python script.

The debug logs use the "default", "secp256r1", and "alternate" keys that are
already present in our testing infrastructure (see pykey.py).

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

Comment 2

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

Attachment: bug 1349312 - part 2/2: add pyct.py and generate test certificate transparency information

This introduces pyct.py with the capability of generating Signed Certificate
Timestamps for our test certificates. Also introduces a simple testcase that
should validate correctly under current CT requirements.

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

Comment 3

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

Comment on attachment 8861596 [details]
bug 1349312 - part 1/2: patch CT implementation to include debug-only test logs

https://reviewboard.mozilla.org/r/133560/#review136528

::: security/certverifier/CTKnownLogs.h:196
(Diff revision 1)
> -    91 }
> +    91 },
> +#ifdef DEBUG
> +  { "Mozilla Test RSA Log 1",
> +    mozilla::ct::CTLogStatus::Included,
> +    0, // no disqualification time
> +    9, // operated by Mozilla Test Org 1

nit: ugh, sigh, why doesn't the tool make these offsets line up with the enumeration in the data structure? Let's not fix that in this patch but... ugh...

Comment 4

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

Comment on attachment 8861597 [details]
bug 1349312 - part 2/2: add pyct.py and generate test certificate transparency information

https://reviewboard.mozilla.org/r/133562/#review137492

::: security/manager/ssl/tests/unit/pyct.py:63
(Diff revision 1)
> +            hashName = 'sha256'
> +            signatureByte = '\3'
> +        elif isinstance(self.key, pykey.RSAKey):
> +            hashName = 'SHA-256'

I'm super confused why these hashNames are different. It appears that python-rsa is going to normalize them, but is this buried in the spec somewhere, or just a copy/paste issue?

::: security/manager/ssl/tests/unit/test_ct.js:28
(Diff revision 1)
> +});
> +
> +function run_test() {
> +  Services.prefs.setIntPref("security.pki.certificate_transparency.mode", 1);
> +  add_tls_server_setup("BadCertServer", "test_ct");
> +  add_connection_test("ct-valid.example.com", PRErrorCodeSuccess, null,

We really should have a negative test, to make sure that case works before checking this in.

Comment 5

[:Cykesiopka]

Comment on attachment 8861596 [details]
bug 1349312 - part 1/2: patch CT implementation to include debug-only test logs

https://reviewboard.mozilla.org/r/133560/#review137742

Looks good.

::: security/manager/tools/getCTKnownLogs.py:220
(Diff revision 1)
> +        MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAwXXGUmYJn3cIKmeR8bh2
> +        w39c5TiwbErNIrHL1G+mWtoq3UHIwkmKxKOzwfYUh/QbaYlBvYClHDwSAkTFhKTE
> +        SDMF5ROMAQbPCL6ahidguuai6PNvI8XZgxO53683g0XazlHU1tzSpss8xwbrzTBw
> +        7JjM5AqlkdcpWn9xxb5maR0rLf7ISURZC8Wj6kn9k7HXU0BfF3N2mZWGZiVHl+1C
> +        aQiICBFCIGmYikP+5Izmh4HdIramnNKDdRMfkysSjOKG+n0lHAYq0n7wFvGHzdVO
> +        gys1uJMPdLqQqovHYWckKrH9bWIUDRjEwLjGj8N0hFcyStfehuZVLx0eGR1xIWjT
> +        uwIDAQAB

Nit: This block should probably be indented one more level for consistency.

Comment 6

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

Comment on attachment 8861596 [details]
bug 1349312 - part 1/2: patch CT implementation to include debug-only test logs

https://reviewboard.mozilla.org/r/133560/#review136528

> nit: ugh, sigh, why doesn't the tool make these offsets line up with the enumeration in the data structure? Let's not fix that in this patch but... ugh...

Uh-oh - I hadn't even noticed this. This is going to bite us in the future if we don't fix it. The numbers in kCTLogList are indices into the kCTLogOperatorList array, but the numbers in kCTLogOperatorList are *not* indices - they're "operator IDs". We should just make those enumerations or some other this-isn't-an-index thing. I filed bug 1360615 for this.

Comment 7

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

Comment on attachment 8861597 [details]
bug 1349312 - part 2/2: add pyct.py and generate test certificate transparency information

https://reviewboard.mozilla.org/r/133562/#review137492

Thanks for the review!

> I'm super confused why these hashNames are different. It appears that python-rsa is going to normalize them, but is this buried in the spec somewhere, or just a copy/paste issue?

This is a deficiency in pykey.py, I think. The two backing libraries that actually do the signing for EC and RSA each take a string argument to describe the hash function to use, but one expects the form "sha256" whereas the other expects "SHA-256". pykey.py should just export constants consumers can use and do the translation itself. I filed bug 1360623.

> We really should have a negative test, to make sure that case works before checking this in.

Good idea - added.

Comment 8

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

Comment on attachment 8861596 [details]
bug 1349312 - part 1/2: patch CT implementation to include debug-only test logs

https://reviewboard.mozilla.org/r/133560/#review137742

Thanks!

> Nit: This block should probably be indented one more level for consistency.

Good catch - fixed.

Comment 9

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

Comment on attachment 8861596 [details]
bug 1349312 - part 1/2: patch CT implementation to include debug-only test logs

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

Comment 10

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

Comment on attachment 8861597 [details]
bug 1349312 - part 2/2: add pyct.py and generate test certificate transparency information

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

Comment 11

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

Comment on attachment 8861597 [details]
bug 1349312 - part 2/2: add pyct.py and generate test certificate transparency information

https://reviewboard.mozilla.org/r/133562/#review137872

Comment 12

[:Cykesiopka]

Comment on attachment 8861597 [details]
bug 1349312 - part 2/2: add pyct.py and generate test certificate transparency information

https://reviewboard.mozilla.org/r/133562/#review138534

Looks good. Sorry for the delay.

::: security/manager/ssl/tests/unit/test_ct/ct-insufficient-scts.example.com.pem.certspec:4
(Diff revision 2)
> +issuer:Test CA
> +subject:ct-insufficient-scts.example.com
> +extension:subjectAlternativeName:*.example.com
> +extension:embeddedSCTList:secp256r1:20160101,alternate:20160101

Just to make sure I actually understand why this is insufficient:
1. The default validity period our certs have is 800 days.
2. 800 days is ~2 years and 2 months, which gets rounded down to N=2 years.
3. => N+1 = 3 embedded SCTs are required to be considered compliant.

Comment 13

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

Comment on attachment 8861597 [details]
bug 1349312 - part 2/2: add pyct.py and generate test certificate transparency information

https://reviewboard.mozilla.org/r/133562/#review138534

Thanks!

> Just to make sure I actually understand why this is insufficient:
> 1. The default validity period our certs have is 800 days.
> 2. 800 days is ~2 years and 2 months, which gets rounded down to N=2 years.
> 3. => N+1 = 3 embedded SCTs are required to be considered compliant.

Correct. I added some documentation in the test.

Comment 14

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

Comment on attachment 8861596 [details]
bug 1349312 - part 1/2: patch CT implementation to include debug-only test logs

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

Comment 15

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

Comment on attachment 8861597 [details]
bug 1349312 - part 2/2: add pyct.py and generate test certificate transparency information

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86fecaba102e
part 1/2: patch CT implementation to include debug-only test logs r=Cykesiopka,jcj
https://hg.mozilla.org/integration/autoland/rev/ee365962dce7
part 2/2: add pyct.py and generate test certificate transparency information r=Cykesiopka,jcj

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/86fecaba102e
https://hg.mozilla.org/mozilla-central/rev/ee365962dce7