Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

limbo: add duplicate_extensions test #65

Merged
merged 4 commits into from
Nov 2, 2023
Merged

limbo: add duplicate_extensions test #65

merged 4 commits into from
Nov 2, 2023

Conversation

woodruffw
Copy link
Collaborator

This adds a new basic namespace, along with a duplicate_extensions testcase that ensures that implementations reject certificates with duplicated extensions.

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw self-assigned this Nov 2, 2023
Copy link
Contributor

@facutuesca facutuesca Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could go in the rfc5280.py file, since it's defined in that doc. Docstring for the test:

    """
    Produces the following **invalid** chain:

    ```
    root -> EE
    ```

    The EE cert contains a duplicated extension, which is
    forbidden under the [RFC 5280 profile].

    > A certificate MUST NOT include more than one instance of a particular
    > extension.

    [RFC 5280 profile]: https://datatracker.ietf.org/doc/html/rfc5280#section-4.2
    """

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is debatable: RFC 5280 includes it as a matter of course, but it's also a "baseline" property of X.509v3:

EXTENSION ::= CLASS {
 &id OBJECT IDENTIFIER UNIQUE,
 &ExtnType }
WITH SYNTAX {
 SYNTAX &ExtnType
 IDENTIFIED BY &id }

(From https://www.itu.int/rec/T-REC-X.509-201910-I/en, page 17)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR I'm okay with it being in rfc5280.py since it's also specified there 🙂

Comment on lines 326 to 331
# NOTE: Add extension manually to bypass validation.
builder._extensions.append(
x509.Extension(
extra_extension.ext.oid, extra_extension.critical, extra_extension.ext
)
)
Copy link
Contributor

@facutuesca facutuesca Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should leave extra_extension as it is (it adds extensions using the "official" Builder.add_extension API), and add a new param (something like extra_unchecked_extensions) that bypasses validation:

        extra_extension: _Extension | None = None,
        extra_unchecked_extensions: list[_Extension] | None = None,
        # ...
        if extra_unchecked_extensions is not None:
            for e in extra_unchecked_extensions:
                builder._extensions.append(
                x509.Extension(
                    e.oid, e.critical, e.ext
                )
            )

My reasoning is that the intent will be clearer for whoever reads the tests ("oh, this test depends on creating a certificate that can't be created using the normal API"), and it's also useful for writing the tests, since most of the time you do want these checks enabled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a good idea. Longer term I'd like to turn extra_extension into extra_extensions as well, but that can be entirely separate.

@facutuesca facutuesca mentioned this pull request Nov 2, 2023
24 tasks
@woodruffw
Copy link
Collaborator Author

LGTM, thanks @facutuesca!

@facutuesca facutuesca self-requested a review November 2, 2023 17:27
@woodruffw woodruffw merged commit f1bebbf into main Nov 2, 2023
6 checks passed
@woodruffw woodruffw deleted the ww/basic branch November 2, 2023 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants