From 0940f0cef62e518dbd045134b5bf159392ea6ad2 Mon Sep 17 00:00:00 2001 From: Erik Sipsma Date: Mon, 29 Apr 2024 19:54:41 -0700 Subject: [PATCH] deregister secret tokens once client disconnects We previously never explicitly removed client ID -> secret token mappings because it theoretically opened more possibilities for malicious attempts to register a client ID with a different token. However, we need to deregister these now since Client IDs are a content hash of the function call/nested exec definition, which means the same client ID can connect and disconnect multiple times per server. The security implications of this also end up being extremely minimal. Registering a client ID with a different secret token was and still is possible *before* a client fully connects. It is possible to after a client disconnects now but this would only amount to a DOS since the "real" client would just be unable to connect. No information would be leaked. It also would have to be in the same server (i.e. a module or nested exec called by the main client directly or transitively). This issue can also be squashed by not leaking the buildkit sock to nested execs/modules, which is possible now by migrating functionality from our shim to our custom executor. There's no immediate plans to do this but the possibility is open whenever needed (or when we make that change for other reasons). Signed-off-by: Erik Sipsma --- core/container.go | 9 ++++++++- core/query.go | 7 +++++-- engine/server/buildkitcontroller.go | 6 ++++++ engine/server/server.go | 11 +++++++---- 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/core/container.go b/core/container.go index 9d2c976d8b3..e8e82c0d66e 100644 --- a/core/container.go +++ b/core/container.go @@ -1007,7 +1007,14 @@ func (container *Container) WithExec(ctx context.Context, opts ContainerExecOpts // this allows executed containers to communicate back to this API if opts.ExperimentalPrivilegedNesting { - clientID, err := container.Query.RegisterCaller(ctx, opts.NestedExecFunctionCall) + callerOpts := opts.NestedExecFunctionCall + if callerOpts == nil { + // default to caching the nested exec + callerOpts = &FunctionCall{ + Cache: true, + } + } + clientID, err := container.Query.RegisterCaller(ctx, callerOpts) if err != nil { return nil, fmt.Errorf("register caller: %w", err) } diff --git a/core/query.go b/core/query.go index be086d49d5f..22114d5cac8 100644 --- a/core/query.go +++ b/core/query.go @@ -18,6 +18,7 @@ import ( "github.com/dagger/dagger/dagql/call" "github.com/dagger/dagger/engine" "github.com/dagger/dagger/engine/buildkit" + "github.com/dagger/dagger/engine/slog" ) // Query forms the root of the DAG and houses all necessary state and @@ -154,8 +155,10 @@ func (q *Query) RegisterCaller(ctx context.Context, call *FunctionCall) (string, // also trim it to 25 chars as it ends up becoming part of service URLs clientID := clientIDDigest.Encoded()[:25] - // break glass for debugging which client is which operation - // bklog.G(ctx).Debugf("CLIENT ID %s = %s", clientID, currentID.Display()) + slog.ExtraDebug("registering nested caller", + "client_id", clientID, + "op", currentID.Display(), + ) if call.Module == nil { callCtx.Deps = q.DefaultDeps diff --git a/engine/server/buildkitcontroller.go b/engine/server/buildkitcontroller.go index a3ad5a58a20..450df5c6e92 100644 --- a/engine/server/buildkitcontroller.go +++ b/engine/server/buildkitcontroller.go @@ -267,6 +267,12 @@ func (e *BuildkitController) Session(stream controlapi.Control_SessionServer) (r if err != nil { return fmt.Errorf("failed to register client: %w", err) } + defer func() { + err := srv.UnregisterClient(opts.ClientID) + if err != nil { + slog.Error("failed to unregister client", "err", err) + } + }() eg.Go(func() error { bklog.G(ctx).Trace("waiting for server") diff --git a/engine/server/server.go b/engine/server/server.go index ffcb20465a4..f7bca64d2dd 100644 --- a/engine/server/server.go +++ b/engine/server/server.go @@ -382,10 +382,6 @@ func (s *DaggerServer) RegisterClient(clientID, clientHostname, secretToken stri return nil } s.clientIDToSecretToken[clientID] = secretToken - // NOTE: we purposely don't delete the secret token, it should never be reused and will be released - // from memory once the dagger server instance corresponding to this buildkit client shuts down. - // Deleting it would make it easier to create race conditions around using the client's session - // before it is fully closed. return nil } @@ -403,6 +399,13 @@ func (s *DaggerServer) VerifyClient(clientID, secretToken string) error { return nil } +func (s *DaggerServer) UnregisterClient(clientID string) error { + s.clientIDMu.Lock() + defer s.clientIDMu.Unlock() + delete(s.clientIDToSecretToken, clientID) + return nil +} + func (s *DaggerServer) ClientCallContext(clientID string) (*core.ClientCallContext, bool) { s.clientCallMu.RLock() defer s.clientCallMu.RUnlock()