From 95b51edef1ce0aa8b5fe9765da17cbebbd5cfcae Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 30 Aug 2023 14:13:01 +0000 Subject: [PATCH 1/7] fix(provisionerd): correctly mutate default tags for PSK auth --- coderd/provisionerdserver/provisionertags.go | 4 +- .../provisionertags_test.go | 80 +++++++++++++++++++ enterprise/cli/provisionerdaemons.go | 3 + enterprise/coderd/provisionerdaemons.go | 28 ++++++- 4 files changed, 113 insertions(+), 2 deletions(-) create mode 100644 coderd/provisionerdserver/provisionertags_test.go diff --git a/coderd/provisionerdserver/provisionertags.go b/coderd/provisionerdserver/provisionertags.go index 7c9e029839d35..d9b0766bff37a 100644 --- a/coderd/provisionerdserver/provisionertags.go +++ b/coderd/provisionerdserver/provisionertags.go @@ -24,7 +24,9 @@ func MutateTags(userID uuid.UUID, tags map[string]string) map[string]string { } switch tags[TagScope] { case ScopeUser: - tags[TagOwner] = userID.String() + if userID != uuid.Nil { + tags[TagOwner] = userID.String() + } case ScopeOrganization: default: tags[TagScope] = ScopeOrganization diff --git a/coderd/provisionerdserver/provisionertags_test.go b/coderd/provisionerdserver/provisionertags_test.go new file mode 100644 index 0000000000000..464695307060c --- /dev/null +++ b/coderd/provisionerdserver/provisionertags_test.go @@ -0,0 +1,80 @@ +package provisionerdserver_test + +import ( + "encoding/json" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/provisionerdserver" +) + +func TestMutateTags(t *testing.T) { + t.Parallel() + + testUserID := uuid.New() + + for _, tt := range []struct { + name string + userID uuid.UUID + tags map[string]string + want map[string]string + }{ + { + name: "nil tags", + userID: uuid.Nil, + tags: nil, + want: map[string]string{ + provisionerdserver.TagScope: provisionerdserver.ScopeOrganization, + }, + }, + { + name: "empty tags", + userID: uuid.Nil, + tags: map[string]string{}, + want: map[string]string{ + provisionerdserver.TagScope: provisionerdserver.ScopeOrganization, + }, + }, + { + name: "user scope", + tags: map[string]string{provisionerdserver.TagScope: provisionerdserver.ScopeUser}, + userID: testUserID, + want: map[string]string{ + provisionerdserver.TagScope: provisionerdserver.ScopeUser, + provisionerdserver.TagOwner: testUserID.String(), + }, + }, + { + name: "organization scope", + tags: map[string]string{provisionerdserver.TagScope: provisionerdserver.ScopeOrganization}, + userID: testUserID, + want: map[string]string{ + provisionerdserver.TagScope: provisionerdserver.ScopeOrganization, + }, + }, + { + name: "invalid scope", + tags: map[string]string{provisionerdserver.TagScope: "360noscope"}, + userID: testUserID, + want: map[string]string{ + provisionerdserver.TagScope: provisionerdserver.ScopeOrganization, + }, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + // make a copy of the map because the function under test + // mutates the map + bytes, err := json.Marshal(tt.tags) + require.NoError(t, err) + var tags map[string]string + err = json.Unmarshal(bytes, &tags) + require.NoError(t, err) + got := provisionerdserver.MutateTags(tt.userID, tags) + require.Equal(t, tt.want, got) + }) + } +} diff --git a/enterprise/cli/provisionerdaemons.go b/enterprise/cli/provisionerdaemons.go index e46756578d542..4fe4551ffb210 100644 --- a/enterprise/cli/provisionerdaemons.go +++ b/enterprise/cli/provisionerdaemons.go @@ -83,6 +83,9 @@ func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { }() logger := slog.Make(sloghuman.Sink(inv.Stderr)) + if ok, _ := inv.ParsedFlags().GetBool("verbose"); ok { + logger = logger.Leveled(slog.LevelDebug) + } errCh := make(chan error, 1) go func() { defer cancel() diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 1129eb4004185..64748e0021609 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -117,7 +117,7 @@ func (p *provisionerDaemonAuth) authorize(r *http.Request, tags map[string]strin if p.psk != "" { psk := r.Header.Get(codersdk.ProvisionerDaemonPSK) if subtle.ConstantTimeCompare([]byte(p.psk), []byte(psk)) == 1 { - return tags, true + return provisionerdserver.MutateTags(uuid.Nil, tags), true } } return nil, false @@ -172,10 +172,12 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) tags, authorized := api.provisionerDaemonAuth.authorize(r, tags) if !authorized { + api.Logger.Warn(ctx, "unauthorized provisioner daemon serve request", slog.F("tags", tags)) httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{Message: "You aren't allowed to create provisioner daemons"}) return } + api.Logger.Debug(ctx, "provisioner authorized", slog.F("tags", tags)) provisioners := make([]database.ProvisionerType, 0) for p := range provisionersMap { @@ -196,6 +198,12 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) Tags: tags, }) if err != nil { + api.Logger.Error(ctx, "write provisioner daemon", + slog.F("name", name), + slog.F("provisioners", provisioners), + slog.F("tags", tags), + slog.Error(err), + ) httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error writing provisioner daemon.", Detail: err.Error(), @@ -205,6 +213,12 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) rawTags, err := json.Marshal(daemon.Tags) if err != nil { + api.Logger.Error(ctx, "marshal provisioner tags", + slog.F("name", name), + slog.F("provisioners", provisioners), + slog.F("tags", tags), + slog.Error(err), + ) httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error marshaling daemon tags.", Detail: err.Error(), @@ -222,6 +236,12 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) CompressionMode: websocket.CompressionDisabled, }) if err != nil { + api.Logger.Error(ctx, "accept provisioner websocket conn", + slog.F("name", name), + slog.F("provisioners", provisioners), + slog.F("tags", tags), + slog.Error(err), + ) httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Internal error accepting websocket connection.", Detail: err.Error(), @@ -267,6 +287,12 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) }, ) if err != nil { + api.Logger.Error(ctx, "create provisioner daemon server", + slog.F("name", name), + slog.F("provisioners", provisioners), + slog.F("tags", tags), + slog.Error(err), + ) _ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("create provisioner daemon server: %s", err)) return } From 4fda8188402916bd31c0627052166ef5ab473ca0 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 30 Aug 2023 15:22:47 +0000 Subject: [PATCH 2/7] address PR comments --- enterprise/coderd/provisionerdaemons.go | 33 +++++++------------------ 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 64748e0021609..3ced0ff4ee113 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -190,6 +190,11 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) } name := namesgenerator.GetRandomName(1) + log := api.Logger.With( + slog.F("name", name), + slog.F("provisioners", provisioners), + slog.F("tags", tags), + ) daemon, err := api.Database.InsertProvisionerDaemon(ctx, database.InsertProvisionerDaemonParams{ ID: uuid.New(), CreatedAt: database.Now(), @@ -198,12 +203,7 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) Tags: tags, }) if err != nil { - api.Logger.Error(ctx, "write provisioner daemon", - slog.F("name", name), - slog.F("provisioners", provisioners), - slog.F("tags", tags), - slog.Error(err), - ) + log.Error(ctx, "write provisioner daemon", slog.Error(err)) httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error writing provisioner daemon.", Detail: err.Error(), @@ -213,12 +213,7 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) rawTags, err := json.Marshal(daemon.Tags) if err != nil { - api.Logger.Error(ctx, "marshal provisioner tags", - slog.F("name", name), - slog.F("provisioners", provisioners), - slog.F("tags", tags), - slog.Error(err), - ) + log.Error(ctx, "marshal provisioner tags", slog.Error(err)) httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error marshaling daemon tags.", Detail: err.Error(), @@ -236,12 +231,7 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) CompressionMode: websocket.CompressionDisabled, }) if err != nil { - api.Logger.Error(ctx, "accept provisioner websocket conn", - slog.F("name", name), - slog.F("provisioners", provisioners), - slog.F("tags", tags), - slog.Error(err), - ) + log.Error(ctx, "accept provisioner websocket conn", slog.Error(err)) httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Internal error accepting websocket connection.", Detail: err.Error(), @@ -287,12 +277,7 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) }, ) if err != nil { - api.Logger.Error(ctx, "create provisioner daemon server", - slog.F("name", name), - slog.F("provisioners", provisioners), - slog.F("tags", tags), - slog.Error(err), - ) + log.Error(ctx, "create provisioner daemon server", slog.Error(err)) _ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("create provisioner daemon server: %s", err)) return } From 79717aa4184f0bc060bd01cd46af2d57a5b5132d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 30 Aug 2023 15:23:02 +0000 Subject: [PATCH 3/7] ignore error log in test --- enterprise/cli/provisionerdaemons_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/enterprise/cli/provisionerdaemons_test.go b/enterprise/cli/provisionerdaemons_test.go index 038ba97ec7a54..5003bfb446389 100644 --- a/enterprise/cli/provisionerdaemons_test.go +++ b/enterprise/cli/provisionerdaemons_test.go @@ -6,7 +6,10 @@ import ( "github.com/stretchr/testify/require" + "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/cli/clitest" + "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" "github.com/coder/coder/v2/enterprise/coderd/license" @@ -17,7 +20,13 @@ import ( func TestProvisionerDaemon_PSK(t *testing.T) { t.Parallel() + logger := slogtest.Make(t, &slogtest.Options{ + IgnoreErrors: true, + }) client, _ := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + Logger: &logger, + }, ProvisionerDaemonPSK: "provisionersftw", LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ @@ -38,7 +47,13 @@ func TestProvisionerDaemon_PSK(t *testing.T) { func TestProvisionerDaemon_SessionToken(t *testing.T) { t.Parallel() + logger := slogtest.Make(t, &slogtest.Options{ + IgnoreErrors: true, + }) client, _ := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + Logger: &logger, + }, ProvisionerDaemonPSK: "provisionersftw", LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ From 89030f2885c28706db2b9692e5806332587dac74 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 31 Aug 2023 09:08:11 +0100 Subject: [PATCH 4/7] do not log context.Canceled errors --- enterprise/cli/provisionerdaemons_test.go | 15 --------------- enterprise/coderd/provisionerdaemons.go | 16 ++++++++++++---- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/enterprise/cli/provisionerdaemons_test.go b/enterprise/cli/provisionerdaemons_test.go index 5003bfb446389..038ba97ec7a54 100644 --- a/enterprise/cli/provisionerdaemons_test.go +++ b/enterprise/cli/provisionerdaemons_test.go @@ -6,10 +6,7 @@ import ( "github.com/stretchr/testify/require" - "cdr.dev/slog/sloggers/slogtest" - "github.com/coder/coder/v2/cli/clitest" - "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" "github.com/coder/coder/v2/enterprise/coderd/license" @@ -20,13 +17,7 @@ import ( func TestProvisionerDaemon_PSK(t *testing.T) { t.Parallel() - logger := slogtest.Make(t, &slogtest.Options{ - IgnoreErrors: true, - }) client, _ := coderdenttest.New(t, &coderdenttest.Options{ - Options: &coderdtest.Options{ - Logger: &logger, - }, ProvisionerDaemonPSK: "provisionersftw", LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ @@ -47,13 +38,7 @@ func TestProvisionerDaemon_PSK(t *testing.T) { func TestProvisionerDaemon_SessionToken(t *testing.T) { t.Parallel() - logger := slogtest.Make(t, &slogtest.Options{ - IgnoreErrors: true, - }) client, _ := coderdenttest.New(t, &coderdenttest.Options{ - Options: &coderdtest.Options{ - Logger: &logger, - }, ProvisionerDaemonPSK: "provisionersftw", LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 3ced0ff4ee113..28a49a97a5a31 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -203,7 +203,9 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) Tags: tags, }) if err != nil { - log.Error(ctx, "write provisioner daemon", slog.Error(err)) + if !xerrors.Is(err, context.Canceled) { + log.Error(ctx, "write provisioner daemon", slog.Error(err)) + } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error writing provisioner daemon.", Detail: err.Error(), @@ -213,7 +215,9 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) rawTags, err := json.Marshal(daemon.Tags) if err != nil { - log.Error(ctx, "marshal provisioner tags", slog.Error(err)) + if !xerrors.Is(err, context.Canceled) { + log.Error(ctx, "marshal provisioner tags", slog.Error(err)) + } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error marshaling daemon tags.", Detail: err.Error(), @@ -231,7 +235,9 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) CompressionMode: websocket.CompressionDisabled, }) if err != nil { - log.Error(ctx, "accept provisioner websocket conn", slog.Error(err)) + if !xerrors.Is(err, context.Canceled) { + log.Error(ctx, "accept provisioner websocket conn", slog.Error(err)) + } httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Internal error accepting websocket connection.", Detail: err.Error(), @@ -277,7 +283,9 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) }, ) if err != nil { - log.Error(ctx, "create provisioner daemon server", slog.Error(err)) + if !xerrors.Is(err, context.Canceled) { + log.Error(ctx, "create provisioner daemon server", slog.Error(err)) + } _ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("create provisioner daemon server: %s", err)) return } From 3e1a94b4deacfaaa9cb919ebab90dd85952cdbca Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 31 Aug 2023 10:03:13 +0000 Subject: [PATCH 5/7] address PR comment --- enterprise/coderd/provisionerdaemons.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 28a49a97a5a31..964eea012dbf6 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -117,7 +117,14 @@ func (p *provisionerDaemonAuth) authorize(r *http.Request, tags map[string]strin if p.psk != "" { psk := r.Header.Get(codersdk.ProvisionerDaemonPSK) if subtle.ConstantTimeCompare([]byte(p.psk), []byte(psk)) == 1 { - return provisionerdserver.MutateTags(uuid.Nil, tags), true + if len(tags) == 0 { + // Directly scope to organization if no tags are provided. + // MutateTags is only meant for scoping based on users. + tags = map[string]string{ + provisionerdserver.TagScope: provisionerdserver.ScopeOrganization, + } + } + return tags, true } } return nil, false From 34ef0a7bda76d65438ed590d31fedc5402443af2 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 1 Sep 2023 09:57:58 +0100 Subject: [PATCH 6/7] unconditionally set tag scope to org for psk auth --- coderd/provisionerdserver/provisionertags.go | 4 +--- enterprise/coderd/provisionerdaemons.go | 9 ++------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/coderd/provisionerdserver/provisionertags.go b/coderd/provisionerdserver/provisionertags.go index d9b0766bff37a..7c9e029839d35 100644 --- a/coderd/provisionerdserver/provisionertags.go +++ b/coderd/provisionerdserver/provisionertags.go @@ -24,9 +24,7 @@ func MutateTags(userID uuid.UUID, tags map[string]string) map[string]string { } switch tags[TagScope] { case ScopeUser: - if userID != uuid.Nil { - tags[TagOwner] = userID.String() - } + tags[TagOwner] = userID.String() case ScopeOrganization: default: tags[TagScope] = ScopeOrganization diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 964eea012dbf6..7ee881839ef57 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -117,13 +117,8 @@ func (p *provisionerDaemonAuth) authorize(r *http.Request, tags map[string]strin if p.psk != "" { psk := r.Header.Get(codersdk.ProvisionerDaemonPSK) if subtle.ConstantTimeCompare([]byte(p.psk), []byte(psk)) == 1 { - if len(tags) == 0 { - // Directly scope to organization if no tags are provided. - // MutateTags is only meant for scoping based on users. - tags = map[string]string{ - provisionerdserver.TagScope: provisionerdserver.ScopeOrganization, - } - } + // If using PSK auth, the daemon is, by definition, scoped to the organization. + tags[provisionerdserver.TagScope] = provisionerdserver.ScopeOrganization return tags, true } } From 475311aa204af4d32119c89ac3affc39b58f6241 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 1 Sep 2023 10:19:52 +0100 Subject: [PATCH 7/7] cli: add some informational logging statements around provisionerd tags --- enterprise/cli/provisionerdaemons.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/enterprise/cli/provisionerdaemons.go b/enterprise/cli/provisionerdaemons.go index 4fe4551ffb210..00b2c9dafdf95 100644 --- a/enterprise/cli/provisionerdaemons.go +++ b/enterprise/cli/provisionerdaemons.go @@ -15,6 +15,7 @@ import ( "github.com/coder/coder/v2/cli/clibase" "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/provisionerdserver" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/provisioner/terraform" "github.com/coder/coder/v2/provisionerd" @@ -65,6 +66,23 @@ func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { return err } + logger := slog.Make(sloghuman.Sink(inv.Stderr)) + if ok, _ := inv.ParsedFlags().GetBool("verbose"); ok { + logger = logger.Leveled(slog.LevelDebug) + } + + if len(tags) != 0 { + logger.Info(ctx, "note: tagged provisioners can currently pick up jobs from untagged templates") + logger.Info(ctx, "see https://github.com/coder/coder/issues/6442 for details") + } + + // When authorizing with a PSK, we automatically scope the provisionerd + // to organization. Scoping to user with PSK auth is not a valid configuration. + if preSharedKey != "" { + logger.Info(ctx, "psk auth automatically sets tag "+provisionerdserver.TagScope+"="+provisionerdserver.ScopeOrganization) + tags[provisionerdserver.TagScope] = provisionerdserver.ScopeOrganization + } + err = os.MkdirAll(cacheDir, 0o700) if err != nil { return xerrors.Errorf("mkdir %q: %w", cacheDir, err) @@ -82,10 +100,6 @@ func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { _ = terraformServer.Close() }() - logger := slog.Make(sloghuman.Sink(inv.Stderr)) - if ok, _ := inv.ParsedFlags().GetBool("verbose"); ok { - logger = logger.Leveled(slog.LevelDebug) - } errCh := make(chan error, 1) go func() { defer cancel()