✨ Add rudimentary user gating #3

csams · 2022-11-08T20:36:00Z

Summary

The user gate is a rudimentary authorization filter that controls which authenticated users are authorized to make requests against CPS. It is dynamically configured by a secret containing rules in the kcp-system namespace of the root workspace.

This filter is meant to be used along with another system that dynamically updates the secret during a user sign-up workflow ahead of general availability. It should not be used in its current form after a more general mechanism provided by CIAM is in place.

Follow on PRs will change the filter so that it injects groups and/or some form of entitlement information into the user info (after parsed from the JWT), which should dovetail into how we want CIAM to control whether users are allowed to access CPS.

The user gate is a rudimentary authorization filter that controls which authenticated users are authorized to make requests against CPS. It is dynamically configured by a secret containing rules in the kcp-system namespace of the root workspace. This filter is meant to be used along with another system that dynamically updates the secret during a user sign-up workflow ahead of general availability. It should not be used after a more general mechanism provided by CIAM is in place.

openshift-ci · 2022-11-08T20:36:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign stevekuznetsov for approval by writing /assign @stevekuznetsov in a comment. 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

sttts · 2022-11-09T07:17:15Z

/assign @hardys

hardys · 2022-11-09T13:49:02Z

Summary of some previous discussions from the upstream PR

There were concerns and suggestions from @sttts and @s-urbaniak about the proxy reading a secret from the root workspace for this.

Personally I agree and think a rules file managed as part of the proxy deployment would be much simpler, but @csams explained that there is a requirement for systems which don't have access to the proxy deployment to update the rules.

hardys · 2022-11-09T13:51:30Z

pkg/proxy/usergating/controller.go

+	resyncPeriod   = 10 * time.Minute
+
+	UserGateNamespace = "kcp-system"
+	UserGateResource  = "usergate.kcp.dev"


Should we add flags for the namespace and secret name as discussed upstream?

Not a blocker but it seems like it might be a mitigation for the reading-root-secrets concern if we can ensure RBAC limits access allowed from the proxy?

Yep, will do.

hardys · 2022-11-09T13:52:06Z

/cc @s-urbaniak

hardys · 2022-11-09T14:49:27Z

We'll need to merge #4 then rebase this so we can get the prow jobs to run

hardys · 2022-11-09T18:48:37Z

@csams please can you rebase now #4 merged, note you'll need to add the new pkg dir to the Dockerfile

ncdc · 2022-11-09T20:22:29Z

cmd/cps-front-proxy/options/options.go

+	Logs     *logs.Options
+}
+
+func NewOptions() *Options {


global comment: please godoc everything that's exported

ncdc · 2022-11-09T20:24:09Z

pkg/proxy/options/usergate.go

+}
+
+func (o *UserGate) AddFlags(fs *pflag.FlagSet) {
+	fs.BoolVar(&o.Enabled, "enable-user-gating", false, "Enabled user gating.")


Suggested change

fs.BoolVar(&o.Enabled, "enable-user-gating", false, "Enabled user gating.")

fs.BoolVar(&o.Enabled, "enable-user-gating", o.Enabled, "Enable user gating.")

ncdc · 2022-11-09T20:24:34Z

pkg/proxy/options/usergate.go

+
+func (o *UserGate) AddFlags(fs *pflag.FlagSet) {
+	fs.BoolVar(&o.Enabled, "enable-user-gating", false, "Enabled user gating.")
+	fs.StringVar(&o.DefaultRulesFile, "user-gating-rules", "", "Provide a YAML file containing user gating rules.")


Suggested change

fs.StringVar(&o.DefaultRulesFile, "user-gating-rules", "", "Provide a YAML file containing user gating rules.")

fs.StringVar(&o.DefaultRulesFile, "user-gating-rules", o.DefaultRulesFile, "Path to a YAML file containing user gating rules.")

ncdc · 2022-11-09T20:31:42Z

pkg/proxy/server.go

+	"os"
+	"time"
+
+	coreinformers "github.com/kcp-dev/client-go/informers"


Suggested change

coreinformers "github.com/kcp-dev/client-go/informers"

kcpinformers "github.com/kcp-dev/client-go/informers"

ncdc · 2022-11-09T20:32:16Z

pkg/proxy/server.go

+	"time"
+
+	coreinformers "github.com/kcp-dev/client-go/informers"
+	coreclient "github.com/kcp-dev/client-go/kubernetes"


Suggested change

coreclient "github.com/kcp-dev/client-go/kubernetes"

kcpkubernetesclientset "github.com/kcp-dev/client-go/kubernetes"

I'm seeing both kcpkubernetesclientset and kcpkubernetesclient in the kcp code - we should standardize. cc @stevekuznetsov

ncdc · 2022-11-09T21:05:41Z

pkg/proxy/usergating/ruleset/ruleset.go

+	"sigs.k8s.io/yaml"
+)
+
+type RuleSetGetter func() (*RuleSet, bool)


Define this where it's used, i.e. not in this package

ncdc · 2022-11-09T21:06:53Z

pkg/proxy/usergating/handler.go

+
+	if rules, found := a.GetRuleSet(); found {
+		if rules.UserNameIsAllowed(info.GetName()) {
+			return authorizer.DecisionAllow


Call kaudit.AddAuditAnnotations before returning any/all decisions to record why

ncdc · 2022-11-09T21:07:11Z

pkg/proxy/usergating/handler.go

+	})
+}
+
+// Authorize whether the user is allowed to make the request


Suggested change

// Authorize whether the user is allowed to make the request

// Authorize determines whether the user is allowed to make the request.

ncdc · 2022-11-09T21:11:05Z

pkg/proxy/usergating/handler.go

+		}
+
+		decision := gate.Authorize(ctx, u)
+		if decision != authorizer.DecisionDeny {


Do we want to allow NoOpinion to be allowed in? cc @s-urbaniak

This is an interesting point, it seems like we should perhaps only have allowed_user_names then deny all other users, or else this isn't really a gate to prevent access by undefined random users?

ncdc · 2022-11-09T21:11:46Z

pkg/proxy/usergating/controller.go

+
+func (c *Controller) process(ctx context.Context, key string) error {
+	logger := klog.FromContext(ctx)
+	logger.WithValues("key", key).Info("updating usergate secret")


We're not really updating the secret?

changed to "updating usergate ruleset"

hardys · 2022-11-10T17:27:55Z

pkg/proxy/usergating/handler.go

+	GetRuleSet ruleset.RuleSetGetter
+}
+
+func WithUserGating(delegate http.Handler, failed http.Handler, getRuleSet ruleset.RuleSetGetter) http.Handler {


I think this file naming would be more consistent with the upstream proxy if it was e.g pkg/proxy/filters/usergating.go ?

hardys · 2022-11-10T17:34:03Z

pkg/proxy/usergating/handler.go

+// Authorize whether the user is allowed to make the request
+func (a *UserGate) Authorize(ctx context.Context, info user.Info) authorizer.Decision {
+	logger := klog.FromContext(ctx).WithValues("component", "user-gating")
+	logger.V(1).Info("checking auth", "user", info.GetName())


This logging is likely to be too noisy I think, should probably either be removed or put at a higher loglevel?

I'll remove it in favor of ncdc's kaudit.AddAuditAnnotations feedback.

hardys · 2022-11-10T17:46:13Z

pkg/proxy/usergating/controller.go

+	"github.com/kcp-dev/client-go/informers"
+	tenancyv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/tenancy/v1alpha1"
+
+	v1 "k8s.io/api/core/v1"


I think aliasing this to corev1 would be more consistent with upstream?

hardys · 2022-11-11T10:08:57Z

pkg/proxy/usergating/handler.go

+		ctx := r.Context()
+		u, ok := request.UserFrom(ctx)
+		if !ok {
+			delegate.ServeHTTP(w, r)


Similar to the comment below, do we want this to be allow-by-default or should this return failed if we can't identify the user?

Do all authentication methods give a user.Info? Even if so, can we still gate them correctly? Thinking about service account tokens generated in workspaces.

From my testing yes, I've tested with token-file, SA token, client-cert and OIDC and they all result in a user.Info containing a username when authentication is successful

Even Anonymous auth sets a username AFAICS (although we probably don't want to enable that at the proxy)

We'll need to test to confirm, but from my SA testing (where I just used the default SA token from the secret in the root workspace), I think we'll need to have a rule like ^system:serviceaccount:.*:*$ or similar

openshift-merge-robot · 2022-11-25T11:52:25Z

@csams: PR needs rebase.

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.

openshift-ci · 2022-12-08T18:32:42Z

@csams: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/lint	`da82edb`	link	true	`/test lint`
ci/prow/test	`da82edb`	link	true	`/test test`
ci/prow/boilerplate	`da82edb`	link	true	`/test boilerplate`
ci/prow/images	`da82edb`	link	true	`/test images`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-ci bot requested review from hardys and ncdc November 8, 2022 20:36

openshift-ci bot assigned hardys Nov 9, 2022

hardys reviewed Nov 9, 2022

View reviewed changes

openshift-ci bot requested a review from s-urbaniak November 9, 2022 13:52

hardys mentioned this pull request Nov 9, 2022

🌱 Add Dockerfile and .ci-operator.yaml #4

Merged

ncdc reviewed Nov 9, 2022

View reviewed changes

hardys reviewed Nov 10, 2022

View reviewed changes

hardys reviewed Nov 11, 2022

View reviewed changes

csams mentioned this pull request Nov 13, 2022

Add support for group membership via the front-proxy kcp-dev/kcp#1794

Closed

csams marked this pull request as draft November 14, 2022 18:50

openshift-ci bot added the do-not-merge/work-in-progress label Nov 14, 2022

hardys mentioned this pull request Nov 21, 2022

✨ Add basic per-user rate-limiting #5

Merged

openshift-merge-robot added the needs-rebase label Nov 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add rudimentary user gating #3

✨ Add rudimentary user gating #3

csams commented Nov 8, 2022

openshift-ci bot commented Nov 8, 2022

sttts commented Nov 9, 2022

hardys commented Nov 9, 2022

hardys Nov 9, 2022

csams Nov 9, 2022

hardys commented Nov 9, 2022

hardys commented Nov 9, 2022

hardys commented Nov 9, 2022

ncdc Nov 9, 2022

ncdc Nov 9, 2022

ncdc Nov 9, 2022

ncdc Nov 9, 2022

ncdc Nov 9, 2022

ncdc Nov 9, 2022

ncdc Nov 9, 2022

ncdc Nov 9, 2022

ncdc Nov 9, 2022

ncdc Nov 9, 2022

hardys Nov 10, 2022 •

edited

Loading

ncdc Nov 9, 2022

csams Nov 9, 2022

hardys Nov 10, 2022

hardys Nov 10, 2022

csams Nov 11, 2022

hardys Nov 10, 2022

hardys Nov 11, 2022

csams Nov 11, 2022 •

edited

Loading

hardys Nov 14, 2022

hardys Nov 14, 2022

openshift-merge-robot commented Nov 25, 2022

openshift-ci bot commented Dec 8, 2022

	fs.BoolVar(&o.Enabled, "enable-user-gating", false, "Enabled user gating.")
	fs.BoolVar(&o.Enabled, "enable-user-gating", o.Enabled, "Enable user gating.")

	fs.StringVar(&o.DefaultRulesFile, "user-gating-rules", "", "Provide a YAML file containing user gating rules.")
	fs.StringVar(&o.DefaultRulesFile, "user-gating-rules", o.DefaultRulesFile, "Path to a YAML file containing user gating rules.")

	coreinformers "github.com/kcp-dev/client-go/informers"
	kcpinformers "github.com/kcp-dev/client-go/informers"

	coreclient "github.com/kcp-dev/client-go/kubernetes"
	kcpkubernetesclientset "github.com/kcp-dev/client-go/kubernetes"

	// Authorize whether the user is allowed to make the request
	// Authorize determines whether the user is allowed to make the request.

✨ Add rudimentary user gating #3

Are you sure you want to change the base?

✨ Add rudimentary user gating #3

Conversation

csams commented Nov 8, 2022

Summary

openshift-ci bot commented Nov 8, 2022

sttts commented Nov 9, 2022

hardys commented Nov 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardys commented Nov 9, 2022

hardys commented Nov 9, 2022

hardys commented Nov 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardys Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csams Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Nov 25, 2022

openshift-ci bot commented Dec 8, 2022

hardys Nov 10, 2022 •

edited

Loading

csams Nov 11, 2022 •

edited

Loading