Skip to content
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

hsts-preload does not work with www-redirect #11591

Open
SvenKirschbaum opened this issue Jul 9, 2024 · 5 comments
Open

hsts-preload does not work with www-redirect #11591

SvenKirschbaum opened this issue Jul 9, 2024 · 5 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@SvenKirschbaum
Copy link

SvenKirschbaum commented Jul 9, 2024

What happened:

Setting hsts-preload: true in the ingress-nginx controller configmap and creating an ingress with the nginx.ingress.kubernetes.io/from-to-www-redirect annotation for the www subdomain of a domain does no result in a hsts preload eligible configuration for the used domain.

The resulting redirects are:

http://domain.tld -> http://www.domain.tld -> https://www.domain.tld (with HSTS header)

and

https://domain.tld (without HSTS header) -> https://www.domain.tld (with HSTS header)

What you expected to happen:

I would expect this configuration to result in the domain being eligible for submission to chromes hsts preload lists. This would require the following redirects:

http://domain.tld -> https://domain.tld (with HSTS header)-> https://www.domain.tld (with HSTS header)

NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.):

NGINX Ingress controller
Release: v1.10.1
Build: 4fb5aac
Repository: https://github.com/kubernetes/ingress-nginx
nginx version: nginx/1.25.3

Kubernetes version (use kubectl version): 1.29.6

Environment:

  • Cloud provider or hardware configuration: k3s single node cluster

  • OS (e.g. from /etc/os-release): Debian GNU/Linux 12 (bookworm)

  • Kernel (e.g. uname -a): Linux 6.1.0-21-amd64 Basic structure  #1 SMP PREEMPT_DYNAMIC Debian 6.1.90-1 (2024-05-03) x86_64 GNU/Linux

  • Install tools: k3s

  • Basic cluster related info:

    • Client Version: v1.29.6+k3s2
      Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
      Server Version: v1.29.6+k3s2
  • How was the ingress-nginx-controller installed:

    • Via helm
    • ingress-nginx ingress-nginx 15 2024-04-26 18:50:45.211182831 +0000 UTC deployed ingress-nginx-4.10.1 1.10.1
    • Values controller: kind: DaemonSet ingressClassResource: default: true service: ipFamilyPolicy: RequireDualStack ipFamilies: - IPv4 - IPv6 externalTrafficPolicy: Local allocateLoadBalancerNodePorts: false metrics: enabled: true serviceMonitor: enabled: true scrapeInterval: 15s config: ssl-reject-handshake: true enable-ocsp: true hsts-preload: true ssl-ciphers: "ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:DHE-RSA-AES256-GCM-SHA384" ssl-ecdh-curve: "secp521r1:secp384r1" ssl-session-timeout: 1d http-snippet: | ssl_conf_command Options PrioritizeChaCha; ssl_conf_command Ciphersuites TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256; otlp-collector-host: grafana-k8s-monitoring-grafana-agent.monitoring.svc.cluster.local enable-opentelemetry: true opentelemetry-trust-incoming-span: false otel-sampler: AlwaysOn otel-sampler-ratio: "1.0" opentelemetry: enabled: true addHeaders: X-Frame-Options: SAMEORIGIN X-Content-Type-Options: nosniff Referrer-Policy: "strict-origin-when-cross-origin" Server-Timing: "traceparent;desc=\"$opentelemetry_context_traceparent\""

How to reproduce this issue:

  • Create a new kind cluster with the ports 80 and 443 exposed:
    curl https://gist.githubusercontent.com/SvenKirschbaum/91eb6f19c5b320b4265794f33d934232/raw/ceacfbc4a57ca8b23ac1858171f02b9794b71522/kind.yaml | kind create cluster --config=-
  • Install ingress-nginx
    kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/controller-v1.11.0/deploy/static/provider/kind/deploy.yaml
  • Enable the hsts-preload option:
    kubectl patch configmap/ingress-nginx-controller -n ingress-nginx --type merge -p '{"data":{"hsts-preload": "true"}}'
  • Create an ingress for a www subdomain and add the annotation nginx.ingress.kubernetes.io/from-to-www-redirect: "true":
    kubectl apply -f https://gist.githubusercontent.com/SvenKirschbaum/364de5c2b3c537b91ac124ec1d9ffc33/raw/2cd2c2c885e614be1a82984f27c9f4c236a88ce4/ingress.yaml
  • Access the domain and observe the presence of the hsts header and the redirects:
$ curl -vk http://localhost 2>&1 |grep "Location\|strict"
< Location: http://www.localhost
$ curl -vk http://www.localhost 2>&1 |grep "Location\|strict"
< Location: https://www.localhost
$ curl -vk https://localhost 2>&1 |grep "Location\|strict"
< Location: https://www.localhost
$ curl -vk https://www.localhost 2>&1 |grep "Location\|strict"
< strict-transport-security: max-age=31536000; includeSubDomains; preload
@SvenKirschbaum SvenKirschbaum added the kind/bug Categorizes issue or PR as related to a bug. label Jul 9, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jul 9, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@longwuyuan
Copy link
Contributor

@SvenKirschbaum what you posted is like a awareness but not really a report that can be analyzed.

The template of a new bug report asks questions and it hints at providing info like the kubectl describe output for the controller pod,svc,configmap and the app pod,svc,ingress. There is also questions for the exact curl command used as is and the logs of the controller pod. Please edit the issue description here and provide all that info to help readers.

I am not sure if the docs sets an expectation that redirection will sustain HSTS but maybe you can write a step by step guide that someone else can use, on a kind or minikube cluster, to copy/paste from your guide and reproduce the problem (maybe using self-signed certs if required or otherwise).

/remove-kind bug
/kind support

Once the problem can be reproduced by others, then you can rea-apply the bug label here.

@k8s-ci-robot k8s-ci-robot added kind/support Categorizes issue or PR as a support question. and removed kind/bug Categorizes issue or PR as related to a bug. labels Jul 10, 2024
@SvenKirschbaum
Copy link
Author

Thanks for your response @longwuyuan .

I didn't provide more information about my environment as i deemed it unnecessary, as the issue described is more of a conceptual nature, afaik it will arise in every ingress-nginx deployment when configuring a ingress in this way. However, I have just updated the reproduction steps in the issue so they are easier to follow.

I think it js debatable if this behavior should be categorized as a bug or rather as a feature request. I personally would have expected the configuration to result in the hsts header being applied to the pre-redirect domain too, and subsequently the domain being eligible for preloading, however it is also correct that this is not specified in the docs. I would however still vouch for changing the current behavior, as this would result in an overall more secure configuration.

I have reapplied the bug label, but please feel free to mark this as a feature request if you deem that more appropriate.

/remove-kind support
/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed kind/support Categorizes issue or PR as a support question. labels Jul 14, 2024
@chengjoey
Copy link
Contributor

@SvenKirschbaum , I reproduced this problem in kind using your example. I checked nginx.conf and found that redirect(from/to www) did not call the header method of ingress_lua. I think this is the reason why the strict-transport-security header is missing. After adding header_filter_by_lua_block to it, it works normally.

function _M.header()
if config.hsts and ngx.var.scheme == "https" and certificate_configured_for_current_request then
local value = "max-age=" .. config.hsts_max_age
if config.hsts_include_subdomains then
value = value .. "; includeSubDomains"
end
if config.hsts_preload then
value = value .. "; preload"
end
ngx.header["Strict-Transport-Security"] = value
end
end

curl -vk https://localhost 2>&1 |grep "Location\|Strict"
< Location: https://www.localhost
< Strict-Transport-Security: max-age=31536000; includeSubDomains; preload

@longwuyuan
Copy link
Contributor

I think this would be a nice to have.

But the idea of repeatedly changing the controller code to make all features work for every use-case has resulted in severe problems ranging from security to maintenance.

We already decided in the project that we will move towards maintaining the core ingress-api functionalities first. We also decided that we will deprecate and remove features that are not directly implementing the K8S ingress api functionalities.

In the above context, it will be nice to have a link posted here, that shows where in the K8S KEP for ingress-api,, there is a acceptable description of this from-to-www feature. That section will explain how to deal with HSTS inegration with the from-to-www feature. Can you post that link here @SvenKirschbaum @chengjoey

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
Development

Successfully merging a pull request may close this issue.

4 participants