Adds support for sslyze>=3.0.0

[SaptakS]

Updates the https_check function to use code that works with sslyze>=3.0.0

• fixes import errors for the current required modules
• uses the new format for certificate analyzer
• updates dependency of sslyze to >=3.0.0

Refs: #209

Also, thanks to Ethan's code snippet that helped in this commit.

🧪 Testing

Tested the code using the current test suite of pshtt in debian 10, with python 3.7 Following is the command I ran: python3 -m pytest

✅ Checklist

• [x] This PR has an informative and human-readable title.
• [x] Changes are limited to a single goal - eschew scope creep!
• [x] All relevant type-of-change labels have been added.
• [x] I have read the CONTRIBUTING document.
• [x] These code changes follow cisagov code standards.
• [x] All new and existing tests pass.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging ce0af6c415ce72d95b4d525aa774a824ab3ab4f0 into 59f2bb0b1d64a79adf13552c711c883771020178 - view on LGTM.com

new alerts:

• 1 for Module is imported with 'import' and 'import from'

[jsf9k]

I think we should do a minor version bump before merging this. I tried to do this myself, but I can't commit to this branch, so I think @SaptakS will have to do it. Here's how:

1. Make sure you have the semver Python package installed
2. From the pshtt project root, simply run ./bump_version.sh minor

[mcdonnnj]

I think we should do a minor version bump before merging this. I tried to do this myself, but I can't commit to this branch, so I think @SaptakS will have to do it. Here's how:

1. Make sure you have the `semver` Python package installed

2. From the `pshtt` project root, simply run `./bump_version.sh minor`

GitHub has a feature to allow maintainers to edit forks: https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

[SaptakS]

I agree with @mcdonnnj that the maintainers with write access should have permission to push to branches of fork. But I have done the version bump and the typo fix in the comment.

cc: @jsf9k @hillaryj

[SaptakS]

The codebase needs to be blackened as well as skeletonized and I'll add an issue for that if one doesn't exist.

I haven't done this in the PR because black is making changes in a lot of unrelated files.

[mcdonnnj]

The codebase needs to be blackened as well as skeletonized and I'll add an issue for that if one doesn't exist.

I haven't done this in the PR because black is making changes in a lot of unrelated files.

That's fine as that is an "us" task, so don't worry about that. It's a long-term maintenance task for this project that we just haven't had the time to do yet.

[hillaryj]

Yup, that's absolutely out of the scope of this PR, although thank you for trying!

I added follow-on task https://github.com/cisagov/pshtt/issues/215 Skeletonize repository and standardize code formatting.

[SaptakS]

Hi @mcdonnnj , is there anything else needed from my side on this PR?

[jsf9k]

Hi @mcdonnnj , is there anything else needed from my side on this PR?

Nope, I think we are just waiting on reviews.

@mcdonnnj, are you good with merging once we get a positive review from you or is there someone else you feel we should wait for?

[mcdonnnj]

Hi @mcdonnnj , is there anything else needed from my side on this PR?

Nope, I think we are just waiting on reviews.

@mcdonnnj, are you good with merging once we get a positive review from you or is there someone else you feel we should wait for?

Review is on my to-do, I will finish it tonight or tomorrow morning. @jsf9k did we want to get anything from @h-m-f-t? Other than that there are no additional blockers from me.

[h-m-f-t]

I'm good here and don't need to review. Thanks @SaptakS!

[SaptakS]

@mcdonnnj @jsf9k @h-m-f-t I made some updates in the code such that it takes into consideration all the certificate_deployments instead of just the first, and also checks if STORE is in trust_store.name of path_validation_results before assessing https_validity check.

Also, I realized there was a bug in my code, where it was checking the root certificate in the chain for self signed, returning true, and then declaring the https chain to be bad. So I fixed that so that we check till the second last certificate in the cert_chain.

@mcdonnnj let me know if this fixes all the requested changes.

[SaptakS]

Hi @mcdonnnj. Any updates on this?

[jsf9k]

Hi @mcdonnnj. Any updates on this?

@SaptakS, let me first apologize for taking so long to re-review this PR. We deeply value your contribution, but I am being pulled in many different directions right now and my time management skills need some work. If you can make the change to consider all certificate deployments (near line 700 of pshtt.py) then I think this PR will be good to go.

I also dig your avatar. It makes me think of Jerry Garcia and Allen Ginsberg. :peace_symbol:

[SaptakS]

If you can make the change to consider all certificate deployments (near line 700 of pshtt.py) then I think this PR will be good to go.

I am not sure I fully understand 😅 . I made the changes to consider all certificate deployments. If you are referring to this line I am not checking if the last cert is self-signed or not, since it's a root certificate.

Do you mean in that case I should check for certificate expiration for all certs, but avoid the self signing check on the last cert? As far as I know root certs will be self-signed, right? @mcdonnnj

I also dig your avatar. It makes me think of Jerry Garcia and Allen Ginsberg.

Thanks!

[jsf9k]

I am not sure I fully understand . I made the changes to consider all certificate deployments. If you are referring to this line I am not checking if the last cert is self-signed or not, since it's a root certificate.

Do you mean in that case I should check for certificate expiration for all certs, but avoid the self signing check on the last cert? As far as I know root certs will be self-signed, right? @mcdonnnj

I just meant to do the things you mentioned in your comments here and here. You're right - it looks like you already did those things. I was paying more attention to the PR comments than to the code - mea culpa.

[eloquence]

Hi folks, over at @freedomofpress we're hoping to land this PR in order to keep the cryptography dependency up-to-date for repos such as https://github.com/freedomofpress/securethenews/ . It looks like it's been approved and all code review comments have been addressed - is there anything we can do to help move it along from here?

[mcdonnnj]

Hi folks, over at @freedomofpress we're hoping to land this PR in order to keep the cryptography dependency up-to-date for repos such as https://github.com/freedomofpress/securethenews/ . It looks like it's been approved and all code review comments have been addressed - is there anything we can do to help move it along from here?

Apologies for this falling off the radar a bit. I need to do one last sweep this week with the final determination on multiple certificate chains in mind to make sure everything lines up. I also wanted to do some last spot testing to ensure there aren't any immediate issues around how we use this package.

So final resolution should be end of the week at the latest.

[SaptakS]

@mcdonnnj @jsf9k I have tried to address the above reviews and comments to handle all certificate deployments. For verified_certificate_chain logics, I created a separate variable has_verified_cert_chain which is true only if all certificate deployments have a verified_certificate_chain and used that variable in the both the if logics. Also, I added the length of the received_certificate_chain for all the certificate deployments to get the endpoint.https_cert_chain_len and used the same in the error log since they were actually the same values it seems.

Let me know if all that sounds good.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 0ae78a49b0b3848cde918653b5e93676471d82e7 into 59f2bb0b1d64a79adf13552c711c883771020178 - view on LGTM.com

new alerts:

• 1 for Unused local variable

[SaptakS]

Hi @mcdonnnj did you get a chance to look at my latest changes?

[eloquence]

Just a quick ping - would be great to get this over the finish line so we can update other dependencies like cryptography in our projects. (We may end up switching to a fork, at least temporarily, to do so.)

[mcdonnnj]

@SaptakS @eloquence Sorry for the radio silence. I will re-review and test everything by Friday and hopefully we won't need any additional changes.

[michaelblyons]

I will re-review and test everything by Friday and hopefully we won't need any additional changes.

Is this a metaphysical "Friday" or an ordinary temporal "Friday?"

[michaelblyons]

@mcdonnnj Nick, I beg you to spend some time on this.

[mcdonnnj]

@mcdonnnj Nick, I beg you to spend some time on this.

It's on the agenda but there have been other pressing development efforts for our team. Due to alignment with some internal needs it's hopefully becoming important enough that I can devote time to getting this situated and merged.

[michaelblyons]

Due to alignment with some internal needs it's hopefully becoming important enough that I can devote time to getting this situated and merged.

It looks like you talked them into PR #224 with a truly astounding 581 commits (25 new), doing black and such. But 5 months later, still no merge?

Even so, sslyze v2 is still pinned in that PR, when there's now a v5. We're 1.5 years from "test everything by Friday."

[epicfaace]

Note that this branch no longer works with sslyze 5. It only works with sslyze up to version 4.

With sslyze 5, I get this error:

$ pshtt example.com
Traceback (most recent call last):
  File "/home/codespace/.python/current/bin/pshtt", line 5, in <module>
    from pshtt.cli import main
  File "/usr/local/python/3.10.4/lib/python3.10/site-packages/pshtt/cli.py", line 27, in <module>
    from . import pshtt
  File "/usr/local/python/3.10.4/lib/python3.10/site-packages/pshtt/pshtt.py", line 30, in <module>
    from sslyze import (
ImportError: cannot import name 'ServerConnectivityTester' from 'sslyze' (/usr/local/python/3.10.4/lib/python3.10/site-packages/sslyze/__init__.py)

[mcdonnnj]

I have merged #224, created the 0.6.8 tag, and published 0.6.8 on PyPI. This pull request will need to be updated accordingly for us to proceed with a re-review, merge, and new release. There are some substantial changes made in the PR I mentioned so you may want to rebase or cherry pick.

Please note that the mentioned release will now allow you to install pshtt from PyPI on Python 3.7 without issue.

[SaptakS]

@mcdonnnj I have rebased the PR on current develop and resolved all merge conflicts. Also, linted the code to match the best practices applied while skeletonizing of the project. I have changed the python_requires in setup.py because sslyze >=3.x.x doesn't support python 3.6 but supports everything above that.

Also, pinned sslyze to <5.0.0 because sslyze 5.0.0 changes the scanner API completely. So if we want to use sslyze 5.x.x, then we need to update the entire code and change sslyze requirement to >=5.0.0. I am not sure what you think is the best decision there.