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

x509: fix handling of multiple URIs in Certificate#crl_uris #776

Open
wants to merge 2 commits into
base: maint-3.0
Choose a base branch
from

Conversation

rhenium
Copy link
Member

@rhenium rhenium commented Jul 9, 2024

The implementation of OpenSSL::X509::Certificate#crl_uris makes the assumption that each DistributionPoint in the CRL distribution points extension contains a single general name of type URI. This is not guaranteed by RFC 5280. A DistributionPoint may only contains something other than a URI, or more than one URI.

Let's include all URIs seen in the extension. If only non-URI pointers are found, return an empty array.

Fixes: #775

@rhenium
Copy link
Member Author

rhenium commented Jul 9, 2024

cc: @bdewater, #275

This PR currently returns an empty array when the certificate includes the extension, but no URIs in it. Should it be nil in that case too?

test_extesion is testing too many features at once and is hard to
navigate. Let's split each chunk apart for more clarity.
@rhenium rhenium force-pushed the ky/x509cert-crl_uris-fix-nil branch from 5fe29a3 to 342f5bd Compare July 9, 2024 13:39
@rhenium rhenium changed the base branch from master to maint-3.0 July 9, 2024 13:39
@bdewater
Copy link
Contributor

I think that'd be nice since an empty array is a bit useless otherwise.

The implementation of OpenSSL::X509::Certificate#crl_uris makes the
assumption that each DistributionPoint in the CRL distribution points
extension contains a single general name of type URI. This is not
guaranteed by RFC 5280. A DistributionPoint may contain zero or more
than one URIs.

Let's include all URIs found in the extension. If only non-URI pointers
are found, return nil.

Fixes: ruby#775
@rhenium rhenium force-pushed the ky/x509cert-crl_uris-fix-nil branch from 342f5bd to 71f4fef Compare August 16, 2024 06:05
@rhenium
Copy link
Member Author

rhenium commented Aug 16, 2024

Fair enough. I changed it to return nil if the extension contains only non-URIs.

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.

Certificate#crl_uris throws exception "undefined method `value' for nil (NoMethodError)"
2 participants