From 34494fb33062401bc7743779e4bdd041ea68b32f Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Thu, 22 May 2025 19:48:23 +1000 Subject: [PATCH] chore: avoid depending on rbac in slim builds (#17959) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I noticed the `coder-vpn.dylib` (of course alongside the Agent/CLI binaries) had grown substantially (from 29MB to 37MB for the dylib), and discovered that importing RBAC in slim builds was the issue This PR removes the dependency on RBAC in slim builds, and adds a compile-time check to ensure it can't be imported in the future: ``` $ make build # github.com/coder/coder/v2/coderd/rbac coderd/rbac/no_slim.go:8:2: initialization cycle: _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS refers to itself make: *** [Makefile:224: build/coder-slim_2.22.1-devel+7e46d24b4_linux_amd64] Error 1 ``` Before and after for `coder-slim_darwin_arm64`: ``` $ gsa before after ┌───────────────────────────────────────────────────────────────────────────────────┐ │ Diff between before and after │ ├─────────┬─────────────────────────────────────────┬──────────┬──────────┬─────────┤ │ PERCENT │ NAME │ OLD SIZE │ NEW SIZE │ DIFF │ ├─────────┼─────────────────────────────────────────┼──────────┼──────────┼─────────┤ │ -100% │ github.com/gorilla/mux │ │ │ +0 B │ │ -100% │ github.com/ammario/tlru │ │ │ +0 B │ │ -100% │ github.com/armon/go-radix │ │ │ +0 B │ │ -0.00% │ gvisor.dev/gvisor │ 2.4 MB │ 2.4 MB │ -4 B │ │ -0.21% │ os │ 155 kB │ 155 kB │ -328 B │ │ -0.23% │ regexp │ 152 kB │ 152 kB │ -346 B │ │ -0.04% │ runtime │ 876 kB │ 876 kB │ -372 B │ │ -100% │ github.com/rcrowley/go-metrics │ 675 B │ │ -675 B │ │ -23.79% │ github.com/cespare/xxhash/v2 │ 3.0 kB │ 2.3 kB │ -715 B │ │ -100% │ github.com/agnivade/levenshtein │ 1.4 kB │ │ -1.4 kB │ │ -100% │ github.com/go-ini/ini │ 1.5 kB │ │ -1.5 kB │ │ -100% │ github.com/xeipuuv/gojsonreference │ 2.4 kB │ │ -2.4 kB │ │ -100% │ github.com/xeipuuv/gojsonpointer │ 5.2 kB │ │ -5.2 kB │ │ -2.43% │ go.opentelemetry.io/otel │ 316 kB │ 309 kB │ -7.7 kB │ │ -2.40% │ slices │ 381 kB │ 372 kB │ -9.2 kB │ │ -0.68% │ crypto │ 1.4 MB │ 1.4 MB │ -9.5 kB │ │ -100% │ github.com/tchap/go-patricia/v2 │ 23 kB │ │ -23 kB │ │ -100% │ github.com/yashtewari/glob-intersection │ 28 kB │ │ -28 kB │ │ -4.35% │ │ 754 kB │ 721 kB │ -33 kB │ │ -100% │ github.com/sirupsen/logrus │ 72 kB │ │ -72 kB │ │ -2.56% │ github.com/coder/coder/v2 │ 3.3 MB │ 3.2 MB │ -84 kB │ │ -100% │ github.com/gobwas/glob │ 107 kB │ │ -107 kB │ │ -100% │ sigs.k8s.io/yaml │ 244 kB │ │ -244 kB │ │ -100% │ github.com/open-policy-agent/opa │ 2.2 MB │ │ -2.2 MB │ ├─────────┼─────────────────────────────────────────┼──────────┼──────────┼─────────┤ │ -7.79% │ __go_buildinfo __DATA │ 18 kB │ 17 kB │ -1.4 kB │ │ -6.81% │ __itablink __DATA_CONST │ 23 kB │ 22 kB │ -1.6 kB │ │ -6.61% │ __typelink __DATA_CONST │ 71 kB │ 66 kB │ -4.7 kB │ │ -2.86% │ __noptrdata __DATA │ 1.0 MB │ 993 kB │ -29 kB │ │ -21.49% │ __data __DATA │ 320 kB │ 251 kB │ -69 kB │ │ -6.19% │ __rodata __DATA_CONST │ 6.0 MB │ 5.6 MB │ -372 kB │ │ -47.19% │ __rodata __TEXT │ 7.6 MB │ 4.0 MB │ -3.6 MB │ ├─────────┼─────────────────────────────────────────┼──────────┼──────────┼─────────┤ │ -14.02% │ before │ 50 MB │ 43 MB │ -7.0 MB │ │ │ after │ │ │ │ └─────────┴─────────────────────────────────────────┴──────────┴──────────┴─────────┘ ``` --- .../coder_users_edit-roles_--help.golden | 3 +- cli/usereditroles.go | 29 ++++++++----------- coderd/database/no_slim.go | 9 +++--- coderd/database/no_slim_slim.go | 14 --------- coderd/httpapi/authz.go | 28 ++++++++++++++++++ coderd/httpapi/authz_slim.go | 13 +++++++++ coderd/httpapi/httpapi.go | 19 ++---------- coderd/httpmw/authz.go | 2 ++ coderd/rbac/no_slim.go | 9 ++++++ coderd/rbac/roles.go | 4 +-- coderd/rbac/roles_test.go | 4 +-- coderd/roles.go | 2 +- docs/reference/cli/users_edit-roles.md | 2 +- 13 files changed, 78 insertions(+), 60 deletions(-) delete mode 100644 coderd/database/no_slim_slim.go create mode 100644 coderd/httpapi/authz.go create mode 100644 coderd/httpapi/authz_slim.go create mode 100644 coderd/rbac/no_slim.go diff --git a/cli/testdata/coder_users_edit-roles_--help.golden b/cli/testdata/coder_users_edit-roles_--help.golden index 02dd9155b4..5a21c152e6 100644 --- a/cli/testdata/coder_users_edit-roles_--help.golden +++ b/cli/testdata/coder_users_edit-roles_--help.golden @@ -8,8 +8,7 @@ USAGE: OPTIONS: --roles string-array A list of roles to give to the user. This removes any existing roles - the user may have. The available roles are: auditor, member, owner, - template-admin, user-admin. + the user may have. -y, --yes bool Bypass prompts. diff --git a/cli/usereditroles.go b/cli/usereditroles.go index 815d8f47dc..5bdad7a668 100644 --- a/cli/usereditroles.go +++ b/cli/usereditroles.go @@ -1,32 +1,19 @@ package cli import ( - "fmt" "slices" - "sort" "strings" "golang.org/x/xerrors" "github.com/coder/coder/v2/cli/cliui" - "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/serpent" ) func (r *RootCmd) userEditRoles() *serpent.Command { client := new(codersdk.Client) - - roles := rbac.SiteRoles() - - siteRoles := make([]string, 0) - for _, role := range roles { - siteRoles = append(siteRoles, role.Identifier.Name) - } - sort.Strings(siteRoles) - var givenRoles []string - cmd := &serpent.Command{ Use: "edit-roles ", Short: "Edit a user's roles by username or id", @@ -34,7 +21,7 @@ func (r *RootCmd) userEditRoles() *serpent.Command { cliui.SkipPromptOption(), { Name: "roles", - Description: fmt.Sprintf("A list of roles to give to the user. This removes any existing roles the user may have. The available roles are: %s.", strings.Join(siteRoles, ", ")), + Description: "A list of roles to give to the user. This removes any existing roles the user may have.", Flag: "roles", Value: serpent.StringArrayOf(&givenRoles), }, @@ -52,13 +39,21 @@ func (r *RootCmd) userEditRoles() *serpent.Command { if err != nil { return xerrors.Errorf("fetch user roles: %w", err) } + siteRoles, err := client.ListSiteRoles(ctx) + if err != nil { + return xerrors.Errorf("fetch site roles: %w", err) + } + siteRoleNames := make([]string, 0, len(siteRoles)) + for _, role := range siteRoles { + siteRoleNames = append(siteRoleNames, role.Name) + } var selectedRoles []string if len(givenRoles) > 0 { // Make sure all of the given roles are valid site roles for _, givenRole := range givenRoles { - if !slices.Contains(siteRoles, givenRole) { - siteRolesPretty := strings.Join(siteRoles, ", ") + if !slices.Contains(siteRoleNames, givenRole) { + siteRolesPretty := strings.Join(siteRoleNames, ", ") return xerrors.Errorf("The role %s is not valid. Please use one or more of the following roles: %s\n", givenRole, siteRolesPretty) } } @@ -67,7 +62,7 @@ func (r *RootCmd) userEditRoles() *serpent.Command { } else { selectedRoles, err = cliui.MultiSelect(inv, cliui.MultiSelectOptions{ Message: "Select the roles you'd like to assign to the user", - Options: siteRoles, + Options: siteRoleNames, Defaults: userRoles.Roles, }) if err != nil { diff --git a/coderd/database/no_slim.go b/coderd/database/no_slim.go index 561466490f..edb81e23ad 100644 --- a/coderd/database/no_slim.go +++ b/coderd/database/no_slim.go @@ -1,8 +1,9 @@ +//go:build slim + package database const ( - // This declaration protects against imports in slim builds, see - // no_slim_slim.go. - //nolint:revive,unused - _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS = "DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS" + // This line fails to compile, preventing this package from being imported + // in slim builds. + _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS = _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS ) diff --git a/coderd/database/no_slim_slim.go b/coderd/database/no_slim_slim.go deleted file mode 100644 index 845ac0df77..0000000000 --- a/coderd/database/no_slim_slim.go +++ /dev/null @@ -1,14 +0,0 @@ -//go:build slim - -package database - -const ( - // This re-declaration will result in a compilation error and is present to - // prevent increasing the slim binary size by importing this package, - // directly or indirectly. - // - // no_slim_slim.go:7:2: _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS redeclared in this block - // no_slim.go:4:2: other declaration of _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS - //nolint:revive,unused - _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS = "DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS" -) diff --git a/coderd/httpapi/authz.go b/coderd/httpapi/authz.go new file mode 100644 index 0000000000..f0f208d31b --- /dev/null +++ b/coderd/httpapi/authz.go @@ -0,0 +1,28 @@ +//go:build !slim + +package httpapi + +import ( + "context" + "net/http" + + "github.com/coder/coder/v2/coderd/rbac" +) + +// This is defined separately in slim builds to avoid importing the rbac +// package, which is a large dependency. +func SetAuthzCheckRecorderHeader(ctx context.Context, rw http.ResponseWriter) { + if rec, ok := rbac.GetAuthzCheckRecorder(ctx); ok { + // If you're here because you saw this header in a response, and you're + // trying to investigate the code, here are a couple of notable things + // for you to know: + // - If any of the checks are `false`, they might not represent the whole + // picture. There could be additional checks that weren't performed, + // because processing stopped after the failure. + // - The checks are recorded by the `authzRecorder` type, which is + // configured on server startup for development and testing builds. + // - If this header is missing from a response, make sure the response is + // being written by calling `httpapi.Write`! + rw.Header().Set("x-authz-checks", rec.String()) + } +} diff --git a/coderd/httpapi/authz_slim.go b/coderd/httpapi/authz_slim.go new file mode 100644 index 0000000000..0ebe7ca01a --- /dev/null +++ b/coderd/httpapi/authz_slim.go @@ -0,0 +1,13 @@ +//go:build slim + +package httpapi + +import ( + "context" + "net/http" +) + +func SetAuthzCheckRecorderHeader(ctx context.Context, rw http.ResponseWriter) { + // There's no RBAC on the agent API, so this is separately defined to + // avoid importing the RBAC package, which is a large dependency. +} diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index 5c5c623474..466d45de82 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -20,7 +20,6 @@ import ( "github.com/coder/websocket/wsjson" "github.com/coder/coder/v2/coderd/httpapi/httpapiconstraints" - "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/tracing" "github.com/coder/coder/v2/codersdk" ) @@ -199,19 +198,7 @@ func Write(ctx context.Context, rw http.ResponseWriter, status int, response int _, span := tracing.StartSpan(ctx) defer span.End() - if rec, ok := rbac.GetAuthzCheckRecorder(ctx); ok { - // If you're here because you saw this header in a response, and you're - // trying to investigate the code, here are a couple of notable things - // for you to know: - // - If any of the checks are `false`, they might not represent the whole - // picture. There could be additional checks that weren't performed, - // because processing stopped after the failure. - // - The checks are recorded by the `authzRecorder` type, which is - // configured on server startup for development and testing builds. - // - If this header is missing from a response, make sure the response is - // being written by calling `httpapi.Write`! - rw.Header().Set("x-authz-checks", rec.String()) - } + SetAuthzCheckRecorderHeader(ctx, rw) rw.Header().Set("Content-Type", "application/json; charset=utf-8") rw.WriteHeader(status) @@ -228,9 +215,7 @@ func WriteIndent(ctx context.Context, rw http.ResponseWriter, status int, respon _, span := tracing.StartSpan(ctx) defer span.End() - if rec, ok := rbac.GetAuthzCheckRecorder(ctx); ok { - rw.Header().Set("x-authz-checks", rec.String()) - } + SetAuthzCheckRecorderHeader(ctx, rw) rw.Header().Set("Content-Type", "application/json; charset=utf-8") rw.WriteHeader(status) diff --git a/coderd/httpmw/authz.go b/coderd/httpmw/authz.go index 53aadb6cb7..9f1f397c85 100644 --- a/coderd/httpmw/authz.go +++ b/coderd/httpmw/authz.go @@ -1,3 +1,5 @@ +//go:build !slim + package httpmw import ( diff --git a/coderd/rbac/no_slim.go b/coderd/rbac/no_slim.go new file mode 100644 index 0000000000..d1baaeade4 --- /dev/null +++ b/coderd/rbac/no_slim.go @@ -0,0 +1,9 @@ +//go:build slim + +package rbac + +const ( + // This line fails to compile, preventing this package from being imported + // in slim builds. + _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS = _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS +) diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go index 89f86b567a..8b98f5f2f2 100644 --- a/coderd/rbac/roles.go +++ b/coderd/rbac/roles.go @@ -798,12 +798,12 @@ func OrganizationRoles(organizationID uuid.UUID) []Role { return roles } -// SiteRoles lists all roles that can be applied to a user. +// SiteBuiltInRoles lists all roles that can be applied to a user. // This is the list of available roles, and not specific to a user // // This should be a list in a database, but until then we build // the list from the builtins. -func SiteRoles() []Role { +func SiteBuiltInRoles() []Role { var roles []Role for _, roleF := range builtInRoles { // Must provide some non-nil uuid to filter out org roles. diff --git a/coderd/rbac/roles_test.go b/coderd/rbac/roles_test.go index 4dfbc8fa2a..5738edfe8c 100644 --- a/coderd/rbac/roles_test.go +++ b/coderd/rbac/roles_test.go @@ -34,7 +34,7 @@ func (a authSubject) Subjects() []authSubject { return []authSubject{a} } // rules. If this is incorrect, that is a mistake. func TestBuiltInRoles(t *testing.T) { t.Parallel() - for _, r := range rbac.SiteRoles() { + for _, r := range rbac.SiteBuiltInRoles() { r := r t.Run(r.Identifier.String(), func(t *testing.T) { t.Parallel() @@ -997,7 +997,7 @@ func TestIsOrgRole(t *testing.T) { func TestListRoles(t *testing.T) { t.Parallel() - siteRoles := rbac.SiteRoles() + siteRoles := rbac.SiteBuiltInRoles() siteRoleNames := make([]string, 0, len(siteRoles)) for _, role := range siteRoles { siteRoleNames = append(siteRoleNames, role.Identifier.Name) diff --git a/coderd/roles.go b/coderd/roles.go index 89e6a964ab..ed650f41fd 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -43,7 +43,7 @@ func (api *API) AssignableSiteRoles(rw http.ResponseWriter, r *http.Request) { return } - httpapi.Write(ctx, rw, http.StatusOK, assignableRoles(actorRoles.Roles, rbac.SiteRoles(), dbCustomRoles)) + httpapi.Write(ctx, rw, http.StatusOK, assignableRoles(actorRoles.Roles, rbac.SiteBuiltInRoles(), dbCustomRoles)) } // assignableOrgRoles returns all org wide roles that can be assigned. diff --git a/docs/reference/cli/users_edit-roles.md b/docs/reference/cli/users_edit-roles.md index 23e0baa42a..04f12ce701 100644 --- a/docs/reference/cli/users_edit-roles.md +++ b/docs/reference/cli/users_edit-roles.md @@ -25,4 +25,4 @@ Bypass prompts. |------|---------------------------| | Type | string-array | -A list of roles to give to the user. This removes any existing roles the user may have. The available roles are: auditor, member, owner, template-admin, user-admin. +A list of roles to give to the user. This removes any existing roles the user may have.