ref/profiles/internal/rt: Ensure the agents client doesn't close before it should.
Currently many objects depend on the principal to finish their cleanup.
Unfortunately we didn't record this dependency becuase non-agent principals
don't get cleaned up, and therefore, don't need to be tracked. The agent
principal, however, uses a client which will get closed at some point. We
need to ensure that the client doesn't get closed before any clients and servers
using the principal are finished.
This CL introduces a dependency arc between the agent principal and the client
and between new clients and servers and their pricipals.
There is still an issue if a user sets a principal, we don't know its dependencies.
If people end up making their own principals this issue will resurface.
Change-Id: Ie9298f25b8a40bfe9815aa6c4cc31eeaf37f540d
diff --git a/profiles/internal/rt/runtime.go b/profiles/internal/rt/runtime.go
index ac33df5..91aaabe 100644
--- a/profiles/internal/rt/runtime.go
+++ b/profiles/internal/rt/runtime.go
@@ -144,7 +144,9 @@
// The client we create here is incomplete (has a nil principal) and only works
// because the agent uses anonymous unix sockets and VCSecurityNone.
// After security is initialized we attach a real client.
- ctx, client, err := r.SetNewClient(ctx)
+ // We do not capture the ctx here on purpose, to avoid anyone accidentally
+ // using this client anywhere.
+ _, client, err := r.SetNewClient(ctx)
if err != nil {
return nil, nil, nil, err
}
@@ -154,7 +156,12 @@
if err != nil {
return nil, nil, nil, err
}
- ctx = r.setPrincipal(ctx, principal)
+ // If the principal is an agent principal, it depends on the client created
+ // above. If not, there's no harm in the dependency.
+ ctx, err = r.setPrincipal(ctx, principal, client)
+ if err != nil {
+ return nil, nil, nil, err
+ }
// Set up secure client.
ctx, _, err = r.SetNewClient(ctx)
@@ -253,7 +260,11 @@
}
sm.Shutdown()
}
- if err = r.addChild(ctx, server, stop, client, vtraceDependency{}); err != nil {
+ deps := []interface{}{client, vtraceDependency{}}
+ if principal != nil {
+ deps = append(deps, principal)
+ }
+ if err = r.addChild(ctx, server, stop, deps...); err != nil {
return nil, err
}
return server, nil
@@ -303,20 +314,27 @@
return newctx, nil
}
-func (*Runtime) setPrincipal(ctx *context.T, principal security.Principal) *context.T {
+func (r *Runtime) setPrincipal(ctx *context.T, principal security.Principal, deps ...interface{}) (*context.T, error) {
// We uniquely identity a principal with "security/principal/<publicKey>"
principalName := "security/principal/" + principal.PublicKey().String()
stats.NewStringFunc(principalName+"/blessingstore", principal.BlessingStore().DebugString)
stats.NewStringFunc(principalName+"/blessingroots", principal.Roots().DebugString)
- return context.WithValue(ctx, principalKey, principal)
+ ctx = context.WithValue(ctx, principalKey, principal)
+ return ctx, r.addChild(ctx, principal, func() {}, deps...)
}
func (r *Runtime) SetPrincipal(ctx *context.T, principal security.Principal) (*context.T, error) {
var err error
newctx := ctx
- newctx = r.setPrincipal(ctx, principal)
-
+ // TODO(mattr, suharshs): If there user gives us some principal that has dependencies
+ // we don't know about, we will not honour those dependencies during shutdown.
+ // For example if they create an agent principal with some client, we don't know
+ // about that, so servers based of this new principal will not prevent the client
+ // from terminating early.
+ if newctx, err = r.setPrincipal(ctx, principal); err != nil {
+ return ctx, err
+ }
if newctx, err = r.setNewStreamManager(newctx); err != nil {
return ctx, err
}
@@ -352,7 +370,11 @@
return ctx, nil, err
}
newctx := context.WithValue(ctx, clientKey, client)
- if err = r.addChild(ctx, client, client.Close, sm, vtraceDependency{}); err != nil {
+ deps := []interface{}{sm, vtraceDependency{}}
+ if p != nil {
+ deps = append(deps, p)
+ }
+ if err = r.addChild(ctx, client, client.Close, deps...); err != nil {
return ctx, nil, err
}
return newctx, client, err
