Allow proxy-ssl-* annotations without proxy-ssl-secret

[tamalsaha]

What this PR does / why we need it:

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• CVE Report (Scanner found CVE and adding report)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

Which issue/s this PR fixes

fixes #6728

How Has This Been Tested?

I have manually tested it using ghcr.io/voyagermesh/ingress-nginx/controller:proxy-ssl-server-name image.

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added unit and/or e2e tests to cover my changes.
• All new and existing tests passed.

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[netlify]

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	555c5eb
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/65af9849fc9fb50007c5e9bf

[k8s-ci-robot]

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/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tamalsaha
Once this PR has been reviewed and has the lgtm label, please assign strongjz for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tamalsaha]

Hello maintainers, if you can take a quick and give some feedback if the pr makes changes in the right direction, I can try getting necessary testing added. So far, if fixes the issue I was facing.

[github-actions]

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.

[k8s-triage-robot]

The lifecycle/frozen label can not be applied to PRs.

This bot removes lifecycle/frozen from PRs because:

• Commenting /lifecycle frozen on a PR has not worked since March 2021
• PRs that remain open for >150 days are unlikely to be easily rebased

You can:

• Rebase this PR and attempt to get it merged
• Close this PR with /close

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

/remove-lifecycle frozen

[strongjz]

why wrap it in else if? line 194 is enough for the bug fix?

[tamalsaha]

I wanted to make sure that the annotation was present. My understanding is that err == ing_errors.ErrMissingAnnotations will mean annotation is missing. So, I check for err == nil to confirm that annotation is present and there is no parsing error.

[strongjz]

@tamalsaha can you move this out of draft and rebase to get 1.29 tests?

[tamalsaha]

@tamalsaha can you move this out of draft and rebase to get 1.29 tests?

Done.

[Pluies]

Hi @strongjz , could you review this rebased version? It would be awesome to get this out and finally fix three-and-a-half year old issue #6728 :)

(Pinging as I ran into the problem today, followed the issue to this PR, and with a review & merge it looks like we could get this over the line relatively straightforwardly 🙏 )

[longwuyuan]

@tamalsaha @Pluies Another PR #11490 wants to make the proxy-ssl-secret optional, by stating that they have a backend that is capable only of TLSv1.3 and not TLSv1.2 or others and the controller does not try TLSv1.3 but terminates the connection after a failed attempt to use TLSv1.2 to their backend (that is only capable of TLSv1.3). So this other user wants to use proxy-ssl-protocols without a secret.

You have not posted any description here but in the issue you linked, its is stated that you want to use proxy-ssl-name and proxy-ssl-server-name without a secret.

My curiosity is very fundamental so hope you can help clarify this for other readers too. See question below ;

• You are using backend-protocol as HTTPS
• The proxy-ssl settings are documented with distinct references to auth using certs
• The proxy-ssl-secret not only asks for certs but also for the CA hash to know the cert presented by the backend
• In this scenario, does your PR imply that TLS connection with backend will be allowed even without checking the CA that signed the cert, that is being presented by the backend ? Or am I completely misunderstanding this

[Pluies]

Hi @longwuyuan 👋

The reason we want to use these annotations is for reverse-proxying to an HTTPS backend that expects SNI as part of the TLS handshake.

We used to have an ingress defined as:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: amplitude-api-proxy
  namespace: core
  annotations:
    cert-manager.io/cluster-issuer: letsencrypt-prod
    nginx.ingress.kubernetes.io/upstream-vhost: api.eu.amplitude.com
    nginx.ingress.kubernetes.io/backend-protocol: "HTTPS"
...

This ingress sends traffic to a service type=ExternalName.

This works working correctly until nginx ingress controller 4.9.x.

Unfortunately, upon upgrading to nginx ingress controller release 4.10.0 which included nginx v1.25, we started getting HTTP 421 errors from upstream, complaining about SNI:

Requested host does not match any Subject Alternative Names (SANs) on TLS certificate [0a5d8dea05c4c0b935bf6256137b80987949927b903445a4415ddc58faa7f4d2] in use with this connection.

Visit https://docs.fastly.com/en/guides/common-400-errors#error-421-misdirected-request for more information.

Looking around, we found a blogpost that mentions this issue and suggests using proxy_ssl_server_name as a fix.

Indeed, once we updated the ingress to:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: amplitude-api-proxy
  namespace: core
  annotations:
    cert-manager.io/cluster-issuer: letsencrypt-prod
    nginx.ingress.kubernetes.io/upstream-vhost: api.eu.amplitude.com
    nginx.ingress.kubernetes.io/backend-protocol: "HTTPS"
    nginx.ingress.kubernetes.io/configuration-snippet: |
      proxy_ssl_server_name on;
      proxy_ssl_name api.eu.amplitude.com;

Then it started working again 👍

Using snippets is not best practice, so we'd rather be able to set proxy_ssl_server_name and proxy_ssl_name via the bespoke nginx ingress controller annotations if possible.

[longwuyuan]

@Pluies thank you for explaining. helps increase clarity.

Due to lack of resources, some aspects are not getting absolutely clear here. Hence my concerns.

You may have already observed (hence created this issue) or I can confirm to you as I have tested this fact. The proxy_ssl_* directives are non existing in the nginx.conf inside the controller pod, when the "proxy-ssl-secret" annotations is not configured. I am inclined to think that its makes simple & most-reasonable sense that there is no question of a client-auth use-case if there is no cert provided via the proxy-ssl-secret annotation. So a expert developer has to confirm that the proxy-ssl-* annotations are/were even written for NON-CLIENT-AUTH or no mTLS use-cases. If it was that simple, then the proxy-ssl-* directives would not be silently dropped in the absense of a cert being provided by the user.

Even another user has the same opinion as you and has proposed #11490 to make the annotation "proxy-ssl-secret" optional.

I am not a developer so I could be completely wrong here. But all my reasoning is concluding that while we need to acknowledge the problem of not being able to set the attributes for HTTPS, from controller to a backend, the attempt to brute-force break the current set of "proxy-ssl-*" annotations clearly designed and working for all the mTLS users securely, should NOT be tampered with.

If an expert comes in and explains the entire current sensibility and also the proposed change in the code for the existing "proxy-ssl-*" annotations, in the context of both uses cases like the client-cert-auth/mTLS use-case and also the NON-client-cert-auth/NON-mTLS use case, it would be a helpful triage.

[longwuyuan]

Otherwise we may introducing, a new hitherto non-existing vulnerability, for the mTLS use-case, of the "proxy_ssl_*" directives of nginx.conf https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_ssl_certificate

[Pluies]

Yeah I assume all of these annotations were added with the specific idea of mTLS in mind, but they're obviously useful for other use-cases such as SNI etc, as confirmed by the multiple issues & PRs to make proxy-ssl-secret not mandatory.

I think it would make sense to remove this limitation. In the current situation, we can set these annotations but they're silently ignored by the nginx-ingress-controller unless proxy-ssl-secret is set, which is a terrible developer experience. And while the workaround with snippets works, snippets are disabled by default, so it doesn't feel like a great solution either.

[longwuyuan]

@Pluies thanks

• I agree on the fact that its a requirement to be able to set attributes for HTTPS connection, from controller to backend
• I also agree that the proposed change will solve the problem you are facing
• I don't know if mTLS users will lose security, if the proxy-ssl-secret is changed to become optional
• The ability to set HTTPS attributes (without client cert) is missing but mTLS configs are relatively secure, sane & logical due to a check for client-cert. mTLS is Not-Broken. And no expert of the controller who knows the code well is saying that its ok to break mTLS when its not broken at all

Eventually a expert will comment I think. Lets wait