From 4f76c5d723b32ca2c004abca7b6046dbbf22ca82 Mon Sep 17 00:00:00 2001 From: tlm Date: Thu, 3 Sep 2026 05:30:10 +0000 Subject: [PATCH 1/5] Add context to workshopctl command execution Pass a standard context.Context to workshopctl commands so request-scoped values can be made available during command execution. This prepares get-secret to access the requesting workshop's instance identity. Use the go-flags command handler because its native Execute interface does not support context propagation. --- internal/overlord/hookstate/ctlcmd/ctlcmd.go | 17 ++++++++++++++--- .../overlord/hookstate/ctlcmd/export_test.go | 11 ++++++++--- internal/overlord/hookstate/ctlcmd/getsecret.go | 8 ++++++-- internal/overlord/hookstate/ctlcmd/health.go | 3 ++- 4 files changed, 30 insertions(+), 9 deletions(-) diff --git a/internal/overlord/hookstate/ctlcmd/ctlcmd.go b/internal/overlord/hookstate/ctlcmd/ctlcmd.go index cc8ccf505..8655b068c 100644 --- a/internal/overlord/hookstate/ctlcmd/ctlcmd.go +++ b/internal/overlord/hookstate/ctlcmd/ctlcmd.go @@ -20,6 +20,7 @@ package ctlcmd import ( "bytes" + "context" "fmt" "io" @@ -97,7 +98,7 @@ type command interface { setContext(context *hookstate.Context) context() *hookstate.Context - Execute(args []string) error + Execute(ctx context.Context, args []string) error } type commandInfo struct { @@ -142,7 +143,7 @@ func (f ForbiddenCommandError) Error() string { var nonRootAllowed = []string{"get-secret", "set-health"} // Run runs the requested command. -func Run(context *hookstate.Context, args []string, uid uint32) (stdout, stderr []byte, err error) { +func Run(hookContext *hookstate.Context, args []string, uid uint32) (stdout, stderr []byte, err error) { if len(args) == 0 { return nil, nil, fmt.Errorf("workshopctl cannot run without args") } @@ -156,12 +157,14 @@ func Run(context *hookstate.Context, args []string, uid uint32) (stdout, stderr // Create stdout/stderr buffers, and make sure commands use them. var stdoutBuffer bytes.Buffer var stderrBuffer bytes.Buffer + activeCommands := make(map[string]command, len(commands)) for name, cmdInfo := range commands { cmd := cmdInfo.generator() cmd.setName(name) cmd.setStdout(&stdoutBuffer) cmd.setStderr(&stderrBuffer) - cmd.setContext(context) + cmd.setContext(hookContext) + activeCommands[name] = cmd theCmd, err := parser.AddCommand(name, cmdInfo.shortHelp, cmdInfo.longHelp, cmd) theCmd.Hidden = cmdInfo.hidden @@ -170,6 +173,14 @@ func Run(context *hookstate.Context, args []string, uid uint32) (stdout, stderr } } + parser.CommandHandler = func(_ flags.Commander, args []string) error { + cmd, ok := activeCommands[parser.Active.Name] + if !ok { + return fmt.Errorf("internal error: active command %q not found", parser.Active.Name) + } + return cmd.Execute(context.TODO(), args) + } + _, err = parser.ParseArgs(args) return stdoutBuffer.Bytes(), stderrBuffer.Bytes(), err } diff --git a/internal/overlord/hookstate/ctlcmd/export_test.go b/internal/overlord/hookstate/ctlcmd/export_test.go index 9e0562ded..b97fec70c 100644 --- a/internal/overlord/hookstate/ctlcmd/export_test.go +++ b/internal/overlord/hookstate/ctlcmd/export_test.go @@ -14,7 +14,10 @@ package ctlcmd -import "fmt" +import ( + "context" + "fmt" +) func AddMockCommand(name string) *MockCommand { return addMockCmd(name, false) @@ -41,7 +44,8 @@ func NewMockCommand() *MockCommand { } } -func (c *MockCommand) Execute(args []string) error { +func (c *MockCommand) Execute(ctx context.Context, args []string) error { + c.ExecuteContext = ctx c.Args = args if c.FakeStdout != "" { @@ -72,5 +76,6 @@ type MockCommand struct { FakeStdout string FakeStderr string - Args []string + ExecuteContext context.Context + Args []string } diff --git a/internal/overlord/hookstate/ctlcmd/getsecret.go b/internal/overlord/hookstate/ctlcmd/getsecret.go index b724b03bd..a3be4c933 100644 --- a/internal/overlord/hookstate/ctlcmd/getsecret.go +++ b/internal/overlord/hookstate/ctlcmd/getsecret.go @@ -14,7 +14,11 @@ package ctlcmd -import "github.com/canonical/workshop/internal/logger" +import ( + "context" + + "github.com/canonical/workshop/internal/logger" +) type getSecretCommand struct { baseCommand @@ -50,7 +54,7 @@ func init() { } // Execute runs the get-secret command, writing the secret value to stdout. -func (c *getSecretCommand) Execute([]string) error { +func (c *getSecretCommand) Execute(context.Context, []string) error { // Log the requested identifier only; never the resolved value. logger.Debugf("get-secret request for %q", c.Secret) diff --git a/internal/overlord/hookstate/ctlcmd/health.go b/internal/overlord/hookstate/ctlcmd/health.go index 65710ce23..9b6797806 100644 --- a/internal/overlord/hookstate/ctlcmd/health.go +++ b/internal/overlord/hookstate/ctlcmd/health.go @@ -18,6 +18,7 @@ package ctlcmd import ( + "context" "errors" "fmt" "regexp" @@ -67,7 +68,7 @@ var ( validCode = regexp.MustCompile(`^[a-z](?:-?[a-z0-9])+$`).MatchString ) -func (c *healthCommand) Execute([]string) error { +func (c *healthCommand) Execute(context.Context, []string) error { if c.Status == "okay" && (len(c.Message) > 0 || len(c.Code) > 0) { return fmt.Errorf(`when status is "okay", message and code must be empty`) } From de389e047c1c9d1943bf8f90f3ddb6c704990a85 Mon Sep 17 00:00:00 2001 From: tlm Date: Thu, 3 Sep 2026 05:39:57 +0000 Subject: [PATCH 2/5] Propagate request context to workshopctl commands Accept a context.Context in ctlcmd.Run and pass it to the selected command during execution. This allows request-scoped values, including the workshop instance ID, to reach get-secret. Pass the HTTP request context from the workshopctl API handler and verify the propagation contract through the mock command's Execute callback. --- internal/daemon/api_workshopctl.go | 10 ++--- internal/overlord/hookstate/ctlcmd/ctlcmd.go | 9 +++- .../overlord/hookstate/ctlcmd/ctlcmd_test.go | 43 ++++++++++++++----- .../overlord/hookstate/ctlcmd/export_test.go | 9 ++-- .../hookstate/ctlcmd/getsecret_test.go | 8 ++-- .../overlord/hookstate/ctlcmd/health_test.go | 10 +++-- 6 files changed, 62 insertions(+), 27 deletions(-) diff --git a/internal/daemon/api_workshopctl.go b/internal/daemon/api_workshopctl.go index de5837dcd..ef5330969 100644 --- a/internal/daemon/api_workshopctl.go +++ b/internal/daemon/api_workshopctl.go @@ -62,15 +62,15 @@ func v1PostWorkshopCtl(c *Command, r *http.Request, _ *userState) Response { // Ignore missing context error to allow 'workshopctl -h' without a context; // Actual context is validated later by get/set. - context, _ := c.d.overlord.HookManager().Context(reqData.ContextID) + hookContext, _ := c.d.overlord.HookManager().Context(reqData.ContextID) if reqData.Stdin != nil { - context.Lock() - context.Set("stdin", reqData.Stdin) - context.Unlock() + hookContext.Lock() + hookContext.Set("stdin", reqData.Stdin) + hookContext.Unlock() } - stdout, stderr, err := ctlcmd.Run(context, reqData.Args, uid) + stdout, stderr, err := ctlcmd.Run(r.Context(), hookContext, reqData.Args, uid) if err != nil { if e, ok := err.(*flags.Error); ok && e.Type == flags.ErrHelp { stdout = []byte(e.Error()) diff --git a/internal/overlord/hookstate/ctlcmd/ctlcmd.go b/internal/overlord/hookstate/ctlcmd/ctlcmd.go index 8655b068c..888e0fadc 100644 --- a/internal/overlord/hookstate/ctlcmd/ctlcmd.go +++ b/internal/overlord/hookstate/ctlcmd/ctlcmd.go @@ -143,7 +143,12 @@ func (f ForbiddenCommandError) Error() string { var nonRootAllowed = []string{"get-secret", "set-health"} // Run runs the requested command. -func Run(hookContext *hookstate.Context, args []string, uid uint32) (stdout, stderr []byte, err error) { +func Run( + ctx context.Context, + hookContext *hookstate.Context, + args []string, + uid uint32, +) (stdout, stderr []byte, err error) { if len(args) == 0 { return nil, nil, fmt.Errorf("workshopctl cannot run without args") } @@ -178,7 +183,7 @@ func Run(hookContext *hookstate.Context, args []string, uid uint32) (stdout, std if !ok { return fmt.Errorf("internal error: active command %q not found", parser.Active.Name) } - return cmd.Execute(context.TODO(), args) + return cmd.Execute(ctx, args) } _, err = parser.ParseArgs(args) diff --git a/internal/overlord/hookstate/ctlcmd/ctlcmd_test.go b/internal/overlord/hookstate/ctlcmd/ctlcmd_test.go index 6ec5844e4..2e9ddcbe8 100644 --- a/internal/overlord/hookstate/ctlcmd/ctlcmd_test.go +++ b/internal/overlord/hookstate/ctlcmd/ctlcmd_test.go @@ -18,6 +18,7 @@ package ctlcmd_test import ( + "context" "strings" "testing" @@ -55,7 +56,7 @@ func (s *ctlcmdSuite) SetUpTest(c *C) { } func (s *ctlcmdSuite) TestNonExistingCommand(c *C) { - stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"foo"}, 0) + stdout, stderr, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"foo"}, 0) c.Check(string(stdout), Equals, "") c.Check(string(stderr), Equals, "") c.Check(err, ErrorMatches, ".*[Uu]nknown command.*") @@ -68,20 +69,42 @@ func (s *ctlcmdSuite) TestCommandOutput(c *C) { mockCommand.FakeStdout = "test stdout" mockCommand.FakeStderr = "test stderr" - stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"mock", "foo"}, 0) + stdout, stderr, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"mock", "foo"}, 0) c.Check(err, IsNil) c.Check(string(stdout), Equals, "test stdout") c.Check(string(stderr), Equals, "test stderr") c.Check(mockCommand.Args, DeepEquals, []string{"foo"}) } +// TestRunPassesContextToCommand verifies that Run forwards its context to the +// selected command. Commands rely on this contract to access request-scoped +// values during Execute. +func (s *ctlcmdSuite) TestRunPassesContextToCommand(c *C) { + mockCommand := ctlcmd.AddMockCommand("mock") + defer ctlcmd.RemoveCommand("mock") + + key := struct{}{} + ctx := context.WithValue(context.Background(), key, "test") + called := false + mockCommand.ExecuteFunc = func(ctx context.Context, _ []string) error { + called = true + c.Check(ctx.Value(key), Equals, "test") + return nil + } + + _, _, err := ctlcmd.Run(ctx, s.mockContext, []string{"mock"}, 0) + + c.Check(err, IsNil) + c.Check(called, Equals, true) +} + func (s *ctlcmdSuite) TestHiddenCommand(c *C) { ctlcmd.AddHiddenMockCommand("mock-hidden") ctlcmd.AddMockCommand("mock-shown") defer ctlcmd.RemoveCommand("mock-hidden") defer ctlcmd.RemoveCommand("mock-shown") - _, _, err := ctlcmd.Run(s.mockContext, []string{"--help"}, 0) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"--help"}, 0) // help message output is returned as *flags.Error with // Type as flags.ErrHelp c.Assert(err, FitsTypeOf, &flags.Error{}) @@ -95,33 +118,33 @@ func (s *ctlcmdSuite) TestHiddenCommand(c *C) { } func (s *ctlcmdSuite) TestRootRequiredCommandFailure(c *C) { - _, _, err := ctlcmd.Run(s.mockContext, []string{"start"}, 1000) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"start"}, 1000) c.Check(err, FitsTypeOf, &ctlcmd.ForbiddenCommandError{}) c.Check(err.Error(), Equals, `cannot use "start" with uid 1000, try with sudo`) } func (s *ctlcmdSuite) TestRunNoArgsFailure(c *C) { - _, _, err := ctlcmd.Run(s.mockContext, []string{}, 0) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{}, 0) c.Check(err, NotNil) } func (s *ctlcmdSuite) TestRunOnlyHelp(c *C) { - _, _, err := ctlcmd.Run(s.mockContext, []string{"-h"}, 1000) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"-h"}, 1000) c.Check(err, NotNil) c.Assert(strings.HasPrefix(err.Error(), "Usage:"), Equals, true) - _, _, err = ctlcmd.Run(s.mockContext, []string{"--help"}, 1000) + _, _, err = ctlcmd.Run(context.TODO(), s.mockContext, []string{"--help"}, 1000) c.Check(err, NotNil) c.Assert(strings.HasPrefix(err.Error(), "Usage:"), Equals, true) } func (s *ctlcmdSuite) TestRunHelpAtAnyPosition(c *C) { - _, _, err := ctlcmd.Run(s.mockContext, []string{"set-health", "a", "-h"}, 1000) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"set-health", "a", "-h"}, 1000) c.Check(err, NotNil) c.Assert(strings.HasPrefix(err.Error(), "Usage:"), Equals, true) - _, _, err = ctlcmd.Run(s.mockContext, []string{"set-health", "a", "b", "--help"}, 1000) + _, _, err = ctlcmd.Run(context.TODO(), s.mockContext, []string{"set-health", "a", "b", "--help"}, 1000) c.Check(err, NotNil) c.Assert(strings.HasPrefix(err.Error(), "Usage:"), Equals, true) } @@ -129,7 +152,7 @@ func (s *ctlcmdSuite) TestRunHelpAtAnyPosition(c *C) { func (s *ctlcmdSuite) TestRunNonRootAllowedCommandWithAllowedCmdAsArg(c *C) { // this test protects us against a future refactor introducing a bug that allows // a root-only command to run without root if an arg is in the nonRootAllowed list - _, _, err := ctlcmd.Run(s.mockContext, []string{"set", "get", "a"}, 1000) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"set", "get", "a"}, 1000) c.Check(err, FitsTypeOf, &ctlcmd.ForbiddenCommandError{}) c.Check(err.Error(), Equals, `cannot use "set" with uid 1000, try with sudo`) } diff --git a/internal/overlord/hookstate/ctlcmd/export_test.go b/internal/overlord/hookstate/ctlcmd/export_test.go index b97fec70c..2f9500a4f 100644 --- a/internal/overlord/hookstate/ctlcmd/export_test.go +++ b/internal/overlord/hookstate/ctlcmd/export_test.go @@ -45,9 +45,12 @@ func NewMockCommand() *MockCommand { } func (c *MockCommand) Execute(ctx context.Context, args []string) error { - c.ExecuteContext = ctx c.Args = args + if c.ExecuteFunc != nil { + return c.ExecuteFunc(ctx, args) + } + if c.FakeStdout != "" { err := c.printf("%s", c.FakeStdout) if err != nil { @@ -75,7 +78,7 @@ type MockCommand struct { ExecuteError bool FakeStdout string FakeStderr string + ExecuteFunc func(context.Context, []string) error - ExecuteContext context.Context - Args []string + Args []string } diff --git a/internal/overlord/hookstate/ctlcmd/getsecret_test.go b/internal/overlord/hookstate/ctlcmd/getsecret_test.go index 14c952f08..ae99b4ab6 100644 --- a/internal/overlord/hookstate/ctlcmd/getsecret_test.go +++ b/internal/overlord/hookstate/ctlcmd/getsecret_test.go @@ -15,6 +15,8 @@ package ctlcmd_test import ( + "context" + "gopkg.in/check.v1" "github.com/canonical/workshop/internal/overlord/hookstate/ctlcmd" @@ -27,7 +29,7 @@ type getSecretSuite struct{} var _ = check.Suite(&getSecretSuite{}) func (s *getSecretSuite) TestGetSecret(c *check.C) { - stdout, stderr, err := ctlcmd.Run(nil, []string{"get-secret", "ollama.ollama-api-key"}, 0) + stdout, stderr, err := ctlcmd.Run(context.TODO(), nil, []string{"get-secret", "ollama.ollama-api-key"}, 0) c.Assert(err, check.IsNil) c.Check(string(stdout), check.Equals, "workshop-placeholder-secret") c.Check(string(stderr), check.Equals, "") @@ -36,7 +38,7 @@ func (s *getSecretSuite) TestGetSecret(c *check.C) { // TestGetSecretMissingArg checks that get-secret requires a secret // identifier argument. func (s *getSecretSuite) TestGetSecretMissingArg(c *check.C) { - _, _, err := ctlcmd.Run(nil, []string{"get-secret"}, 0) + _, _, err := ctlcmd.Run(context.TODO(), nil, []string{"get-secret"}, 0) c.Check(err, check.ErrorMatches, ".*the required argument `.` was not provided.*") } @@ -44,7 +46,7 @@ func (s *getSecretSuite) TestGetSecretMissingArg(c *check.C) { // both the socket-activated systemd path and SDK wrapper scripts invoke it // as the workshop user. func (s *getSecretSuite) TestGetSecretNonRoot(c *check.C) { - stdout, _, err := ctlcmd.Run(nil, []string{"get-secret", "ollama.ollama-api-key"}, 1000) + stdout, _, err := ctlcmd.Run(context.TODO(), nil, []string{"get-secret", "ollama.ollama-api-key"}, 1000) c.Assert(err, check.IsNil) c.Check(string(stdout), check.Equals, "workshop-placeholder-secret") } diff --git a/internal/overlord/hookstate/ctlcmd/health_test.go b/internal/overlord/hookstate/ctlcmd/health_test.go index 544208b01..5790b4384 100644 --- a/internal/overlord/hookstate/ctlcmd/health_test.go +++ b/internal/overlord/hookstate/ctlcmd/health_test.go @@ -18,6 +18,8 @@ package ctlcmd_test import ( + "context" + "gopkg.in/check.v1" "github.com/canonical/workshop/internal/overlord/healthstate" @@ -102,13 +104,13 @@ func (s *healthSuite) TestBadArgs(c *check.C) { } for i, t := range table { - _, _, err := ctlcmd.Run(nil, t.args, 0) + _, _, err := ctlcmd.Run(context.TODO(), nil, t.args, 0) c.Check(err, check.ErrorMatches, t.err, check.Commentf("%d", i)) } } func (s *healthSuite) TestRegularRun(c *check.C) { - _, _, err := ctlcmd.Run(s.mockContext, []string{"set-health", "waiting", "message", "--code=some-code"}, 0) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"set-health", "waiting", "message", "--code=some-code"}, 0) c.Assert(err, check.IsNil) s.mockContext.Lock() @@ -122,7 +124,7 @@ func (s *healthSuite) TestRegularRun(c *check.C) { } func (s *healthSuite) TestMessageTruncation(c *check.C) { - _, _, err := ctlcmd.Run(s.mockContext, []string{"set-health", "error", "Sometimes messages will get a little bit too verbose and this can lead to some rather nasty UX (as well as potential memory problems in extreme cases) so we kinda have to deal with that", "--code=some-code"}, 0) + _, _, err := ctlcmd.Run(context.TODO(), s.mockContext, []string{"set-health", "error", "Sometimes messages will get a little bit too verbose and this can lead to some rather nasty UX (as well as potential memory problems in extreme cases) so we kinda have to deal with that", "--code=some-code"}, 0) c.Assert(err, check.IsNil) s.mockContext.Lock() @@ -141,7 +143,7 @@ func (s *healthSuite) TestRegularRunIncorrectHook(c *check.C) { ctx, err := hookstate.NewContext(task, s.state, setup, s.mockHandler, "") c.Assert(err, check.IsNil) - _, _, err = ctlcmd.Run(ctx, []string{"set-health", "waiting", "message", "--code=some-code"}, 0) + _, _, err = ctlcmd.Run(context.TODO(), ctx, []string{"set-health", "waiting", "message", "--code=some-code"}, 0) c.Assert(err, check.ErrorMatches, `"set-health" is only allowed from a "check-health" hook`) s.mockContext.Lock() From 584515a9b3ddbc93b03ed2a236b70e8ddac5a28b Mon Sep 17 00:00:00 2001 From: tlm Date: Fri, 4 Sep 2026 01:27:12 +0000 Subject: [PATCH 3/5] Resolve workshopctl contexts from instance IDs Use an existing hook context when a workshopctl request supplies a cookie. For cookie-less requests, verify that the requesting user owns the workshop instance ID before creating a taskless ephemeral hook context. Log internal lookup failures while returning a generic error response to avoid exposing backend details to callers. --- internal/daemon/api_workshopctl.go | 70 +++++++++++- internal/daemon/api_workshopctl_test.go | 107 +++++++++++++++++- internal/overlord/hookstate/manager.go | 6 + internal/overlord/hookstate/manager_test.go | 45 ++++++++ internal/overlord/workshopstate/manager.go | 41 +++++++ .../overlord/workshopstate/manager_test.go | 104 ++++++++++++++++- 6 files changed, 368 insertions(+), 5 deletions(-) create mode 100644 internal/overlord/hookstate/manager_test.go diff --git a/internal/daemon/api_workshopctl.go b/internal/daemon/api_workshopctl.go index ef5330969..63fe228bb 100644 --- a/internal/daemon/api_workshopctl.go +++ b/internal/daemon/api_workshopctl.go @@ -20,7 +20,10 @@ import ( "github.com/jessevdk/go-flags" + "github.com/canonical/workshop/internal/logger" + "github.com/canonical/workshop/internal/overlord/hookstate" "github.com/canonical/workshop/internal/overlord/hookstate/ctlcmd" + "github.com/canonical/workshop/internal/workshop" ) // workshopCtlOptions holds the various options with which workshopctl is invoked. @@ -60,9 +63,10 @@ func v1PostWorkshopCtl(c *Command, r *http.Request, _ *userState) Response { return statusForbidden("cannot get remote user: %w", err) } - // Ignore missing context error to allow 'workshopctl -h' without a context; - // Actual context is validated later by get/set. - hookContext, _ := c.d.overlord.HookManager().Context(reqData.ContextID) + hookContext, response := workshopctlHookContext(c, r, reqData.ContextID) + if response != nil { + return response + } if reqData.Stdin != nil { hookContext.Lock() @@ -86,3 +90,63 @@ func v1PostWorkshopCtl(c *Command, r *http.Request, _ *userState) Response { return SyncResponse(result, http.StatusOK) } + +// workshopctlHookContext returns the hook context used to execute a +// workshopctl command. A supplied cookie selects an existing context; +// otherwise the workshop instance ID is validated before creating an +// ephemeral context. +func workshopctlHookContext( + c *Command, + r *http.Request, + contextID string, +) (*hookstate.Context, Response) { + if contextID != "" { + return workshopctlHookContextFromCookie(c, contextID) + } + return workshopctlHookContextFromInstanceID(c, r) +} + +// workshopctlHookContextFromCookie returns the active or long-lived context +// identified by the supplied workshop cookie. An invalid cookie is rejected +// rather than falling back to instance ID authentication. +func workshopctlHookContextFromCookie( + c *Command, + contextID string, +) (*hookstate.Context, Response) { + hookContext, err := c.d.overlord.HookManager().Context(contextID) + if err != nil { + return nil, statusBadRequest("cannot get workshop context: %w", err) + } + return hookContext, nil +} + +// workshopctlHookContextFromInstanceID validates that the requesting user owns +// the workshop instance identified by the request context, then creates a +// taskless context for this individual workshopctl invocation. +func workshopctlHookContextFromInstanceID( + c *Command, + r *http.Request, +) (*hookstate.Context, Response) { + instanceID, _ := r.Context(). + Value(workshop.ContextWorkshopInstanceID).(string) + if instanceID == "" { + return nil, statusBadRequest("workshop instance ID not supplied") + } + + valid, err := c.d.overlord.WorkshopManager(). + OwnsWorkshopInstanceID(r.Context(), instanceID) + if err != nil { + logger.Noticef("cannot validate workshop instance ID: %v", err) + return nil, statusInternalError("internal error occurred validating workshop instance id") + } + if !valid { + return nil, statusForbidden("invalid workshop instance ID") + } + + hookContext, err := c.d.overlord.HookManager().NewEphemeralContext() + if err != nil { + logger.Noticef("cannot create workshop context: %v", err) + return nil, statusInternalError("internal error occurred") + } + return hookContext, nil +} diff --git a/internal/daemon/api_workshopctl_test.go b/internal/daemon/api_workshopctl_test.go index 69dba5a79..1d6da7ae3 100644 --- a/internal/daemon/api_workshopctl_test.go +++ b/internal/daemon/api_workshopctl_test.go @@ -16,14 +16,35 @@ package daemon import ( "bytes" + "context" "net/http" "gopkg.in/check.v1" + + "github.com/canonical/workshop/internal/workshop" + "github.com/canonical/workshop/internal/workshop/fakebackend" ) -func (s *apiSuite) TestWorkshopHelpCtlNoContext(c *check.C) { +func (s *apiSuite) addWorkshopWithInstanceID(instanceID string) { + s.b.Workshops[s.project.ProjectId] = map[string]*fakebackend.FakeWorkshop{ + "test-workshop": { + Workshop: &workshop.Workshop{ + Name: "test-workshop", + Project: s.project, + InstanceID: instanceID, + }, + }, + } +} + +// TestWorkshopHelpCtlWithoutCookie checks that help works through the +// cookie-less workshopctl path. The test relies on the middleware-provided +// workshop instance ID being present in the request context so the handler can +// validate the caller and create an ephemeral hook context. +func (s *apiSuite) TestWorkshopHelpCtlWithoutCookie(c *check.C) { // Setup s.daemon(c) + s.addWorkshopWithInstanceID("instance-id") wctl := apiCmd("/v1/workshopctl") buf := bytes.NewBufferString(`{"args":["-h"]}`) @@ -31,6 +52,12 @@ func (s *apiSuite) TestWorkshopHelpCtlNoContext(c *check.C) { req, err := s.createProjectsRequest("POST", "/v1/workshopctl", buf) c.Assert(err, check.IsNil) + req = req.WithContext(context.WithValue( + req.Context(), + workshop.ContextWorkshopInstanceID, + "instance-id", + )) + // Execute rsp := v1PostWorkshopCtl(wctl, req, nil).(*resp) @@ -41,3 +68,81 @@ func (s *apiSuite) TestWorkshopHelpCtlNoContext(c *check.C) { _, err = rsp.MarshalJSON() c.Assert(err, check.IsNil) } + +// TestWorkshopCtlRequiresInstanceIDWithoutCookie checks that workshopctl +// requests without a hook cookie must identify their workshop instance. +func (s *apiSuite) TestWorkshopCtlRequiresInstanceIDWithoutCookie(c *check.C) { + s.daemon(c) + wctl := apiCmd("/v1/workshopctl") + buf := bytes.NewBufferString(`{"args":["get-secret","sdk.secret"]}`) + req, err := s.createProjectsRequest("POST", "/v1/workshopctl", buf) + c.Assert(err, check.IsNil) + + rsp := v1PostWorkshopCtl(wctl, req, nil).(*resp) + + c.Check(rsp.Status, check.Equals, http.StatusBadRequest) +} + +// TestWorkshopCtlRejectsUnknownInstanceID checks that a request without a hook +// cookie cannot use an instance ID outside the requesting user's workshops. +func (s *apiSuite) TestWorkshopCtlRejectsUnknownInstanceID(c *check.C) { + s.daemon(c) + wctl := apiCmd("/v1/workshopctl") + buf := bytes.NewBufferString(`{"args":["get-secret","sdk.secret"]}`) + req, err := s.createProjectsRequest("POST", "/v1/workshopctl", buf) + c.Assert(err, check.IsNil) + req = req.WithContext(context.WithValue( + req.Context(), + workshop.ContextWorkshopInstanceID, + "unknown-instance", + )) + + rsp := v1PostWorkshopCtl(wctl, req, nil).(*resp) + + c.Check(rsp.Status, check.Equals, http.StatusForbidden) +} + +// TestWorkshopCtlAcceptsOwnedInstanceID checks that a request without a hook +// cookie may run after its instance ID is matched to the requesting user. +func (s *apiSuite) TestWorkshopCtlAcceptsOwnedInstanceID(c *check.C) { + s.daemon(c) + s.addWorkshopWithInstanceID("instance-id") + + wctl := apiCmd("/v1/workshopctl") + buf := bytes.NewBufferString( + `{"args":["get-secret","sdk.secret"],"stdin":"dGVzdA=="}`, + ) + req, err := s.createProjectsRequest("POST", "/v1/workshopctl", buf) + c.Assert(err, check.IsNil) + req = req.WithContext(context.WithValue( + req.Context(), + workshop.ContextWorkshopInstanceID, + "instance-id", + )) + + rsp := v1PostWorkshopCtl(wctl, req, nil).(*resp) + + c.Check(rsp.Status, check.Equals, http.StatusOK) +} + +// TestWorkshopCtlCookieSkipsInstanceIDValidation checks that a valid hook +// cookie remains the authentication mechanism and does not require an instance +// ID lookup. +func (s *apiSuite) TestWorkshopCtlCookieSkipsInstanceIDValidation(c *check.C) { + s.daemon(c) + st := s.d.overlord.State() + st.Lock() + st.Set("workshop-cookies", map[string]string{"cookie-id": "test-workshop"}) + st.Unlock() + + wctl := apiCmd("/v1/workshopctl") + buf := bytes.NewBufferString( + `{"context-id":"cookie-id","args":["get-secret","sdk.secret"]}`, + ) + req, err := s.createProjectsRequest("POST", "/v1/workshopctl", buf) + c.Assert(err, check.IsNil) + + rsp := v1PostWorkshopCtl(wctl, req, nil).(*resp) + + c.Check(rsp.Status, check.Equals, http.StatusOK) +} diff --git a/internal/overlord/hookstate/manager.go b/internal/overlord/hookstate/manager.go index 47e77f930..25334dd0d 100644 --- a/internal/overlord/hookstate/manager.go +++ b/internal/overlord/hookstate/manager.go @@ -101,6 +101,12 @@ func (w *HookManager) Ensure() error { return nil } +// NewEphemeralContext returns a context that is not associated with a hook +// task. Its data is retained only for the lifetime of the context. +func (m *HookManager) NewEphemeralContext() (*Context, error) { + return NewContext(nil, m.state, &HookSetup{}, nil, "") +} + func (m *HookManager) ephemeralContext(cookieID string) (context *Context, err error) { var contexts map[string]string m.state.Lock() diff --git a/internal/overlord/hookstate/manager_test.go b/internal/overlord/hookstate/manager_test.go new file mode 100644 index 000000000..bf74acd30 --- /dev/null +++ b/internal/overlord/hookstate/manager_test.go @@ -0,0 +1,45 @@ +// Copyright (c) 2026 Canonical Ltd +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License version 3 as +// published by the Free Software Foundation. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +package hookstate + +import ( + . "gopkg.in/check.v1" + + "github.com/canonical/workshop/internal/overlord/state" +) + +type managerSuite struct { + state *state.State + manager *HookManager +} + +var _ = Suite(&managerSuite{}) + +func (s *managerSuite) SetUpTest(*C) { + s.state = state.New(nil) + s.manager = &HookManager{state: s.state} +} + +// TestNewEphemeralContext checks that the manager creates a context with its +// state but without a hook task, which is the contract required by +// cookie-less workshopctl requests after their instance ID is validated. +func (s *managerSuite) TestNewEphemeralContext(c *C) { + ctx, err := s.manager.NewEphemeralContext() + + c.Assert(err, IsNil) + c.Check(ctx.IsEphemeral(), Equals, true) + c.Check(ctx.State(), Equals, s.state) + c.Check(ctx.ID(), Not(Equals), "") +} diff --git a/internal/overlord/workshopstate/manager.go b/internal/overlord/workshopstate/manager.go index 73a32a111..45e642f08 100644 --- a/internal/overlord/workshopstate/manager.go +++ b/internal/overlord/workshopstate/manager.go @@ -126,6 +126,47 @@ func (w *WorkshopManager) Workshop(ctx context.Context, name, pId string) (*work return workshop, nil } +// OwnsWorkshopInstanceID reports whether the user in ctx owns a workshop with +// the specified backend instance identifier. If ctx does not identify a user, +// it returns false without an error. +func (w *WorkshopManager) OwnsWorkshopInstanceID( + ctx context.Context, + instanceID string, +) (bool, error) { + if instanceID == "" { + return false, nil + } + + projects, err := w.backend.UserProjects(ctx) + if err != nil { + return false, err + } + + for _, project := range projects { + projectCtx := context.WithValue( + ctx, + workshop.ContextProjectId, + project.ProjectId, + ) + workshops, err := w.backend.ProjectWorkshops(projectCtx) + if err != nil { + return false, fmt.Errorf( + "cannot list workshops for project %q: %w", + project.ProjectId, + err, + ) + } + + for _, candidate := range workshops { + if candidate.InstanceID == instanceID { + return true, nil + } + } + } + + return false, nil +} + // Returns latest file for a workshop. The state must be locked, as listing // projects can update project metadata. func (w *WorkshopManager) WorkshopFile(ctx context.Context, name, pId string) (*workshop.File, error) { diff --git a/internal/overlord/workshopstate/manager_test.go b/internal/overlord/workshopstate/manager_test.go index b29441acc..279518d08 100644 --- a/internal/overlord/workshopstate/manager_test.go +++ b/internal/overlord/workshopstate/manager_test.go @@ -15,6 +15,8 @@ package workshopstate_test import ( + "context" + "gopkg.in/check.v1" "github.com/canonical/workshop/internal/overlord/state" @@ -26,7 +28,7 @@ import ( type managerSuite struct { state *state.State - backend workshop.Backend + backend *fakebackend.FakeWorkshopBackend runner *state.TaskRunner manager *workshopstate.WorkshopManager } @@ -43,6 +45,29 @@ func (s *managerSuite) SetUpTest(c *check.C) { s.manager = workshopstate.New(s.state, s.runner) } +func (s *managerSuite) addWorkshop( + c *check.C, + ctx context.Context, + name string, + instanceID string, +) { + project, _, err := s.backend.CreateOrLoadProject(ctx, c.MkDir()) + c.Assert(err, check.IsNil) + + if s.backend.Workshops[project.ProjectId] == nil { + s.backend.Workshops[project.ProjectId] = make( + map[string]*fakebackend.FakeWorkshop, + ) + } + s.backend.Workshops[project.ProjectId][name] = &fakebackend.FakeWorkshop{ + Workshop: &workshop.Workshop{ + Name: name, + Project: *project, + InstanceID: instanceID, + }, + } +} + func (s *managerSuite) TestAddHandlers(c *check.C) { workshopstate.New(s.state, s.runner) @@ -63,3 +88,80 @@ func (s *managerSuite) TestAddHandlers(c *check.C) { "remove-state-storage", }) } + +// TestOwnsWorkshopInstanceIDReturnsTrueForOwnedWorkshop checks that the +// manager finds an instance ID belonging to a workshop owned by the user in +// context. +func (s *managerSuite) TestOwnsWorkshopInstanceIDReturnsTrueForOwnedWorkshop( + c *check.C, +) { + ctx := context.WithValue( + context.Background(), + workshop.ContextUser, + "test-user", + ) + s.addWorkshop(c, ctx, "test-workshop", "instance-id") + + owns, err := s.manager.OwnsWorkshopInstanceID(ctx, "instance-id") + + c.Assert(err, check.IsNil) + c.Check(owns, check.Equals, true) +} + +// TestOwnsWorkshopInstanceIDReturnsFalseForUnknownID checks that the manager +// rejects an instance ID that does not identify one of the user's workshops. +func (s *managerSuite) TestOwnsWorkshopInstanceIDReturnsFalseForUnknownID( + c *check.C, +) { + ctx := context.WithValue( + context.Background(), + workshop.ContextUser, + "test-user", + ) + s.addWorkshop(c, ctx, "test-workshop", "instance-id") + + owns, err := s.manager.OwnsWorkshopInstanceID(ctx, "unknown-id") + + c.Assert(err, check.IsNil) + c.Check(owns, check.Equals, false) +} + +// TestOwnsWorkshopInstanceIDReturnsFalseForEmptyID checks that an empty +// instance ID is not considered to be owned by the user. +func (s *managerSuite) TestOwnsWorkshopInstanceIDReturnsFalseForEmptyID( + c *check.C, +) { + ctx := context.WithValue( + context.Background(), + workshop.ContextUser, + "test-user", + ) + + owns, err := s.manager.OwnsWorkshopInstanceID(ctx, "") + + c.Assert(err, check.IsNil) + c.Check(owns, check.Equals, false) +} + +// TestOwnsWorkshopInstanceIDReturnsFalseForAnotherUsersWorkshop checks that +// workshop ownership is scoped to the user in context. +func (s *managerSuite) TestOwnsWorkshopInstanceIDReturnsFalseForAnotherUsersWorkshop( + c *check.C, +) { + ownerCtx := context.WithValue( + context.Background(), + workshop.ContextUser, + "owner", + ) + s.addWorkshop(c, ownerCtx, "test-workshop", "instance-id") + requestCtx := context.WithValue( + context.Background(), + workshop.ContextUser, + "another-user", + ) + + owns, err := s.manager.OwnsWorkshopInstanceID(requestCtx, "instance-id") + + c.Assert(err, check.IsNil) + c.Check(owns, check.Equals, false) +} From 5e11b0800964db4ea1d366a4da337661f8f05780 Mon Sep 17 00:00:00 2001 From: tlm Date: Mon, 7 Sep 2026 04:05:11 +0000 Subject: [PATCH 4/5] Use a named context key type in ctlcmd test Avoid anonymous struct context keys in TestRunPassesContextToCommand to satisfy staticcheck SA1029 and prevent key collisions. --- internal/overlord/hookstate/ctlcmd/ctlcmd_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/overlord/hookstate/ctlcmd/ctlcmd_test.go b/internal/overlord/hookstate/ctlcmd/ctlcmd_test.go index 2e9ddcbe8..3efb722a0 100644 --- a/internal/overlord/hookstate/ctlcmd/ctlcmd_test.go +++ b/internal/overlord/hookstate/ctlcmd/ctlcmd_test.go @@ -83,7 +83,8 @@ func (s *ctlcmdSuite) TestRunPassesContextToCommand(c *C) { mockCommand := ctlcmd.AddMockCommand("mock") defer ctlcmd.RemoveCommand("mock") - key := struct{}{} + type contextKey struct{} + key := contextKey{} ctx := context.WithValue(context.Background(), key, "test") called := false mockCommand.ExecuteFunc = func(ctx context.Context, _ []string) error { From 12adcec87151afaf29c84a4713697cdfce8cb9a0 Mon Sep 17 00:00:00 2001 From: tlm Date: Mon, 7 Sep 2026 04:38:35 +0000 Subject: [PATCH 5/5] Remove unused persisted-cookie fallback from workshopctl No production code writes workshop-cookies to state, so the fallback cannot resolve contexts during normal operation. Ordinary workshopctl requests now use the validated instance-ID path and NewEphemeralContext. Resolve hook context IDs only against active contexts and return a generic HTTP 400 for unknown IDs. This removes the state lookup that could expose internal errors to callers and misclassify them as client errors. Return a plain error because no caller needs a sentinel. Rename the request helper to workshopctlHookContextFromContextID to match the context-id field. Replace the artificial persisted-cookie fixture with coverage for active lookup and rejection of unknown cookies, including when a valid instance ID is supplied. --- internal/daemon/api_workshopctl.go | 13 ++++++----- internal/daemon/api_workshopctl_test.go | 26 ++++++++++++--------- internal/overlord/hookstate/manager.go | 26 +++------------------ internal/overlord/hookstate/manager_test.go | 26 +++++++++++++++++++++ 4 files changed, 51 insertions(+), 40 deletions(-) diff --git a/internal/daemon/api_workshopctl.go b/internal/daemon/api_workshopctl.go index 63fe228bb..a12325c59 100644 --- a/internal/daemon/api_workshopctl.go +++ b/internal/daemon/api_workshopctl.go @@ -101,21 +101,22 @@ func workshopctlHookContext( contextID string, ) (*hookstate.Context, Response) { if contextID != "" { - return workshopctlHookContextFromCookie(c, contextID) + return workshopctlHookContextFromContextID(c, contextID) } return workshopctlHookContextFromInstanceID(c, r) } -// workshopctlHookContextFromCookie returns the active or long-lived context -// identified by the supplied workshop cookie. An invalid cookie is rejected -// rather than falling back to instance ID authentication. -func workshopctlHookContextFromCookie( +// workshopctlHookContextFromContextID returns the active hook context +// identified by the request's context ID, supplied to hooks as WORKSHOP_COOKIE. +// An invalid context ID is rejected rather than falling back to instance ID +// authentication. +func workshopctlHookContextFromContextID( c *Command, contextID string, ) (*hookstate.Context, Response) { hookContext, err := c.d.overlord.HookManager().Context(contextID) if err != nil { - return nil, statusBadRequest("cannot get workshop context: %w", err) + return nil, statusBadRequest("invalid workshop cookie") } return hookContext, nil } diff --git a/internal/daemon/api_workshopctl_test.go b/internal/daemon/api_workshopctl_test.go index 1d6da7ae3..700186114 100644 --- a/internal/daemon/api_workshopctl_test.go +++ b/internal/daemon/api_workshopctl_test.go @@ -125,24 +125,28 @@ func (s *apiSuite) TestWorkshopCtlAcceptsOwnedInstanceID(c *check.C) { c.Check(rsp.Status, check.Equals, http.StatusOK) } -// TestWorkshopCtlCookieSkipsInstanceIDValidation checks that a valid hook -// cookie remains the authentication mechanism and does not require an instance -// ID lookup. -func (s *apiSuite) TestWorkshopCtlCookieSkipsInstanceIDValidation(c *check.C) { +// TestWorkshopCtlRejectsUnknownCookie checks that an invalid cookie produces +// a generic client error even when a valid workshop instance ID is supplied. +func (s *apiSuite) TestWorkshopCtlRejectsUnknownCookie(c *check.C) { s.daemon(c) - st := s.d.overlord.State() - st.Lock() - st.Set("workshop-cookies", map[string]string{"cookie-id": "test-workshop"}) - st.Unlock() - + s.addWorkshopWithInstanceID("instance-id") wctl := apiCmd("/v1/workshopctl") buf := bytes.NewBufferString( - `{"context-id":"cookie-id","args":["get-secret","sdk.secret"]}`, + `{"context-id":"unknown-cookie","args":["get-secret","sdk.secret"]}`, ) req, err := s.createProjectsRequest("POST", "/v1/workshopctl", buf) c.Assert(err, check.IsNil) + req = req.WithContext(context.WithValue( + req.Context(), + workshop.ContextWorkshopInstanceID, + "instance-id", + )) rsp := v1PostWorkshopCtl(wctl, req, nil).(*resp) - c.Check(rsp.Status, check.Equals, http.StatusOK) + c.Check(rsp.Status, check.Equals, http.StatusBadRequest) + c.Check(rsp.Type, check.Equals, ResponseTypeError) + c.Assert(rsp.Result, check.FitsTypeOf, &errorResult{}) + c.Check(rsp.Result.(*errorResult).Message, check.Equals, + "invalid workshop cookie") } diff --git a/internal/overlord/hookstate/manager.go b/internal/overlord/hookstate/manager.go index 25334dd0d..61de27825 100644 --- a/internal/overlord/hookstate/manager.go +++ b/internal/overlord/hookstate/manager.go @@ -15,7 +15,7 @@ package hookstate import ( - "fmt" + "errors" "regexp" "sync" "time" @@ -107,34 +107,14 @@ func (m *HookManager) NewEphemeralContext() (*Context, error) { return NewContext(nil, m.state, &HookSetup{}, nil, "") } -func (m *HookManager) ephemeralContext(cookieID string) (context *Context, err error) { - var contexts map[string]string - m.state.Lock() - defer m.state.Unlock() - err = m.state.Get("workshop-cookies", &contexts) - if err != nil { - return nil, fmt.Errorf("cannot get workshop cookies: %v", err) - } - if _, ok := contexts[cookieID]; ok { - // create new ephemeral context - context, err = NewContext(nil, m.state, &HookSetup{}, nil, cookieID) - return context, err - } - return nil, fmt.Errorf("invalid workshop cookie requested") -} - -// Context obtains the context for the given cookie ID. +// Context obtains the active hook context for the given cookie ID. func (m *HookManager) Context(cookieID string) (*Context, error) { m.contextsMutex.RLock() defer m.contextsMutex.RUnlock() - var err error context, ok := m.contexts[cookieID] if !ok { - context, err = m.ephemeralContext(cookieID) - if err != nil { - return nil, err - } + return nil, errors.New("invalid workshop cookie requested") } return context, nil diff --git a/internal/overlord/hookstate/manager_test.go b/internal/overlord/hookstate/manager_test.go index bf74acd30..cc70cbc14 100644 --- a/internal/overlord/hookstate/manager_test.go +++ b/internal/overlord/hookstate/manager_test.go @@ -32,6 +32,32 @@ func (s *managerSuite) SetUpTest(*C) { s.manager = &HookManager{state: s.state} } +// TestContextReturnsActiveContext checks that cookie lookup returns the +// registered hook context without requiring persisted cookies. +func (s *managerSuite) TestContextReturnsActiveContext(c *C) { + s.state.Lock() + task := s.state.NewTask("run-hook", "Run test hook") + s.state.Unlock() + active, err := NewContext(task, s.state, &HookSetup{}, nil, "cookie-id") + c.Assert(err, IsNil) + s.manager.contexts = map[string]*Context{active.ID(): active} + + ctx, err := s.manager.Context(active.ID()) + + c.Assert(err, IsNil) + c.Check(ctx, Equals, active) + c.Check(ctx.IsEphemeral(), Equals, false) +} + +// TestContextRejectsUnknownCookie checks that an unregistered cookie returns +// an invalid-cookie error rather than a state lookup error. +func (s *managerSuite) TestContextRejectsUnknownCookie(c *C) { + ctx, err := s.manager.Context("unknown-cookie") + + c.Check(ctx, IsNil) + c.Check(err, ErrorMatches, "invalid workshop cookie requested") +} + // TestNewEphemeralContext checks that the manager creates a context with its // state but without a hook task, which is the contract required by // cookie-less workshopctl requests after their instance ID is validated.