Add support for client authentication, some minor refactoring

[konklone]

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.

[echudow]

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]

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]

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

[echudow]

@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]

@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]

@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]

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

[jsf9k]

@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]

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

[jsf9k]

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:

• https://gotowork-piv-dr.ed.gov
• https://access.cpsc.gov
• https://access2.cpsc.gov
• https://workspace.usda.gov

[IanLee1521]

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]

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]

@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]

@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]

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.

[jsf9k]

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

[jsf9k]

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]

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.