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

Fix Cannot fetch certificate: no endpoints available for service "http:sealed-secrets-controller:" #648

Merged
merged 3 commits into from
Jan 13, 2022

Conversation

@sathieu
Copy link
Contributor Author

sathieu commented Oct 26, 2021

And include a fix for #502.

@sathieu
Copy link
Contributor Author

sathieu commented Dec 15, 2021

@juan131 Please review 🙏.

@juan131
Copy link
Collaborator

juan131 commented Dec 16, 2021

hanks so much for your contribution @sathieu!!

We're performing a major refactorization of the chart at #687
Let's wait until this is done before continue with your changes

@juan131 juan131 self-assigned this Dec 16, 2021
@juan131 juan131 moved this from Inbox to Backlog in Sealed Secrets Dec 16, 2021
@juan131
Copy link
Collaborator

juan131 commented Dec 16, 2021

@sathieu the new major version was just released!

$ helm repo update sealed-secrets
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "sealed-secrets" chart repository
Update Complete. ⎈Happy Helming!⎈
$  helm search repo sealed-secrets/sealed-secrets
NAME                         	CHART VERSION	APP VERSION	DESCRIPTION
sealed-secrets/sealed-secrets	2.0.0        	v0.17.1    	Helm chart for the sealed-secrets controller.

Please feel to rebase this branch in your fork with the latest changes in the main branch, and update this PR.

Copy link
Collaborator

@juan131 juan131 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the PR @sathieu !! Please check my comments

cmd/kubeseal/main.go Outdated Show resolved Hide resolved
@sathieu sathieu changed the title Use Istio port name naming convention Fix Cannot fetch certificate: no endpoints available for service "http:sealed-secrets-controller:" Jan 4, 2022
@sathieu sathieu requested a review from juan131 January 4, 2022 09:44
@sathieu
Copy link
Contributor Author

sathieu commented Jan 4, 2022

@juan131 Comments addressed. Please review again.

@andoshin11
Copy link

This is indeed critical one. Appreciate the fix.

@sathieu
Copy link
Contributor Author

sathieu commented Jan 11, 2022

@juan131 Anything missing in this PR?

Copy link
Collaborator

@juan131 juan131 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I can confirm this fix works

@sathieu
Copy link
Contributor Author

sathieu commented Jan 13, 2022

@juan131 Maybe this fix deserve a major version bump?

Also the CI is failing, is this related to this change?

Copy link
Collaborator

@juan131 juan131 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sathieu I just realized that this fixes the chart.. but it breaks the compatibility with the controller.yaml manifests that we publish at:

These manifests don't have a "http" name in the service port.

maybe this fix deserve a major version bump?

Why do you think so? I don't see that they break backwards compatibility.

@juan131
Copy link
Collaborator

juan131 commented Jan 13, 2022

@sathieu I updated your branch with a suggestion that dynamically obtains the svc port name. Please let me know what you think

Copy link
Collaborator

@alvneiayu alvneiayu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, LGTM

@sathieu
Copy link
Contributor Author

sathieu commented Jan 13, 2022

@juan131 Thanks. This is great! lgtm!

@juan131 juan131 merged commit 2b67e52 into bitnami-labs:main Jan 13, 2022
Sealed Secrets automation moved this from In progress to Completed Jan 13, 2022
@juan131 juan131 mentioned this pull request Jan 13, 2022
@linkvt
Copy link

linkvt commented Jan 19, 2022

Hello @juan131 , I'm addressing this here because I think your change broke the sealed-secrets-controller for k8s users with no permissions in kube-system - no blame intended. Edit: Confirmed to be broken with 0.17.2 but works with 0.17.1 kubeseal CLI.

As you can see in this line the proxier role only grants permissions to get services/proxy and not services

resources:
- services/proxy
verbs:

I guess this is because Services(namespace).ProxyGet was replaced with Services(namespace).Get. Was there a reason for this? Otherwise I could create a PR to add - services to the Roles resources field to make it work again.

Thanks for checking.

@juan131
Copy link
Collaborator

juan131 commented Jan 20, 2022

Hi @linkvt

You're totally right!! That should be pretty straight forward to fix since it just requires to update the RBAC permissions. I'll send a PR to fix it.

Was there a reason for this?

We still use a proxy to access the service (I didn't change that), but previously we access the services to obtain the service port name information.

@chrisob
Copy link

chrisob commented Jan 21, 2022

@juan131 Coming off of @linkvt 's comment, is there no alternative that would support using the original Services(namespace).ProxyGet? Reason being, just simply allowing get services in RBAC as is the case in #715 punches a bigger hole in RBAC which may not be desired in multitenant environments (especially considering any authenticated user can get the entire service's manifest!).

Maybe I'm a little over cautious, but I feel that only allowing services/proxy RBAC was a nice solution which prevented random users from discovering more information about sealed-secrets-controller than they need, a-la the principle of least privilege.

@chrisob
Copy link

chrisob commented Jan 21, 2022

Just brainstorming here, but one approach could be to control this "discover service endpoint" functionality via feature flag, but by default have kubeseal assume the default http port & not attempt any service lookup.

@juan131
Copy link
Collaborator

juan131 commented Jan 24, 2022

Hi @chrisob

Thanks for sharing your concerns, we really appreciate your opinion. IMHO, the security risk is not that high.

It limits the scope to the namespace where the SealedSecrets controller is installed, offering the opportunity to use a dedicated namespace for the controller so authenticated users can exclusively obtain the controller service metadata. Furthermore, this service is by default a "ClusterIP" service that exposes a single HTTP port, I don't think we add much value in terms of security by hiding this info from authenticated users.

@mkmik
Copy link
Collaborator

mkmik commented Jan 24, 2022

My plan for fixing this was to create a ConfigMap with the public key
and avoid all this mess with actively trying to talking to the controller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Sealed Secrets
  
Completed
7 participants