-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Changed is_hsts() function to check both https endpoints. #204
base: develop
Are you sure you want to change the base?
Conversation
If an https endpoint is live then its HSTS status is used to determine the domain's HSTS status.
@@ -1199,16 +1199,20 @@ def is_missing_intermediate_cert(domain): | |||
|
|||
def is_hsts(domain): | |||
""" | |||
Domain has HSTS if its canonical HTTPS endpoint has HSTS. | |||
Domain has HSTS if both https and httpswww endpoints have HSTS when live. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The canonical endpoint was (I think) introduced by @h-m-f-t in an effort to make the HTTPS report results more digestible by the less technical. I'd have to know his thoughts on this change before approving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @climber-girl 's point was that if both endpoints are live then both should have HSTS. I think this section of RFC6797 touches on what I mean although it is in the context of a web application and the includeSubDomains directive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't completely understand all of the reasons for only focusing on the canonical endpoint. It seems like we assume that that is where most users will end up, but plenty of others may use the other endpoint (if it is live), and attackers could take advantage of the other endpoint (via spear-phishing URLs and the like) if it is not sufficiently protected.
strictly_forces_https
already looks at both http endpoints, not just the canonical one, but downgrades_https
currently only looks at the canonical https endpoint. If we switch this HSTS check to check both endpoints, then we should probably change some other checks to look at both endpoints (such as downgrades_https
and maybe even follow-on sslyze
checks). Should most of these checks for both endpoints be in new "Strictly_Enforces" checks or should these be changed in the existing checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just remember that this has the potential to negatively effect the results for a number of domains, so be prepared for questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution works for me- the docstring is clear and the code is clean. 👍
This code will fail if any of the hsts values are None (which is possible if there is a problem getting the HSTS header such as when Client Authentication is required)
The hsts values for any endpoint can be None, so the code needs to gracefully handle when it is None by viewing it as Unknown and only using the other actual values instead.
I committed changes to address when the hsts field is None. There might be a more concise way to do this using Python logic, but I was very confused at first seeing that in Python when there are None's the order of an |
@echudow in |
@mcdonnnj You're right. Sorry, I was testing this code change in my fork which is a little different and leaves |
@echudow Ah, that makes sense. I agree that there's no harm in leaving the change. I just wanted to make sure it was needed and I see why it's useful for compatibility with your fork. |
Work-in-progress: DO NOT MERGE!!
This PR is to fix #194 by checking both https and httpswww endpoints to determine HSTS status for the domain.