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 <[email protected]>

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)
  }

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

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")

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()