Server-only authentication of backends and per-location SSL config

[janosi]

What this PR does / why we need it:

1. It enables the one-way authentication of backends based on the ca.crt configured in the annotation proxy-ssl-secret.
2. It enables the configuration of different proxy-ssl-secrets for the different locations of the same server

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #4688 #4503

Special notes for your reviewer:

[k8s-ci-robot]

Welcome @janosi!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @janosi. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[aledbf]

/ok-to-test

[codecov-io]

Codecov Report

Merging #4689 into master will decrease coverage by 0.02%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #4689      +/-   ##
==========================================
- Coverage   58.62%   58.59%   -0.03%     
==========================================
  Files          88       88              
  Lines        6743     7057     +314     
==========================================
+ Hits         3953     4135     +182     
- Misses       2353     2443      +90     
- Partials      437      479      +42

Impacted Files	Coverage Δ
internal/ingress/controller/store/store.go	58.51% <0%> (-0.13%)	⬇️
internal/ingress/controller/store/backend_ssl.go	42.85% <0%> (-0.45%)	⬇️
internal/ingress/controller/controller.go	53.42% <100%> (+2.98%)	⬆️
...nternal/ingress/annotations/secureupstream/main.go	91.66% <0%> (-8.34%)	⬇️
internal/watch/file_watcher.go	80.76% <0%> (-3.85%)	⬇️
internal/ingress/zz_generated.deepcopy.go	0% <0%> (ø)	⬆️
internal/ingress/types.go	0% <0%> (ø)	⬆️
internal/ingress/metric/collectors/process.go	90.42% <0%> (+2.12%)	⬆️
internal/ingress/types_equals.go	23.73% <0%> (+6.34%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43fa61c...cc84bd4. Read the comment docs.

[aledbf]

/retest

[janosi]

/assign @bowei

[aledbf]

Why are you removing this?
Without it, you can overwrite the server certificate, if the annotation is used in two different Ingress rules for the same hostname.

[janosi]

Exactly that is the purpose here. The intent is to move the proxy-ssl rules to location level instead of the current server level. So users can then define certificates on location (backend/service) level. I.e. the different locations under the same server can have different certificates. This is an ingress usage pattern in our cloud.

[aledbf]

Why are you doing this?
Only CA certificates should be stored on disk. Everything else is handled by Lua, dynamically.

[janosi]

When I used the original version and I defined proxy-ssl-secret in my Ingress the result was a config file in which all of these: proxy_ssl_trusted_cert, proxy_ssl_certificate, proxy_ssl_certificate_key pointed to the same file on disk. It was my experience with the original version. I.e. it wanted to have the tls.crt and tls.key from a file on a disk originally.
If I understand the original code well it does the following: puts the client cert and key into sslCert, then if server CA is present then it first stores the sslCert (i.e. client key and cert) on disk first, then it appends the server CA to the file.
In order to preserve this function I kept the writing the client key and cert to disk in my code. I tested it, it works. Both when there is only ca.crt in the proxy-ssl-secret, and also when there are additional tls.key and tls.crt.

[janosi]

@aledbf I would like to ask, could you please help me in finding that lua script that is supposed to manage the client key and cert? I would like to see the whole picture (even though my experience with the original version showed that the controller configured nginx to take those from the CAFile from disk) Thank you!

[janosi]

@aledbf Is there anything I can do for this PR?

[aledbf]

Is there anything I can do for this PR?

Don't break the current behavior. I understand your goal here but this change will break existing ingresses after an upgrade.

[janosi]

@aledbf
I do not think it breaks any behavior. The proxy-ssl parameters are moved to the location level from server level, but those are still there and applied on every location under the same server. I.e .the functionality is the same.
Also about the storing the secrets on disk: it is not new here, the only thing I did is that I moved the disk writing out from the condition that checks whether the ca.crt is available or not (and I think ca.crt is always available, as that is a must for the minimal server/backend authentication - so the tls.key and tls.crt were always written to disk in reality). So, writing the tls.key and tls.crt to disk is not a new thing that I introduced here.
Please help me in understanding what it breaks from functionality perspective.
One scenario I can imagine: someone has 2 Ingresses, that both points to the same host but different paths. In one of them she defines proxy-ssl-secret, but not in the other one. And as the result all the locations are actually under the same proxy-ssl parameters (because of the common host/server).
With this change she should put the proxy-ssl-secret into both Ingress definitions to get to the same goal.
Though, personally, if I define an Ingress with proxy-ssl-secret, and another Ingress without proxy-ssl-secret I would not expect that the proxy-ssl parameters are applied also on the 2nd one due to the first Ingress . In order to expect such behavior there should be a confirmed hierarchical dependency among Ingresses, i.e. when the parameters of an Ingress are inherited into another Ingress- which is not true for the current Ingress resource in k8s.
On the other hand it fixes the problem that people currently cannot use one-way auth to the backends, and that people currently cannot have different secrets for the different paths.

[janosi]

@aledbf I restored the functionality that puts the proxy_ssl parameters on server level. I.e. the old functionality is restored. Beside that the new logic puts the proxy_ssl parameters to location level, too.
Luckily if both a server and a corresponding location have proxy_ssl parameters the parameters of the location are taken into account - i.e. the missing/incorrect functionality is fixed now, while preserving the old logic.
Also the logic that writes secrets to the disk is restored now.
Please check.

[janosi]

@aledbf Could you please check and comment? Thank you!

[janosi]

@aledbf @ElvinEfendi Could any of you please check this? Thank you!

[aledbf]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, janosi

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [aledbf]

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

[janosi]

@aledbf Thank you!

[EronWright]

For those who find themselves here, this PR does make it possible to use a secret containing only a CA certificate bundle, as referenced by the nginx.ingress.kubernetes.io/proxy-ssl-secret annotation. Thanks again @janosi!

apiVersion: v1
kind: Secret
metadata:
  name: proxy-ssl-secret
data:
  ca.crt: (base64-encoded PEM file)
type: Opaque

The set of annotations that worked for me to use an HTTPS backend:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    "nginx.ingress.kubernetes.io/backend-protocol": "HTTPS"
    "nginx.ingress.kubernetes.io/proxy-ssl-secret": "ingress-nginx/ingress-nginx-proxy-ssl-secret"
    "nginx.ingress.kubernetes.io/proxy-ssl-verify": "on"
    "nginx.ingress.kubernetes.io/proxy-ssl-server-name": "on"
    "nginx.ingress.kubernetes.io/proxy-ssl-name": "backend.example"
    "nginx.ingress.kubernetes.io/upstream-vhost": "backend.example"