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

Add option to forbid plain http requests (where ssl-redirect is unsafe) #11391

Open
awoimbee opened this issue May 29, 2024 · 10 comments · May be fixed by #12384
Open

Add option to forbid plain http requests (where ssl-redirect is unsafe) #11391

awoimbee opened this issue May 29, 2024 · 10 comments · May be fixed by #12384
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@awoimbee
Copy link
Contributor

I host an API, a webapp (and much more).
I want HTTP requests to the webapp to be redirected to HTTPS -> I use ssl-redirect.
I want HTTP requests to the API to return a 4XX "http_unsupported: This endpoint is only accessible over HTTPS."

Reason:
If an API consumer misconfigures his client to use plain HTTP, he won't know about it but all his secret tokens will be sent plaintext.

See hackernews API Shouldn't Redirect HTTP to HTTPS.

Most APIs (that don't redirect to HTTPS) return a 403, npm returns a 426 with no Upgrade header.

@awoimbee awoimbee added the kind/feature Categorizes issue or PR as related to a new feature. label May 29, 2024
@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels May 29, 2024
@longwuyuan
Copy link
Contributor

Interesting, this seems like a absolute requirement for related users.

/triage accepted

What have you tried so far ? From the current list of features I mean.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 29, 2024
@awoimbee
Copy link
Contributor Author

awoimbee commented May 29, 2024

What have you tried so far ?

Oh yeah sorry.

This works:

nginx.ingress.kubernetes.io/configuration-snippet: |
    if ($scheme = http) {
        return 403 "This endpoint is only accessible via https. You are currently using plaintext http.";
    }

It results in:

$ curl -i http://myapi.example.com                                                                           
HTTP/1.1 403 Forbidden
Date: Wed, 29 May 2024 12:57:22 GMT
Content-Type: text/html
Content-Length: 94
Connection: keep-alive

This endpoint is only accessible via https. You are currently using plaintext http.
$ curl -i https://myapi.example.com | head -n 1
HTTP/2 200

Note that this works even if ssl-redirect and force-ssl-redirect are true in the configmap.

@longwuyuan
Copy link
Contributor

Thanks, that if is the only way to do this as the ingress API does not offer any spec to deal with this use-case. And that if will go sit in the related server block.

So a feature would be to instrument that if from a configMap key or a annotation. That looks like a bunch of code. And it will still be this if funciton, in essence.

Lets wait for other comments.

Copy link

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.

@github-actions github-actions bot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Jun 29, 2024
@awoimbee
Copy link
Contributor Author

Update: sometimes nginx is running behind another proxy (with use-forwarded-headers: true), in which case this works:

nginx.ingress.kubernetes.io/configuration-snippet: |
            set $forwarded_scheme $http_x_forwarded_proto;
            if ($forwarded_scheme = "") {
                set $forwarded_scheme $scheme;
            }
            if ($forwarded_scheme = "http") {
              return 403 "This endpoint is only accessible via HTTPS. You are currently using plaintext HTTP.";
            }

@gavinkflam
Copy link

Hi @awoimbee, @strongjz, @longwuyuan,

I’ve just submitted a PR #12384 introducing two new annotations: ssl-forbid-http and force-ssl-forbid-http.

These annotations are implemented using Lua and are designed to function similarly to ssl-redirect and force-ssl-redirect. Specifically:

  • ssl-forbid-http: Enforces the restriction only when the ingress has TLS.
  • force-ssl-forbid-http: Enforces the restriction regardless of whether TLS is present.

Looking forward to your feedback!

@longwuyuan
Copy link
Contributor

Revisiting this after months caused a fresh perspective, in light of the status of this controller.

  • Root cause of problem described seems a user input of HTTP with sensitive info like TOKENS
  • It will be a useful feature to have to protect against above mentioned problem
  • But this project has to focus on securing the controller out of the box as well as implement the Gateway-API
  • There is lack of resources to do much anything else
  • The resource shortage has even caused the proejct to deprecate existing working popular stable features

So in the light of above factors, implementing new annotations that are not implied by the upstream K8S KEP for the Ingress-API but rather implied by a user error seems not feasible. Not because of lack of interest but because of lack of resources to support/maintain and enhance such features. This originates from the direction that the project needs to do rock solid implementation of the K8S Ingress-API and the implied functions.

So thanks for the contribution, but there are practical problems like validating the annotation and making such annotations rich enough to handle all kinds of user initiated situations. Design wise, a application development team and a user ought to NOT send tokens over HTTP/PlainText, to begin with.

Wait for other comments but I would not recommend the implementation of proposed new annotations. I know its not what I initially indicated but syncing with reality is high priority.

@gavinkflam
Copy link

gavinkflam commented Nov 19, 2024

API-only endpoints should disable HTTP altogether and only support encrypted connections. When that is not possible, API endpoints should fail requests made over unencrypted HTTP connections instead of redirecting them.

According to OWASP, the best practice for HTTPS API services is disable HTTP or fail HTTP requests. Redirection is a common but insecure practice. Misconfigured clients would inadvertently expose sensitive information such as API keys without ever knowing about it.

In my opinion, this is an important security feature. Without this feature, the only way to implement such best practice is snippets. However, snippets are almost always disabled for good reasons - prevent arbitrary code injection.

The implementation of this feature is simple. It is just a small twist of the existing SSL redirect annotations. The maintenance overhead is minimal and totally justify the benefits.

Resource shortage - do we have a backlog somewhere? I'm happy to contribute and help.

@longwuyuan
Copy link
Contributor

@gavinkflam thanks for the contribution efforts and also thanks for the detailed pot.

I think that if you attend the community meeting and explain this, it may get comments from the maintainers.

@gavinkflam
Copy link

@longwuyuan Thank you. I’ll try to join. Is this the weekly meeting on Thursdays 4 PM UTC? Also, does it typically last the full hour?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

Successfully merging a pull request may close this issue.

4 participants