From abe253d7c2c6966667575ee9854a1fcfd60ad5ed Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 22 Jul 2024 18:42:26 +0000 Subject: [PATCH 01/10] feat: accept provisioner keys for provisioner auth --- coderd/database/dbauthz/dbauthz.go | 75 ++++++++++++++-------- coderd/httpmw/csrf.go | 7 +++ coderd/httpmw/provisionerdaemon.go | 82 ++++++++++++++++++++----- coderd/provisionerkey/provisionerkey.go | 24 +++++++- codersdk/client.go | 3 + codersdk/provisionerdaemons.go | 11 +++- enterprise/coderd/coderd.go | 8 ++- enterprise/coderd/provisionerdaemons.go | 22 ++++--- 8 files changed, 175 insertions(+), 57 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 82f26c31da3e6..a5d08493f0846 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -158,34 +158,49 @@ func ActorFromContext(ctx context.Context) (rbac.Subject, bool) { } var ( - subjectProvisionerd = rbac.Subject{ - FriendlyName: "Provisioner Daemon", - ID: uuid.Nil.String(), - Roles: rbac.Roles([]rbac.Role{ - { - Identifier: rbac.RoleIdentifier{Name: "provisionerd"}, - DisplayName: "Provisioner Daemon", - Site: rbac.Permissions(map[string][]policy.Action{ - // TODO: Add ProvisionerJob resource type. - rbac.ResourceFile.Type: {policy.ActionRead}, - rbac.ResourceSystem.Type: {policy.WildcardSymbol}, - rbac.ResourceTemplate.Type: {policy.ActionRead, policy.ActionUpdate}, - // Unsure why provisionerd needs update and read personal - rbac.ResourceUser.Type: {policy.ActionRead, policy.ActionReadPersonal, policy.ActionUpdatePersonal}, - rbac.ResourceWorkspaceDormant.Type: {policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceStop}, - rbac.ResourceWorkspace.Type: {policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceStart, policy.ActionWorkspaceStop}, - rbac.ResourceApiKey.Type: {policy.WildcardSymbol}, - // When org scoped provisioner credentials are implemented, - // this can be reduced to read a specific org. + subjectProvisionerd = func(orgID uuid.UUID) rbac.Subject { + sitePermissions := map[string][]policy.Action{ + // TODO: Add ProvisionerJob resource type. + rbac.ResourceFile.Type: {policy.ActionRead}, + rbac.ResourceSystem.Type: {policy.WildcardSymbol}, + rbac.ResourceTemplate.Type: {policy.ActionRead, policy.ActionUpdate}, + // Unsure why provisionerd needs update and read personal + rbac.ResourceUser.Type: {policy.ActionRead, policy.ActionReadPersonal, policy.ActionUpdatePersonal}, + rbac.ResourceWorkspaceDormant.Type: {policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceStop}, + rbac.ResourceWorkspace.Type: {policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceStart, policy.ActionWorkspaceStop}, + rbac.ResourceApiKey.Type: {policy.WildcardSymbol}, + // When org scoped provisioner credentials are implemented, + // this can be reduced to read a specific org. + rbac.ResourceOrganization.Type: {policy.ActionRead}, + rbac.ResourceGroup.Type: {policy.ActionRead}, + } + orgPermissions := map[string][]rbac.Permission{} + + if orgID != uuid.Nil { + // replace site wide org permissions with org scoped permissions + delete(sitePermissions, rbac.ResourceOrganization.Type) + orgPermissions = map[string][]rbac.Permission{ + orgID.String(): rbac.Permissions(map[string][]policy.Action{ rbac.ResourceOrganization.Type: {policy.ActionRead}, - rbac.ResourceGroup.Type: {policy.ActionRead}, }), - Org: map[string][]rbac.Permission{}, - User: []rbac.Permission{}, - }, - }), - Scope: rbac.ScopeAll, - }.WithCachedASTValue() + } + } + + return rbac.Subject{ + FriendlyName: "Provisioner Daemon", + ID: uuid.Nil.String(), + Roles: rbac.Roles([]rbac.Role{ + { + Identifier: rbac.RoleIdentifier{Name: "provisionerd"}, + DisplayName: "Provisioner Daemon", + Site: rbac.Permissions(sitePermissions), + Org: orgPermissions, + User: []rbac.Permission{}, + }, + }), + Scope: rbac.ScopeAll, + }.WithCachedASTValue() + } subjectAutostart = rbac.Subject{ FriendlyName: "Autostart", @@ -261,7 +276,13 @@ var ( // AsProvisionerd returns a context with an actor that has permissions required // for provisionerd to function. func AsProvisionerd(ctx context.Context) context.Context { - return context.WithValue(ctx, authContextKey{}, subjectProvisionerd) + return context.WithValue(ctx, authContextKey{}, subjectProvisionerd(uuid.Nil)) +} + +// AsProvisionerd returns a context with an actor that has permissions required +// for an org scoped provisionerd to function. +func AsOrganizationProvisionerd(ctx context.Context, orgID uuid.UUID) context.Context { + return context.WithValue(ctx, authContextKey{}, subjectProvisionerd(orgID)) } // AsAutostart returns a context with an actor that has permissions required diff --git a/coderd/httpmw/csrf.go b/coderd/httpmw/csrf.go index 2bb0dd0a20037..e868019bac23b 100644 --- a/coderd/httpmw/csrf.go +++ b/coderd/httpmw/csrf.go @@ -93,6 +93,13 @@ func CSRF(secureCookie bool) func(next http.Handler) http.Handler { return true } + if r.Header.Get(codersdk.ProvisionerDaemonKey) != "" { + // If present, the provisioner daemon also is providing an api key + // that will make them exempt from CSRF. But this is still useful + // for enumerating the external auths. + return true + } + // If the X-CSRF-TOKEN header is set, we can exempt the func if it's valid. // This is the CSRF check. sent := r.Header.Get("X-CSRF-TOKEN") diff --git a/coderd/httpmw/provisionerdaemon.go b/coderd/httpmw/provisionerdaemon.go index d0fbfe0e6bcf4..7caa621fa3a05 100644 --- a/coderd/httpmw/provisionerdaemon.go +++ b/coderd/httpmw/provisionerdaemon.go @@ -8,6 +8,7 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/provisionerkey" "github.com/coder/coder/v2/codersdk" ) @@ -19,11 +20,13 @@ func ProvisionerDaemonAuthenticated(r *http.Request) bool { } type ExtractProvisionerAuthConfig struct { - DB database.Store - Optional bool + DB database.Store + Optional bool + PSK string + MultiOrgEnabled bool } -func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig, psk string) func(next http.Handler) http.Handler { +func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -36,26 +39,50 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig, ps httpapi.Write(ctx, w, code, response) } - if psk == "" { - // No psk means external provisioner daemons are not allowed. - // So their auth is not valid. - handleOptional(http.StatusBadRequest, codersdk.Response{ - Message: "External provisioner daemons not enabled", - }) + if !opts.MultiOrgEnabled { + fallbackToPSK(ctx, opts.PSK, next, w, r, handleOptional) return } - token := r.Header.Get(codersdk.ProvisionerDaemonPSK) - if token == "" { + key := r.Header.Get(codersdk.ProvisionerDaemonKey) + if key == "" { + if opts.PSK == "" { + handleOptional(http.StatusUnauthorized, codersdk.Response{ + Message: "provisioner daemon key required", + }) + return + } + + fallbackToPSK(ctx, opts.PSK, next, w, r, handleOptional) + return + } + + id, keyValue, err := provisionerkey.Parse(key) + if err != nil { handleOptional(http.StatusUnauthorized, codersdk.Response{ - Message: "provisioner daemon auth token required", + Message: "provisioner daemon key invalid", + }) + return + } + + pk, err := opts.DB.GetProvisionerKeyByID(ctx, id) + if err != nil { + if httpapi.Is404Error(err) { + handleOptional(http.StatusUnauthorized, codersdk.Response{ + Message: "provisioner daemon key invalid", + }) + return + } + + handleOptional(http.StatusInternalServerError, codersdk.Response{ + Message: "get provisioner daemon key: " + err.Error(), }) return } - if subtle.ConstantTimeCompare([]byte(token), []byte(psk)) != 1 { + if subtle.ConstantTimeCompare(pk.HashedSecret, provisionerkey.HashSecret(keyValue)) != 1 { handleOptional(http.StatusUnauthorized, codersdk.Response{ - Message: "provisioner daemon auth token invalid", + Message: "provisioner daemon key invalid", }) return } @@ -65,8 +92,33 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig, ps // authenticated provisioner daemon. ctx = context.WithValue(ctx, provisionerDaemonContextKey{}, true) // nolint:gocritic // Authenticating as a provisioner daemon. - ctx = dbauthz.AsProvisionerd(ctx) + ctx = dbauthz.AsOrganizationProvisionerd(ctx, pk.OrganizationID) next.ServeHTTP(w, r.WithContext(ctx)) }) } } + +func fallbackToPSK(ctx context.Context, psk string, next http.Handler, w http.ResponseWriter, r *http.Request, handleOptional func(code int, response codersdk.Response)) { + if psk == "" { + handleOptional(http.StatusUnauthorized, codersdk.Response{ + Message: "External provisioner daemons not enabled", + }) + return + } + + token := r.Header.Get(codersdk.ProvisionerDaemonPSK) + if subtle.ConstantTimeCompare([]byte(token), []byte(psk)) != 1 { + handleOptional(http.StatusUnauthorized, codersdk.Response{ + Message: "provisioner daemon psk invalid", + }) + return + } + + // The PSK does not indicate a specific provisioner daemon. So just + // store a boolean so the caller can check if the request is from an + // authenticated provisioner daemon. + ctx = context.WithValue(ctx, provisionerDaemonContextKey{}, true) + // nolint:gocritic // Authenticating as a provisioner daemon. + ctx = dbauthz.AsProvisionerd(ctx) + next.ServeHTTP(w, r.WithContext(ctx)) +} diff --git a/coderd/provisionerkey/provisionerkey.go b/coderd/provisionerkey/provisionerkey.go index 4df23125be2d3..f8056c0fa2670 100644 --- a/coderd/provisionerkey/provisionerkey.go +++ b/coderd/provisionerkey/provisionerkey.go @@ -3,6 +3,7 @@ package provisionerkey import ( "crypto/sha256" "fmt" + "strings" "github.com/google/uuid" "golang.org/x/xerrors" @@ -18,7 +19,7 @@ func New(organizationID uuid.UUID, name string) (database.InsertProvisionerKeyPa if err != nil { return database.InsertProvisionerKeyParams{}, "", xerrors.Errorf("generate token: %w", err) } - hashedSecret := sha256.Sum256([]byte(secret)) + hashedSecret := HashSecret(secret) token := fmt.Sprintf("%s:%s", id, secret) return database.InsertProvisionerKeyParams{ @@ -26,6 +27,25 @@ func New(organizationID uuid.UUID, name string) (database.InsertProvisionerKeyPa CreatedAt: dbtime.Now(), OrganizationID: organizationID, Name: name, - HashedSecret: hashedSecret[:], + HashedSecret: hashedSecret, }, token, nil } + +func Parse(token string) (uuid.UUID, string, error) { + parts := strings.Split(token, ":") + if len(parts) != 2 { + return uuid.UUID{}, "", xerrors.Errorf("invalid token format") + } + + id, err := uuid.Parse(parts[0]) + if err != nil { + return uuid.UUID{}, "", xerrors.Errorf("parse id: %w", err) + } + + return id, parts[1], nil +} + +func HashSecret(secret string) []byte { + h := sha256.Sum256([]byte(secret)) + return h[:] +} diff --git a/codersdk/client.go b/codersdk/client.go index f1ac87981759b..cf013a25c3ce8 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -79,6 +79,9 @@ const ( // ProvisionerDaemonPSK contains the authentication pre-shared key for an external provisioner daemon ProvisionerDaemonPSK = "Coder-Provisioner-Daemon-PSK" + // ProvisionerDaemonKey contains the authentication key for an external provisioner daemon + ProvisionerDaemonKey = "Coder-Provisioner-Daemon-Key" + // BuildVersionHeader contains build information of Coder. BuildVersionHeader = "X-Coder-Build-Version" diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index d6a8ba1e6f2fe..38ab975d7db9f 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -189,6 +189,8 @@ type ServeProvisionerDaemonRequest struct { Tags map[string]string `json:"tags"` // PreSharedKey is an authentication key to use on the API instead of the normal session token from the client. PreSharedKey string `json:"pre_shared_key"` + // ProvisionerKey is an authentication key to use on the API instead of the normal session token from the client. + ProvisionerKey string `json:"provisioner_key"` } // ServeProvisionerDaemon returns the gRPC service for a provisioner daemon @@ -223,7 +225,12 @@ func (c *Client) ServeProvisionerDaemon(ctx context.Context, req ServeProvisione headers := http.Header{} headers.Set(BuildVersionHeader, buildinfo.Version()) - if req.PreSharedKey == "" { + // nolint:gocritic // Need to support multiple exclusive auth flows. + if req.ProvisionerKey != "" { + headers.Set(ProvisionerDaemonKey, req.ProvisionerKey) + } else if req.PreSharedKey != "" { + headers.Set(ProvisionerDaemonPSK, req.PreSharedKey) + } else { // use session token if we don't have a PSK. jar, err := cookiejar.New(nil) if err != nil { @@ -234,8 +241,6 @@ func (c *Client) ServeProvisionerDaemon(ctx context.Context, req ServeProvisione Value: c.SessionToken(), }}) httpClient.Jar = jar - } else { - headers.Set(ProvisionerDaemonPSK, req.PreSharedKey) } conn, res, err := websocket.Dial(ctx, serverURL.String(), &websocket.DialOptions{ diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 784695a7ac2e3..d57fe22cf55af 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -284,9 +284,11 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { api.provisionerDaemonsEnabledMW, apiKeyMiddlewareOptional, httpmw.ExtractProvisionerDaemonAuthenticated(httpmw.ExtractProvisionerAuthConfig{ - DB: api.Database, - Optional: true, - }, api.ProvisionerDaemonPSK), + DB: api.Database, + Optional: true, + PSK: api.ProvisionerDaemonPSK, + MultiOrgEnabled: api.AGPL.Experiments.Enabled(codersdk.ExperimentMultiOrganization), + }), // Either a user auth or provisioner auth is required // to move forward. httpmw.RequireAPIKeyOrProvisionerDaemonAuth(), diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index e74f2821092b9..34916358460a9 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -79,6 +79,7 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { type provisionerDaemonAuth struct { psk string + db database.Store authorizer rbac.Authorizer } @@ -101,14 +102,21 @@ func (p *provisionerDaemonAuth) authorize(r *http.Request, orgID uuid.UUID, tags } } - // Check for PSK + // Check for provisioner key or PSK auth. provAuth := httpmw.ProvisionerDaemonAuthenticated(r) - if provAuth { - // If using PSK auth, the daemon is, by definition, scoped to the organization. - tags = provisionersdk.MutateTags(uuid.Nil, tags) - return tags, true + if !provAuth { + return nil, false } - return nil, false + + // ensure provisioner daemon subject can read organization + _, err := p.db.GetOrganizationByID(ctx, orgID) + if err != nil { + return nil, false + } + + // If using provisioner key / PSK auth, the daemon is, by definition, scoped to the organization. + tags = provisionersdk.MutateTags(uuid.Nil, tags) + return tags, true } // Serves the provisioner daemon protobuf API over a WebSocket. @@ -209,7 +217,7 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) ) authCtx := ctx - if r.Header.Get(codersdk.ProvisionerDaemonPSK) != "" { + if r.Header.Get(codersdk.ProvisionerDaemonPSK) != "" || r.Header.Get(codersdk.ProvisionerDaemonKey) != "" { //nolint:gocritic // PSK auth means no actor in request, // so use system restricted. authCtx = dbauthz.AsSystemRestricted(ctx) From 6837b572d07e7d4a7a40e6e811935853fd693208 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 22 Jul 2024 18:48:44 +0000 Subject: [PATCH 02/10] fix panic --- enterprise/coderd/coderd.go | 1 + 1 file changed, 1 insertion(+) diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index d57fe22cf55af..05263f339b10a 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -109,6 +109,7 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { provisionerDaemonAuth: &provisionerDaemonAuth{ psk: options.ProvisionerDaemonPSK, authorizer: options.Authorizer, + db: options.Database, }, } // This must happen before coderd initialization! From 56a880d608ad71a2db22f825ad484a16d53442a4 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Tue, 23 Jul 2024 16:30:54 +0000 Subject: [PATCH 03/10] tests --- coderd/database/dbauthz/dbauthz.go | 1 + coderd/httpmw/provisionerdaemon.go | 17 ++- codersdk/provisionerdaemons.go | 13 +- enterprise/coderd/provisionerdaemons_test.go | 148 +++++++++++++++++++ 4 files changed, 167 insertions(+), 12 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index a5d08493f0846..a06d6a9db90e8 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -260,6 +260,7 @@ var ( rbac.ResourceOrganization.Type: {policy.ActionCreate, policy.ActionRead}, rbac.ResourceOrganizationMember.Type: {policy.ActionCreate}, rbac.ResourceProvisionerDaemon.Type: {policy.ActionCreate, policy.ActionUpdate}, + rbac.ResourceProvisionerKeys.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionDelete}, rbac.ResourceUser.Type: rbac.ResourceUser.AvailableActions(), rbac.ResourceWorkspaceDormant.Type: {policy.ActionUpdate, policy.ActionDelete, policy.ActionWorkspaceStop}, rbac.ResourceWorkspace.Type: {policy.ActionUpdate, policy.ActionDelete, policy.ActionWorkspaceStart, policy.ActionWorkspaceStop, policy.ActionSSH}, diff --git a/coderd/httpmw/provisionerdaemon.go b/coderd/httpmw/provisionerdaemon.go index 7caa621fa3a05..396517ce513a5 100644 --- a/coderd/httpmw/provisionerdaemon.go +++ b/coderd/httpmw/provisionerdaemon.go @@ -40,6 +40,13 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig) fu } if !opts.MultiOrgEnabled { + if opts.PSK == "" { + handleOptional(http.StatusUnauthorized, codersdk.Response{ + Message: "External provisioner daemons not enabled", + }) + return + } + fallbackToPSK(ctx, opts.PSK, next, w, r, handleOptional) return } @@ -65,7 +72,8 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig) fu return } - pk, err := opts.DB.GetProvisionerKeyByID(ctx, id) + // nolint:gocritic // System must check if the provisioner key is valid. + pk, err := opts.DB.GetProvisionerKeyByID(dbauthz.AsSystemRestricted(ctx), id) if err != nil { if httpapi.Is404Error(err) { handleOptional(http.StatusUnauthorized, codersdk.Response{ @@ -99,13 +107,6 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig) fu } func fallbackToPSK(ctx context.Context, psk string, next http.Handler, w http.ResponseWriter, r *http.Request, handleOptional func(code int, response codersdk.Response)) { - if psk == "" { - handleOptional(http.StatusUnauthorized, codersdk.Response{ - Message: "External provisioner daemons not enabled", - }) - return - } - token := r.Header.Get(codersdk.ProvisionerDaemonPSK) if subtle.ConstantTimeCompare([]byte(token), []byte(psk)) != 1 { handleOptional(http.StatusUnauthorized, codersdk.Response{ diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index 38ab975d7db9f..b1269e1156656 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -225,13 +225,18 @@ func (c *Client) ServeProvisionerDaemon(ctx context.Context, req ServeProvisione headers := http.Header{} headers.Set(BuildVersionHeader, buildinfo.Version()) - // nolint:gocritic // Need to support multiple exclusive auth flows. + + if req.ProvisionerKey != "" && req.PreSharedKey != "" { + return nil, xerrors.Errorf("cannot provide both a provisioner key and a pre-shared key") + } if req.ProvisionerKey != "" { headers.Set(ProvisionerDaemonKey, req.ProvisionerKey) - } else if req.PreSharedKey != "" { + } + if req.PreSharedKey != "" { headers.Set(ProvisionerDaemonPSK, req.PreSharedKey) - } else { - // use session token if we don't have a PSK. + } + if req.ProvisionerKey == "" && req.PreSharedKey == "" { + // use session token if we don't have a PSK or provisioner key. jar, err := cookiejar.New(nil) if err != nil { return nil, xerrors.Errorf("create cookie jar: %w", err) diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index c7c256f041c8b..5325443acb43d 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -18,6 +18,8 @@ import ( "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/provisionerkey" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" @@ -552,6 +554,152 @@ func TestProvisionerDaemonServe(t *testing.T) { require.NoError(t, err) require.Len(t, daemons, 0) }) + + t.Run("ProvisionerKeyAuth", func(t *testing.T) { + t.Parallel() + + insertParams, token, err := provisionerkey.New(uuid.Nil, "dont-TEST-me") + require.NoError(t, err) + + tcs := []struct { + name string + psk string + multiOrgFeatureEnabled bool + multiOrgExperimentEnabled bool + insertParams database.InsertProvisionerKeyParams + requestProvisionerKey string + requestPSK string + errStatusCode int + }{ + { + name: "MultiOrgDisabledPSKAuthOK", + psk: "provisionersftw", + requestPSK: "provisionersftw", + }, + { + name: "MultiOrgExperimentDisabledPSKAuthOK", + multiOrgFeatureEnabled: true, + psk: "provisionersftw", + requestPSK: "provisionersftw", + }, + { + name: "MultiOrgFeatureDisabledPSKAuthOK", + multiOrgExperimentEnabled: true, + psk: "provisionersftw", + requestPSK: "provisionersftw", + }, + { + name: "MultiOrgEnabledPSKAuthOK", + psk: "provisionersftw", + multiOrgFeatureEnabled: true, + multiOrgExperimentEnabled: true, + requestPSK: "provisionersftw", + }, + { + name: "MultiOrgEnabledKeyAuthOK", + psk: "provisionersftw", + multiOrgFeatureEnabled: true, + multiOrgExperimentEnabled: true, + insertParams: insertParams, + requestProvisionerKey: token, + }, + { + name: "MultiOrgEnabledPSKAuthDisabled", + multiOrgFeatureEnabled: true, + multiOrgExperimentEnabled: true, + requestPSK: "provisionersftw", + errStatusCode: http.StatusUnauthorized, + }, + { + name: "WrongKey", + multiOrgFeatureEnabled: true, + multiOrgExperimentEnabled: true, + requestProvisionerKey: "provisionersftw", + errStatusCode: http.StatusUnauthorized, + }, + { + name: "IdOKKeyWrong", + multiOrgFeatureEnabled: true, + multiOrgExperimentEnabled: true, + requestProvisionerKey: insertParams.ID.String() + ":" + "wrong", + errStatusCode: http.StatusUnauthorized, + }, + { + name: "IdWrongKeyOK", + multiOrgFeatureEnabled: true, + multiOrgExperimentEnabled: true, + requestProvisionerKey: uuid.NewString() + ":" + token, + errStatusCode: http.StatusUnauthorized, + }, + { + name: "TokenOnly", + multiOrgFeatureEnabled: true, + multiOrgExperimentEnabled: true, + requestProvisionerKey: token, + errStatusCode: http.StatusUnauthorized, + }, + } + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + features := license.Features{ + codersdk.FeatureExternalProvisionerDaemons: 1, + } + if tc.multiOrgFeatureEnabled { + features[codersdk.FeatureMultipleOrganizations] = 1 + } + dv := coderdtest.DeploymentValues(t) + if tc.multiOrgExperimentEnabled { + dv.Experiments.Append(string(codersdk.ExperimentMultiOrganization)) + } + client, db, user := coderdenttest.NewWithDatabase(t, &coderdenttest.Options{ + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: features, + }, + ProvisionerDaemonPSK: tc.psk, + Options: &coderdtest.Options{ + DeploymentValues: dv, + }, + }) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + if tc.insertParams.Name != "" { + tc.insertParams.OrganizationID = user.OrganizationID + // nolint:gocritic // test + _, err := db.InsertProvisionerKey(dbauthz.AsSystemRestricted(ctx), tc.insertParams) + require.NoError(t, err) + } + + another := codersdk.New(client.URL) + srv, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + ID: uuid.New(), + Name: testutil.MustRandString(t, 63), + Organization: user.OrganizationID, + Provisioners: []codersdk.ProvisionerType{ + codersdk.ProvisionerTypeEcho, + }, + Tags: map[string]string{ + provisionersdk.TagScope: provisionersdk.ScopeOrganization, + }, + PreSharedKey: tc.requestPSK, + ProvisionerKey: tc.requestProvisionerKey, + }) + if tc.errStatusCode != 0 { + require.Error(t, err) + var apiError *codersdk.Error + require.ErrorAs(t, err, &apiError) + require.Equal(t, http.StatusUnauthorized, apiError.StatusCode()) + return + } + + require.NoError(t, err) + err = srv.DRPCConn().Close() + require.NoError(t, err) + }) + } + }) } func TestGetProvisionerDaemons(t *testing.T) { From 32ff0009f477cf65f90e8aa3fa54aebcf43c3416 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Tue, 23 Jul 2024 16:50:37 +0000 Subject: [PATCH 04/10] limit on api side --- coderd/httpmw/provisionerdaemon.go | 7 +++++ codersdk/provisionerdaemons.go | 3 -- enterprise/coderd/provisionerdaemons_test.go | 29 ++++++++++++++++++-- 3 files changed, 33 insertions(+), 6 deletions(-) diff --git a/coderd/httpmw/provisionerdaemon.go b/coderd/httpmw/provisionerdaemon.go index 396517ce513a5..24eb40e0011ea 100644 --- a/coderd/httpmw/provisionerdaemon.go +++ b/coderd/httpmw/provisionerdaemon.go @@ -51,6 +51,7 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig) fu return } + psk := r.Header.Get(codersdk.ProvisionerDaemonPSK) key := r.Header.Get(codersdk.ProvisionerDaemonKey) if key == "" { if opts.PSK == "" { @@ -63,6 +64,12 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig) fu fallbackToPSK(ctx, opts.PSK, next, w, r, handleOptional) return } + if psk != "" { + handleOptional(http.StatusBadRequest, codersdk.Response{ + Message: "provisioner daemon key and psk provided, but only one is allowed", + }) + return + } id, keyValue, err := provisionerkey.Parse(key) if err != nil { diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index b1269e1156656..e8be78525d6e6 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -226,9 +226,6 @@ func (c *Client) ServeProvisionerDaemon(ctx context.Context, req ServeProvisione headers.Set(BuildVersionHeader, buildinfo.Version()) - if req.ProvisionerKey != "" && req.PreSharedKey != "" { - return nil, xerrors.Errorf("cannot provide both a provisioner key and a pre-shared key") - } if req.ProvisionerKey != "" { headers.Set(ProvisionerDaemonKey, req.ProvisionerKey) } diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index 5325443acb43d..139a97199ee92 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "strings" "testing" "github.com/google/uuid" @@ -614,28 +615,50 @@ func TestProvisionerDaemonServe(t *testing.T) { name: "WrongKey", multiOrgFeatureEnabled: true, multiOrgExperimentEnabled: true, + insertParams: insertParams, requestProvisionerKey: "provisionersftw", errStatusCode: http.StatusUnauthorized, }, { - name: "IdOKKeyWrong", + name: "IdOKKeyValueWrong", multiOrgFeatureEnabled: true, multiOrgExperimentEnabled: true, + insertParams: insertParams, requestProvisionerKey: insertParams.ID.String() + ":" + "wrong", errStatusCode: http.StatusUnauthorized, }, { - name: "IdWrongKeyOK", + name: "IdWrongKeyValueOK", multiOrgFeatureEnabled: true, multiOrgExperimentEnabled: true, + insertParams: insertParams, requestProvisionerKey: uuid.NewString() + ":" + token, errStatusCode: http.StatusUnauthorized, }, { - name: "TokenOnly", + name: "KeyValueOnly", + multiOrgFeatureEnabled: true, + multiOrgExperimentEnabled: true, + insertParams: insertParams, + requestProvisionerKey: strings.Split(token, ":")[1], + errStatusCode: http.StatusUnauthorized, + }, + { + name: "KeyAndPSK", multiOrgFeatureEnabled: true, multiOrgExperimentEnabled: true, + psk: "provisionersftw", + insertParams: insertParams, requestProvisionerKey: token, + requestPSK: "provisionersftw", + errStatusCode: http.StatusUnauthorized, + }, + { + name: "None", + multiOrgFeatureEnabled: true, + multiOrgExperimentEnabled: true, + psk: "provisionersftw", + insertParams: insertParams, errStatusCode: http.StatusUnauthorized, }, } From c9376a0834288c1d4f51fffe9b8d331955c15a30 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Wed, 24 Jul 2024 16:19:39 +0000 Subject: [PATCH 05/10] revert rbac changes --- coderd/database/dbauthz/dbauthz.go | 75 +++++++++---------------- coderd/httpmw/provisionerdaemon.go | 15 ++++- coderd/provisionerkey/provisionerkey.go | 5 ++ enterprise/coderd/provisionerdaemons.go | 9 +-- 4 files changed, 49 insertions(+), 55 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index a06d6a9db90e8..1dab5dbf96172 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -158,49 +158,34 @@ func ActorFromContext(ctx context.Context) (rbac.Subject, bool) { } var ( - subjectProvisionerd = func(orgID uuid.UUID) rbac.Subject { - sitePermissions := map[string][]policy.Action{ - // TODO: Add ProvisionerJob resource type. - rbac.ResourceFile.Type: {policy.ActionRead}, - rbac.ResourceSystem.Type: {policy.WildcardSymbol}, - rbac.ResourceTemplate.Type: {policy.ActionRead, policy.ActionUpdate}, - // Unsure why provisionerd needs update and read personal - rbac.ResourceUser.Type: {policy.ActionRead, policy.ActionReadPersonal, policy.ActionUpdatePersonal}, - rbac.ResourceWorkspaceDormant.Type: {policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceStop}, - rbac.ResourceWorkspace.Type: {policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceStart, policy.ActionWorkspaceStop}, - rbac.ResourceApiKey.Type: {policy.WildcardSymbol}, - // When org scoped provisioner credentials are implemented, - // this can be reduced to read a specific org. - rbac.ResourceOrganization.Type: {policy.ActionRead}, - rbac.ResourceGroup.Type: {policy.ActionRead}, - } - orgPermissions := map[string][]rbac.Permission{} - - if orgID != uuid.Nil { - // replace site wide org permissions with org scoped permissions - delete(sitePermissions, rbac.ResourceOrganization.Type) - orgPermissions = map[string][]rbac.Permission{ - orgID.String(): rbac.Permissions(map[string][]policy.Action{ + subjectProvisionerd = rbac.Subject{ + FriendlyName: "Provisioner Daemon", + ID: uuid.Nil.String(), + Roles: rbac.Roles([]rbac.Role{ + { + Identifier: rbac.RoleIdentifier{Name: "provisionerd"}, + DisplayName: "Provisioner Daemon", + Site: rbac.Permissions(map[string][]policy.Action{ + // TODO: Add ProvisionerJob resource type. + rbac.ResourceFile.Type: {policy.ActionRead}, + rbac.ResourceSystem.Type: {policy.WildcardSymbol}, + rbac.ResourceTemplate.Type: {policy.ActionRead, policy.ActionUpdate}, + // Unsure why provisionerd needs update and read personal + rbac.ResourceUser.Type: {policy.ActionRead, policy.ActionReadPersonal, policy.ActionUpdatePersonal}, + rbac.ResourceWorkspaceDormant.Type: {policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceStop}, + rbac.ResourceWorkspace.Type: {policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceStart, policy.ActionWorkspaceStop}, + rbac.ResourceApiKey.Type: {policy.WildcardSymbol}, + // When org scoped provisioner credentials are implemented, + // this can be reduced to read a specific org. rbac.ResourceOrganization.Type: {policy.ActionRead}, + rbac.ResourceGroup.Type: {policy.ActionRead}, }), - } - } - - return rbac.Subject{ - FriendlyName: "Provisioner Daemon", - ID: uuid.Nil.String(), - Roles: rbac.Roles([]rbac.Role{ - { - Identifier: rbac.RoleIdentifier{Name: "provisionerd"}, - DisplayName: "Provisioner Daemon", - Site: rbac.Permissions(sitePermissions), - Org: orgPermissions, - User: []rbac.Permission{}, - }, - }), - Scope: rbac.ScopeAll, - }.WithCachedASTValue() - } + Org: map[string][]rbac.Permission{}, + User: []rbac.Permission{}, + }, + }), + Scope: rbac.ScopeAll, + }.WithCachedASTValue() subjectAutostart = rbac.Subject{ FriendlyName: "Autostart", @@ -277,13 +262,7 @@ var ( // AsProvisionerd returns a context with an actor that has permissions required // for provisionerd to function. func AsProvisionerd(ctx context.Context) context.Context { - return context.WithValue(ctx, authContextKey{}, subjectProvisionerd(uuid.Nil)) -} - -// AsProvisionerd returns a context with an actor that has permissions required -// for an org scoped provisionerd to function. -func AsOrganizationProvisionerd(ctx context.Context, orgID uuid.UUID) context.Context { - return context.WithValue(ctx, authContextKey{}, subjectProvisionerd(orgID)) + return context.WithValue(ctx, authContextKey{}, subjectProvisionerd) } // AsAutostart returns a context with an actor that has permissions required diff --git a/coderd/httpmw/provisionerdaemon.go b/coderd/httpmw/provisionerdaemon.go index 24eb40e0011ea..243af82598ff8 100644 --- a/coderd/httpmw/provisionerdaemon.go +++ b/coderd/httpmw/provisionerdaemon.go @@ -95,24 +95,33 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig) fu return } - if subtle.ConstantTimeCompare(pk.HashedSecret, provisionerkey.HashSecret(keyValue)) != 1 { + if provisionerkey.Compare(pk.HashedSecret, provisionerkey.HashSecret(keyValue)) { handleOptional(http.StatusUnauthorized, codersdk.Response{ Message: "provisioner daemon key invalid", }) return } - // The PSK does not indicate a specific provisioner daemon. So just + // The provisioner key does not indicate a specific provisioner daemon. So just // store a boolean so the caller can check if the request is from an // authenticated provisioner daemon. ctx = context.WithValue(ctx, provisionerDaemonContextKey{}, true) + // store key used to authenticate the request + ctx = context.WithValue(ctx, provisionerKeyAuthContextKey{}, pk) // nolint:gocritic // Authenticating as a provisioner daemon. - ctx = dbauthz.AsOrganizationProvisionerd(ctx, pk.OrganizationID) + ctx = dbauthz.AsProvisionerd(ctx) next.ServeHTTP(w, r.WithContext(ctx)) }) } } +type provisionerKeyAuthContextKey struct{} + +func ProvisionerKeyAuthOptional(r *http.Request) (database.ProvisionerKey, bool) { + user, ok := r.Context().Value(provisionerKeyAuthContextKey{}).(database.ProvisionerKey) + return user, ok +} + func fallbackToPSK(ctx context.Context, psk string, next http.Handler, w http.ResponseWriter, r *http.Request, handleOptional func(code int, response codersdk.Response)) { token := r.Header.Get(codersdk.ProvisionerDaemonPSK) if subtle.ConstantTimeCompare([]byte(token), []byte(psk)) != 1 { diff --git a/coderd/provisionerkey/provisionerkey.go b/coderd/provisionerkey/provisionerkey.go index f8056c0fa2670..70354c140b73e 100644 --- a/coderd/provisionerkey/provisionerkey.go +++ b/coderd/provisionerkey/provisionerkey.go @@ -2,6 +2,7 @@ package provisionerkey import ( "crypto/sha256" + "crypto/subtle" "fmt" "strings" @@ -49,3 +50,7 @@ func HashSecret(secret string) []byte { h := sha256.Sum256([]byte(secret)) return h[:] } + +func Compare(a []byte, b []byte) bool { + return subtle.ConstantTimeCompare(a, b) != 1 +} diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 34916358460a9..dbb9d252c07c1 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -108,10 +108,11 @@ func (p *provisionerDaemonAuth) authorize(r *http.Request, orgID uuid.UUID, tags return nil, false } - // ensure provisioner daemon subject can read organization - _, err := p.db.GetOrganizationByID(ctx, orgID) - if err != nil { - return nil, false + pk, ok := httpmw.ProvisionerKeyAuthOptional(r) + if ok { + if orgID != pk.OrganizationID { + return nil, false + } } // If using provisioner key / PSK auth, the daemon is, by definition, scoped to the organization. From 1e9b79a7858944c3a041e3ecd32a807c6f513180 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Wed, 24 Jul 2024 16:34:18 +0000 Subject: [PATCH 06/10] move to mw --- coderd/httpmw/provisionerdaemon.go | 8 ++++++++ enterprise/coderd/coderd.go | 2 +- enterprise/coderd/provisionerdaemons.go | 7 ------- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/coderd/httpmw/provisionerdaemon.go b/coderd/httpmw/provisionerdaemon.go index 243af82598ff8..45c8a62a5d183 100644 --- a/coderd/httpmw/provisionerdaemon.go +++ b/coderd/httpmw/provisionerdaemon.go @@ -30,6 +30,7 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig) fu return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() + org := OrganizationParam(r) handleOptional := func(code int, response codersdk.Response) { if opts.Optional { @@ -102,6 +103,13 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig) fu return } + if pk.OrganizationID != org.ID { + handleOptional(http.StatusUnauthorized, codersdk.Response{ + Message: "provisioner daemon key invalid", + }) + return + } + // The provisioner key does not indicate a specific provisioner daemon. So just // store a boolean so the caller can check if the request is from an // authenticated provisioner daemon. diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 05263f339b10a..de0b5d49eaf9f 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -284,6 +284,7 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { r.Use( api.provisionerDaemonsEnabledMW, apiKeyMiddlewareOptional, + httpmw.ExtractOrganizationParam(api.Database), httpmw.ExtractProvisionerDaemonAuthenticated(httpmw.ExtractProvisionerAuthConfig{ DB: api.Database, Optional: true, @@ -293,7 +294,6 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { // Either a user auth or provisioner auth is required // to move forward. httpmw.RequireAPIKeyOrProvisionerDaemonAuth(), - httpmw.ExtractOrganizationParam(api.Database), ) r.With(apiKeyMiddleware).Get("/", api.provisionerDaemons) r.With(apiKeyMiddlewareOptional).Get("/serve", api.provisionerDaemonServe) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index dbb9d252c07c1..6a94ad30659fa 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -108,13 +108,6 @@ func (p *provisionerDaemonAuth) authorize(r *http.Request, orgID uuid.UUID, tags return nil, false } - pk, ok := httpmw.ProvisionerKeyAuthOptional(r) - if ok { - if orgID != pk.OrganizationID { - return nil, false - } - } - // If using provisioner key / PSK auth, the daemon is, by definition, scoped to the organization. tags = provisionersdk.MutateTags(uuid.Nil, tags) return tags, true From 1214b6275c272dff95e440d968def49d92bc9b85 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Wed, 24 Jul 2024 17:09:36 +0000 Subject: [PATCH 07/10] move back to authorize --- coderd/httpmw/provisionerdaemon.go | 8 -------- enterprise/coderd/coderd.go | 2 +- enterprise/coderd/provisionerdaemons.go | 7 +++++++ 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/coderd/httpmw/provisionerdaemon.go b/coderd/httpmw/provisionerdaemon.go index 45c8a62a5d183..243af82598ff8 100644 --- a/coderd/httpmw/provisionerdaemon.go +++ b/coderd/httpmw/provisionerdaemon.go @@ -30,7 +30,6 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig) fu return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - org := OrganizationParam(r) handleOptional := func(code int, response codersdk.Response) { if opts.Optional { @@ -103,13 +102,6 @@ func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig) fu return } - if pk.OrganizationID != org.ID { - handleOptional(http.StatusUnauthorized, codersdk.Response{ - Message: "provisioner daemon key invalid", - }) - return - } - // The provisioner key does not indicate a specific provisioner daemon. So just // store a boolean so the caller can check if the request is from an // authenticated provisioner daemon. diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index de0b5d49eaf9f..05263f339b10a 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -284,7 +284,6 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { r.Use( api.provisionerDaemonsEnabledMW, apiKeyMiddlewareOptional, - httpmw.ExtractOrganizationParam(api.Database), httpmw.ExtractProvisionerDaemonAuthenticated(httpmw.ExtractProvisionerAuthConfig{ DB: api.Database, Optional: true, @@ -294,6 +293,7 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { // Either a user auth or provisioner auth is required // to move forward. httpmw.RequireAPIKeyOrProvisionerDaemonAuth(), + httpmw.ExtractOrganizationParam(api.Database), ) r.With(apiKeyMiddleware).Get("/", api.provisionerDaemons) r.With(apiKeyMiddlewareOptional).Get("/serve", api.provisionerDaemonServe) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 6a94ad30659fa..e6036765d477c 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -108,6 +108,13 @@ func (p *provisionerDaemonAuth) authorize(r *http.Request, orgID uuid.UUID, tags return nil, false } + pk, ok := httpmw.ProvisionerKeyAuthOptional(r) + if ok { + if pk.OrganizationID != orgID { + return nil, false + } + } + // If using provisioner key / PSK auth, the daemon is, by definition, scoped to the organization. tags = provisionersdk.MutateTags(uuid.Nil, tags) return tags, true From c7c00e70e758fc5daedbda3c2fce150d186d1478 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Wed, 24 Jul 2024 17:42:22 +0000 Subject: [PATCH 08/10] fix authorization with pk given --- enterprise/coderd/provisionerdaemons.go | 51 ++++++++++++++++--------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index e6036765d477c..3e3e28b1cba42 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -83,41 +83,53 @@ type provisionerDaemonAuth struct { authorizer rbac.Authorizer } -// authorize returns mutated tags and true if the given HTTP request is authorized to access the provisioner daemon -// protobuf API, and returns nil, false otherwise. -func (p *provisionerDaemonAuth) authorize(r *http.Request, orgID uuid.UUID, tags map[string]string) (map[string]string, bool) { +// authorize returns mutated tags if the given HTTP request is authorized to access the provisioner daemon +// protobuf API, and returns nil, err otherwise. +func (p *provisionerDaemonAuth) authorize(r *http.Request, orgID uuid.UUID, tags map[string]string) (map[string]string, error) { ctx := r.Context() - apiKey, ok := httpmw.APIKeyOptional(r) - if ok { + apiKey, apiKeyOK := httpmw.APIKeyOptional(r) + pk, pkOK := httpmw.ProvisionerKeyAuthOptional(r) + provAuth := httpmw.ProvisionerDaemonAuthenticated(r) + if !provAuth && !apiKeyOK { + return nil, xerrors.New("no API key or provisioner key provided") + } + if apiKeyOK && pkOK { + return nil, xerrors.New("Both API key and provisioner key authentication provided. Only one is allowed.") + } + + if apiKeyOK { tags = provisionersdk.MutateTags(apiKey.UserID, tags) if tags[provisionersdk.TagScope] == provisionersdk.ScopeUser { // Any authenticated user can create provisioner daemons scoped // for jobs that they own, - return tags, true + return tags, nil } ua := httpmw.UserAuthorization(r) - if err := p.authorizer.Authorize(ctx, ua, policy.ActionCreate, rbac.ResourceProvisionerDaemon.InOrg(orgID)); err == nil { - // User is allowed to create provisioner daemons - return tags, true + err := p.authorizer.Authorize(ctx, ua, policy.ActionCreate, rbac.ResourceProvisionerDaemon.InOrg(orgID)) + if err != nil { + if !provAuth { + return nil, xerrors.New("user unauthorized") + } + + // If using provisioner key / PSK auth, the daemon is, by definition, scoped to the organization. + tags = provisionersdk.MutateTags(uuid.Nil, tags) + return tags, nil } - } - // Check for provisioner key or PSK auth. - provAuth := httpmw.ProvisionerDaemonAuthenticated(r) - if !provAuth { - return nil, false + // User is allowed to create provisioner daemons + return tags, nil } pk, ok := httpmw.ProvisionerKeyAuthOptional(r) if ok { if pk.OrganizationID != orgID { - return nil, false + return nil, xerrors.New("provisioner key unauthorized") } } // If using provisioner key / PSK auth, the daemon is, by definition, scoped to the organization. tags = provisionersdk.MutateTags(uuid.Nil, tags) - return tags, true + return tags, nil } // Serves the provisioner daemon protobuf API over a WebSocket. @@ -180,12 +192,13 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) api.Logger.Warn(ctx, "unnamed provisioner daemon") } - tags, authorized := api.provisionerDaemonAuth.authorize(r, organization.ID, tags) - if !authorized { - api.Logger.Warn(ctx, "unauthorized provisioner daemon serve request", slog.F("tags", tags)) + tags, err := api.provisionerDaemonAuth.authorize(r, organization.ID, tags) + if err != nil { + api.Logger.Warn(ctx, "unauthorized provisioner daemon serve request", slog.F("tags", tags), slog.Error(err)) httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: fmt.Sprintf("You aren't allowed to create provisioner daemons with scope %q", tags[provisionersdk.TagScope]), + Detail: err.Error(), }, ) return From 5d470f00b393ae2afffec31d93287d0ec037c1c9 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Wed, 24 Jul 2024 17:48:37 +0000 Subject: [PATCH 09/10] forbid user and pk auth --- enterprise/coderd/provisionerdaemons.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 3e3e28b1cba42..b785abf6f36c5 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -111,7 +111,7 @@ func (p *provisionerDaemonAuth) authorize(r *http.Request, orgID uuid.UUID, tags return nil, xerrors.New("user unauthorized") } - // If using provisioner key / PSK auth, the daemon is, by definition, scoped to the organization. + // If using PSK auth, the daemon is, by definition, scoped to the organization. tags = provisionersdk.MutateTags(uuid.Nil, tags) return tags, nil } @@ -120,8 +120,7 @@ func (p *provisionerDaemonAuth) authorize(r *http.Request, orgID uuid.UUID, tags return tags, nil } - pk, ok := httpmw.ProvisionerKeyAuthOptional(r) - if ok { + if pkOK { if pk.OrganizationID != orgID { return nil, xerrors.New("provisioner key unauthorized") } From 1a45338396a19c04ee4d7e515b2765ca03667dab Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Wed, 24 Jul 2024 17:48:45 +0000 Subject: [PATCH 10/10] forbid user and pk auth --- enterprise/coderd/provisionerdaemons.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index b785abf6f36c5..4f9748f2d265b 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -111,6 +111,8 @@ func (p *provisionerDaemonAuth) authorize(r *http.Request, orgID uuid.UUID, tags return nil, xerrors.New("user unauthorized") } + // Allow fallback to PSK auth if the user is not allowed to create provisioner daemons. + // This is to preserve backwards compatibility with existing user provisioner daemons. // If using PSK auth, the daemon is, by definition, scoped to the organization. tags = provisionersdk.MutateTags(uuid.Nil, tags) return tags, nil