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

Default HSTS config almost guaranteed to break email links with transactional email - among other things. #549

Closed
MichaelJCole opened this Issue Apr 3, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@MichaelJCole

MichaelJCole commented Apr 3, 2017

HSTS caused me a problem today. I disabled it in my app about a month ago. The nginx ingress controller kept serving the header though.

Restarting the nginx-controller stopped serving the header, but some damage was done - HSTS is a horrible idea, especially if you are using a CNAME to 3rd party services for transactional/marketing email and want to do click tracking.

Not sure if this is an issue with nginx or the controller.

@pieterlange

This comment has been minimized.

Contributor

pieterlange commented Apr 5, 2017

HSTS is configurable: https://github.com/kubernetes/ingress/blob/master/controllers/nginx/configuration.md#allowed-parameters-in-configuration-configmap

If you serve your Ingress over https, it is enabled by default.

@MichaelJCole MichaelJCole changed the title from HSTS header cached long after backend stopped serving it to Default HSTS config almost guaranteed to break email links with transactional email. Apr 5, 2017

@MichaelJCole

This comment has been minimized.

MichaelJCole commented Apr 5, 2017

Hi Pieter, well snap! That's where it's hiding.

Hey, HSTS is DANGEROUS and having it on by default I think is a HUGE MISTAKE

Here's why I think that, and an example:

Once HSTS is deployed you can't take it back without configuring the individual browsers. It's dangerous because if you don't understand it and deploy it, you may be suddenly re-architecting around it. I have services that specifically need to be http, and those are broken for 6 months now for anyone who's ever visited my site.

This problem also showed up for me as a broken signup process - where users couldn't verify their email addresses.

For email deliverability or other 3rd party services, they often want a CNAME. E.g. email.myservice.com -CNAME-> clicktracker.theirservice.com

The click tracking server replaces these links with https://email.myservice.com - which doesn't have a cert - so the links lead to a scary browser warning.

The default configuration here silently breaks that for 6 months without any work-around. I can confirm this for SparkPost, MailChimp, CampaignMonitor, and SendInBlue. Probably most email providers use CNAMEs for click tracking.

I disabled it in from my app a month ago, only to find out it's reenabled now.

EVERYONE I'VE MARKETED MY SERVICE TOO MAY HAVE PROBLEMS USING IT FOR 6 MONTHS BECAUSE OF THIS DEFAULT SETTING. And I'm bummed about that.

Can I suggest either:

  1. Not enabling it by default. The no-undo is extremely risky behavior.
  2. Lowering the max-age to days not months.
  3. At a bare minimum, not including subdomains. This is how the CNAME works on these services.

This config loaded the gun and shot the foot. I didn't figure it out it was my foot till a month later.

It get that moving to https is important, but HSTS isn't an easy button.

@MichaelJCole MichaelJCole changed the title from Default HSTS config almost guaranteed to break email links with transactional email. to Default HSTS config almost guaranteed to break email links with transactional email - among other things. Apr 5, 2017

@aledbf

This comment has been minimized.

Member

aledbf commented Apr 5, 2017

@MichaelJCole right now you can customize this hsts parameters in the configmap:

  • hsts-max-age
  • hsts-include-subdomains

I will add a new parameter to allow disabling the preload

If you need to disable this you can start setting hsts-max-age: 0 in order to avoid/update the time in the browsers and after a while you can remove the hsts feature setting hsts: "false.

About your comments: is almost impossible to build a one size fits all solution (not an excuse, sadly a reality) so the default we choose are based on common use cases that provides the "best" ootb UX. In particular the current hsts values were taken in consideration from this sites:

@MichaelJCole

This comment has been minimized.

MichaelJCole commented Apr 5, 2017

It is possible to build a one-size-doesn't-cause-hidden-problems-you-can't-walk-back solution, and that would be to not enable HSTS by default.

Thanks for the suggestions, setting to 0 can't fix it for the group, just for one user. Everyone else will be stuck in pre-fail or post-fail. I'll figure something out.

@aledbf

This comment has been minimized.

Member

aledbf commented Apr 5, 2017

It is possible to build a one-size-doesn't-cause-hidden-problems-you-can't-walk-back solution, and that would be to not enable HSTS by default.

Sure. You just need to use an ingress controller with a configmap that by default disable the hsts feature.

The default configuration is published here:
https://github.com/kubernetes/ingress/blob/master/controllers/nginx/configuration.md#default-configuration-options

@MichaelJCole

This comment has been minimized.

MichaelJCole commented Apr 5, 2017

@aledbf, I would have to travel back in time to do that now. Which is why it's not a good default choice.

Anyways, thanks for your project and attention to this issue. Have a great week!

@aledbf

This comment has been minimized.

Member

aledbf commented Apr 13, 2017

Closing. Pease reopen if you have another question.

@aledbf aledbf closed this Apr 13, 2017

@MichaelJCole

This comment has been minimized.

MichaelJCole commented Apr 17, 2017

If you find this, I was wrong about the behavior of setting max-age=0.

https://tools.ietf.org/html/rfc6797 says max-age=0 turns HSTS off, which is the best solution.

Here's my configmap:

apiVersion: v1
data:
  hsts: "true"
  hsts-include-subdomains: "true"
  hsts-max-age: "0"
  hsts-preload: "false"
kind: ConfigMap
metadata:
  name: nginx-load-balancer-conf
---

Note that the preload is dangerous because it implies consent for browser makers to permanently enable HSTS on your site - regardless of headers. https://www.owasp.org/index.php/HTTP_Strict_Transport_Security_Cheat_Sheet

@marians marians referenced this issue Jul 25, 2018

Merged

Disable HSTS #23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment