app-root redirect occurs before https when ssl-redirect is true

rnwolfe commented 3 years ago

NGINX Ingress controller version:

  Release:       v0.34.1
  Build:         v20200715-ingress-nginx-2.11.0-8-gda5fa45e2
  Repository:    https://github.com/kubernetes/ingress-nginx
  nginx version: nginx/1.19.1

Other annotations:

  annotations:
    kubernetes.io/ingress.class: nginx
    cert-manager.io/issuer: 'letsencrypt-prod'
    nginx.ingress.kubernetes.io/rewrite-target: /$1
    nginx.ingress.kubernetes.io/ssl-redirect: 'true'
    nginx.ingress.kubernetes.io/app-root: '/admin/'
    nginx.ingress.kubernetes.io/configuration-snippet: |
      more_set_headers "X-Frame-Options: DENY";
      more_set_headers "X-Content-Type-Options: nosniff";
      more_set_headers "X-XSS-Protection: 1; mode=block";
      more_set_headers "Strict-Transport-Security: max-age=31536000; includeSubDomains";
      more_set_headers "Content-Security-Policy: default-src 'self'; connect-src 'self' https://xxxxx.auth0.com; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline'; img-src 'self' https://xxxx.s3.amazonaws.com https://*.gravatar.com https://*.wp.com data:;";

Kubernetes version (use kubectl version): v1.17.9-eks-4c6976

Environment:

Cloud provider or hardware configuration: AWS EKS

What happened: When accessing http://app1.mydomain.com with app-root and ssl-redirect defined, the ingress redirects to http://app1.mydomain.com/app-root before https://app1.mydomain.com/

Redirect chain: http://app1.ingress.com -> http://app1.ingress.com/app1 -> https://app1.ingress.com/app1

What you expected to happen: I expected ssl-redirect to be completed before app-root. Having an extra HTTP hop in the redirect chain is a security vulnerability.

Expected redirect chain: http://app1.ingress.com -> https://app1.ingress.com/-> https://app1.ingress.com/app-root OR Expected redirect chain: http://app1.ingress.com -> https://app1.ingress.com/app-root

How to reproduce it: Use nginx-ingress with app-root configured and ssl-redirect set to true.

Anything else we need to know: This is a commonly flagged security vulnerability:

Which if left unresolved, would leave nginx-ingress unusable in production environments with high-security requirements.

This was mentioned by @heikowissmann in #5261 which was closed after the initial, related concern was addressed in a PR. So, I'm opening this issue to bring attention to this.

/kind bug

winrono commented 3 years ago

Any update on this? Seems like it leads to loss of HSTS headers

rnwolfe commented 3 years ago

This is quite a nagging issue for me? Any updates/guidance?

jstachowiak commented 3 years ago

I also created a similar issue but for a slightly different scenario in #6597.

fejta-bot commented 3 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale

rnwolfe commented 3 years ago

/remove-lifecycle stale

fejta-bot commented 3 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale

iamNoah1 commented 3 years ago

Hi @rnwolfe @winrono @jstachowiak is that still an issue for you guys and did you tried using newer versions of ingress-nginx?

rnwolfe commented 3 years ago

@iamNoah1 Yes, still an issue.

I just updated to v0.47.0 from v0.34.1 and the redirect chain is still the same.

Here's the chain for the app mentioned in the original post but now running v0.47.0 in AWS.

Request URL	# Redirects	Status Code	URL
http://connect.domain.com	2	302	http://connect.domain.com
		308	http://connect.domain.com/admin/
		200	https://connect.domain.com/admin

(Tested using https://httpstatus.io/)

I haven't tried the new v1.0.0-alpha release, but looking at the diff between the two it doesn't seem like it would have any relevant changes: https://github.com/kubernetes/ingress-nginx/compare/controller-v0.47.0...v1.0.0-alpha.1

iamNoah1 commented 3 years ago

Thanks for the info @rnwolfe

/remove-lifecycle stale

rikatz commented 3 years ago

/assign @tao12345666333

rikatz commented 2 years ago

/assign

rikatz commented 2 years ago

So this PR may be related: https://github.com/kubernetes/ingress-nginx/pull/5266

rikatz commented 2 years ago

So I've found that in past we did the upgrade before the redirect but now not. Here is why:

rewrite_by_lua_block {
                lua_ingress.rewrite({
                    force_ssl_redirect = true,
                    ssl_redirect = true,
                    force_no_ssl_redirect = false,
                    preserve_trailing_slash = false,
                    use_port_in_redirects = false,
                    global_throttle = { namespace = "", limit = 0, window_size = 0, key = { }, ignored_cidrs = { } },
                })
                balancer.rewrite()
                plugins.run()
            }

                       return 301 https://www.example.nl/;

So apparently the return always run before the rewrite, which is used to move to TLS.

We can use a directive here to force the lua rewrite blocks to work before the rest in nginx conf ( rewrite_by_lua_no_postpone on;) but it works partially. Also this should be set as a global flag, which may not be desired.

For example, in case of app-root the following directive is added and always runs first:

  if ($uri = /) {
                        return 302 $scheme://$http_host/admin/;
                }

While in case of permanent-redirect this is added inside the "location /" the directive return 301 https://www.example.nl/; is added inside the location so it's respected.

I got to say this is not as easy to solve, because while the "*_redirects" can be moved to the same rewrite logics of the SSL Rewrite and doing some "early return" for the TLS upgrade in connection, the app-root is a bit harder, as it always assumes: "if user calls location '/' then redirect to /approot". So for this to work, the appRoot must always be in the "server" directive, which leads me to think how the conflict solving of this happens (what happens if two ingresses pointing to the same host but different paths and with this set up will do?)

Anyway, need to discuss with others a bit better how to move forward with this, for example:

If we move the *redirect to inside the ngx.rewrite module, do all the validations there half of the problems are solved
To deal with the appRoot and the fromToWWW we should suggest that the owner of a vhost creates an ingress with path "/" (even if it does not exists) and set the redirect directives there, and maybe deprecate appRoot.

I need some more opinions here @ElvinEfendi @tao12345666333 @strongjz

strongjz commented 2 years ago

I don't know about the Lua bits but from what I am reading,

Http -> Https -> redirect

When a client (the browser), issues a https request to the server (nginx), it initiates an SSL session by way of an SSL handshake. When, and only when, the handshake succeeds and the session is established, the browser sends the actual HTTP request containing the hostname. Since SSL/TLS is not only a way to provide encryption for data connections, but also a way for the server to authenticate itself to the client. As part of the authentication process, the browser validates the identity of the server. One of the validation checks is to match the certificate subject name with the hostname that the browser intends to request content from. If this validation fails, the browser issues a warning to the user.

Not sure if this is helpful.

https://serverfault.com/questions/258378/remove-www-and-redirect-to-https-with-nginx/258424#258424 https://serverfault.com/a/358625/162720

rikatz commented 2 years ago

yeah, I will cycle back to this as soon as I finish the other improvements :)

iamNoah1 commented 2 years ago

/triage accepted /priority important-soon

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot commented 2 years ago

The issue has been marked as an important bug and triaged. Such issues are automatically marked as frozen when hitting the rotten state to avoid missing important bugs.

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle frozen

cilindrox commented 1 year ago

Just got bitten by this. Similar setup: ssl-redirect, http->https upgrade occurs before hitting the redirect config. In our particular scenario, the nginx.ingress.kubernetes.io/permanent-redirect-code: '301' doesn't kick in before the global 308.

hanskhe commented 1 year ago

This error appears to make a domain ineligible for HSTS pre-loading. https://hstspreload.org can be used to check if a domain can be added to the HSTS pre-load list. A site that performs http://example.com -> http://www.example.com -> https://www.example.com will get an error stating it is not eligible for HSTS pre-loading.

The code that checks the domain is also clear on this requirement.

https://github.com/chromium/hstspreload/blob/master/redirects.go#L136-L150

As such, sites that wish to both

Redirect non-www to www and
Use HSTS pre-loading

are not able to use ingress-nginx.

A agree with @rnwolfe that this makes ingress-nginx unsuitable for production environments. HSTS pre-loading should be standard for any production site in 2022, and being able to redirect non-www to www is most likely a requirement for a significant portion of users.

jonasbuehrle commented 1 year ago

We had the same redirect issue as @rnwolfe and found a workaround. The following annotations were used initially:

nginx.ingress.kubernetes.io/app-root: /app-root/
nginx.ingress.kubernetes.io/force-ssl-redirect: "true"
nginx.ingress.kubernetes.io/service-upstream: "true"
nginx.ingress.kubernetes.io/ssl-redirect: "true"

We replaced the app-root annotation with a configuration snippet that uses the same logic but https only. remove app-root annotation: nginx.ingress.kubernetes.io/app-root add configuration-snippet annotation:

nginx.ingress.kubernetes.io/configuration-snippet: |- 
     if ($uri = /) {
         return 302 https://$http_host/app-root/;
    }

The configuration snippet is identical to the app-root behaviour but replaces $scheme with 'https'. The new behaviour is that when you request http://example.com/ you are immediately redirected to https://example.com/app-root/.

If your app-root is 'admin' you want to change the uri in the configuration snippet:

nginx.ingress.kubernetes.io/configuration-snippet: |- 
     if ($uri = /) {
         return 302 https://$http_host/admin/;
    }

mrfoulds commented 1 year ago

I have also encountered this behaviour and can confirm that @jonasbuehrle snippet was a workaround for me. I'd like to know if this is something likely to be fixed in the future. Adding configuration-snippets here and there always feels a little clunky and harder to manage throughout our kubernetes estate.

longwuyuan commented 1 year ago

The project is now in stabilization phase. As per comments above, you can see that the plan is to work on it after the stabilization work is completed.

k8s-triage-robot commented 1 year ago

This issue is labeled with priority/important-soon but has not been updated in over 90 days, and should be re-triaged. Important-soon issues must be staffed and worked on either currently, or very soon, ideally in time for the next release.

You can:

Confirm that this issue is still relevant with /triage accepted (org members only)
Deprioritize it with /priority important-longterm or /priority backlog
Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

miskr-instructure commented 11 months ago

In case it helps someone, you can accomplish HSTS/preload by basically reimplementing it in the Ingress object's annotation:

    nginx.ingress.kubernetes.io/server-snippet: |-
      more_set_headers "Strict-Transport-Security: max-age=31536000; includeSubdomains; preload";
      if ($scheme = "https") {
        return 301 https://some-host/some/path;
      }
      return 301 https://$http_host$request_uri;

Cons:

will silently override/duplicate HSTS specific settings from the nginx configmap and from other ingress annotations, that is problematic but workable for a few Ingress objects
produces a warning on hstspreload.org because it makes nginx send the Strict-Transport-Security in the initial plain http redirect too, but that doesn't forbid registration; I couldn't find a way to conditionally send the header (no, putting it in the if block won't work)

EliasZ commented 11 months ago

Any timeline or path to getting this fixed?

cc @longwuyuan @rikatz

hanskhe commented 5 months ago

Just a heads up that if your domain was previously added to the Chrome HSTS preload list, you should check that status again. We have seen our domains now removed due to this issue, and get the following error:

Status: example.com was previously submitted to the preload list, but has been removed.

kubernetes / ingress-nginx

app-root redirect occurs before https when ssl-redirect is true #6340