-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
apache: add general http-header enhacement #1395
Conversation
Thanks @sagi! Preliminarily: can you structure this so that the enhancement is called "http-header", and it's up to the client to call it with |
Sure. I'm on it. |
Now we've a general http-header enhancement. There's a new dict in constants.py where for each header_name (i.e. Strict-Transport-Security, Upgrade-Insecure-Requests and maybe in the future HPKP) there is a list of header arguments. |
hi @pde, I think now is a good time to review. |
|
||
|
||
HSTS_ARGS = ["always", "set", "Strict-Transport-Security", | ||
"\"max-age=31536000; includeSubDomains\""] |
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.
Is there any reason for this level of detail to be inside the plugins themselves? In the long run my thought for the "right" way to do HSTS enablement in interactive mode would be to start it off with a very short TTL, and gradually increase it if the admin has left the header in place. But freedom to implement that kind of functionality would require the "details" of the headers to be controlled in the main client, not the plugins.
(I'm being a little picky about this because once we've picked a particular set of semantics for then "http-header" enhancement that essentially becomes an API between the client and plugins).
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.
We discussed it on #letsencrypt-dev
: We'll create an API wrapper on top of augeas for a more versatile http header placements (for instance: adaptive HSTS max-age / HPKP). In the mean time we'll keep the simple http-header placement method that is contained in the PR.
if redirect: | ||
self.redirect_to_ssl(domains) | ||
self.apply_enhancement(domains, "redirect") |
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.
Each of these calls to apply_enhancement
should be wrapped in a try: catch PluginError
block that suppresses the exception that is raised if the enhancement is already in place.
Otherwise, if a domain already redirects from HTTP to HTTPS, and the user runs letsencrypt install --redirect --uir --hsts
, this call to apply_enhancement
will raise PluginError: Let's Encrypt has already enabled redirection
and not continue to apply the other enhancements.
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.
Or perhaps stop raising exceptions in those cases, and just pass a note to the user saying that we found the thing in place and aren't doing it again.
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.
Actually, this is probably the wrong place to do this, because if we're enhancing N domains, we want to try the later ones even if the enhancement has already been applied to the first.
Overall, this is great! Most of my comments should be easy fixes; the question about header matching is tricky; I'd like to hear your thoughts about the least-incorrect way to do it for now. |
with error_handler.ErrorHandler(self._recovery_routine_with_msg, msg): | ||
for dom in domains: | ||
try: | ||
self.installer.enhance(dom, "redirect") | ||
self.installer.enhance(dom, enhancement, options) | ||
except errors.PluginError: |
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.
```
except PluginError, err:
if "already enabled redirection" in repr(err) or re.search("Existing.*header", repr(e)):
logger.info("Enhancement %s for %s already in place", enhancement, dom)
else:
# current error handling code including `raise`
```
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.
Per IRC:
<sagi__> pdeee: I'll change it to ensure-http-header. Regarding the not crashing if enhancements are already in place -
<sagi__> It's not that easy
<pdeee> not least, because of bmw's great error handling code :)
<pdeee> for your new header calls, I think we know that no recovery routine is required
<sagi__> yes, the problem is with the redirect
<pdeee> we need to check in the case of redirect...
<pdeee> so if redirect needs real recovery, what we'd do is:
<pdeee> move the "with error_handler.ErrorHandler" logic inside of the for loop
<pdeee> create a new error handler type, which knows about these non-fatal enhancement errors
<pdeee> and cleans up if required but doesn't exit out when they happen
I think we should rename the enhancement from "http-header" to "ensure-http-header", both to capture its semantics and to leave room for the day when we deprecate it. Other than that and the exception handling code I suggested above, this is ready to merge. |
@pde, I can take a look at this tomorrow morning EST if you'd like. |
Seems to be working well in tests. Merging! |
apache: add general http-header enhacement [needs revision]
Closes #582
Mostly closes #812 (though --uir isn't on or prompted by default) and #1611 (but no HPKP for now)