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 valid wildcard domains to be parsed without error #25

Merged
merged 4 commits into from May 5, 2017

Conversation

redNixon
Copy link

@redNixon redNixon commented May 1, 2017

#24 Similar to the fix implemented by Cryptography (pyca/cryptography@666252c#diff-61af608275d527065eb127c6c04220dd), this will allow domains that contain a single leading wildcard(see RFC 2595) to be detected as a DNSName type.

@mathiasertl
Copy link
Owner

mathiasertl commented May 1, 2017

Hi! Thanks for the pull request, this is definitely a great improvement. I'll look over in the next few days.

Btw, I think the CI tests failing are not an issue of your PR, it's a general issue with the testsuite. If not, I'll get back to you ;-). Other then that, I'd also welcome a testcase for a wildcard cert (see the function header in the example, these are executed doctests!).

@redNixon
Copy link
Author

redNixon commented May 4, 2017

The exact Travis failure looked to be dependent on when your CA was created, I made the days-till-expiration message dynamic to make running the tests across a working dev branch easier. I also added in the doctests for wildcard handling to test valid and invalid wildcard combinations.

@mathiasertl mathiasertl merged commit ba2af05 into mathiasertl:master May 5, 2017
@mathiasertl
Copy link
Owner

@redNixon Thanks for the PR, I just merged this.

A few comments:

  1. The doctest failed because you missed a branch. The function parses DNS:*.example.com more strictly then *.example.com (which is above your change).
  2. While your code was correct, I strongly recommend against using one-line constructs like foo if bar else bla. Not only does it trick code coverage tests, I have seen horrible bugs come from this in larger python projects.
  3. Thanks for the test fix as well. I'm not happy with the test itself, since it really should create it's own CA (e.g. the current test will fail ~10 years from now), so I will probably change it again ;-).

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.

None yet

2 participants