Add support for client authentication #286

jsf9k · 2018-11-23T20:14:44Z

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

…ient Authentication and whether the certs are publicly trusted.

scanners/pshtt.py

jsf9k · 2018-11-30T18:22:06Z

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

Someone had accidentally committed a merge conflict.

jsf9k

See my comment. The changes in utils/scan_utils.py are breaking some of the tests, so we need to decide if we want to use action=store_true or not. I don't think it's needed for the options where it was added.

utils/scan_utils.py

This is for the dmarc, mx, spf, and starttls CLI options.

These options are the trustymail options --mx, --dmarc, --spf, and --starttls.

jsf9k · 2019-03-18T15:30:27Z

@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 · 2019-03-19T18:01:52Z

@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 · 2019-03-20T02:04:09Z

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.

… websites that require client certs and include DNSSEC data

jsf9k · 2019-03-20T10:47:46Z

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

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.

The differences between 0.6.0 and 0.6.1 are purely cosmetic, but we may as well be current.

IanLee1521

Some feedback from me on coding style / practice, but nothing that I saw as a show stopper.

Is this what you re-ran the BOD 18-01 scans against and confirmed it it OK?

IanLee1521 · 2019-03-20T17:17:38Z

scanners/pshtt.py

@@ -108,26 +110,26 @@ def to_rows(data):
    row = []
    for field in headers:
        value = data[field]
-


If you're removing all of this logic, then this block can collapse to row = [data[field] for field in headers]

IanLee1521 · 2019-03-20T17:18:13Z

scanners/pshtt.py

-        if (field != "HSTS Header") and (field != "HSTS Max Age") and (field != "Redirect To"):
-            if value is None:
-                value = False
-
        row.append(value)

    return [row]


Do we really intend to return a list of the row list?

@konklone did this intentionally. This is what all the scanners (are supposed to) do. If we change it here we need to change it in all scanners, and we also need to change the code that gathers all the CSV rows into a single results file. All doable, but I think that is best done in a separate PR.

IanLee1521 · 2019-03-20T17:21:51Z

scanners/sslyze.py

@@ -405,10 +461,26 @@ def analyze_certs(certs):
    # Signature of the leaf certificate only.
    data['certs']['leaf_signature'] = leaf.signature_hash_algorithm.name

+    if(leaf.signature_hash_algorithm.name == "MD5"):


These two blocks could be simplified to:

sig_alg = leaf.signature_hash_algorithm.name data['certs']['md5_signed_certificate'] = (sig_alg == "MD5") data['certs']['sha1_signed_certificate'] = (sig_alg == "SHA1")

Yes, that is better.

IanLee1521 · 2019-03-20T17:24:22Z

scanners/sslyze.py

    # Beginning and expiration dates of the leaf certificate
    data['certs']['not_before'] = leaf.not_valid_before
    data['certs']['not_after'] = leaf.not_valid_after

+    now = datetime.datetime.now()
+    if (now < leaf.not_valid_before) or (now > leaf.not_valid_after):


This could be simplified to:

data['certs']['expired_certificate'] = not(leaf.not_valid_before < now < leaf.not_valid_after)

IanLee1521 · 2019-03-20T17:24:58Z

scanners/sslyze.py

@@ -497,16 +569,26 @@ def cert_issuer_name(parsed):
    return attrs[0].value


+# Analyze the results of a renegotiation test
+def analyze_reneg(data, reneg):
+    if (reneg.accepts_client_renegotiation is True) and (reneg.supports_secure_renegotiation is False):


This could simply be:

if reneg.accepts_client_renegotiation and not reneg.supports_secure_renegotiation:

And then further, the whole block could be:

data['config']['insecure_renegotiation'] = reneg.accepts_client_renegotiation and not reneg.supports_secure_renegotiation

IanLee1521 · 2019-03-20T17:28:05Z

scanners/sslyze.py

+    def run_scan(scan_type, command, errors):
+        if(errors >= 2):
+            return None, errors
+        logging.debug("\t\t{} scan.".format(scan_type))


FWIW, I think with the logging calls, it might actually be preferred to use formatting similar to:

logging.debug("\t\t%s scan.", scan_type)

Which matches more what is done in the documentation, where the parameters (scan_type in this case) are passed to logging.debug as arguments, rather than doing the substitution first: https://docs.python.org/3/library/logging.html#logging.Logger.debug

I'm not at all opposed to this change, but I think we should make it wholesale, across all scanners, in a separate pull request.

IanLee1521 · 2019-03-20T17:28:28Z

scanners/sslyze.py

+        try:
+            result = scanner.run_scan_command(server_info, command)
+        except Exception as err:
+            logging.warning("{}: Error during {} scan.".format(server_info.hostname, scan_type))


Should this be logging.error(...) ?

IanLee1521 · 2019-03-20T18:19:05Z

scanners/sslyze.py

@@ -589,7 +686,7 @@ def queue(command):
            return None, None, None, None, None, None, None

    # Initialize commands and result containers
-    sslv2, sslv3, tlsv1, tlsv1_1, tlsv1_2, tlsv1_3, certs = None, None, None, None, None, None
+    sslv2, sslv3, tlsv1, tlsv1_1, tlsv1_2, tlsv1_3, certs, reneg = None, None, None, None, None, None, None, None


This can be simplified to:

sslv2 = sslv3 = tlsv1 = tlsv1_1 = tlsv1_2 = tlsv1_3 = certs = reneg = None

IanLee1521 · 2019-03-20T18:20:32Z

scanners/sslyze.py

@@ -602,6 +699,9 @@ def queue(command):
    if options.get("sslyze-certs", True) is True:
        queue(CertificateInfoScanCommand())

+    if options.get("sslyze-reneg", True) is True:


This should just be if options.get("sslyze-reneg", True):

🦅 👁️

jsf9k · 2019-03-20T19:02:57Z

@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 · 2019-03-20T19:24:15Z

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 · 2019-03-20T19:28:34Z

@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 · 2019-03-20T20:02:01Z

That seems totally reasonable to me. 👍

jsf9k · 2019-03-20T21:19:34Z

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.

echudow and others added 5 commits October 10, 2018 22:35

Added ca-file argument for pshtt and added pshtt output fields for Cl…

c35efea

…ient Authentication and whether the certs are publicly trusted.

Fixed bug in ca-file argument.

324054b

Merge remote-tracking branch '18F/master'

0f516cf

Merge remote-tracking branch '18F/master'

99f5eb9

Remove some whitespace that offends flake8

03866d5

konklone reviewed Nov 23, 2018

View reviewed changes

scanners/pshtt.py Show resolved Hide resolved

Merge branch 'master' into master

8eb43b8

konklone mentioned this pull request Nov 30, 2018

Add support for client authentication, some minor refactoring cisagov/pshtt#179

Merged

Reorder some of the values when they are written to CSV

c89e956

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

echudow and others added 16 commits December 18, 2018 16:26

Added specific info to sslyze for weak cipher tasks

05b3083

Merged changes

cf81846

Merged changes

652d6a2

Make flake8 happy

95aa0f5

Merge branch 'master' into master

333d23e

Merge branch 'master' into master

a8b66d1

Refactored to handle some errors

d6e1970

Added an HTTPS Live field

133a5fc

Merge branch 'master' of https://github.com/echudow/domain-scan

80d27f1

Added public trust with intermediate certs arg

aeb980d

Merge remote-tracking branch '18F/master'

10458ca

Make flake8 happy

07be179

Added ca_file option to help validate and construct higher trust chains

050e26b

trustymail updates

860392e

trustymail updates

15bd499

Make a few whitespace changes to make flake8 happy

4240b17

Fix scanners/trustymail.py file

369f91b

Someone had accidentally committed a merge conflict.

jsf9k commented Mar 17, 2019

View reviewed changes

utils/scan_utils.py Show resolved Hide resolved

echudow and others added 5 commits March 17, 2019 19:12

Adding extra trustymail command-line arguments back for compatibility

c676313

Merge branch 'master' of https://github.com/18F/domain-scan

af596de

Put back the action='store_true' statements

cf8a291

This is for the dmarc, mx, spf, and starttls CLI options.

Adjust tests to pass now that some options have action='store_true'

0848d11

These options are the trustymail options --mx, --dmarc, --spf, and --starttls.

Don't convert None to False

e513aa7

jsf9k mentioned this pull request Mar 19, 2019

Rework for new fields cisagov/saver#41

Merged

jsf9k self-assigned this Mar 20, 2019

Update to use the latest pshtt and trustymail, which correctly handle…

b812928

… websites that require client certs and include DNSSEC data

jsf9k mentioned this pull request Mar 20, 2019

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

Merged

Update to use pshtt 0.6.1

2019622

The differences between 0.6.0 and 0.6.1 are purely cosmetic, but we may as well be current.

jsf9k requested review from echudow and removed request for konklone March 20, 2019 16:17

IanLee1521 approved these changes Mar 20, 2019

View reviewed changes

jsf9k merged commit 6eed3ef into 18F:master Mar 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for client authentication #286

Add support for client authentication #286

jsf9k commented Nov 23, 2018 •

edited

Loading

jsf9k commented Nov 30, 2018

jsf9k left a comment •

edited

Loading

jsf9k commented Mar 18, 2019

jsf9k commented Mar 19, 2019 •

edited

Loading

jsf9k commented Mar 20, 2019

jsf9k commented Mar 20, 2019 •

edited

Loading

IanLee1521 left a comment

IanLee1521 Mar 20, 2019

jsf9k Mar 20, 2019

IanLee1521 Mar 20, 2019

jsf9k Mar 20, 2019

IanLee1521 Mar 20, 2019

jsf9k Mar 20, 2019

IanLee1521 Mar 20, 2019

jsf9k Mar 20, 2019

IanLee1521 Mar 20, 2019

jsf9k Mar 20, 2019

IanLee1521 Mar 20, 2019

jsf9k Mar 20, 2019

IanLee1521 Mar 20, 2019

IanLee1521 Mar 20, 2019

jsf9k Mar 20, 2019

IanLee1521 Mar 20, 2019

jsf9k Mar 20, 2019

jsf9k commented Mar 20, 2019

IanLee1521 commented Mar 20, 2019

jsf9k commented Mar 20, 2019 •

edited

Loading

IanLee1521 commented Mar 20, 2019

jsf9k commented Mar 20, 2019

Add support for client authentication #286

Add support for client authentication #286

Conversation

jsf9k commented Nov 23, 2018 • edited Loading

jsf9k commented Nov 30, 2018

jsf9k left a comment • edited Loading

Choose a reason for hiding this comment

jsf9k commented Mar 18, 2019

jsf9k commented Mar 19, 2019 • edited Loading

jsf9k commented Mar 20, 2019

jsf9k commented Mar 20, 2019 • edited Loading

IanLee1521 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsf9k commented Mar 20, 2019

IanLee1521 commented Mar 20, 2019

jsf9k commented Mar 20, 2019 • edited Loading

IanLee1521 commented Mar 20, 2019

jsf9k commented Mar 20, 2019

jsf9k commented Nov 23, 2018 •

edited

Loading

jsf9k left a comment •

edited

Loading

jsf9k commented Mar 19, 2019 •

edited

Loading

jsf9k commented Mar 20, 2019 •

edited

Loading

jsf9k commented Mar 20, 2019 •

edited

Loading