Adds support for sslyze>=3.0.0 #214

SaptakS · 2021-01-19T19:30:10Z

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

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

lgtm-com · 2021-01-19T19:40:55Z

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

new alerts:

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

pshtt/pshtt.py

hillaryj

Thanks for the contribution!

I've tested via the python tests as well as ran the tool locally (Mac OSX Catalina with Python 3.8.3) and got a nice little results.csv file out. Several other folks are reviewing as well - great work!

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

Thanks again and great work!

jsf9k

This all looks great to me. A big thank you to @SaptakS and @Ethanljf for working on this!

jsf9k · 2021-01-26T15:28:29Z

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:

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

mcdonnnj · 2021-01-26T17:19:09Z

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 · 2021-01-26T18:29:38Z

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 · 2021-01-26T18:31:44Z

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 · 2021-01-26T18:45:35Z

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 · 2021-01-26T19:28:57Z

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

I added follow-on task #215 Skeletonize repository and standardize code formatting.

SaptakS · 2021-02-01T20:06:25Z

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

jsf9k · 2021-02-01T20:14:19Z

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 · 2021-02-01T21:17:58Z

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 · 2021-02-01T21:18:55Z

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

- fix comments - use == instead of is Co-authored-by: Nick M. <[email protected]>

- consider all deployments and if all validations succeed, then consider it trusted - use path_validation_results instead of received_certificate_chain to consider if STORE is included in trust_store - check self signed validation in cert_chain till the second last cert since the last cert is root cert and hence is self signed anyways - put all logic inside one loop over path_validation_results

SaptakS · 2023-05-26T11:16:13Z

I have rebased, resolved conflicts and pushed. Waiting for the tests to pass. I think someone with permission need to approve the workflows.

.github/workflows/build.yml

michaelblyons · 2023-05-30T22:36:38Z

Are the coveralls complaints innocuous?

mcdonnnj · 2023-05-30T22:44:06Z

Are the coveralls complaints innocuous?

Relatively. Coverage decreased because total lines of code increased, but the current configuration doesn't do extensive testing for what is being modified in the first place. It's unreasonable to ask that testing be added from scratch to cover these changes so manual testing will have to suffice. As a result the coveralls complaints will be ignored.

jsf9k

These changes seem correct to me, and I'm inclined to approve. Have you done any testing beyond the (admittedly underwhelming) test suite? What I mean is, have you been using these changes to test against a wide array of servers?

In any event, we should probably do some testing on our end, just to get a warm and fuzzy that everything is still working. I can start looking into that next week.

harrislapiroff · 2023-06-12T18:38:20Z

@jsf9k How's that looking? We're very interested in getting this merged :)

michaelblyons · 2023-07-03T20:49:16Z

Hi folks. How was June? How's the review coming?

tgoetheyn · 2023-08-14T12:57:14Z

Well, Python 3.7 has reached end-of-life.
so hopefully someone comes back from vacation with enough energy to finally tackle this pull request

gbinal · 2023-10-11T14:38:57Z

Hi folks,

Just to help nudge this, my team is trying to use pshtt right now and can't at all at this point (some of the second order dependencies appear to no longer be hosted anymore).

Pretty please with sugar on top, can you all merge this PR?

jsf9k · 2023-10-16T15:21:20Z

This needs another rebase on top of develop before it can be merged, but we (cisagov) don't have permission to push changes to the PR branch. I did the rebase locally and it went cleanly, so this should be a five second task.

Alternatively, you can click the box to allow us to push up changes and I'll take care of it.

dav3r · 2023-10-16T15:42:16Z

@SaptakS Can you also please bump the minor version by running this command?

./bump_version.sh minor

This will need to be done prior to merging.

felddy · 2023-10-16T15:53:30Z

Add support for sslyze 3 (superset of PR #214)

felddy · 2023-10-16T20:17:26Z

#248 was merged with a superset of these changes rebased to develop, as well as a few of the outstanding change requests. I'm waiting on some "eventual consistency" with GitHub's view of this PR. I think it will realize it is merged at some point. If not, I'll come back and close it.

Thank you to everyone that contributed to the PR.

SaptakS force-pushed the sslyze3-support branch 3 times, most recently from c77fdc1 to 8a50ca8 Compare January 19, 2021 21:29

mcdonnnj requested review from jsf9k, echudow, mcdonnnj, dav3r, felddy and hillaryj January 20, 2021 17:40

mcdonnnj added the improvement This issue or pull request will add or improve functionality, maintainability, or ease of use label Jan 20, 2021

mcdonnnj linked an issue Jan 20, 2021 that may be closed by this pull request

Sslyze version incompatibility #209

Open

mcdonnnj assigned jsf9k and mcdonnnj Jan 20, 2021

hillaryj reviewed Jan 26, 2021

View reviewed changes

pshtt/pshtt.py Outdated Show resolved Hide resolved

hillaryj approved these changes Jan 26, 2021

View reviewed changes

jsf9k approved these changes Jan 26, 2021

View reviewed changes

jsf9k requested a review from h-m-f-t January 26, 2021 15:28

h-m-f-t removed request for h-m-f-t and echudow February 1, 2021 21:19

SaptakS and others added 8 commits May 26, 2023 13:38

Fixes typo in comment

870d026

Add fixes and suggestions from code review

7cc15e6

- fix comments - use == instead of is Co-authored-by: Nick M. <[email protected]>

Consider all certificate deployments for verified certificate chain

a0c7f66

Updates sslyze version to <5.0.0, upgrade required python to >=3.7

1f91486

Lints the python code

b995ada

Updates python versions in github workflow build

14987cc

Updates python version in build.yml

edb863e

SaptakS force-pushed the sslyze3-support branch from 57f20fa to edb863e Compare May 26, 2023 11:04

SaptakS requested review from dav3r, felddy and jasonodoom as code owners May 26, 2023 11:04

mcdonnnj reviewed May 26, 2023

View reviewed changes

.github/workflows/build.yml Outdated Show resolved Hide resolved

Removes Python 3.6 from build

3aed808

jsf9k reviewed Jun 1, 2023

View reviewed changes

jsf9k mentioned this pull request Oct 16, 2023

Pin pshtt to the latest version we support cisagov/domain-scan#5

Merged

5 tasks

felddy mentioned this pull request Oct 16, 2023

Add support for sslyze 3 (superset of PR #214) #248

Merged

9 tasks

felddy added a commit that referenced this pull request Oct 16, 2023

Merge pull request #248 from cisagov/improvement/sslyze3

4f9b94f

Add support for sslyze 3 (superset of PR #214)

felddy closed this Oct 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for sslyze>=3.0.0 #214

Adds support for sslyze>=3.0.0 #214

SaptakS commented Jan 19, 2021

lgtm-com bot commented Jan 19, 2021

hillaryj left a comment

jsf9k left a comment

jsf9k commented Jan 26, 2021

mcdonnnj commented Jan 26, 2021

SaptakS commented Jan 26, 2021

SaptakS commented Jan 26, 2021

mcdonnnj commented Jan 26, 2021

hillaryj commented Jan 26, 2021 •

edited

Loading

SaptakS commented Feb 1, 2021

jsf9k commented Feb 1, 2021

mcdonnnj commented Feb 1, 2021

h-m-f-t commented Feb 1, 2021

SaptakS commented May 26, 2023

michaelblyons commented May 30, 2023

mcdonnnj commented May 30, 2023

jsf9k left a comment

harrislapiroff commented Jun 12, 2023

michaelblyons commented Jul 3, 2023

tgoetheyn commented Aug 14, 2023

gbinal commented Oct 11, 2023

jsf9k commented Oct 16, 2023

dav3r commented Oct 16, 2023

felddy commented Oct 16, 2023

felddy commented Oct 16, 2023

Adds support for sslyze>=3.0.0 #214

Adds support for sslyze>=3.0.0 #214

Conversation

SaptakS commented Jan 19, 2021

🧪 Testing

✅ Checklist

lgtm-com bot commented Jan 19, 2021

hillaryj left a comment

Choose a reason for hiding this comment

jsf9k left a comment

Choose a reason for hiding this comment

jsf9k commented Jan 26, 2021

mcdonnnj commented Jan 26, 2021

SaptakS commented Jan 26, 2021

SaptakS commented Jan 26, 2021

mcdonnnj commented Jan 26, 2021

hillaryj commented Jan 26, 2021 • edited Loading

SaptakS commented Feb 1, 2021

jsf9k commented Feb 1, 2021

mcdonnnj commented Feb 1, 2021

h-m-f-t commented Feb 1, 2021

SaptakS commented May 26, 2023

michaelblyons commented May 30, 2023

mcdonnnj commented May 30, 2023

jsf9k left a comment

Choose a reason for hiding this comment

harrislapiroff commented Jun 12, 2023

michaelblyons commented Jul 3, 2023

tgoetheyn commented Aug 14, 2023

gbinal commented Oct 11, 2023

jsf9k commented Oct 16, 2023

dav3r commented Oct 16, 2023

felddy commented Oct 16, 2023

felddy commented Oct 16, 2023

hillaryj commented Jan 26, 2021 •

edited

Loading