Add support for client authentication, some minor refactoring #179

konklone · 2018-10-23T14:10:35Z

I'm opening this to more easily track @echudow's work on adding client cert authentication to pshtt.

There is some additional refactoring work in here, and possibly some logical changes. Let's use this PR to hash out the changes that would make pshtt usable for DHS, GSA, and DoD, without creating unintended regressions from existing reporting.

See also 18F/domain-scan#286.

…re publicly trusted.

echudow · 2018-10-23T16:07:17Z

Because of the client cert issue, there could be some times when the initial request fails (and so it would not seem to be valid) even though it actually is valid so I changed one part where the initial request does not complete to assume that it is still valid and just couldn't complete the TLS handshake for other reasons, and then look at the sslyze results later to see if the HTTPS isn't valid.

echudow · 2018-10-23T16:07:53Z

I also refactored some of the redirect code and I think fixed a bug there where it would report a redirect when there were no redirects.

echudow · 2018-10-23T16:08:39Z

I also changed most of the log output to include the URL for easier troubleshooting based on the logs.

echudow · 2018-10-24T13:57:10Z

@konklone Following up on the discussion from the other thread, are the results from my version actually harsher than the earlier pshtt version? If so, which field? It sounds like we're focusing on the Valid HTTPS field, but in the original version that field would only be true if the first request successfully completed, but the Python requests module I'm pretty sure throws an exception instead if the server cert is self-signed or has a bad chain (including missing Intermediate CA certs which is another issue), so the earlier version of pshtt should flag those as not valid. If there's an exception, then pshtt tries again without cert validation but the pshtt logic did not mark those as valid https in that case. Is this an issue in the original pshtt too or is there a different issue that I'm not seeing?

konklone · 2018-10-24T14:17:59Z

@echudow My question about whether this was harsher on untrusted certs was a genuine question. :) I'm not sure, and have to go back and look at what the current outputs are.

echudow · 2018-10-24T16:06:38Z

@konklone Ok, let me know what you find out and then I'll work through any issues so hopefully we can all use the same code in the end.

jsf9k · 2018-11-13T16:53:53Z

@echudow and @konklone
Sorry for letting this fall through the cracks. Somehow I missed it. I'm looking at it now.

jsf9k · 2018-11-13T20:46:15Z

@echudow can you give me write permissions to your fork? I want to merge the upstream changes into your branch, but I can't commit there. The result of the merge is here. There were no conflicts.

The alternative is for me to create a pull request to your branch.

echudow · 2018-11-13T21:03:52Z

Ok, I just gave you and Eric access to my forks of pshtt and domain-scan.

Note that changing the logic from: if domain.https.live == False and domain.httpswww.live == False: to: if not domain.https.live and not domain.httpswww.live: changes the result when both domain.https.live and domain.httpswww.live are both None. This is because None != False but not None is True. I think that in the edge case where domain.https.live == None and domain.httpswww.live == None it makes more sense to return "N/A" anyway.

jsf9k · 2018-11-30T18:12:57Z

Reviewing the code, I don't see any problems with these changes. I could easily be missing something, but I think the only differences from the old code will be the new fields and sometimes the values of "liveness" and "is HTTPS valid". Still, I need to do a full pshtt scan and compare with the old results. I'm on vacation next week, but I can do that after I get back.

In the meantime, I added some reviewers in hope of getting more eyes on this pull request. If folks want to test, I can also provide a few URLs that require client certificates:

IanLee1521 · 2018-12-01T02:15:13Z

I don't have much experience with testing / using client cert sites, but this functionally looks fine to me. Do we have a verification that there aren't any regressions?

echudow · 2018-12-18T19:38:57Z

Just fixed a bug that I introduced that prevented following some http redirects. I'm sure that would have caused some issues with some incorrect data. Has anyone compared the output to previous runs to see if there are any other bugs or regressions? Hopefully it just adds some data instead of some earlier errors.

IanLee1521 · 2019-03-15T00:49:54Z

@jsf9k / @echudow -- Do you feel like this is ready to be merged? Looks like travis passed the PR build but hung on the branch build (sure...), I'm OK ignoring the coveralls drop which is the only real "fail" right now, though alternatively, is their more testing we could add for this to get the test coverage up?

Also, note that you are in a much better position than I am to confirm that this is working as expected and doesn't cause regressions.

If you're happy with it though, I can do the actual code review part for what I can comment on.

jsf9k · 2019-03-15T13:13:58Z

@IanLee1521 and @echudow, this PR will break the BOD 18-01 scanning. This is because it treats some fields that were boolean as a union of a boolean and a string.

This does give us more information than we had before, but I think it's important for the data fields to have a set type - particularly for the JSON output. What if we by default assign fields a type of None and only set them to True or False if we actually know the result? That will still break the BOD 18-01 scanning, and I will have to adjust it accordingly, but I think it's cleaner than using strings and booleans in the same field.

I can probably find some time to work on that whis weekend if you guys agree.

Thanks to @IanLee1521 for bumping this PR. We need to get this approved and merged since it's practically old enough to vote at this point. 👴

echudow · 2019-03-15T15:17:41Z

Thanks guys for looking at this again. Leaving fields as None should work for us. Before, pshtt would automatically turn almost all the fields that are None to False, but with the changes I made there are cases where there are partial scans and only some of the data is known and others are unknown. Leaving them as None to indicate that they are unknown works for me and then we can convert those to "Unknown" when we put it up on our dashboard.

This is required to make the test pass after the changes in the last commit.

jsf9k · 2019-03-18T02:42:34Z

This pull request introduces 1 alert when merging 52b9c7b into 97c51ac - view on LGTM.com

new alerts:

1 for Unnecessary pass

Comment posted by LGTM.com

The block where the "Unknown" results were being set is tricky because of the continue statements. This time I was careful to remove only the statements that set fields from None to "Unknown" and leave the continue statements.

jsf9k · 2019-03-18T03:11:21Z

This pull request introduces 1 alert when merging fbc318a into 97c51ac - view on LGTM.com

new alerts:

1 for Unnecessary pass

Comment posted by LGTM.com

jsf9k · 2019-03-18T11:28:25Z

This pull request fixes 2 alerts when merging f20eca4 into 97c51ac - view on LGTM.com

fixed alerts:

2 for Unnecessary pass

Comment posted by LGTM.com

jsf9k

Based on the results from doing a full test run with the new code, I think this PR is finally ready to be merged.

The latest saver and scanner account for the changes made by cisagov/pshtt#179, cisagov/trustymail#110, and 18F/domain-scan#286. See cisagov/saver#41 and cisagov/scanner#31 for details.

…mmit_hook Add a pre-commit hook to run `pip-audit`

Added test for required Client Authentication and whether the certs a…

e16dd24

…re publicly trusted.

Merge remote-tracking branch 'dhs/develop' into develop

c9263c0

jsf9k mentioned this pull request Nov 23, 2018

Add support for client authentication 18F/domain-scan#286

Merged

jsf9k added 9 commits November 23, 2018 15:24

Merge remote-tracking branch 'dhs/develop' into develop

1b6a00b

Update test to match @echudow's new code

716292c

Add missing commas

48bbf90

Fix test to agree with latest code

5d293ae

This time the test is really fixed!

9a84999

Get rid of some warnings about using the deprecated logging.warn()

63061cc

Fix a bunch of warnings that flake8 complains about

836fa98

Bump version from 0.5.2 to 0.6.0

16e1aaa

jsf9k requested review from jsf9k, IanLee1521, h-m-f-t and a team November 30, 2018 18:20

jsf9k self-assigned this Nov 30, 2018

Fixed bug that prevents following some http redirects

cb6aba4

echudow and others added 11 commits January 25, 2019 06:56

Allow for Unknown in cert fields

ba780fa

Fixed missing not

4e20c97

Using canonical endpoint for cert checks

88fd78e

Added missing intermediate CA checking for public trust

bbbce65

Have custom trust as unknown if no custom trust store

f049bb5

Merge branch 'develop' into develop

ffe02c4

Merge branch 'develop' into develop

a0ffba9

Make flake8 happy

3ed17e9

Make flake8 happy

4f8687c

Make failing test pass

2aaf61a

Make failing test pass

1ef2170

jsf9k added 3 commits March 17, 2019 22:09

Remove code that was setting fields with value None to "Unknown"

204c8a7

Change 'Unknown' to None in test

0938c7b

This is required to make the test pass after the changes in the last commit.

Modify list comprehension in test to handle None

52b9c7b

jsf9k added 2 commits March 17, 2019 22:47

Rework the removal of "Unknown" from the pshtt result

d32198a

The block where the "Unknown" results were being set is tricky because of the continue statements. This time I was careful to remove only the statements that set fields from None to "Unknown" and leave the continue statements.

Add a comment warning about the tricky continue logic

fbc318a

Remove some unnecessary pass statements that LGTM reported

f20eca4

jsf9k mentioned this pull request Mar 19, 2019

Rework for new fields cisagov/saver#41

Merged

jsf9k approved these changes Mar 20, 2019

View reviewed changes

jsf9k merged commit cd5d0c9 into cisagov:develop Mar 20, 2019

This was referenced Mar 20, 2019

Use the latest saver and scanner Docker images cisagov/orchestrator#69

Merged

pshtt may not be handling client cert authentication properly #153

Closed

cisagovbot pushed a commit that referenced this pull request Jan 31, 2025

Merge pull request #179 from cisagov/improvement/add_pip-audit_pre-co…

5801cec

…mmit_hook Add a pre-commit hook to run `pip-audit`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for client authentication, some minor refactoring #179

Add support for client authentication, some minor refactoring #179

konklone commented Oct 23, 2018 •

edited by jsf9k

Loading

echudow commented Oct 23, 2018

echudow commented Oct 23, 2018

echudow commented Oct 23, 2018

echudow commented Oct 24, 2018

konklone commented Oct 24, 2018

echudow commented Oct 24, 2018

jsf9k commented Nov 13, 2018

jsf9k commented Nov 13, 2018 •

edited

Loading

echudow commented Nov 13, 2018

jsf9k commented Nov 30, 2018 •

edited

Loading

IanLee1521 commented Dec 1, 2018

echudow commented Dec 18, 2018

IanLee1521 commented Mar 15, 2019

jsf9k commented Mar 15, 2019 •

edited

Loading

echudow commented Mar 15, 2019

jsf9k commented Mar 18, 2019

jsf9k commented Mar 18, 2019

jsf9k commented Mar 18, 2019

jsf9k left a comment

Add support for client authentication, some minor refactoring #179

Add support for client authentication, some minor refactoring #179

Conversation

konklone commented Oct 23, 2018 • edited by jsf9k Loading

echudow commented Oct 23, 2018

echudow commented Oct 23, 2018

echudow commented Oct 23, 2018

echudow commented Oct 24, 2018

konklone commented Oct 24, 2018

echudow commented Oct 24, 2018

jsf9k commented Nov 13, 2018

jsf9k commented Nov 13, 2018 • edited Loading

echudow commented Nov 13, 2018

jsf9k commented Nov 30, 2018 • edited Loading

IanLee1521 commented Dec 1, 2018

echudow commented Dec 18, 2018

IanLee1521 commented Mar 15, 2019

jsf9k commented Mar 15, 2019 • edited Loading

echudow commented Mar 15, 2019

jsf9k commented Mar 18, 2019

jsf9k commented Mar 18, 2019

jsf9k commented Mar 18, 2019

jsf9k left a comment

Choose a reason for hiding this comment

konklone commented Oct 23, 2018 •

edited by jsf9k

Loading

jsf9k commented Nov 13, 2018 •

edited

Loading

jsf9k commented Nov 30, 2018 •

edited

Loading

jsf9k commented Mar 15, 2019 •

edited

Loading