"core": Get rid of prefix-matching on BlessingPatterns
Martin Abadi (@abadi) recently made a very convincing argument
that default prefix matching on BlessingPatterns leads
to surprising (and undesirable) properties on semantics of
'Deny' clauses and groups in ACLS.
The argument can be found here:
https://docs.google.com/a/google.com/document/d/1pvENXMkfKHMok1GNU8dy6jPPACzsJ9EjVyG642Pk1CI/edit
This CL gets rid of prefix-matching on BlessingPatterns
and fixes all relevant tests in the core repo. Fixes for other repos
will follow soon.
After this CL, a pattern "a/b/c" wil only be matched by
"a/b/c", "a/b/c/x", "a/b/c/y", "a/b/c/x/y", etc., and a
pattern "a/b/c/$" will only be matched by "a/b/c".
Change-Id: I8bbfe77479283a1b8cf6fd2cd8391e6aa8e2c2c9
diff --git a/security/blessingroots_test.go b/security/blessingroots_test.go
index a3bae58..c27f86d 100644
--- a/security/blessingroots_test.go
+++ b/security/blessingroots_test.go
@@ -52,8 +52,8 @@
},
{
root: t[1],
- recognized: []string{"google", "google/foo", "google/foo/bar"},
- notRecognized: []string{"google/bar", "veyron", "veyron/foo", "foo", "foo/bar"},
+ recognized: []string{"google/foo", "google/foo/bar"},
+ notRecognized: []string{"google", "google/bar", "veyron", "veyron/foo", "foo", "foo/bar"},
},
{
root: t[2],
diff --git a/services/mgmt/binary/impl/acl_test.go b/services/mgmt/binary/impl/acl_test.go
index 704a3fa..6a2767b 100644
--- a/services/mgmt/binary/impl/acl_test.go
+++ b/services/mgmt/binary/impl/acl_test.go
@@ -75,6 +75,65 @@
return nil
}
+func TestBinaryCreateACL(t *testing.T) {
+ ctx, shutdown := testutil.InitForTest()
+ defer shutdown()
+ veyron2.GetNamespace(ctx).CacheCtl(naming.DisableCache(true))
+
+ selfPrincipal := tsecurity.NewPrincipal("self")
+ selfCtx, err := veyron2.SetPrincipal(ctx, selfPrincipal)
+ if err != nil {
+ t.Fatalf("SetPrincipal failed: %v", err)
+ }
+ dir, childPrincipal := tsecurity.ForkCredentials(selfPrincipal, "child")
+ defer os.RemoveAll(dir)
+ childCtx, err := veyron2.SetPrincipal(ctx, childPrincipal)
+ if err != nil {
+ t.Fatalf("SetPrincipal failed: %v", err)
+ }
+
+ sh, deferFn := mgmttest.CreateShellAndMountTable(t, childCtx, veyron2.GetPrincipal(childCtx))
+ defer deferFn()
+ // make selfCtx and childCtx have the same Namespace Roots as set by
+ // CreateShellAndMountTable
+ veyron2.GetNamespace(selfCtx).SetRoots(veyron2.GetNamespace(childCtx).Roots()...)
+
+ // setup mock up directory to put state in
+ storedir, cleanup := mgmttest.SetupRootDir(t, "bindir")
+ defer cleanup()
+ prepDirectory(t, storedir)
+
+ _, nms := mgmttest.RunShellCommand(t, sh, nil, binaryCmd, "bini", storedir)
+ pid := mgmttest.ReadPID(t, nms)
+ defer syscall.Kill(pid, syscall.SIGINT)
+
+ vlog.VI(2).Infof("Self uploads a shared and private binary.")
+ binary := repository.BinaryClient("bini/private")
+ if err := binary.Create(childCtx, 1, repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
+ t.Fatalf("Create() failed %v", err)
+ }
+ fakeDataPrivate := testData()
+ if streamErr, err := invokeUpload(t, childCtx, binary, fakeDataPrivate, 0); streamErr != nil || err != nil {
+ t.Fatalf("invokeUpload() failed %v, %v", err, streamErr)
+ }
+
+ vlog.VI(2).Infof("Validate that the ACL also allows Self")
+ acl, _, err := binary.GetACL(selfCtx)
+ if err != nil {
+ t.Fatalf("GetACL failed: %v", err)
+ }
+ expected := access.TaggedACLMap{
+ "Admin": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}},
+ "Read": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}},
+ "Write": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}},
+ "Debug": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}},
+ "Resolve": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}},
+ }
+ if got, want := acl.Normalize(), expected.Normalize(); !reflect.DeepEqual(got, want) {
+ t.Errorf("got %#v, expected %#v ", got, want)
+ }
+}
+
func TestBinaryRootACL(t *testing.T) {
ctx, shutdown := testutil.InitForTest()
defer shutdown()
@@ -141,7 +200,13 @@
if err != nil {
t.Fatalf("GetACL failed: %v", err)
}
- expected := access.TaggedACLMap{"Admin": access.ACL{In: []security.BlessingPattern{"self"}, NotIn: []string{}}, "Read": access.ACL{In: []security.BlessingPattern{"self"}, NotIn: []string{}}, "Write": access.ACL{In: []security.BlessingPattern{"self"}, NotIn: []string{}}, "Debug": access.ACL{In: []security.BlessingPattern{"self"}, NotIn: []string{}}, "Resolve": access.ACL{In: []security.BlessingPattern{"self"}, NotIn: []string{}}}
+ expected := access.TaggedACLMap{
+ "Admin": access.ACL{In: []security.BlessingPattern{"self"}},
+ "Read": access.ACL{In: []security.BlessingPattern{"self"}},
+ "Write": access.ACL{In: []security.BlessingPattern{"self"}},
+ "Debug": access.ACL{In: []security.BlessingPattern{"self"}},
+ "Resolve": access.ACL{In: []security.BlessingPattern{"self"}},
+ }
if got, want := acl.Normalize(), expected.Normalize(); !reflect.DeepEqual(got, want) {
t.Errorf("got %#v, expected %#v ", got, want)
}
@@ -177,7 +242,13 @@
vlog.VI(2).Infof(" Verify that bini/private's acls are updated.")
binary = repository.BinaryClient("bini/private")
- updated := access.TaggedACLMap{"Admin": access.ACL{In: []security.BlessingPattern{"self/$"}, NotIn: []string{}}, "Read": access.ACL{In: []security.BlessingPattern{"self/$"}, NotIn: []string{}}, "Write": access.ACL{In: []security.BlessingPattern{"self/$"}, NotIn: []string{}}, "Debug": access.ACL{In: []security.BlessingPattern{"self/$"}, NotIn: []string{}}, "Resolve": access.ACL{In: []security.BlessingPattern{"self/$"}, NotIn: []string{}}}
+ updated := access.TaggedACLMap{
+ "Admin": access.ACL{In: []security.BlessingPattern{"self/$"}},
+ "Read": access.ACL{In: []security.BlessingPattern{"self/$"}},
+ "Write": access.ACL{In: []security.BlessingPattern{"self/$"}},
+ "Debug": access.ACL{In: []security.BlessingPattern{"self/$"}},
+ "Resolve": access.ACL{In: []security.BlessingPattern{"self/$"}},
+ }
acl, _, err = binary.GetACL(selfCtx)
if err != nil {
t.Fatalf("GetACL failed: %v", err)
@@ -239,19 +310,12 @@
vlog.VI(2).Infof("Other can read acls for bini/otherbinary.")
updated = access.TaggedACLMap{
- "Admin": access.ACL{
- In: []security.BlessingPattern{"self/other"},
- NotIn: []string{}},
- "Read": access.ACL{
- In: []security.BlessingPattern{"self/other"},
- NotIn: []string{}},
- "Write": access.ACL{
- In: []security.BlessingPattern{"self/other"},
- NotIn: []string{}},
- "Debug": access.ACL{In: []security.BlessingPattern{"self/other"},
- NotIn: []string{}},
- "Resolve": access.ACL{In: []security.BlessingPattern{"self/other"},
- NotIn: []string{}}}
+ "Admin": access.ACL{In: []security.BlessingPattern{"self/$", "self/other"}},
+ "Read": access.ACL{In: []security.BlessingPattern{"self/$", "self/other"}},
+ "Write": access.ACL{In: []security.BlessingPattern{"self/$", "self/other"}},
+ "Debug": access.ACL{In: []security.BlessingPattern{"self/$", "self/other"}},
+ "Resolve": access.ACL{In: []security.BlessingPattern{"self/$", "self/other"}},
+ }
acl, _, err = binary.GetACL(otherCtx)
if err != nil {
t.Fatalf("GetACL failed: %v", err)
@@ -342,11 +406,11 @@
t.Fatalf("GetACL failed: %#v", err)
}
expected := access.TaggedACLMap{
- "Admin": access.ACL{In: []security.BlessingPattern{"self/child"}, NotIn: []string{}},
- "Read": access.ACL{In: []security.BlessingPattern{"self/child"}, NotIn: []string{}},
- "Write": access.ACL{In: []security.BlessingPattern{"self/child"}, NotIn: []string{}},
- "Debug": access.ACL{In: []security.BlessingPattern{"self/child"}, NotIn: []string{}},
- "Resolve": access.ACL{In: []security.BlessingPattern{"self/child"}, NotIn: []string{}},
+ "Admin": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}, NotIn: []string{}},
+ "Read": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}, NotIn: []string{}},
+ "Write": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}, NotIn: []string{}},
+ "Debug": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}, NotIn: []string{}},
+ "Resolve": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}, NotIn: []string{}},
}
if got, want := acl.Normalize(), expected.Normalize(); !reflect.DeepEqual(got, want) {
t.Errorf("got %#v, expected %#v ", got, want)
@@ -363,11 +427,11 @@
t.Fatalf("GetACL failed: %#v", err)
}
expected = access.TaggedACLMap{
- "Admin": access.ACL{In: []security.BlessingPattern{"self/child"}, NotIn: []string{}},
- "Read": access.ACL{In: []security.BlessingPattern{"self/child"}, NotIn: []string{"self"}},
- "Write": access.ACL{In: []security.BlessingPattern{"self/child"}, NotIn: []string{}},
- "Debug": access.ACL{In: []security.BlessingPattern{"self/child"}, NotIn: []string{}},
- "Resolve": access.ACL{In: []security.BlessingPattern{"self/child"}, NotIn: []string{}},
+ "Admin": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}, NotIn: []string{}},
+ "Read": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}, NotIn: []string{"self"}},
+ "Write": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}, NotIn: []string{}},
+ "Debug": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}, NotIn: []string{}},
+ "Resolve": access.ACL{In: []security.BlessingPattern{"self/$", "self/child"}, NotIn: []string{}},
}
if got, want := acl.Normalize(), expected.Normalize(); !reflect.DeepEqual(got, want) {
t.Errorf("got %#v, expected %#v ", got, want)
diff --git a/services/mgmt/binary/impl/service.go b/services/mgmt/binary/impl/service.go
index f8b7586..ccc92b6 100644
--- a/services/mgmt/binary/impl/service.go
+++ b/services/mgmt/binary/impl/service.go
@@ -66,6 +66,7 @@
ErrInvalidParts = verror.Register(pkgPath+".errInvalidParts", verror.NoRetry, "{1:}{2:} invalid number of binary parts{:_}")
ErrInvalidPart = verror.Register(pkgPath+".errInvalidPart", verror.NoRetry, "{1:}{2:} invalid binary part number{:_}")
ErrOperationFailed = verror.Register(pkgPath+".errOperationFailed", verror.NoRetry, "{1:}{2:} operation failed{:_}")
+ ErrNotAuthorized = verror.Register(pkgPath+".errNotAuthorized", verror.NoRetry, "{1:}{2:} none of the client's blessings are valid {:_}")
)
// TODO(jsimsa): When VDL supports composite literal constants, remove
@@ -87,15 +88,23 @@
const BufferLength = 4096
+func prefixPatterns(blessings []string) []security.BlessingPattern {
+ var patterns []security.BlessingPattern
+ for _, b := range blessings {
+ patterns = append(patterns, security.BlessingPattern(b).PrefixPatterns()...)
+ }
+ return patterns
+}
+
// insertACLs configures the starting ACL set for a newly "Create"-d binary based
// on the caller's blessings.
func insertACLs(dir string, principal security.Principal, locks *acls.Locks, blessings []string) error {
tam := make(access.TaggedACLMap)
- // Add the invoker's blessings.
- for _, b := range blessings {
+ // Add the invoker's blessings and all its prefixes.
+ for _, p := range prefixPatterns(blessings) {
for _, tag := range access.AllTypicalTags() {
- tam.Add(security.BlessingPattern(b), string(tag))
+ tam.Add(p, string(tag))
}
}
return locks.SetPathACL(principal, dir, tam, "")
@@ -125,6 +134,10 @@
lp := context.LocalPrincipal()
rb, _ := context.RemoteBlessings().ForContext(context)
+ if len(rb) == 0 {
+ // None of the client's blessings are valid.
+ return verror.New(ErrNotAuthorized, context.Context())
+ }
if err := insertACLs(aclPath(i.state.rootDir, i.suffix), lp, i.locks, rb); err != nil {
vlog.Errorf("insertACLs(%v, %v) failed: %v", lp, rb, err)
return verror.New(ErrOperationFailed, context.Context())
@@ -382,12 +395,16 @@
ctx.LocalPrincipal(), aclPath(i.state.rootDir, i.suffix))
if os.IsNotExist(err) {
- // No ACL file which implies a nil authorizer.
+ // No ACL file found which implies a nil authorizer. This results in default authorization.
+ // Therefore we return an ACL that mimics the default authorization policy (i.e., the ACL
+ // is matched by all blessings that are either extensions of one of the local blessings or
+ // can be extended to form one of the local blessings.)
tam := make(access.TaggedACLMap)
+
lb, _ := ctx.LocalBlessings().ForContext(ctx)
- for _, b := range lb {
+ for _, p := range prefixPatterns(lb) {
for _, tag := range access.AllTypicalTags() {
- tam.Add(security.BlessingPattern(b), string(tag))
+ tam.Add(p, string(tag))
}
}
return tam, "", nil
diff --git a/services/mgmt/device/impl/dispatcher.go b/services/mgmt/device/impl/dispatcher.go
index 37e0a55..6674f3e 100644
--- a/services/mgmt/device/impl/dispatcher.go
+++ b/services/mgmt/device/impl/dispatcher.go
@@ -170,10 +170,13 @@
// Create ACLs to transfer devicemanager permissions to the provided identity.
acl := make(access.TaggedACLMap)
for _, n := range names {
- for _, tag := range access.AllTypicalTags() {
- // TODO(caprita, ataly, ashankar): Do we really need the NonExtendable restriction
- // below?
- acl.Add(security.BlessingPattern(n).MakeNonExtendable(), string(tag))
+ // TODO(caprita, ataly, ashankar): Do we really need the NonExtendable restriction
+ // below?
+ patterns := security.BlessingPattern(n).MakeNonExtendable().PrefixPatterns()
+ for _, p := range patterns {
+ for _, tag := range access.AllTypicalTags() {
+ acl.Add(p, string(tag))
+ }
}
}
if err := d.locks.SetPathACL(principal, d.getACLDir(), acl, ""); err != nil {
diff --git a/services/mgmt/device/impl/impl_test.go b/services/mgmt/device/impl/impl_test.go
index 4ddc44c..d429120 100644
--- a/services/mgmt/device/impl/impl_test.go
+++ b/services/mgmt/device/impl/impl_test.go
@@ -945,7 +945,7 @@
}
expectedACL := make(access.TaggedACLMap)
for _, tag := range access.AllTypicalTags() {
- expectedACL[string(tag)] = access.ACL{In: []security.BlessingPattern{"root/self/mydevice/$"}}
+ expectedACL[string(tag)] = access.ACL{In: []security.BlessingPattern{"root/$", "root/self/$", "root/self/mydevice/$"}}
}
var b bytes.Buffer
if err := expectedACL.WriteTo(&b); err != nil {
