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

Make sure that one of full_name, relative_name or crl_issuer is set i… #7710

Merged
merged 1 commit into from
Oct 15, 2022

Conversation

mathiasertl
Copy link
Contributor

Noticed that it's possible to create Distribution Points with no full name, relative name or issuer:

>>> x509.DistributionPoint(None, None, None, None)
<DistributionPoint(full_name=None, relative_name=None, reasons=None, crl_issuer=None)>

RFC 5280, section 4.2.1.13 says:

... either distributionPoint or cRLIssuer MUST be present.

Copy link
Member

@alex alex left a comment

Choose a reason for hiding this comment

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

Looks like there's a coverage issue, presumabely because some other test case is now triggering this check?

@mathiasertl
Copy link
Contributor Author

Just checking this. Discovered that there is a similar check further down in the constructor:

        if reasons and not crl_issuer and not (full_name or relative_name):
            raise ValueError(
                "You must supply crl_issuer, full_name, or relative_name when "
                "reasons is not None"
            )

... and the test_reason_only test would no longer reach these lines.

Now rereading RFC5280 to see if there is any basis for the above (none on my second read...)

@mathiasertl
Copy link
Contributor Author

mathiasertl commented Oct 15, 2022

Ah, hmm, the preceeding line says

a DistributionPoint MUST NOT consist of only the reasons field;

so the check now not covered is a valid check, strictly speaking. But the new check is still valid and covers the "only reasons flags" as well. If you agree, I would remove the check in line 633 (and leave the test).

@alex
Copy link
Member

alex commented Oct 15, 2022

Works for me.

@mathiasertl
Copy link
Contributor Author

done. Thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants