search

18F / domain-scan

A lightweight pipeline, locally or in Lambda, for scanning things like HTTPS, third party service use, and web accessibility.
Other
370 stars 137 forks source link

Add support for client authentication #286

Closed jsf9k closed 5 years ago

jsf9k commented 5 years ago

Creating a pull request from @echudow's fork. This pull request goes along with dhs-ncats/pshtt#179 and should not be merged before that one is.

jsf9k commented 5 years ago

Added some reviewers in hope of getting more eyes on these changes.

jsf9k commented 5 years ago

@echudow and @IanLee1521, I'm doing a full BOD 18-01 scan in my staging environment (with echudow/pshtt, echudow/trustymail, and echudow/domain-scan) as a means of regression testing. I will hopefully have results to share tomorrow.

jsf9k commented 5 years ago

@echudow and @IanLee1521 Update: the scans ran. I ran into some difficulty saving the results, but I am fixing that now. I should have reports generating by the end of the day.

Second update: Reports are generating! I will take a look at them tonight or tomorrow morning.

Third update: The HTTPS reports are looking good. I verified that some sites that require client certs are now passing, and they weren't before. Except for that, the overall results look very similar to those from last weekend. So that's good news!

Fourth update: The Trustworthy Email reports look almost identical to the ones from the past weekend's run. So it looks like those are good too!

jsf9k commented 5 years ago

Based on the results from a full run with the new code, I think this PR is ready to be merged. I can't approve the PR myself, though, since I created it. @echudow or @IanLee1521, can either of you approve this PR? After that we should be able to merge.

jsf9k commented 5 years ago

Once cisagov/trustymail#110 is merged I will rerun the CI for this repo and it will pass.

jsf9k commented 5 years ago

@IanLee1521, yes this is the code I used to run the BOD 18-01 scans and compare against the "official" results from this past weekend. The only two commits that weren't included were b812928 and 2019622, but since I was running in AWS Lambda those wouldn't change anything. I did make the analogous changes in cisagov/lambda_functions#34, and I used that to generate the Lambda functions that were used for the test run.

IanLee1521 commented 5 years ago

Sounds good, then yes, I think once we get the build tests sorted out, we can merge this (not sure if you want to address some of the other code improvements I suggested, or do that elsewhere.

jsf9k commented 5 years ago

@IanLee1521, I like your changes but I'd prefer they be addressed in a couple of separate PRs. There's a lot in this one already. What do you think? Do you want to create a new PRs with some or all of those changes once this one is merged? If not then I can do it.

The failing build test is only because I went ahead and committed the change to use trustymail version 0.7.0, and that won't exist until cisagov/trustymail#110 is approved, merged, tagged, and the CI runs. So that other PR is holding this one up.

IanLee1521 commented 5 years ago

That seems totally reasonable to me. 👍

jsf9k commented 5 years ago

OK, I will continue waiting for someone to review cisagov/trustymail#110 then. Once that happens and that PR is merged I will merge this one.