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

Using default-tls-certificate & watch-namespace does not work if secret is not in watched namespace #9196

Closed
danielnachtrub opened this issue Oct 20, 2022 · 16 comments · Fixed by #11947
Assignees
Labels
area/docs kind/documentation Categorizes issue or PR as related to documentation. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@danielnachtrub
Copy link

What happened:

Using the combination of watch-namespace and default-tls-certificate flag leads to an error loading the default-tls-certificate if the certificate is not in the namespace that is watched.

7 controller.go:1102] 
Error loading custom default certificate, falling back to generated default: 
local SSL certificate ingress-namespace/default-tls-certificate was not found

My best guess is that the store used to access the configuration is scoped to the namespace and does not allow "escaping" this (which is good during runtime from a security perspective).

What you expected to happen:

Either state in in the documentation that default-tls-certificate needs to be within watch-namespace(s) if configured or maybe better allow the default-tls-certificate to be stored in another namespace (as it might be unintended that the secret is accessible by the watched namespace's operators and apps).

NGINX Ingress controller version: >= 1.1.1
Kubernetes version: >= 1.23

@danielnachtrub danielnachtrub added the kind/bug Categorizes issue or PR as related to a bug. label Oct 20, 2022
@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 Oct 20, 2022
@longwuyuan
Copy link
Contributor

/remove-kind bug
I wonder if the namespce scope of secrets is relevant here
image

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. and removed kind/bug Categorizes issue or PR as related to a bug. labels Oct 20, 2022
@danielnachtrub
Copy link
Author

There should not be any relation. The nginx controller itself has sufficent permissions to access the secret, i've tested this using curl from within the controller with the sa's token.

This behavior has not been there since ever, it came with around 1.1.0 of the controller.

@longwuyuan
Copy link
Contributor

longwuyuan commented Oct 20, 2022 via email

@danielnachtrub
Copy link
Author

You can limit the nginx ingress to only watch for ingress resources in a given namespace. This is very useful if you have running multiple ingress controllers on the same cluster and want to separate them.

A description of the environment is available here: https://blog.nuvotex.de/kubernetes-nginx-ingress-default-ssl-certificate/

@longwuyuan
Copy link
Contributor

  • Default install does not watch a namespace. So flag default-ssl-certificate can use one secret from any namespace
  • A user decides to watch ingresses only in one namespace, for a given controller. Yet the default-ssl-certificate flag, for that specific controller, should use a secret from some other namespace but not the namespace that the user decided to watch, using that specific controller.

Does this describe your expected behavior.

@danielnachtrub
Copy link
Author

Exactly - that's the desired state.

We want a clear separation of secrets and ingress resources as controller & ingresses are managed by dedicated teams and the application developers are not supposed to be able to extract the TLS secret (yet still need to be able to manage secrets in the application namespace).

@longwuyuan
Copy link
Contributor

longwuyuan commented Oct 20, 2022

So you expect secrets continue to be a name-scoped object, but secrets in one namespace scope should be read by ingresses from another namespace. That seems odd.

@danielnachtrub
Copy link
Author

Secrets are always scoped, that's part of the resource definition and should for good reasons always be that way.

The ingress controller (deployment) runs on the same namespace where the secrets are stored - the ClusterRoleBinding created by the helm chart grants the controller permissions to read secrets of all namespaces anyway.

The important distinction is:

  • Ingress resources are defined in the application namespace (which is managed by application developers)
  • The default tls secret is defined in the ingress namespace (which is managed by the cluster maintainers)

Moving the tls secret into the application namespace results in the fact that the application developer can reveal or even alter the default tls certificate (which might be intended for a custom tls certificate but should not be the case for the default one).

@longwuyuan
Copy link
Contributor

And does the service account in play impact anything ?

@danielnachtrub
Copy link
Author

The permissions of the account are fine.

@longwuyuan
Copy link
Contributor

@danielnachtrub the project is in feature freeze phase as announced months ago so only critical bug fixes or CVE patches are the changes being accepted. There is some highest priority work in progress to stabilize the codebase. This is FYI because this feature can not be given priority until Feb 2023 or even maybe later.

I am not certain of the improvement that will occur as in does it really protect the default-ssl-cert. So have to wait for comments from others.

@danielnachtrub
Copy link
Author

There's no need to change the code in the first place - an improvement would be to document the behavior (for example annotate in the default-tls-certificate parameter that if watch-namespace or watch-namespace-selector is used the certificate needs to be located within these namespaces).

Besides that the priority of this is not very high, it's just undocumented behaviour that takes quite some time to discover.

@bumarcell
Copy link

We have the same problem and we're not using watch-namespace or watch-namespace-selector.
The secret in in the ingress NS and I made sure the SA can get it, still not found..

@peter-resnick
Copy link

I'm also experiencing this problem without using the watching namespace.

Below are the logs, the ingress controller clearly states it can't find the certs in ingress-nginx/ds-aks-tls and yet I can immediately show that the secret exists in that location, and I've verified those secrets are the tls.key, tls.crt and ca.crt that I want to be used

W0116 23:24:08.090295       7 controller.go:1226] Error loading custom default certificate, falling back to generated default:
local SSL certificate ingress-nginx/ds-aks-tls was not found
% kubectl get secrets -n ingress-nginx
NAME                      TYPE                DATA   AGE
ds-aks-tls                kubernetes.io/tls   3      4d2h

@longwuyuan
Copy link
Contributor

@peter-resnick the original issue description refers to a outdated unsupported version of the controller with the context being too many changes occured since the original post.

  • The error message you posted is "error loading" but there is no other information that helps a reader dive into the state of the secret or the controller
  • I used minikube and this works for me just fine and I can not reproduce
  • A detailed step by step procedure is needed, that somebody can copy/paste from, to reproduce on minikube or kind cluster
  • The reproduce procedure can not use a self-signed cert
  • If the install of the controller is customized in any way, then the diff or the values file used must be provided here

@longwuyuan
Copy link
Contributor

/assign

The original ask is that the project document the current expected behaviour explicitly. So did that. This issue will be closed on merging the PR visible above.

/area docs
/kind documentation
/triage accepted

@k8s-ci-robot k8s-ci-robot added area/docs kind/documentation Categorizes issue or PR as related to documentation. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs kind/documentation Categorizes issue or PR as related to documentation. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants