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

change x509 validation to allow parsing of some "bad" certs #3862

Closed
wants to merge 2 commits into from

Conversation

reaperhulk
Copy link
Member

We now validate on these fields when adding the objects to the CertificateBuilder.

This PR does not attempt to migrate all validation logic. Each extension has logic that may be specific to Python or enforcement of an RFC restriction. Before migrating more logic I'd like to see examples of certificates that encode things in that specific invalid way.

Fixes #3857 and #3856

tests/test_x509.py Outdated Show resolved Hide resolved
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.

Please put the vector in a separate PR.

You also seem to not be handling these validations in CSR, CRL, and whatever-the-hell-else builders.

src/cryptography/x509/extensions.py Outdated Show resolved Hide resolved
@reaperhulk reaperhulk force-pushed the allow-garbage branch 2 times, most recently from fcb13ea to 3db524f Compare August 24, 2017 23:00
@reaperhulk
Copy link
Member Author

This is ready for review again. Still outstanding: whether we want to switch to adding _validate() to every extension class rather than the (new) getattr approach.

@jcrowgey
Copy link
Contributor

I have a dataset of about 20 certificates (and still growing) which break the validation steps in this library. All were harvested in the wild. If there's some way to publish these examples for you conveniently, I'd love to contribute. Currently, I just monkey patch the init methods on lots of the Extension and Name classes to remove the validation checks.

@2rs2ts
Copy link

2rs2ts commented Jan 12, 2019

Is this PR abandoned?

@jcrowgey
Copy link
Contributor

jcrowgey commented Jan 12, 2019

@2rs2ts what timing. I just released (a few hours ago) the library I use which does the monkeypatching to allow loading of certs which don't always follow best practices.

https://github.com/jcrowgey/x5092json

@2rs2ts
Copy link

2rs2ts commented Jan 14, 2019

@jcrowgey nice... I am not sure your library solves my problem, but it is indeed cool! 😄

@reaperhulk reaperhulk force-pushed the allow-garbage branch 2 times, most recently from 8bb4e69 to cf3f2ae Compare January 18, 2019 21:22
We now validate on these fields when adding the objects to the
various builders
@reaperhulk
Copy link
Member Author

This has been pushed 5 times now and is no less problematic now than it was 3 years ago. I think to get "bad" cert parsing we're going to need to have someone commit to the large amount of work necessary for a hazmat x509 parsing API. When that gets implemented we can have this.

In the meantime the monkeypatching model is probably the best path to bypass this type of validation if needed.

@reaperhulk reaperhulk closed this Jul 5, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 4, 2020
@reaperhulk reaperhulk deleted the allow-garbage branch January 9, 2023 01:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

Support non-standard X509 country names
4 participants