Skip to content

Commit

Permalink
Fix kubectl builder perms checker
Browse files Browse the repository at this point in the history
  • Loading branch information
mszostok committed Feb 12, 2024
1 parent efcab7f commit 0208b3c
Show file tree
Hide file tree
Showing 7 changed files with 152 additions and 105 deletions.
65 changes: 0 additions & 65 deletions internal/executor/kubectl/accessreview/review.go

This file was deleted.

3 changes: 2 additions & 1 deletion internal/executor/kubectl/builder/deps.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/kubeshop/botkube/internal/command"
"github.com/kubeshop/botkube/pkg/api"
)

type (
Expand All @@ -29,6 +30,6 @@ type (

// AuthChecker provides an option to check if we can run a kubectl commands with a given permission.
AuthChecker interface {
CheckUserAccess(ns, verb, resource, name string) error
ValidateUserAccess(ns, verb, resource, name string) (bool, *api.Section)
}
)
27 changes: 8 additions & 19 deletions internal/executor/kubectl/builder/kubectl.go
Original file line number Diff line number Diff line change
Expand Up @@ -442,25 +442,10 @@ func (e *Kubectl) buildCommandPreview(state stateDetails) []api.Section {
return []api.Section{InternalErrorSection()}
}

err = e.authCheck.CheckUserAccess(state.namespace, state.verb, state.resourceType, state.resourceName)
if err != nil {
return []api.Section{
{
Base: api.Base{
Header: "Missing permissions",
Body: api.Body{
Plaintext: err.Error(),
},
},
Context: []api.ContextItem{
{
Text: "To learn more about `kubectl` RBAC visit https://docs.botkube.io/configuration/executor/kubectl.",
},
},
},
}
allowed, authMsgFeedback := e.authCheck.ValidateUserAccess(state.namespace, state.verb, state.resourceType, state.resourceName)
if !allowed && authMsgFeedback != nil {
return []api.Section{*authMsgFeedback}
}

if resourceDetails.SlashSeparatedInCommand && state.resourceName == "" {
// we should not render the command as it will be invalid anyway without the resource name
return nil
Expand All @@ -487,7 +472,11 @@ func (e *Kubectl) buildCommandPreview(state stateDetails) []api.Section {
cmd = fmt.Sprintf("%s --filter=%q", cmd, state.filter)
}

return PreviewSection(cmd, FilterSection())
out := PreviewSection(cmd, FilterSection())
if authMsgFeedback != nil {
out = append(out, *authMsgFeedback)
}
return out
}

func (e *Kubectl) message(msg string) (api.Message, error) {
Expand Down
42 changes: 25 additions & 17 deletions internal/executor/kubectl/builder/kubectl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/kubeshop/botkube/pkg/api"
"github.com/kubeshop/botkube/pkg/execute/kubectl"
"github.com/kubeshop/botkube/pkg/loggerx"
"github.com/kubeshop/botkube/pkg/ptr"
)

const (
Expand Down Expand Up @@ -161,7 +162,7 @@ func TestShouldPrintErrMessageIfUserHasInsufficientPerms(t *testing.T) {
state = fixStateForAllDropdowns()
kcExecutor = &fakeKcExecutor{}
nsLister = &fakeNamespaceLister{}
authCheck = &fakeAuthChecker{fixErr: errors.New("not enough permissions")}
authCheck = &fakeAuthChecker{fixFeedbackMsg: ptr.FromType(fixMissingPermsFeedbackMessage())}
guard = kubectl.NewFakeCommandGuard()
cmd = "@builder --verbs"
expMsg = fixInsufficientPermsMessage(fixAllDropdown(true)...)
Expand Down Expand Up @@ -193,25 +194,29 @@ func fixInsufficientPermsMessage(dropdowns ...api.Select) api.Message {
Items: dropdowns,
},
},
{
Base: api.Base{
Header: "Missing permissions",
Body: api.Body{
Plaintext: "not enough permissions",
},
},
Context: api.ContextItems{
api.ContextItem{
Text: "To learn more about `kubectl` RBAC visit https://docs.botkube.io/configuration/executor/kubectl.",
},
},
},
fixMissingPermsFeedbackMessage(),
},
OnlyVisibleForYou: true,
ReplaceOriginal: true,
}
}

func fixMissingPermsFeedbackMessage() api.Section {
return api.Section{
Base: api.Base{
Header: "Missing permissions",
Body: api.Body{
Plaintext: "not enough permissions",
},
},
Context: api.ContextItems{
api.ContextItem{
Text: "To learn more about `kubectl` RBAC visit https://docs.botkube.io/configuration/executor/kubectl.",
},
},
}
}

func TestShouldPrintErrMessageIfGuardFails(t *testing.T) {
// given
var (
Expand Down Expand Up @@ -571,11 +576,14 @@ func (f *fakeNamespaceLister) List(_ context.Context, _ metav1.ListOptions) (*co
}

type fakeAuthChecker struct {
fixErr error
fixFeedbackMsg *api.Section
}

func (r *fakeAuthChecker) CheckUserAccess(ns, verb, resource, name string) error {
return r.fixErr
func (r *fakeAuthChecker) ValidateUserAccess(ns, verb, resource, name string) (bool, *api.Section) {
if r.fixFeedbackMsg != nil {
return false, r.fixFeedbackMsg
}
return true, nil
}

type fakeErrCommandGuard struct {
Expand Down
115 changes: 115 additions & 0 deletions internal/executor/kubectl/builder/review.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
package builder

import (
"context"
"fmt"

"github.com/kubeshop/botkube/pkg/api"
"github.com/kubeshop/botkube/pkg/ptr"
"github.com/sirupsen/logrus"
"golang.org/x/exp/slices"

authv1 "k8s.io/api/authorization/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
v1 "k8s.io/client-go/kubernetes/typed/authorization/v1"
)

// resourceVerbs is a list of verbs that are supported by Kubernetes api-server natively.
// Copied from: https://github.com/kubernetes/kubernetes/blob/release-1.29/staging/src/k8s.io/kubectl/pkg/cmd/auth/cani.go#L106
var resourceVerbs = []string{"get", "list", "watch", "create", "update", "patch", "delete", "deletecollection", "use", "bind", "impersonate", "*"}

// K8sAuth provides functionality to check if we have enough permissions to run given kubectl command.
type K8sAuth struct {
cli v1.AuthorizationV1Interface
log logrus.FieldLogger
}

// NewK8sAuth return a new K8sAuth instance.
func NewK8sAuth(cli v1.AuthorizationV1Interface) *K8sAuth {
return &K8sAuth{
cli: cli,
}
}

// ValidateUserAccess validates that a given verbs are allowed. Returns user-facing message if not allowed.
func (c *K8sAuth) ValidateUserAccess(ns, verb, resource, name string) (bool, *api.Section) {
var subresource string

// kubectl logs/pods [NAME] should be translated into 'get logs pod [NAME]'
// as the `log` is a subresource, same as scale, etc.
//
// We try to support as much as we can but this can grow even with custom plugins,
// so we return warnings in case of unknown verbs instead of blocking the operation.
switch verb {
case "logs", "log":
verb = "get"
subresource = "log"
case "describe":
verb = "get"
case "api-resources", "api-versions":
// no specific permission needed
return true, nil
case "top":
verb = "get"
subresource = "metrics.k8s.io"
}

ctx := context.Background()
review := authv1.SelfSubjectAccessReview{
Spec: authv1.SelfSubjectAccessReviewSpec{
ResourceAttributes: &authv1.ResourceAttributes{
Namespace: ns,
Verb: verb,
Resource: resource,
Subresource: subresource,
Name: name,
},
},
}
out, err := c.cli.SelfSubjectAccessReviews().Create(ctx, &review, metav1.CreateOptions{})
if err != nil {
c.log.WithFields(logrus.Fields{
"error": err.Error(),
}).Error("Failed to create access review")
return false, ptr.FromType(InternalErrorSection())
}

wasKnownVerb := slices.Contains(resourceVerbs, verb)

if !wasKnownVerb && !out.Status.Allowed {
// in this case we allow it anyway, as the API server may be wrong about it.
return true, &api.Section{
Context: []api.ContextItem{
{
Text: ":warning: Unable to verify if the action can be performed. Running this command may lead to an unauthorized error. To learn more about `kubectl` RBAC visit https://docs.botkube.io/configuration/executor/kubectl.",
},
},
}
}

if !out.Status.Allowed {
msg := ":exclamation: You don't have enough permission to run this command.\n"
if out.Status.Reason != "" {
msg = fmt.Sprintf("%sReason: %s\n", msg, out.Status.Reason)
}
return false, c.notAllowedMessage(msg)
}

return false, nil
}

func (c *K8sAuth) notAllowedMessage(msg string) *api.Section {
return &api.Section{
Base: api.Base{
Header: "Missing permissions",
Body: api.Body{
Plaintext: msg,
},
},
Context: []api.ContextItem{
{
Text: "To learn more about `kubectl` RBAC visit https://docs.botkube.io/configuration/executor/kubectl.",
},
},
}
}
3 changes: 1 addition & 2 deletions internal/executor/kubectl/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"k8s.io/client-go/tools/clientcmd"

"github.com/kubeshop/botkube/internal/command"
"github.com/kubeshop/botkube/internal/executor/kubectl/accessreview"
"github.com/kubeshop/botkube/internal/executor/kubectl/builder"
"github.com/kubeshop/botkube/pkg/api"
"github.com/kubeshop/botkube/pkg/api/executor"
Expand Down Expand Up @@ -121,7 +120,7 @@ func (e *Executor) Execute(ctx context.Context, in executor.ExecuteInput) (execu
return executor.ExecuteOutput{}, fmt.Errorf("while creating builder dependecies: %w", err)
}

kcBuilder := builder.NewKubectl(scopedKubectlRunner, cfg.InteractiveBuilder, log, guard, cfg.DefaultNamespace, k8sCli.CoreV1().Namespaces(), accessreview.NewK8sAuth(k8sCli.AuthorizationV1()))
kcBuilder := builder.NewKubectl(scopedKubectlRunner, cfg.InteractiveBuilder, log, guard, cfg.DefaultNamespace, k8sCli.CoreV1().Namespaces(), builder.NewK8sAuth(k8sCli.AuthorizationV1()))
msg, err := kcBuilder.Handle(ctx, cmd, in.Context.IsInteractivitySupported, in.Context.SlackState)
if err != nil {
return executor.ExecuteOutput{}, fmt.Errorf("while running command builder: %w", err)
Expand Down
2 changes: 1 addition & 1 deletion test/helpers/plugin_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func main() {
portInt, err := strconv.Atoi(port)
loggerx.ExitOnError(err, "while starting server")

binDir := filepath.Join(dir, "plugin-dist")
binDir := filepath.Join(dir, "../plugin-dist")
indexEndpoint, startServerFn := fake.NewPluginServer(fake.PluginConfig{
BinariesDirectory: binDir,
Server: fake.PluginServer{
Expand Down

0 comments on commit 0208b3c

Please sign in to comment.