Merge "veyron/services/mgmt/suidhelper: put each application in a new session group"
diff --git a/lib/modules/shell.go b/lib/modules/shell.go
index c6cd898..cc14137 100644
--- a/lib/modules/shell.go
+++ b/lib/modules/shell.go
@@ -136,7 +136,7 @@
return sh, nil
}
-func (sh *Shell) getChildCredentials() (*os.File, error) {
+func (sh *Shell) getChildCredentials() (c *os.File, err error) {
if sh.ctx == nil {
return nil, nil
}
@@ -146,11 +146,16 @@
if err != nil {
return nil, err
}
+ defer func() {
+ if err != nil {
+ conn.Close()
+ }
+ }()
ctx, cancel := context.WithCancel(sh.ctx)
+ defer cancel()
if ctx, _, err = veyron2.SetNewStreamManager(ctx); err != nil {
return nil, err
}
- defer cancel()
syscall.ForkLock.RLock()
fd, err := syscall.Dup(int(conn.Fd()))
if err != nil {
@@ -161,6 +166,7 @@
syscall.ForkLock.RUnlock()
p, err := agent.NewAgentPrincipal(ctx, fd, veyron2.GetClient(ctx))
if err != nil {
+ syscall.Close(fd)
return nil, err
}
blessingForChild, err := root.Bless(p.PublicKey(), rootBlessing, childBlessingExtension, security.UnconstrainedUse())
diff --git a/lib/stats/sysstats/sysstats.go b/lib/stats/sysstats/sysstats.go
index 75c0c17..9579280 100644
--- a/lib/stats/sysstats/sysstats.go
+++ b/lib/stats/sysstats/sysstats.go
@@ -21,6 +21,7 @@
stats.NewInteger("system/num-cpu").Set(int64(runtime.NumCPU()))
stats.NewIntegerFunc("system/num-goroutine", func() int64 { return int64(runtime.NumGoroutine()) })
stats.NewString("system/version").Set(runtime.Version())
+ stats.NewInteger("system/pid").Set(int64(os.Getpid()))
if hostname, err := os.Hostname(); err == nil {
stats.NewString("system/hostname").Set(hostname)
}
diff --git a/lib/stats/sysstats/sysstats_test.go b/lib/stats/sysstats/sysstats_test.go
index ee2f835..d157b5e 100644
--- a/lib/stats/sysstats/sysstats_test.go
+++ b/lib/stats/sysstats/sysstats_test.go
@@ -31,3 +31,14 @@
t.Errorf("unexpected Alloc value. Got %v, want != 0", v)
}
}
+
+func TestPid(t *testing.T) {
+ obj, err := stats.GetStatsObject("system/pid")
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ expected := int64(os.Getpid())
+ if got := obj.Value(); got != expected {
+ t.Errorf("unexpected result. Got %q, want %q", got, expected)
+ }
+}
diff --git a/lib/unixfd/unixfd.go b/lib/unixfd/unixfd.go
index fd17a93..fa0fe85 100644
--- a/lib/unixfd/unixfd.go
+++ b/lib/unixfd/unixfd.go
@@ -231,12 +231,16 @@
// This is to work around a race on OS X where it appears we can close
// the file descriptor before it gets transfered over the socket.
f := local.releaseFile()
+ syscall.ForkLock.Lock()
fd, err := syscall.Dup(int(f.Fd()))
if err != nil {
+ syscall.ForkLock.Unlock()
f.Close()
rfile.Close()
return nil, err
}
+ syscall.CloseOnExec(fd)
+ syscall.ForkLock.Unlock()
newConn, err := net.FileConn(f)
f.Close()
if err != nil {
@@ -290,11 +294,15 @@
return nil, n, nil, nil
}
result := Addr(uintptr(fd))
+ syscall.ForkLock.Lock()
fd, err = syscall.Dup(fd)
if err != nil {
+ syscall.ForkLock.Unlock()
CloseUnixAddr(result)
return nil, n, nil, err
}
+ syscall.CloseOnExec(fd)
+ syscall.ForkLock.Unlock()
file := os.NewFile(uintptr(fd), "newconn")
newconn, err := net.FileConn(file)
file.Close()
diff --git a/runtimes/google/ipc/client.go b/runtimes/google/ipc/client.go
index 628e1d3..c0d41df 100644
--- a/runtimes/google/ipc/client.go
+++ b/runtimes/google/ipc/client.go
@@ -79,6 +79,9 @@
errAuthNoPatternMatch = verror.Register(pkgPath+".authNoPatternMatch",
verror.NoRetry, "server blessings {3} do not match pattern {4}")
+ errAuthServerNotAllowed = verror.Register(pkgPath+".authServerNotAllowed",
+ verror.NoRetry, "set of allowed servers {3} not matched by server blessings {4}")
+
errDefaultAuthDenied = verror.Register(pkgPath+".defaultAuthDenied", verror.NoRetry, "default authorization precludes talking to server with blessings{:3}")
errBlessingGrant = verror.Register(pkgPath+".blessingGrantFailed", verror.NoRetry, "failed to grant blessing to server with blessings {3}{:4}")
@@ -661,6 +664,17 @@
}
for _, o := range opts {
switch v := o.(type) {
+ case options.AllowedServersPolicy:
+ allowed := false
+ for _, p := range v {
+ if p.MatchedBy(serverBlessings...) {
+ allowed = true
+ break
+ }
+ }
+ if !allowed {
+ return nil, nil, verror.Make(errAuthServerNotAllowed, ctx, v, serverBlessings)
+ }
case ipc.Granter:
if b, err := v.Grant(flow.RemoteBlessings()); err != nil {
return nil, nil, verror.Make(errBlessingGrant, ctx, serverBlessings, err)
diff --git a/runtimes/google/ipc/full_test.go b/runtimes/google/ipc/full_test.go
index 45f3a61..f46b0b7 100644
--- a/runtimes/google/ipc/full_test.go
+++ b/runtimes/google/ipc/full_test.go
@@ -6,7 +6,6 @@
"fmt"
"io"
"net"
- "os"
"path/filepath"
"reflect"
"runtime"
@@ -352,10 +351,8 @@
}
func matchesErrorPattern(err error, id verror.IDAction, pattern string) bool {
- if len(pattern) > 0 && err != nil {
- if strings.Index(err.Error(), pattern) < 0 {
- fmt.Fprintf(os.Stderr, "got error msg: %q, expected: %q\n", err, pattern)
- }
+ if len(pattern) > 0 && err != nil && strings.Index(err.Error(), pattern) < 0 {
+ return false
}
if err == nil && id.ID == "" {
return true
@@ -427,73 +424,91 @@
func TestRPCServerAuthorization(t *testing.T) {
const (
- vcErr = "VC handshake failed"
- nameErr = "do not match pattern"
+ vcErr = "VC handshake failed"
+ nameErr = "do not match pattern"
+ allowedErr = "set of allowed servers"
)
var (
pprovider, pclient, pserver = tsecurity.NewPrincipal("root"), tsecurity.NewPrincipal(), tsecurity.NewPrincipal()
pdischarger = pprovider
-
- now = time.Now()
-
- serverName = "mountpoint/server"
- dischargeServerName = "mountpoint/dischargeserver"
+ now = time.Now()
+ noErrID verror.IDAction
// Third-party caveats on blessings presented by server.
- cavTPValid = mkThirdPartyCaveat(pdischarger.PublicKey(), dischargeServerName, mkCaveat(security.ExpiryCaveat(now.Add(24*time.Hour))))
- cavTPExpired = mkThirdPartyCaveat(pdischarger.PublicKey(), dischargeServerName, mkCaveat(security.ExpiryCaveat(now.Add(-1*time.Second))))
+ cavTPValid = mkThirdPartyCaveat(pdischarger.PublicKey(), "mountpoint/dischargeserver", mkCaveat(security.ExpiryCaveat(now.Add(24*time.Hour))))
+ cavTPExpired = mkThirdPartyCaveat(pdischarger.PublicKey(), "mountpoint/dischargeserver", mkCaveat(security.ExpiryCaveat(now.Add(-1*time.Second))))
// Server blessings.
bServer = bless(pprovider, pserver, "server")
bServerExpired = bless(pprovider, pserver, "server", mkCaveat(security.ExpiryCaveat(time.Now().Add(-1*time.Second))))
bServerTPValid = bless(pprovider, pserver, "serverWithTPCaveats", cavTPValid)
bServerTPExpired = bless(pprovider, pserver, "serverWithTPCaveats", cavTPExpired)
+ bTwoBlessings, _ = security.UnionOfBlessings(bServer, bServerTPValid)
mgr = imanager.InternalNew(naming.FixedRoutingID(0x1111111))
ns = tnaming.NewSimpleNamespace()
tests = []struct {
- server security.Blessings // blessings presented by the server to the client.
- pattern security.BlessingPattern // pattern on the server identity expected by the client.
+ server security.Blessings // blessings presented by the server to the client.
+ name string // name provided by the client to StartCall
+ allowed options.AllowedServersPolicy // option provided to StartCall.
errID verror.IDAction
err string
}{
- // Client accepts talking to the server only if the server's blessings match the provided pattern
- {bServer, security.AllPrincipals, verror.IDAction{}, ""},
- {bServer, "root/server", verror.IDAction{}, ""},
- {bServer, "root/otherserver", verror.NotTrusted, nameErr},
- {bServer, "otherroot/server", verror.NotTrusted, nameErr},
+ // Client accepts talking to the server only if the
+ // server's blessings match the provided pattern
+ {bServer, "mountpoint/server", nil, noErrID, ""},
+ {bServer, "[root/server]mountpoint/server", nil, noErrID, ""},
+ {bServer, "[root/otherserver]mountpoint/server", nil, verror.NotTrusted, nameErr},
+ {bServer, "[otherroot/server]mountpoint/server", nil, verror.NotTrusted, nameErr},
- // and, if the server's blessing has third-party caveats then the server provides
- // appropriate discharges.
- {bServerTPValid, security.AllPrincipals, verror.IDAction{}, ""},
- {bServerTPValid, "root/serverWithTPCaveats", verror.IDAction{}, ""},
- {bServerTPValid, "root/otherserver", verror.NotTrusted, nameErr},
- {bServerTPValid, "otherroot/server", verror.NotTrusted, nameErr},
+ // and, if the server's blessing has third-party
+ // caveats then the server provides appropriate
+ // discharges.
+ {bServerTPValid, "mountpoint/server", nil, noErrID, ""},
+ {bServerTPValid, "[root/serverWithTPCaveats]mountpoint/server", nil, noErrID, ""},
+ {bServerTPValid, "[root/otherserver]mountpoint/server", nil, verror.NotTrusted, nameErr},
+ {bServerTPValid, "[otherroot/server]mountpoint/server", nil, verror.NotTrusted, nameErr},
- // Client does not talk to a server that presents expired blessings.
- {bServerExpired, security.AllPrincipals, verror.NotTrusted, vcErr},
+ // Client does not talk to a server that presents
+ // expired blessings (because the blessing store is
+ // configured to only talk to root/...).
+ {bServerExpired, "mountpoint/server", nil, verror.NotTrusted, vcErr},
- // Client does not talk to a server that fails to provide discharges for
- // third-party caveats on the blessings presented by it.
- {bServerTPExpired, security.AllPrincipals, verror.NotTrusted, vcErr},
+ // Client does not talk to a server that fails to
+ // provide discharges for third-party caveats on the
+ // blessings presented by it.
+ {bServerTPExpired, "mountpoint/server", nil, verror.NotTrusted, vcErr},
+
+ // Testing the AllowedServersPolicy option.
+ {bServer, "mountpoint/server", options.AllowedServersPolicy{"otherroot/..."}, verror.NotTrusted, allowedErr},
+ {bServer, "[root/server]mountpoint/server", options.AllowedServersPolicy{"otherroot/..."}, verror.NotTrusted, allowedErr},
+ {bServer, "[otherroot/server]mountpoint/server", options.AllowedServersPolicy{"root/server"}, verror.NotTrusted, nameErr},
+ {bServer, "[root/server]mountpoint/server", options.AllowedServersPolicy{"root/..."}, noErrID, ""},
+ // Server presents two blessings: One that satisfies
+ // the pattern provided to StartCall and one that
+ // satisfies the AllowedServersPolicy, so the server is
+ // authorized.
+ {bTwoBlessings, "[root/serverWithTPCaveats]mountpoint/server", options.AllowedServersPolicy{"root/server"}, noErrID, ""},
}
)
- _, server := startServer(t, pserver, mgr, ns, serverName, testServerDisp{&testServer{}})
- defer stopServer(t, server, ns, serverName)
+ _, server := startServer(t, pserver, mgr, ns, "mountpoint/server", testServerDisp{&testServer{}})
+ defer stopServer(t, server, ns, "mountpoint/server")
// Start the discharge server.
- _, dischargeServer := startServer(t, pdischarger, mgr, ns, dischargeServerName, testutil.LeafDispatcher(&dischargeServer{}, &acceptAllAuthorizer{}))
- defer stopServer(t, dischargeServer, ns, dischargeServerName)
+ _, dischargeServer := startServer(t, pdischarger, mgr, ns, "mountpoint/dischargeserver", testutil.LeafDispatcher(&dischargeServer{}, &acceptAllAuthorizer{}))
+ defer stopServer(t, dischargeServer, ns, "mountpoint/dischargeserver")
- // Make the client and server principals trust root certificates from pprovider
+ // Make the client and server principals trust root certificates from
+ // pprovider
pclient.AddToRoots(pprovider.BlessingStore().Default())
pserver.AddToRoots(pprovider.BlessingStore().Default())
- // Set a blessing that the client is willing to share with servers with blessings
- // from pprovider.
+ // Set a blessing that the client is willing to share with servers with
+ // blessings from pprovider.
pclient.BlessingStore().Set(bless(pprovider, pclient, "client"), "root/...")
+
for i, test := range tests {
- name := fmt.Sprintf("(%q@%q)", test.pattern, test.server)
+ name := fmt.Sprintf("(Name:%q, Server:%q, Allowed:%v)", test.name, test.server, test.allowed)
if err := pserver.BlessingStore().SetDefault(test.server); err != nil {
t.Fatalf("SetDefault failed on server's BlessingStore: %v", err)
}
@@ -506,9 +521,12 @@
t.Errorf("%s: failed to create client: %v", name, err)
continue
}
- ctx := testContextWithoutDeadline()
- dctx, cancel := context.WithTimeout(ctx, 10*time.Second)
- call, err := client.StartCall(dctx, fmt.Sprintf("[%s]%s/suffix", test.pattern, serverName), "Method", nil)
+ ctx, cancel := context.WithTimeout(testContextWithoutDeadline(), 10*time.Second)
+ var opts []ipc.CallOpt
+ if test.allowed != nil {
+ opts = append(opts, test.allowed)
+ }
+ call, err := client.StartCall(ctx, test.name, "Method", nil, opts...)
if !matchesErrorPattern(err, test.errID, test.err) {
t.Errorf(`%d: %s: client.StartCall: got error "%v", want to match "%v"`, i, name, err, test.err)
} else if call != nil {
@@ -516,8 +534,12 @@
if proof == nil {
t.Errorf("%s: Returned nil for remote blessings", name)
}
- if !test.pattern.MatchedBy(blessings...) {
- t.Errorf("%s: %q.MatchedBy(%v) failed", name, test.pattern, blessings)
+ // Currently all tests are configured so that the only
+ // blessings presented by the server that are
+ // recognized by the client match the pattern
+ // "root/..."
+ if len(blessings) < 1 || !security.BlessingPattern("root/...").MatchedBy(blessings...) {
+ t.Errorf("%s: Client sees server as %v, expected a single blessing matching root/...", name, blessings)
}
}
cancel()
diff --git a/security/agent/client.go b/security/agent/client.go
index f3128f9..77c6dde 100644
--- a/security/agent/client.go
+++ b/security/agent/client.go
@@ -57,7 +57,8 @@
// NewAgentPrincipal returns a security.Pricipal using the PrivateKey held in a remote agent process.
// 'fd' is the socket for connecting to the agent, typically obtained from
// os.GetEnv(agent.FdVarName).
-// 'ctx' should not have a deadline, and should never be cancelled.
+// 'ctx' should not have a deadline, and should never be cancelled while the
+// principal is in use.
func NewAgentPrincipal(ctx *context.T, fd int, insecureClient ipc.Client) (security.Principal, error) {
f := os.NewFile(uintptr(fd), "agent_client")
defer f.Close()
diff --git a/services/mgmt/device/impl/app_service.go b/services/mgmt/device/impl/app_service.go
index 1538c98..1a49e46 100644
--- a/services/mgmt/device/impl/app_service.go
+++ b/services/mgmt/device/impl/app_service.go
@@ -124,7 +124,6 @@
"reflect"
"strconv"
"strings"
- "sync"
"time"
"v.io/core/veyron2"
@@ -189,12 +188,6 @@
type securityAgentState struct {
// Security agent key manager client.
keyMgrAgent *keymgr.Agent
- // Ensures only one security agent connection socket is created
- // at any time, preventing fork/exec from potentially passing
- // down sockets meant for other children (as per ribrdb@, Go's
- // exec implementation does not prune the set of files passed
- // down to only include those specified in cmd.ExtraFiles).
- startLock sync.Mutex
}
// appService implements the Device manager's Application interface.
@@ -483,14 +476,23 @@
vlog.Errorf("NewPrincipal() failed %v", err)
return verror2.Make(ErrOperationFailed, nil)
}
- defer conn.Close()
-
- // TODO(caprita): release the socket created by NewAgentPrincipal.
- if p, err = agent.NewAgentPrincipal(ctx, int(conn.Fd()), veyron2.GetClient(ctx)); err != nil {
+ agentctx, cancel := context.WithCancel(ctx)
+ if agentctx, _, err = veyron2.SetNewStreamManager(agentctx); err != nil {
+ cancel()
+ conn.Close()
+ vlog.Errorf("SetNewStreamManager failed: %v", err)
+ return verror2.Make(ErrOperationFailed, nil)
+ }
+ defer cancel()
+ if p, err = agent.NewAgentPrincipal(agentctx, int(conn.Fd()), veyron2.GetClient(agentctx)); err != nil {
+ conn.Close()
vlog.Errorf("NewAgentPrincipal() failed: %v", err)
return verror2.Make(ErrOperationFailed, nil)
}
info.SecurityAgentHandle = handle
+ // conn will be closed when the connection to the agent is shut
+ // down, as a result of cancel() shutting down the stream
+ // manager. No need to call conn.Close().
} else {
credentialsDir := filepath.Join(instanceDir, "credentials")
// TODO(caprita): The app's system user id needs access to this dir.
@@ -771,24 +773,16 @@
cfg.Set(mgmt.ParentBlessingConfigKey, info.DeviceManagerPeerPattern)
// Set up any agent-specific state.
- // NOTE(caprita): This ought to belong in genCmd, but we do it here
- // to avoid holding on to the lock for too long.
- //
- // TODO(caprita): We need to take care to grab/release the lock
- // excluding concurrent start operations. See if we can make this more
- // robust.
+ // NOTE(caprita): This ought to belong in genCmd.
var agentCleaner func()
if sa := i.securityAgent; sa != nil {
- sa.startLock.Lock()
file, err := sa.keyMgrAgent.NewConnection(info.SecurityAgentHandle)
if err != nil {
- sa.startLock.Unlock()
vlog.Errorf("NewConnection(%v) failed: %v", info.SecurityAgentHandle, err)
return err
}
agentCleaner = func() {
file.Close()
- sa.startLock.Unlock()
}
// We need to account for the file descriptors corresponding to
// std{err|out|in} as well as the implementation-specific pipes
