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

Allow validating V3 certificates that have no extensions #241

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

janimo
Copy link

@janimo janimo commented Sep 21, 2021

This is a really minimal (incomplete?) change to allow V3 certificates without extensions (like those used by Tor).
Probably the MissingOrMalformedExtensions error should be renamed to MalformedExtensions if this looks ok?

Copy link

@robin-kunzler robin-kunzler left a comment

Choose a reason for hiding this comment

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

@janimo : Thanks a lot for addressing this and creating the PR.

This looks good, and it solves the issue for me (it accepts the certificates without extensions that I would like to use).

Regarding the 'completeness' of this change:

  • I looked into the X509 certificate structure and noticed that after the 'subjectPublicKeyInfo' field, the certificate might optionally contain 'issuerUniqueID' and 'subjectUniqueID'. See here. This would still appear before the 'extensions'.
  • Thus, theoretically, a certificate without extensions that has one of the UniqueID fields would be rejected. However, AFAIU such certificates were also rejected before this change, since the code assumes them to be absent and tries to parse anything that comes at this point as extensions.
  • This behaviour is probably fine since the UniqueIDs are discouraged (see here). The RFC recommends to be capable of parsing them, but that should be considered a separate issue that may not be so important, IMO.

@briansmith : Does this change look reasonable to you?

Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

This looks good to me except it needs a test that verifies that an end-entity certificate with no extensions works in all aspects except name validation.

src/cert.rs Outdated Show resolved Hide resolved
src/cert.rs Outdated Show resolved Hide resolved
@janimo
Copy link
Author

janimo commented Dec 11, 2021

I've addressed the two comments. AIUI extra test cases apart from the current simple one are blocked on #248 ?

@djc
Copy link

djc commented Feb 10, 2023

@janimo would you be interested in rebasing this PR and submitting a PR to rustls/webpki?

If not, would you mind if someone else takes your PR and rebases it?

@janimo
Copy link
Author

janimo commented Feb 10, 2023

@djc hello, I cannot do it in the next few days, so anyone is welcome to adapt it. Thanks.

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.

4 participants