-
Notifications
You must be signed in to change notification settings - Fork 155
fix: K8s API does not accept unauthorized requests #2111
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
base: main
Are you sure you want to change the base?
Conversation
Hey, thanks for the PR. Do you have more information about this issue? The PR linked in the issue was from almost 10 years ago so I'm uncertain why this is now coming up. I also am not too familiar with Kubernetes setups, so can you confirm if |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2111 +/- ##
===========================================
- Coverage 57.93% 43.55% -14.38%
===========================================
Files 50 72 +22
Lines 3119 5705 +2586
===========================================
+ Hits 1807 2485 +678
- Misses 1154 2989 +1835
- Partials 158 231 +73 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ab11d59
to
4fe495f
Compare
Hi @haydentherapper thank you for the review. I can confirm that the token file is always present (as you can see in the Kubernetes doc https://kubernetes.io/docs/tasks/run-application/access-api-from-pod/#directly-accessing-the-rest-api). For more information about this issue, you can read https://www.redhat.com/en/blog/what-you-need-to-know-red-hat-openshift-416
|
If this is specific to OpenShift, can we make this configurable whether or not to attach a token? I'd like to avoid the risk breaking any existing deployments. |
@haydentherapper yes, I agree that we should be careful with the backward compatibility. I do not want to add another variable to the https://pkg.go.dev/github.com/sigstore/fulcio/pkg/config#OIDCIssuer that is specific only to I added the |
@haydentherapper could you please re-review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link to the Kubernetes PR was for a 1.5 release, but it looks like in just the next release they changed its behavior - https://kubernetes.io/docs/reference/access-authn-authz/authentication/#anonymous-requests - so it doesn't seem like that PR is telling the full story.
It looks like Kubernetes supports adding certain endpoints to an AuthenticationConfiguration
list to allow anonymous access, could OpenShift consider doing that?
Could access to this endpoint be granted to the system:openshift:public-info-viewer
role, which looks like it is already assigned to the anonymous user and is intended for OIDC workflows?
_, verifierPresent := fc.verifiers[k8sIssuerURL] | ||
k8sIssuer, issuerConfigured := fc.GetIssuer(k8sIssuerURL) | ||
if issuerConfigured && !verifierPresent { | ||
// configure static validator for k8s that cover metaIssuers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain more why the k8s issuer would be present in the config but not already have its provider/verifier created in the loop above? This check doesn't look necessary to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first cycle traverses over static OIDCIssuers. The for cycle covers the following configuration:
OIDCIssuers: map[string]OIDCIssuer{
k8sIssuerURL: {
IssuerURL: k8sIssuerURL,
ClientID: "sigstore",
Type: IssuerTypeKubernetes,
},
},
(test https://github.com/bouskaJ/fulcio/blob/fix_2110/pkg/config/config_test.go#L866-L873)
For MetaIssuer the GetVerifier function will not get any static verifier without adding it explicitly (see https://github.com/bouskaJ/fulcio/blob/fix_2110/pkg/config/config.go#L197). The function would create a new verifier dynamically in that case. This new client will use http.Client without the authorization.
I am creating the additional static verifier in case the k8s issuer can be resolved by https://github.com/bouskaJ/fulcio/blob/fix_2110/pkg/config/config.go#L156 but it is not present yet.
This behaviour is tested by https://github.com/bouskaJ/fulcio/blob/fix_2110/pkg/config/config_test.go#L875C3-L887C5
return nil | ||
} | ||
|
||
var ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These could all be consts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they could, but I make them vars
because it allows me to change the value for testing purposes. https://github.com/bouskaJ/fulcio/blob/fix_2110/pkg/config/config_test.go#L905
@cmurphy Thank you for your comments.
yes, I linked the very old PR that might not be relevant to recent changes but the https://www.redhat.com/en/blog/what-you-need-to-know-red-hat-openshift-416 is current and comprehensive source.
I agree that both Kubernetes and OpenShift allow for role modifications. However, I don't think it's a good practice to lower the default security on the cluster. We should instead focus on finding a way to work within the cluster's default configuration. |
Signed-off-by: Jan Bouska <[email protected]>
Summary
Fixes #2110
Release Note
kubernetes.default.svc OIDC
provider