tests: Add `x509-limbo` test #1

tetsuo-cpp · 2023-08-17T08:18:33Z

No description provided.

tetsuo-cpp · 2023-08-17T08:26:52Z

At the moment, pathlen::intermediate-pathlen-must-not-increase is failing as the verification succeeds when we don't expect it to.

If I temporarily modify the testcase in the limbo.json to say SUCCESS, I get past that test but I see an actual segfault in the Rust.

Fatal Python error: Segmentation fault

Thread 0x000000016dffb000 (most recent call first):
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/execnet/gateway_base.py", line 474 in read
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/execnet/gateway_base.py", line 507 in from_io
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/execnet/gateway_base.py", line 1049 in _thread_receiver
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/execnet/gateway_base.py", line 296 in run
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/execnet/gateway_base.py", line 361 in _perform_spawn

Current thread 0x00000001fd6e9e00 (most recent call first):
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/cryptography/hazmat/backends/openssl/backend.py", line 782 in load_der_public_key
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/cryptography/hazmat/primitives/serialization/base.py", line 64 in load_der_public_key
  File "/Users/tetsuo/Code/cryptography/tests/x509/test_verification.py", line 58 in _limbo_testcase
  File "/Users/tetsuo/Code/cryptography/tests/x509/test_verification.py", line 173 in test_limbo
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/_pytest/python.py", line 194 in pytest_pyfunc_call
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_callers.py", line 80 in _multicall
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_manager.py", line 112 in _hookexec
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_hooks.py", line 433 in __call__
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/_pytest/python.py", line 1788 in runtest
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/_pytest/runner.py", line 169 in pytest_runtest_call
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_callers.py", line 80 in _multicall
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_manager.py", line 112 in _hookexec
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_hooks.py", line 433 in __call__
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/_pytest/runner.py", line 262 in <lambda>
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/_pytest/runner.py", line 341 in from_call
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/_pytest/runner.py", line 261 in call_runtest_hook
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/_pytest/runner.py", line 222 in call_and_report
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/_pytest/runner.py", line 133 in runtestprotocol
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/_pytest/runner.py", line 114 in pytest_runtest_protocol
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_callers.py", line 80 in _multicall
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_manager.py", line 112 in _hookexec
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_hooks.py", line 433 in __call__
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/xdist/remote.py", line 174 in run_one_test
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/xdist/remote.py", line 157 in pytest_runtestloop
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_callers.py", line 80 in _multicall
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_manager.py", line 112 in _hookexec
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_hooks.py", line 433 in __call__
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/_pytest/main.py", line 324 in _main
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/_pytest/main.py", line 270 in wrap_session
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/_pytest/main.py", line 317 in pytest_cmdline_main
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_callers.py", line 80 in _multicall
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_manager.py", line 112 in _hookexec
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/pluggy/_hooks.py", line 433 in __call__
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/xdist/remote.py", line 355 in <module>
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/execnet/gateway_base.py", line 1157 in executetask
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/execnet/gateway_base.py", line 296 in run
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/execnet/gateway_base.py", line 361 in _perform_spawn
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/execnet/gateway_base.py", line 343 in integrate_as_primary_thread
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/execnet/gateway_base.py", line 1142 in serve
  File "/Users/tetsuo/Code/cryptography/.nox/tests/lib/python3.9/site-packages/execnet/gateway_base.py", line 1640 in serve
  File "<string>", line 8 in <module>
  File "<string>", line 1 in <module>
[gw5] node down: Not properly terminated
F
replacing crashed worker gw5

I'll work on debugging these issues and work on getting some fixes in.

woodruffw · 2023-08-17T09:58:26Z

tests/x509/test_verification.py

+        for testcase in testcases:
+            _limbo_testcase(testcase)


I think we want to use the subtests fixture here, with subtests.test() as a context manager to provide distinct subtests for each of these testcases.

Example from another unit test:

https://github.com/trail-of-forks/cryptography/blob/0d213f339146212f517e293f3cf0443931a2701e/tests/x509/test_name.py#L18-L27

Ah, I was looking for something like that. Will do that now.

This doesn't seem to work (at least now how I was expecting). Even in the test_name.py example, if I place an assert False within the subtest, I only see one test failure.

Yeah, that's surprising (although I admittedly don't know subtests too well). Maybe @reaperhulk or @alex has ideas here 🙂

Hahahha, funny story: So the behavior of the upstream pytest-subtests does kind of what you'd expect: runs them all.

We have our own custom version that only runs one, and this is because of performance (not the performance of running the tests, but rather the overhead of the subtest machinery itself): https://github.com/pyca/cryptography/blob/main/tests/conftest.py#L52

Ah, gotcha -- so your custom version fails fast on the first failure, and only runs them all if they all pass?

Correct. It's advantage over just having the loop is handling skips correctly. (Also that it's API compatible with proper subtests so if they're ever faster, we can switch back)

Makes sense, thanks for explaining!

woodruffw · 2023-08-17T09:59:40Z

Segfault in load_der_public_key, that's not good 🙂

woodruffw · 2023-08-17T13:01:44Z

NB: We still need to debug that segfault, but from a previous conversation with @tetsuo-cpp and @reaperhulk: the intermediate_pathlen_must_not_increase is incorrect anyways, since RFC 5280 permits this as part of supporting multiple validation paths.

In other words: we should debug here first, then flip that testcase's expected result (should succeed, not fail).

tetsuo-cpp · 2023-08-17T16:37:28Z

I've been looking into this segfault but haven't quite figured it out yet. As we discussed, it seems to be coming from this line. However, this code looks functionally identical to Certificate.public_key here.

So I'd have expected that if I call public_key on each of the certificates in the limbo.json, I'd be able to trigger the seg fault but I can't seem to. That leads me to believe that there's something about the version in PyCryptoOps that is causing this (or at least in combination with a certain certificate).

tetsuo-cpp · 2023-09-03T12:03:54Z

I've put together a list of failing Limbo tests + some commentary on whether I think they're legitimate failures or not:

pathlen::intermediate-pathlen-must-not-increase: We discussed this above, this testcase is incorrect and we need to flip the result. (pathlen: Fix testcase for intermediates that widen pathlen C2SP/x509-limbo#33)
rfc5280::chain-untrusted-root: We raise an error due to have an empty store since we don't have any trusted certificates in this testcase. @woodruffw and I discussed this and concluded that the check is legitimate and that we should amend the testcase to put some random certificate in the trusted list rather than leaving it empty. (rfc5280: Populate the trusted store in chain-untrusted-root C2SP/x509-limbo#34)
webpki::cryptographydotio-chain (and others): The EE cert has a SAN but we don't populate the expected_peer_name for that testcase. We can fix the test to check the SAN but it's worth having a discussion whether it's appropriate for the library to enforce this. If I disable this check, I also run into the EE certificate contains unaccounted critical extensions due to the fact that we don't check the key usage extension which is marked as critical. The relevant part of the cert looks like this:

        X509v3 extensions:
            X509v3 Key Usage: critical
                Digital Signature, Key Encipherment
            X509v3 Extended Key Usage:
                TLS Web Server Authentication, TLS Web Client Authentication
            X509v3 Basic Constraints: critical
                CA:FALSE

To summarise, we need to: populate the peer name in the testcase and begin supporting the key usage extension in Limbo (I believe it's in the spec but we don't use it yet). (C2SP/x509-limbo#35)

webpki::public-suffix-wildcard-san: This should fail but it doesn't. I don't know the details but from reading the testcase description, it looks like this has been researched properly so I'd guess that the relevant check is missing.
webpki::malformed-aia: Same as above.
rfc5280::leaf-ku-keycertsign: Same as above. (https://github.com/trail-of-forks/cryptography/pull/3)
rfc5280::ca_nameconstraints-*: Most of these are failing because they don't check the peer name. As part of this, we'll also need to support LDAP distinguished names in the spec. There also seems to be a bug with the name constraints work which I'm working through at the moment.

woodruffw · 2023-09-05T15:19:48Z

webpki::public-suffix-wildcard-san

Yeah, this is an incredibly annoying one: technically path validation should reject wildcards on "public suffixes" as a matter of policy, but this makes validation dependent on either (1) an online service, or (2) a possibly stale pre-baked list of public suffixes. I'm pretty sure this is why neither OpenSSL nor Go bothers with this check, so perhaps we should give it some kind of "pedantic" marker and explicitly skip it.

webpki::malformed-aia

Yeah, this is almost certainly a missing check.

rfc5280::leaf-ku-keycertsign

This one points to an error that needs to be corrected in our API: we conflate the terms "leaf", "EE", and "non-CA cert" when in reality a leaf in a chain doesn't have to be an EE (it can be a CA).

In other words: we really need a permits_leaf function, which dispatches to either permits_ca or permits_ee based on the state of KeyUsage and BasicConstraints. From there, permits_ee could grow the additional checks needed here.

rfc5280::ca_nameconstraints-*

We can probably treat a lot of these as skips for the initial MVP (especially the distinguished name ones). We should definitely support DNS name constraints, though.

tests: Use subtests in `test_limbo` Use the correct peer name types tests: Flip the Limbo validation kind to `SERVER`

populating it now

tetsuo-cpp · 2023-10-23T15:15:06Z

@woodruffw This is up-to-date and passing now. Provided that we're happy with pyca#47, we should be able to merge this.

woodruffw · 2023-10-23T21:43:57Z

src/rust/cryptography-x509-validation/src/lib.rs

@@ -236,6 +236,7 @@ where
        &self,
        working_cert: &'a Certificate<'work>,
        current_depth: u8,
+        is_leaf: bool,


Rather than a new is_leaf state, maybe check current_depth == 1? Unless I'm missing something.

The reason I can't do this is that self-issued certificates don't increase the depth. So if current_depth == 1, we don't know whether that's actually the leaf certificate or the previous certificate just happened to be self-issued.

I'll leave a comment to that effect.

Ah yeah, that's a pain. I think is_leaf is pretty ugly here, but I can't think of a better way to do this.

Yeah... definitely not ideal.

woodruffw · 2023-10-23T21:44:27Z

src/rust/cryptography-x509-validation/src/policy/mod.rs

 const RFC5280_CRITICAL_CA_EXTENSIONS: &[asn1::ObjectIdentifier] =
-    &[BASIC_CONSTRAINTS_OID, KEY_USAGE_OID];
+    &[BASIC_CONSTRAINTS_OID, KEY_USAGE_OID, NAME_CONSTRAINTS_OID];
 const RFC5280_CRITICAL_EE_EXTENSIONS: &[asn1::ObjectIdentifier] = &[
    BASIC_CONSTRAINTS_OID,
    SUBJECT_ALTERNATIVE_NAME_OID,


I need to remove these, now that we have discrete extension policy APIs 🙂

I can make a PR for that. 👍

Thanks! I think the "right" way to do this is to iterate through all of the extensions in permits_basic and fail if any critical extensions are not covered by an extension policy (whether basic, EE, or CA).

Yep! Sounds good.

tests/x509/test_verification.py

woodruffw · 2023-10-23T21:46:28Z

@woodruffw This is up-to-date and passing now. Provided that we're happy with pyca#47, we should be able to merge this.

Thanks, this is looking really good! I left a couple of Qs on C2SP/x509-limbo#47.

woodruffw

LGTM!

@tetsuo-cpp does limbo.json need to be updated again, now that we've merged C2SP/x509-limbo#47?

tetsuo-cpp · 2023-10-24T04:35:53Z

LGTM!

@tetsuo-cpp does limbo.json need to be updated again, now that we've merged trailofbits/x509-limbo#47?

Should be good to go! The limbo.json in this branch is the same as the one in x509-limbo:main.

woodruffw · 2023-10-24T05:01:04Z

Excellent! Merging.

tetsuo-cpp requested a review from woodruffw August 17, 2023 08:26

woodruffw reviewed Aug 17, 2023

View reviewed changes

woodruffw mentioned this pull request Sep 5, 2023

rust: handle the case where the leaf cert is a CA #3

Merged

woodruffw force-pushed the tob-x509-cv-skeleton branch from de853c3 to 69eb9a1 Compare September 7, 2023 23:54

tetsuo-cpp force-pushed the alex/limbo-testing branch from c871a00 to 658237d Compare September 26, 2023 13:56

tetsuo-cpp force-pushed the alex/limbo-testing branch from 04d41a8 to 4c96b6e Compare October 6, 2023 05:35

tetsuo-cpp added 6 commits October 23, 2023 15:35

tests: Add x509-limbo test

ca35288

tests: Use subtests in `test_limbo` Use the correct peer name types tests: Flip the Limbo validation kind to `SERVER`

tests: Update limbo.json

3009ec2

tests: Fix Limbo tests that exercise unsupported features

2a89c28

test: Use new server verifier API

fbeeec7

test: Don't allow empty peer name since the API requires it

e61d527

rust: Add name constraints OID to critical extensions list

0a6dfa5

tetsuo-cpp force-pushed the alex/limbo-testing branch from 4c96b6e to 0a6dfa5 Compare October 23, 2023 07:48

tetsuo-cpp added 4 commits October 24, 2023 01:55

rust: Fix check for leaf certificates when applying name constraints

4acbc15

test: Remove assert for extended_key_usage Limbo data since we're

f0282a7

populating it now

test: Update limbo.json

569b838

test: Skip EKU Limbo tests

d74d57b

tetsuo-cpp requested a review from woodruffw October 23, 2023 15:14

woodruffw reviewed Oct 23, 2023

View reviewed changes

tests/x509/test_verification.py Outdated Show resolved Hide resolved

tetsuo-cpp added 2 commits October 24, 2023 13:09

test: Add comments to explain why we're skipping certain Limbo tests

7cc3cd9

rust: Leave comment explaining is_leaf parameter

4f1d943

tetsuo-cpp requested a review from woodruffw October 24, 2023 02:15

woodruffw approved these changes Oct 24, 2023

View reviewed changes

woodruffw merged commit 625fa41 into tob-x509-cv-skeleton Oct 24, 2023
50 of 55 checks passed

woodruffw deleted the alex/limbo-testing branch October 24, 2023 05:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: Add `x509-limbo` test #1

tests: Add `x509-limbo` test #1

tetsuo-cpp commented Aug 17, 2023

tetsuo-cpp commented Aug 17, 2023

woodruffw Aug 17, 2023

tetsuo-cpp Aug 17, 2023

tetsuo-cpp Aug 17, 2023

tetsuo-cpp Aug 17, 2023

woodruffw Aug 17, 2023

alex Aug 17, 2023

woodruffw Aug 17, 2023

alex Aug 17, 2023

woodruffw Aug 17, 2023

woodruffw commented Aug 17, 2023

woodruffw commented Aug 17, 2023

tetsuo-cpp commented Aug 17, 2023

tetsuo-cpp commented Sep 3, 2023 •

edited

Loading

woodruffw commented Sep 5, 2023

tetsuo-cpp commented Oct 23, 2023

woodruffw Oct 23, 2023

tetsuo-cpp Oct 24, 2023 •

edited

Loading

tetsuo-cpp Oct 24, 2023

woodruffw Oct 24, 2023

tetsuo-cpp Oct 24, 2023

woodruffw Oct 23, 2023

tetsuo-cpp Oct 24, 2023

woodruffw Oct 24, 2023

tetsuo-cpp Oct 24, 2023

woodruffw commented Oct 23, 2023

woodruffw left a comment

tetsuo-cpp commented Oct 24, 2023

woodruffw commented Oct 24, 2023

tests: Add x509-limbo test #1

tests: Add x509-limbo test #1

Conversation

tetsuo-cpp commented Aug 17, 2023

tetsuo-cpp commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw commented Aug 17, 2023

woodruffw commented Aug 17, 2023

tetsuo-cpp commented Aug 17, 2023

tetsuo-cpp commented Sep 3, 2023 • edited Loading

woodruffw commented Sep 5, 2023

tetsuo-cpp commented Oct 23, 2023

Choose a reason for hiding this comment

tetsuo-cpp Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw commented Oct 23, 2023

woodruffw left a comment

Choose a reason for hiding this comment

tetsuo-cpp commented Oct 24, 2023

woodruffw commented Oct 24, 2023

tests: Add `x509-limbo` test #1

tests: Add `x509-limbo` test #1

tetsuo-cpp commented Sep 3, 2023 •

edited

Loading

tetsuo-cpp Oct 24, 2023 •

edited

Loading