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

Add support for exposing server certificate #75

Merged
merged 4 commits into from
Feb 22, 2024
Merged

Add support for exposing server certificate #75

merged 4 commits into from
Feb 22, 2024

Conversation

Nicoretti
Copy link
Contributor

Dear pytest-localserver maintainers,

First off, I want to send a big 👍 your way. Thanks for providing and maintaining this plugin!

While I was using it in the context of HTTPS, I stumbled upon a few minor issues.
I tried addressing all of these in this PR.

I hope you find the updates useful. If there's anything that needs tweaking or if you'd like me to make any changes, please don't hesitate to let me know.

best

Nico

@diazona
Copy link
Contributor

diazona commented Feb 16, 2024

Thanks for sending a contribution Nico! I'll review this properly later (over the weekend, or tonight if I have time), but for starters, I wonder if you could explain the use cases that led you to propose these changes? (This will probably be good info to edit into the commit messages at some point, but for now we can discuss it in comments.)

@diazona diazona self-assigned this Feb 16, 2024
@diazona diazona added the enhancement New feature or request label Feb 16, 2024
@Nicoretti
Copy link
Contributor Author

Hi @diazona,

No worries take your time. My use cases are quite similar to the ones I've covered in the unit tests of this PR.

When dealing with SSL/TLS, there are three interesting variations, especially concerning server certificate validation when it's not signed by an official Certificate Authority (CA).

  1. Verify and abort the connection if validation fails.
  2. Proceed without verification (implicitly trust the certificate).
  3. Add the certificate to a list of trusted certificates.

Testing scenario 3. requires access to the server's certificate. The httpsserver fixture, with its self-signed certificate, is ideal for this purpose. However, to accurately test this scenario, the server certificate must be made accessible to the client, which was not previously implemented.

While I was on it, I encountered and addressed a few other issues:

  • The parameters for sslcontext were passed in an incorrect order. This mistake was subtle, as it didn't manifest while the server's key and certificate were combined in a single .pem file.
  • I separated the server.pem into individual key and certificate files (key and crt), allowing them to be exposed separately for more granular control.

best
Nico

@Nicoretti
Copy link
Contributor Author

For more context: My use cases can be found here and the test(s) you can find here.

@Nicoretti
Copy link
Contributor Author

Nicoretti commented Feb 16, 2024

This PR likely shows the important bits best.

Copy link
Contributor

@diazona diazona left a comment

Choose a reason for hiding this comment

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

Thanks for your patience. I like the general idea, I just left some suggestions (which I'm happy to discuss if there's anything you don't agree with).

As for commit organization, I would suggest combining the commits that split the certificate/key files and that make the corresponding changes in the code, so you have three commits in all:

  • One that fixes the ordering of cert and key
  • One that splits the key into a separate file and makes corresponding updates in the code
  • One that adds the certificate() method (and url() if we wind up keeping that one)

Optionally you could break out the tests into a fourth commit, but that's not necessary. The goal is basically to make sure that each commit has a clear purpose, and that the code compiles on each commit (to make bisection easier, if we ever need to do that). This is not a requirement, though, so it's your call if you want to go that far.

pytest_localserver/cert.crt Outdated Show resolved Hide resolved
pytest_localserver/https.py Outdated Show resolved Hide resolved
pytest_localserver/https.py Outdated Show resolved Hide resolved
pytest_localserver/server.key Outdated Show resolved Hide resolved
pytest_localserver/https.py Outdated Show resolved Hide resolved
tests/test_https.py Outdated Show resolved Hide resolved
tests/test_https.py Outdated Show resolved Hide resolved
tests/test_https.py Outdated Show resolved Hide resolved
@Nicoretti
Copy link
Contributor Author

...
Optionally you could break out the tests into a fourth commit, but that's not necessary. The goal is basically to make sure that each commit has a clear purpose, and that the code compiles on each commit (to make bisection easier, if we ever need to do that). This is not a requirement, though, so it's your call if you want to go that far.

I totally understand the appeal of having clean and "building" commits (I originally come from a Gerrit-based workflow). While GitHub makes this workflow tricky for various reasons (imho), I'm totally with you on this. So, I suggest once you are satisfied with the PR, I'll squash and rebase it into 3-4 commits.

Copy link
Contributor

@diazona diazona left a comment

Choose a reason for hiding this comment

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

Whoops I forgot to "officially" leave a review when I commented

@Nicoretti
Copy link
Contributor Author

@diazona quick heads-up: I'll update the commits and the certificate when I have some downtime. However, it might take me a couple of days.

Just so you know, I intend to follow through on this PR. I'm just a bit short on time over the next few days.

@diazona
Copy link
Contributor

diazona commented Feb 20, 2024

No problem at all, we're in no rush here and it's fine if this takes a few days or even longer to wrap up. I'll be around to help get it finished up whenever you're ready!

@Nicoretti
Copy link
Contributor Author

@diazona just updated the PR let me know if you need me to do further changes.

Copy link
Contributor

@diazona diazona left a comment

Choose a reason for hiding this comment

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

Looks good now! Thanks again for the changes.

@diazona
Copy link
Contributor

diazona commented Feb 22, 2024

BTW did you have an interest in getting this PR into a release soon? I hadn't planned on making another release until version 1, whenever that happens, but it'll probably be a while until then. If you'd like these changes available on PyPI earlier, I can make a prerelease to include them or even a version 0.9.

@Nicoretti
Copy link
Contributor Author

@diazona, there's no immediate rush on my end. My goal is to enhance coverage for certain edge cases eventually.

@diazona
Copy link
Contributor

diazona commented Feb 22, 2024

Sounds good, I'll leave it for whenever the next release happens, then. Thanks again!

@diazona diazona merged commit 91a41bb into pytest-dev:master Feb 22, 2024
51 checks passed
@Nicoretti Nicoretti deleted the feature/add-support-for-exposing-server-certificate branch February 28, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants