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

Generate client certs dynamically #1725

Merged
merged 7 commits into from Nov 4, 2019

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Oct 31, 2019

Opening this pull request to get a review of the last commit: pquentin@22da6fd

I'll rebase it if/when the other pull requests get merged.

Copy link
Member

@sethmlarson sethmlarson left a comment

Looks pretty good, a couple questions for you

test/conftest.py Outdated


@pytest.fixture(scope="session")
def generated_certs(tmp_path_factory):
Copy link
Member

@sethmlarson sethmlarson Oct 31, 2019

Choose a reason for hiding this comment

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

Hate to be nit-picky about names but can we call this certs_dir or something shorter? :)

Copy link
Member Author

@pquentin pquentin Oct 31, 2019

Choose a reason for hiding this comment

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

Don't worry, I love nitpicks! You're right, certs_dir would be better.

Copy link
Member Author

@pquentin pquentin Nov 3, 2019

Choose a reason for hiding this comment

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

Fixed, thanks.

# OpenSSL looks up certificates by the hash for their name, see c_rehash
# TODO infer the bytes using `cryptography.x509.Name.public_bytes`.
# https://github.com/pyca/cryptography/pull/3236
shutil.copyfile(DEFAULT_CA, str(tmpdir / "b6b9ccf9.0"))
Copy link
Member

@sethmlarson sethmlarson Oct 31, 2019

Choose a reason for hiding this comment

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

This will be changed once we move to generating server certs as well?

Copy link
Member Author

@pquentin pquentin Oct 31, 2019

Choose a reason for hiding this comment

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

Yes, it will change. I'm not sure yet how that will look. Ideally I'll be able to get the hash programmatically, maybe via trustme. It's also possible that it will stay static because it depends only on the name which could stay static. Not sure, really. I've spend quite some time trying to figure this out but don't have a satisfying answer here yet. :(

@codecov-io
Copy link

codecov-io commented Nov 3, 2019

Codecov Report

Merging #1725 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1725   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files          22       22           
  Lines        2006     2006           
=======================================
  Hits         1999     1999           
  Misses          7        7

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4b36ad...1c8bc1e. Read the comment docs.

pquentin added 7 commits Nov 3, 2019
This was not possibly until now because of functool.wraps for older
Pythons.
Instead of storing it in git.
In order to support older Python versions.
We keep the existing client_intermediate.pem because it's used in the
password tests.

And we still rely on the existing root CA to generate the client
certificate because it's still used for the server certificate. Since
the CA uses a 1024-bit key, those client_intermediate tests still don't
work on macOS 10.15, but this commit still is a step forward.
@pquentin pquentin force-pushed the dynamic-trustme-certs branch from 9cf5fae to 1c8bc1e Compare Nov 3, 2019
Copy link
Member

@sethmlarson sethmlarson left a comment

LGTM!

@sethmlarson sethmlarson merged commit efe8230 into urllib3:master Nov 4, 2019
2 checks passed
This was referenced Nov 4, 2019
@pquentin pquentin deleted the dynamic-trustme-certs branch Nov 30, 2019
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

3 participants