From f4f1b1b69f1e50758352a28650da471d56ac85a4 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Wed, 24 Jul 2024 19:10:00 +0000 Subject: [PATCH 1/7] feat: add --key flag to provisionerd start --- enterprise/cli/provisionerdaemonstart.go | 30 ++++++++++++++++--- enterprise/cli/provisionerdaemonstart_test.go | 9 ++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/enterprise/cli/provisionerdaemonstart.go b/enterprise/cli/provisionerdaemonstart.go index 8acff05a84e69..e9980e6a731d6 100644 --- a/enterprise/cli/provisionerdaemonstart.go +++ b/enterprise/cli/provisionerdaemonstart.go @@ -46,6 +46,7 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command { pollInterval time.Duration pollJitter time.Duration preSharedKey string + provisionerKey string verbose bool prometheusEnable bool @@ -113,6 +114,19 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command { return err } + if provisionerKey != "" { + if preSharedKey != "" { + return xerrors.New("cannot provide both provisioner key --key and pre-shared key --psk") + } + if len(rawTags) > 0 { + return xerrors.New("cannot provide tags when using provisioner key") + } + // _, err := provisionerkey.Parse(provisionerKey) + // if err != nil { + // return xerrors.Errorf("parse provisioner key: %w", err) + // } + } + logOpts := []clilog.Option{ clilog.WithFilter(logFilter...), clilog.WithHuman(logHuman), @@ -136,10 +150,10 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command { logger.Info(ctx, "note: untagged provisioners can only pick up jobs from untagged templates") } - // 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 "+provisionersdk.TagScope+"="+provisionersdk.ScopeOrganization) + // When authorizing with a PSK / provisioner key, we automatically scope the provisionerd + // to organization. Scoping to user with PSK / provisioner key auth is not a valid configuration. + if preSharedKey != "" || provisionerKey != "" { + logger.Info(ctx, "psk or provisioner key auth automatically sets tag "+provisionersdk.TagScope+"="+provisionersdk.ScopeOrganization) tags[provisionersdk.TagScope] = provisionersdk.ScopeOrganization } @@ -213,6 +227,7 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command { Tags: tags, PreSharedKey: preSharedKey, Organization: org.ID, + // ProvisionerKey: provisionerKey, }) }, &provisionerd.Options{ Logger: logger, @@ -296,6 +311,13 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command { Description: "Pre-shared key to authenticate with Coder server.", Value: serpent.StringOf(&preSharedKey), }, + { + Flag: "key", + Env: "CODER_PROVISIONER_DAEMON_KEY", + Description: "Provisioner key to authenticate with Coder server.", + Value: serpent.StringOf(&provisionerKey), + Hidden: true, + }, { Flag: "name", Env: "CODER_PROVISIONER_DAEMON_NAME", diff --git a/enterprise/cli/provisionerdaemonstart_test.go b/enterprise/cli/provisionerdaemonstart_test.go index b8e785ec45a95..31e1a31e829a6 100644 --- a/enterprise/cli/provisionerdaemonstart_test.go +++ b/enterprise/cli/provisionerdaemonstart_test.go @@ -301,6 +301,15 @@ func TestProvisionerDaemon_SessionToken(t *testing.T) { }) } +func TestProvisionerDaemon_ProvisionerKey(t *testing.T) { + t.Parallel() + + t.Run("OK", func(t *testing.T) { + t.Parallel() + + }) +} + //nolint:paralleltest,tparallel // Test uses a static port. func TestProvisionerDaemon_PrometheusEnabled(t *testing.T) { // Ephemeral ports have a tendency to conflict and fail with `bind: address already in use` error. From 47aa9c4d4f98c48257fad986c5528f9ef318af31 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Thu, 25 Jul 2024 15:46:55 +0000 Subject: [PATCH 2/7] tests --- enterprise/cli/provisionerdaemonstart.go | 21 ++-- enterprise/cli/provisionerdaemonstart_test.go | 103 ++++++++++++++++++ 2 files changed, 114 insertions(+), 10 deletions(-) diff --git a/enterprise/cli/provisionerdaemonstart.go b/enterprise/cli/provisionerdaemonstart.go index e9980e6a731d6..5d6183a60dbc2 100644 --- a/enterprise/cli/provisionerdaemonstart.go +++ b/enterprise/cli/provisionerdaemonstart.go @@ -24,6 +24,7 @@ import ( "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/cli/cliutil" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/provisionerkey" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/drpc" "github.com/coder/coder/v2/provisioner/terraform" @@ -84,8 +85,8 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command { return xerrors.Errorf("current organization: %w", err) } - if preSharedKey == "" { - return xerrors.New("must provide a pre-shared key when not authenticated as a user") + if preSharedKey == "" && provisionerKey == "" { + return xerrors.New("must provide a pre-shared key or provisioner key when not authenticated as a user") } org = codersdk.Organization{MinimalOrganization: codersdk.MinimalOrganization{ID: uuid.Nil}} @@ -121,10 +122,10 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command { if len(rawTags) > 0 { return xerrors.New("cannot provide tags when using provisioner key") } - // _, err := provisionerkey.Parse(provisionerKey) - // if err != nil { - // return xerrors.Errorf("parse provisioner key: %w", err) - // } + _, _, err := provisionerkey.Parse(provisionerKey) + if err != nil { + return xerrors.Errorf("parse provisioner key: %w", err) + } } logOpts := []clilog.Option{ @@ -224,10 +225,10 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command { Provisioners: []codersdk.ProvisionerType{ codersdk.ProvisionerTypeTerraform, }, - Tags: tags, - PreSharedKey: preSharedKey, - Organization: org.ID, - // ProvisionerKey: provisionerKey, + Tags: tags, + PreSharedKey: preSharedKey, + Organization: org.ID, + ProvisionerKey: provisionerKey, }) }, &provisionerd.Options{ Logger: logger, diff --git a/enterprise/cli/provisionerdaemonstart_test.go b/enterprise/cli/provisionerdaemonstart_test.go index 31e1a31e829a6..b3c2f16cb5627 100644 --- a/enterprise/cli/provisionerdaemonstart_test.go +++ b/enterprise/cli/provisionerdaemonstart_test.go @@ -307,6 +307,109 @@ func TestProvisionerDaemon_ProvisionerKey(t *testing.T) { t.Run("OK", func(t *testing.T) { t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + dv := coderdtest.DeploymentValues(t) + dv.Experiments.Append(string(codersdk.ExperimentMultiOrganization)) + client, user := coderdenttest.New(t, &coderdenttest.Options{ + ProvisionerDaemonPSK: "provisionersftw", + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureExternalProvisionerDaemons: 1, + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + Options: &coderdtest.Options{ + DeploymentValues: dv, + }, + }) + // nolint:gocritic // test + res, err := client.CreateProvisionerKey(ctx, user.OrganizationID, codersdk.CreateProvisionerKeyRequest{ + Name: "dont-TEST-me", + }) + require.NoError(t, err) + inv, conf := newCLI(t, "provisionerd", "start", "--key", res.Key, "--name=matt-daemon") + err = conf.URL().Write(client.URL.String()) + require.NoError(t, err) + pty := ptytest.New(t).Attach(inv) + clitest.Start(t, inv) + pty.ExpectNoMatchBefore(ctx, "check entitlement", "starting provisioner daemon") + pty.ExpectMatchContext(ctx, "matt-daemon") + + var daemons []codersdk.ProvisionerDaemon + require.Eventually(t, func() bool { + daemons, err = client.ProvisionerDaemons(ctx) + if err != nil { + return false + } + return len(daemons) == 1 + }, testutil.WaitShort, testutil.IntervalSlow) + require.Equal(t, "matt-daemon", daemons[0].Name) + require.Equal(t, provisionersdk.ScopeOrganization, daemons[0].Tags[provisionersdk.TagScope]) + require.Equal(t, buildinfo.Version(), daemons[0].Version) + require.Equal(t, proto.CurrentVersion.String(), daemons[0].APIVersion) + }) + + t.Run("NoPSK", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + dv := coderdtest.DeploymentValues(t) + dv.Experiments.Append(string(codersdk.ExperimentMultiOrganization)) + client, user := coderdenttest.New(t, &coderdenttest.Options{ + ProvisionerDaemonPSK: "provisionersftw", + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureExternalProvisionerDaemons: 1, + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + Options: &coderdtest.Options{ + DeploymentValues: dv, + }, + }) + // nolint:gocritic // test + res, err := client.CreateProvisionerKey(ctx, user.OrganizationID, codersdk.CreateProvisionerKeyRequest{ + Name: "dont-TEST-me", + }) + require.NoError(t, err) + inv, conf := newCLI(t, "provisionerd", "start", "--psk", "provisionersftw", "--key", res.Key, "--name=matt-daemon") + err = conf.URL().Write(client.URL.String()) + require.NoError(t, err) + err = inv.WithContext(ctx).Run() + require.ErrorContains(t, err, "cannot provide both provisioner key --key and pre-shared key --psk") + }) + + t.Run("NoTags", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + dv := coderdtest.DeploymentValues(t) + dv.Experiments.Append(string(codersdk.ExperimentMultiOrganization)) + client, user := coderdenttest.New(t, &coderdenttest.Options{ + ProvisionerDaemonPSK: "provisionersftw", + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureExternalProvisionerDaemons: 1, + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + Options: &coderdtest.Options{ + DeploymentValues: dv, + }, + }) + // nolint:gocritic // test + res, err := client.CreateProvisionerKey(ctx, user.OrganizationID, codersdk.CreateProvisionerKeyRequest{ + Name: "dont-TEST-me", + }) + require.NoError(t, err) + inv, conf := newCLI(t, "provisionerd", "start", "--tag", "mykey=yourvalue", "--key", res.Key, "--name=matt-daemon") + err = conf.URL().Write(client.URL.String()) + require.NoError(t, err) + err = inv.WithContext(ctx).Run() + require.ErrorContains(t, err, "cannot provide tags when using provisioner key") }) } From aa314a302048deef7ccc9a83e3b378d41e81368e Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Thu, 25 Jul 2024 15:59:21 +0000 Subject: [PATCH 3/7] fix test --- enterprise/cli/provisionerdaemonstart_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/cli/provisionerdaemonstart_test.go b/enterprise/cli/provisionerdaemonstart_test.go index b3c2f16cb5627..2e37c6f6710e0 100644 --- a/enterprise/cli/provisionerdaemonstart_test.go +++ b/enterprise/cli/provisionerdaemonstart_test.go @@ -153,7 +153,7 @@ func TestProvisionerDaemon_PSK(t *testing.T) { ctx, cancel := context.WithTimeout(inv.Context(), testutil.WaitLong) defer cancel() err = inv.WithContext(ctx).Run() - require.ErrorContains(t, err, "must provide a pre-shared key when not authenticated as a user") + require.ErrorContains(t, err, "must provide a pre-shared key or provisioner key when not authenticated as a user") }) } From 7b83b7e1bbd5ab87aa983b1dce168d7c62335e7d Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Thu, 25 Jul 2024 16:12:14 +0000 Subject: [PATCH 4/7] more tests --- enterprise/cli/provisionerdaemonstart_test.go | 49 ++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/enterprise/cli/provisionerdaemonstart_test.go b/enterprise/cli/provisionerdaemonstart_test.go index 2e37c6f6710e0..f1eb0853cc13e 100644 --- a/enterprise/cli/provisionerdaemonstart_test.go +++ b/enterprise/cli/provisionerdaemonstart_test.go @@ -338,7 +338,7 @@ func TestProvisionerDaemon_ProvisionerKey(t *testing.T) { var daemons []codersdk.ProvisionerDaemon require.Eventually(t, func() bool { - daemons, err = client.ProvisionerDaemons(ctx) + daemons, err = client.OrganizationProvisionerDaemons(ctx, user.OrganizationID) if err != nil { return false } @@ -411,6 +411,53 @@ func TestProvisionerDaemon_ProvisionerKey(t *testing.T) { err = inv.WithContext(ctx).Run() require.ErrorContains(t, err, "cannot provide tags when using provisioner key") }) + + t.Run("AnotherOrg", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + dv := coderdtest.DeploymentValues(t) + dv.Experiments.Append(string(codersdk.ExperimentMultiOrganization)) + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + ProvisionerDaemonPSK: "provisionersftw", + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureExternalProvisionerDaemons: 1, + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + Options: &coderdtest.Options{ + DeploymentValues: dv, + }, + }) + anotherOrg := coderdtest.CreateOrganization(t, client, coderdtest.CreateOrganizationOptions{}) + // nolint:gocritic // test + res, err := client.CreateProvisionerKey(ctx, anotherOrg.ID, codersdk.CreateProvisionerKeyRequest{ + Name: "dont-TEST-me", + }) + require.NoError(t, err) + inv, conf := newCLI(t, "provisionerd", "start", "--org", anotherOrg.ID.String(), "--key", res.Key, "--name=matt-daemon") + err = conf.URL().Write(client.URL.String()) + require.NoError(t, err) + pty := ptytest.New(t).Attach(inv) + clitest.Start(t, inv) + pty.ExpectNoMatchBefore(ctx, "check entitlement", "starting provisioner daemon") + pty.ExpectMatchContext(ctx, "matt-daemon") + + var daemons []codersdk.ProvisionerDaemon + require.Eventually(t, func() bool { + daemons, err = client.OrganizationProvisionerDaemons(ctx, anotherOrg.ID) + if err != nil { + return false + } + return len(daemons) == 1 + }, testutil.WaitShort, testutil.IntervalSlow) + require.Equal(t, "matt-daemon", daemons[0].Name) + require.Equal(t, provisionersdk.ScopeOrganization, daemons[0].Tags[provisionersdk.TagScope]) + require.Equal(t, buildinfo.Version(), daemons[0].Version) + require.Equal(t, proto.CurrentVersion.String(), daemons[0].APIVersion) + }) } //nolint:paralleltest,tparallel // Test uses a static port. From 727abcdc40700d4b9bb9863f77071d8d3ebb35be Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Thu, 25 Jul 2024 16:30:35 +0000 Subject: [PATCH 5/7] restrict route --- enterprise/coderd/provisionerdaemons.go | 53 +++++++++++--------- enterprise/coderd/provisionerdaemons_test.go | 3 -- 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 4f9748f2d265b..ff5eb70944529 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -13,6 +13,7 @@ import ( "github.com/hashicorp/yamux" "github.com/moby/moby/pkg/namesgenerator" "go.opentelemetry.io/otel/trace" + "golang.org/x/exp/maps" "golang.org/x/xerrors" "nhooyr.io/websocket" "storj.io/drpc/drpcmux" @@ -97,39 +98,43 @@ func (p *provisionerDaemonAuth) authorize(r *http.Request, orgID uuid.UUID, tags 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, nil + // Provisioner Key Auth + if pkOK { + if pk.OrganizationID != orgID { + return nil, xerrors.New("provisioner key unauthorized") } - ua := httpmw.UserAuthorization(r) - err := p.authorizer.Authorize(ctx, ua, policy.ActionCreate, rbac.ResourceProvisionerDaemon.InOrg(orgID)) - if err != nil { - if !provAuth { - 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 + if tags != nil && !maps.Equal(tags, map[string]string{}) { + return nil, xerrors.New("tags are not allowed when using a provisioner key") } - // User is allowed to create provisioner daemons + // If using provisioner key / PSK auth, the daemon is, by definition, scoped to the organization. + // Use the provisioner key tags here. + tags = provisionersdk.MutateTags(uuid.Nil, pk.Tags) return tags, nil } - if pkOK { - if pk.OrganizationID != orgID { - return nil, xerrors.New("provisioner key unauthorized") + // User Auth + 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, nil + } + ua := httpmw.UserAuthorization(r) + err := p.authorizer.Authorize(ctx, ua, policy.ActionCreate, rbac.ResourceProvisionerDaemon.InOrg(orgID)) + if err != nil { + if !provAuth { + 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 } - // If using provisioner key / PSK auth, the daemon is, by definition, scoped to the organization. - tags = provisionersdk.MutateTags(uuid.Nil, tags) + // User is allowed to create provisioner daemons return tags, nil } diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index 68055df5b77f5..451ff2249a15d 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -703,9 +703,6 @@ func TestProvisionerDaemonServe(t *testing.T) { Provisioners: []codersdk.ProvisionerType{ codersdk.ProvisionerTypeEcho, }, - Tags: map[string]string{ - provisionersdk.TagScope: provisionersdk.ScopeOrganization, - }, PreSharedKey: tc.requestPSK, ProvisionerKey: tc.requestProvisionerKey, }) From e54e662b83e4117986ac6db261e641a9f5b92af7 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Thu, 25 Jul 2024 16:43:31 +0000 Subject: [PATCH 6/7] fix tags with key flag --- enterprise/cli/provisionerdaemonstart.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/enterprise/cli/provisionerdaemonstart.go b/enterprise/cli/provisionerdaemonstart.go index 5d6183a60dbc2..9e520334feeda 100644 --- a/enterprise/cli/provisionerdaemonstart.go +++ b/enterprise/cli/provisionerdaemonstart.go @@ -154,9 +154,14 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command { // When authorizing with a PSK / provisioner key, we automatically scope the provisionerd // to organization. Scoping to user with PSK / provisioner key auth is not a valid configuration. if preSharedKey != "" || provisionerKey != "" { - logger.Info(ctx, "psk or provisioner key auth automatically sets tag "+provisionersdk.TagScope+"="+provisionersdk.ScopeOrganization) + logger.Info(ctx, "psk automatically sets tag "+provisionersdk.TagScope+"="+provisionersdk.ScopeOrganization) tags[provisionersdk.TagScope] = provisionersdk.ScopeOrganization } + if provisionerKey != "" { + logger.Info(ctx, "provisioner key auth automatically sets tag "+provisionersdk.TagScope+" empty") + // no scope tag will default to org scope + delete(tags, provisionersdk.TagScope) + } err = os.MkdirAll(cacheDir, 0o700) if err != nil { From 10b09edf958caf4f09b4fb274bcfcc8cf9adfc4f Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Thu, 25 Jul 2024 16:43:41 +0000 Subject: [PATCH 7/7] fix tags with key flag --- enterprise/cli/provisionerdaemonstart.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/cli/provisionerdaemonstart.go b/enterprise/cli/provisionerdaemonstart.go index 9e520334feeda..b0dfff227dbe3 100644 --- a/enterprise/cli/provisionerdaemonstart.go +++ b/enterprise/cli/provisionerdaemonstart.go @@ -153,7 +153,7 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command { // When authorizing with a PSK / provisioner key, we automatically scope the provisionerd // to organization. Scoping to user with PSK / provisioner key auth is not a valid configuration. - if preSharedKey != "" || provisionerKey != "" { + if preSharedKey != "" { logger.Info(ctx, "psk automatically sets tag "+provisionersdk.TagScope+"="+provisionersdk.ScopeOrganization) tags[provisionersdk.TagScope] = provisionersdk.ScopeOrganization }