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

feat: limit controller access to control plane secrets #2522

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from 22 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions charts/kargo/templates/controller/cluster-roles.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,6 @@ rules:
verbs:
- create
- patch
- apiGroups:
- ""
resources:
- secrets
verbs:
- get
- list
- watch
- apiGroups:
- kargo.akuity.io
resources:
Expand Down Expand Up @@ -68,6 +60,23 @@ rules:
- warehouses/status
verbs:
- patch
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: kargo-controller-secrets-readonly
labels:
{{- include "kargo.labels" . | nindent 4 }}
{{- include "kargo.controller.labels" . | nindent 4 }}
rules:
- apiGroups:
- ""
resources:
- secrets
verbs:
- get
- list
- watch
{{- if and .Values.controller.argocd.integrationEnabled (not .Values.controller.argocd.watchArgocdNamespaceOnly) }}
---
apiVersion: rbac.authorization.k8s.io/v1
Expand Down
8 changes: 8 additions & 0 deletions cmd/controlplane/management_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"github.com/akuity/kargo/internal/api/kubernetes"
"github.com/akuity/kargo/internal/controller/management/namespaces"
"github.com/akuity/kargo/internal/controller/management/projects"
"github.com/akuity/kargo/internal/controller/management/serviceaccounts"
"github.com/akuity/kargo/internal/logging"
"github.com/akuity/kargo/internal/os"
versionpkg "github.com/akuity/kargo/internal/version"
Expand Down Expand Up @@ -78,6 +79,13 @@
return fmt.Errorf("error setting up Projects reconciler: %w", err)
}

if err := serviceaccounts.SetupReconcilerWithManager(
kargoMgr,
serviceaccounts.ReconcilerConfigFromEnv(),
); err != nil {
return fmt.Errorf("error setting up ServiceAccount reconciler: %w", err)

Check warning on line 86 in cmd/controlplane/management_controller.go

View check run for this annotation

Codecov / codecov/patch

cmd/controlplane/management_controller.go#L82-L86

Added lines #L82 - L86 were not covered by tests
}

if err := kargoMgr.Start(ctx); err != nil {
return fmt.Errorf("error starting kargo manager: %w", err)
}
Expand Down
74 changes: 74 additions & 0 deletions internal/controller/management/projects/projects.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ type reconciler struct {

ensureAPIAdminPermissionsFn func(context.Context, *kargoapi.Project) error

ensureControllerPermissionsFn func(context.Context, *kargoapi.Project) error

ensureDefaultProjectRolesFn func(context.Context, *kargoapi.Project) error

createServiceAccountFn func(
Expand All @@ -100,6 +102,12 @@ type reconciler struct {
...client.CreateOption,
) error

listServiceAccountsFn func(
context.Context,
client.ObjectList,
...client.ListOption,
) error

createRoleFn func(
context.Context,
client.Object,
Expand Down Expand Up @@ -147,8 +155,10 @@ func newReconciler(kubeClient client.Client, cfg ReconcilerConfig) *reconciler {
r.patchOwnerReferencesFn = kargoapi.PatchOwnerReferences
r.ensureFinalizerFn = kargoapi.EnsureFinalizer
r.ensureAPIAdminPermissionsFn = r.ensureAPIAdminPermissions
r.ensureControllerPermissionsFn = r.ensureControllerPermissions
r.ensureDefaultProjectRolesFn = r.ensureDefaultProjectRoles
r.createServiceAccountFn = r.client.Create
r.listServiceAccountsFn = r.client.List
r.createRoleFn = r.client.Create
r.createRoleBindingFn = r.client.Create
return r
Expand Down Expand Up @@ -231,6 +241,10 @@ func (r *reconciler) syncProject(
return status, fmt.Errorf("error ensuring project admin permissions: %w", err)
}

if err = r.ensureControllerPermissionsFn(ctx, project); err != nil {
return status, fmt.Errorf("error ensuring controller permissions: %w", err)
}

if err = r.ensureDefaultProjectRolesFn(ctx, project); err != nil {
return status, fmt.Errorf("error ensuring default project roles: %w", err)
}
Expand Down Expand Up @@ -392,6 +406,66 @@ func (r *reconciler) ensureAPIAdminPermissions(
return nil
}

func (r *reconciler) ensureControllerPermissions(
fykaa marked this conversation as resolved.
Show resolved Hide resolved
ctx context.Context,
project *kargoapi.Project,
) error {
logger := logging.LoggerFromContext(ctx).WithValues(
"project", project.Name,
"namespace", project.Name,
)

// Get all ServiceAccounts labeled as controller ServiceAccounts
controllerSAs := &corev1.ServiceAccountList{}
if err := r.listServiceAccountsFn(
ctx, controllerSAs, client.InNamespace(r.cfg.KargoNamespace),
client.MatchingLabels{"app.kubernetes.io/component": "controller"},
); err != nil {
return fmt.Errorf("error listing controller ServiceAccounts: %w", err)
}

// Create/update a RoleBinding for each ServiceAccount
for _, sa := range controllerSAs.Items {
roleBindingName := fmt.Sprintf("%s-readonly-secrets", sa.Name)
loggerWithSA := logger.WithValues("serviceAccount", sa.Name, "roleBinding", roleBindingName)

roleBinding := &rbacv1.RoleBinding{
ObjectMeta: metav1.ObjectMeta{
Name: roleBindingName,
Namespace: project.Name,
},
RoleRef: rbacv1.RoleRef{
APIGroup: rbacv1.GroupName,
Kind: "ClusterRole",
Name: "kargo-controller-secrets-readonly",
},
Subjects: []rbacv1.Subject{
{
Kind: "ServiceAccount",
Name: sa.Name,
Namespace: r.cfg.KargoNamespace,
},
},
}

if err := r.createRoleBindingFn(ctx, roleBinding); err != nil {
if !kubeerr.IsAlreadyExists(err) {
return fmt.Errorf("error creating RoleBinding %q for ServiceAccount %q in Project namespace %q: %w",
roleBinding.Name, sa.Name, project.Name, err)
}
if err := r.client.Update(ctx, roleBinding); err != nil {
return fmt.Errorf("error updating existing RoleBinding %q in Project namespace %q: %w",
roleBinding.Name, project.Name, err)
}
loggerWithSA.Debug("Updated RoleBinding for ServiceAccount", "serviceAccount", sa.Name)
fykaa marked this conversation as resolved.
Show resolved Hide resolved
continue
}
loggerWithSA.Debug("Created RoleBinding for ServiceAccount", "serviceAccount", sa.Name)
fykaa marked this conversation as resolved.
Show resolved Hide resolved
}

return nil
}

func (r *reconciler) ensureDefaultProjectRoles(
ctx context.Context,
project *kargoapi.Project,
Expand Down
74 changes: 74 additions & 0 deletions internal/controller/management/projects/projects_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,32 @@ func TestSyncProject(t *testing.T) {
require.Equal(t, kargoapi.ProjectPhaseInitializing, status.Phase)
},
},
{
name: "error ensuring controller permissions",
reconciler: &reconciler{
ensureNamespaceFn: func(
_ context.Context,
project *kargoapi.Project,
) (kargoapi.ProjectStatus, error) {
return *project.Status.DeepCopy(), nil
},
ensureAPIAdminPermissionsFn: func(
context.Context,
*kargoapi.Project,
) error {
return nil
},
ensureControllerPermissionsFn: func(
context.Context,
*kargoapi.Project,
) error {
return errors.New("something went wrong")
},
},
assertions: func(t *testing.T, _ kargoapi.ProjectStatus, err error) {
require.ErrorContains(t, err, "something went wrong")
},
},
{
name: "error ensuring default project roles",
reconciler: &reconciler{
Expand All @@ -255,6 +281,12 @@ func TestSyncProject(t *testing.T) {
) error {
return nil
},
ensureControllerPermissionsFn: func(
context.Context,
*kargoapi.Project,
) error {
return nil
},
ensureDefaultProjectRolesFn: func(
context.Context,
*kargoapi.Project,
Expand Down Expand Up @@ -283,6 +315,12 @@ func TestSyncProject(t *testing.T) {
) error {
return nil
},
ensureControllerPermissionsFn: func(
context.Context,
*kargoapi.Project,
) error {
return nil
},
ensureDefaultProjectRolesFn: func(
context.Context,
*kargoapi.Project,
Expand Down Expand Up @@ -664,6 +702,42 @@ func TestEnsureAPIAdminPermissions(t *testing.T) {
}
}

func TestEnsureControllerPermissions(t *testing.T) {
testCases := []struct {
name string
reconciler *reconciler
assertions func(*testing.T, error)
}{
{
name: "error listing service accounts",
reconciler: &reconciler{
listServiceAccountsFn: func(
context.Context,
client.ObjectList,
...client.ListOption,
) error {
return errors.New("something went wrong")
},
},
assertions: func(t *testing.T, err error) {
require.ErrorContains(t, err, "error listing controller ServiceAccounts")
require.ErrorContains(t, err, "something went wrong")
},
},
}
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
testCase.assertions(
t,
testCase.reconciler.ensureControllerPermissions(
context.Background(),
&kargoapi.Project{},
),
)
})
}
}

func TestEnsureDefaultProjectRoles(t *testing.T) {
testCases := []struct {
name string
Expand Down
Loading