services/mgmt/lib/acls: adjust hierarchicalAuthorizer inheritance
Modify hierarchicalAuthorizer to only inherit root permissions for
blessings with the Admin tag.
Change-Id: I2579a6bce142dfe7c2d96ee36c5f195b05440f8d
diff --git a/services/mgmt/binary/impl/acl_test.go b/services/mgmt/binary/impl/acl_test.go
index 54cb252..281a594 100644
--- a/services/mgmt/binary/impl/acl_test.go
+++ b/services/mgmt/binary/impl/acl_test.go
@@ -65,6 +65,10 @@
return nil
}
+func b(name string) repository.BinaryClientStub {
+ return repository.BinaryClient(name)
+}
+
func TestBinaryCreateACL(t *testing.T) {
ctx, shutdown := testutil.InitForTest()
defer shutdown()
@@ -98,17 +102,16 @@
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 {
+ if err := b("bini/private").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 {
+ if streamErr, err := invokeUpload(t, childCtx, b("bini/private"), 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)
+ acl, _, err := b("bini/private").GetACL(selfCtx)
if err != nil {
t.Fatalf("GetACL failed: %v", err)
}
@@ -156,37 +159,32 @@
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(selfCtx, 1, repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
+ if err := b("bini/private").Create(selfCtx, 1, repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
t.Fatalf("Create() failed %v", err)
}
fakeDataPrivate := testData()
- if streamErr, err := invokeUpload(t, selfCtx, binary, fakeDataPrivate, 0); streamErr != nil || err != nil {
+ if streamErr, err := invokeUpload(t, selfCtx, b("bini/private"), fakeDataPrivate, 0); streamErr != nil || err != nil {
t.Fatalf("invokeUpload() failed %v, %v", err, streamErr)
}
- binary = repository.BinaryClient("bini/shared")
- if err := binary.Create(selfCtx, 1, repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
+ if err := b("bini/shared").Create(selfCtx, 1, repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
t.Fatalf("Create() failed %v", err)
}
fakeDataShared := testData()
- if streamErr, err := invokeUpload(t, selfCtx, binary, fakeDataShared, 0); streamErr != nil || err != nil {
+ if streamErr, err := invokeUpload(t, selfCtx, b("bini/shared"), fakeDataShared, 0); streamErr != nil || err != nil {
t.Fatalf("invokeUpload() failed %v, %v", err, streamErr)
}
vlog.VI(2).Infof("Verify that in the beginning other can't access bini/private or bini/shared")
- binary = repository.BinaryClient("bini/private")
- if _, _, err := binary.Stat(otherCtx); !verror.Is(err, verror.ErrNoAccess.ID) {
+ if _, _, err := b("bini/private").Stat(otherCtx); !verror.Is(err, verror.ErrNoAccess.ID) {
t.Fatalf("Stat() should have failed but didn't: %v", err)
}
- binary = repository.BinaryClient("bini/shared")
- if _, _, err := binary.Stat(otherCtx); !verror.Is(err, verror.ErrNoAccess.ID) {
+ if _, _, err := b("bini/shared").Stat(otherCtx); !verror.Is(err, verror.ErrNoAccess.ID) {
t.Fatalf("Stat() should have failed but didn't: %v", err)
}
vlog.VI(2).Infof("Validate the ACL file on bini/private.")
- binary = repository.BinaryClient("bini/private")
- acl, _, err := binary.GetACL(selfCtx)
+ acl, _, err := b("bini/private").GetACL(selfCtx)
if err != nil {
t.Fatalf("GetACL failed: %v", err)
}
@@ -202,8 +200,7 @@
}
vlog.VI(2).Infof("Validate the ACL file on bini/private.")
- binary = repository.BinaryClient("bini/private")
- acl, etag, err := binary.GetACL(selfCtx)
+ acl, etag, err := b("bini/private").GetACL(selfCtx)
if err != nil {
t.Fatalf("GetACL failed: %v", err)
}
@@ -226,12 +223,11 @@
acl.Clear("self", string(tag))
acl.Add("self/$", string(tag))
}
- if err := binary.SetACL(selfCtx, acl, etag); err != nil {
+ if err := b("bini/private").SetACL(selfCtx, acl, etag); err != nil {
t.Fatalf("SetACL failed: %v", err)
}
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/$"}},
"Read": access.ACL{In: []security.BlessingPattern{"self/$"}},
@@ -239,7 +235,7 @@
"Debug": access.ACL{In: []security.BlessingPattern{"self/$"}},
"Resolve": access.ACL{In: []security.BlessingPattern{"self/$"}},
}
- acl, _, err = binary.GetACL(selfCtx)
+ acl, _, err = b("bini/private").GetACL(selfCtx)
if err != nil {
t.Fatalf("GetACL failed: %v", err)
}
@@ -251,50 +247,58 @@
// root level. Self has to set one explicitly to enable sharing. This way, self
// can't accidentally expose the server without setting a root ACL.
vlog.VI(2).Infof(" Verify that other still can't access bini/shared.")
- binary = repository.BinaryClient("bini/shared")
- if _, _, err := binary.Stat(otherCtx); !verror.Is(err, verror.ErrNoAccess.ID) {
+ if _, _, err := b("bini/shared").Stat(otherCtx); !verror.Is(err, verror.ErrNoAccess.ID) {
t.Fatalf("Stat() should have failed but didn't: %v", err)
}
vlog.VI(2).Infof("Self sets a root ACL.")
- binary = repository.BinaryClient("bini")
newRootACL := make(access.TaggedACLMap)
for _, tag := range access.AllTypicalTags() {
newRootACL.Add("self/$", string(tag))
}
- if err := binary.SetACL(selfCtx, newRootACL, ""); err != nil {
+ if err := b("bini").SetACL(selfCtx, newRootACL, ""); err != nil {
t.Fatalf("SetACL failed: %v", err)
}
vlog.VI(2).Infof("Verify that other can access bini/shared now but not access bini/private.")
- binary = repository.BinaryClient("bini/shared")
- if _, _, err := binary.Stat(otherCtx); err != nil {
+ if _, _, err := b("bini/shared").Stat(otherCtx); err != nil {
t.Fatalf("Stat() shouldn't have failed: %v", err)
}
- binary = repository.BinaryClient("bini/private")
- if _, _, err := binary.Stat(otherCtx); !verror.Is(err, verror.ErrNoAccess.ID) {
+ if _, _, err := b("bini/private").Stat(otherCtx); !verror.Is(err, verror.ErrNoAccess.ID) {
t.Fatalf("Stat() should have failed but didn't: %v", err)
}
vlog.VI(2).Infof("Other still can't create so Self gives Other right to Create.")
- binary = repository.BinaryClient("bini")
- acl, tag, err := binary.GetACL(selfCtx)
+ acl, tag, err := b("bini").GetACL(selfCtx)
if err != nil {
t.Fatalf("GetACL() failed: %v", err)
}
- acl.Add("self", string("Write"))
- err = binary.SetACL(selfCtx, acl, tag)
+
+ // More than one ACL change will result in the same functional result in
+ // this test: that self/other acquires the right to invoke Create at the
+ // root. In particular:
+ //
+ // a. acl.Add("self", "Write ")
+ // b. acl.Add("self/other", "Write")
+ // c. acl.Add("self/other/$", "Write")
+ //
+ // will all give self/other the right to invoke Create but in the case of
+ // (a) it will also extend this right to self's delegates (because of the
+ // absence of the $) including other and in (b) will also extend the
+ // Create right to all of other's delegates. Since (c) is the minimum
+ // case, use that.
+ acl.Add("self/other/$", string("Write"))
+ err = b("bini").SetACL(selfCtx, acl, tag)
if err != nil {
t.Fatalf("SetACL() failed: %v", err)
}
vlog.VI(2).Infof("Other creates bini/otherbinary")
- binary = repository.BinaryClient("bini/otherbinary")
- if err := binary.Create(otherCtx, 1, repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
+ if err := b("bini/otherbinary").Create(otherCtx, 1, repository.MediaInfo{Type: "application/octet-stream"}); err != nil {
t.Fatalf("Create() failed %v", err)
}
fakeDataOther := testData()
- if streamErr, err := invokeUpload(t, otherCtx, binary, fakeDataOther, 0); streamErr != nil || err != nil {
+ if streamErr, err := invokeUpload(t, otherCtx, b("bini/otherbinary"), fakeDataOther, 0); streamErr != nil || err != nil {
t.FailNow()
}
@@ -306,7 +310,7 @@
"Debug": access.ACL{In: []security.BlessingPattern{"self/$", "self/other"}},
"Resolve": access.ACL{In: []security.BlessingPattern{"self/$", "self/other"}},
}
- acl, _, err = binary.GetACL(otherCtx)
+ acl, _, err = b("bini/otherbinary").GetACL(otherCtx)
if err != nil {
t.Fatalf("GetACL failed: %v", err)
}
@@ -314,67 +318,96 @@
t.Errorf("got %#v, expected %#v ", got, want)
}
- vlog.VI(2).Infof("Other tries to exclude self by adding self to Read's notin")
- acl, tag, err = binary.GetACL(otherCtx)
+ vlog.VI(2).Infof("Other tries to exclude self by removing self from the ACL set")
+ acl, tag, err = b("bini/otherbinary").GetACL(otherCtx)
if err != nil {
t.Fatalf("GetACL() failed: %v", err)
}
- acl.Blacklist("self", string("Read"))
- err = binary.SetACL(otherCtx, acl, tag)
+ acl.Clear("self/$")
+ err = b("bini/otherbinary").SetACL(otherCtx, acl, tag)
if err != nil {
t.Fatalf("SetACL() failed: %v", err)
}
- vlog.VI(2).Infof("But self's rights are inherited from root so self can still access despite blacklist.")
- binary = repository.BinaryClient("bini/otherbinary")
- if _, _, err := binary.Stat(selfCtx); err != nil {
+ vlog.VI(2).Infof("Verify that other can make this change.")
+ updated = access.TaggedACLMap{
+ "Admin": access.ACL{In: []security.BlessingPattern{"self/other"}},
+ "Read": access.ACL{In: []security.BlessingPattern{"self/other"}},
+ "Write": access.ACL{In: []security.BlessingPattern{"self/other"}},
+ "Debug": access.ACL{In: []security.BlessingPattern{"self/other"}},
+ "Resolve": access.ACL{In: []security.BlessingPattern{"self/other"}},
+ }
+ acl, _, err = b("bini/otherbinary").GetACL(otherCtx)
+ if err != nil {
+ t.Fatalf("GetACL failed: %v", err)
+ }
+ if got, want := acl.Normalize(), updated.Normalize(); !reflect.DeepEqual(want, got) {
+ t.Errorf("got %#v, expected %#v ", got, want)
+ }
+
+ vlog.VI(2).Infof("But self's rights are inherited from root so self can still access despite this.")
+ if _, _, err := b("bini/otherbinary").Stat(selfCtx); err != nil {
t.Fatalf("Stat() shouldn't have failed: %v", err)
}
vlog.VI(2).Infof("Self petulantly blacklists other back.")
- binary = repository.BinaryClient("bini")
- acl, tag, err = binary.GetACL(selfCtx)
+ acl, tag, err = b("bini").GetACL(selfCtx)
if err != nil {
t.Fatalf("GetACL() failed: %v", err)
}
for _, tag := range access.AllTypicalTags() {
acl.Blacklist("self/other", string(tag))
}
- err = binary.SetACL(selfCtx, acl, tag)
+ err = b("bini").SetACL(selfCtx, acl, tag)
if err != nil {
t.Fatalf("SetACL() failed: %v", err)
}
vlog.VI(2).Infof("And now other can do nothing at affecting the root. Other should be penitent.")
- binary = repository.BinaryClient("bini/nototherbinary")
- if err := binary.Create(otherCtx, 1, repository.MediaInfo{Type: "application/octet-stream"}); !verror.Is(err, verror.ErrNoAccess.ID) {
+ if err := b("bini/nototherbinary").Create(otherCtx, 1, repository.MediaInfo{Type: "application/octet-stream"}); !verror.Is(err, verror.ErrNoAccess.ID) {
t.Fatalf("Create() should have failed %v", err)
}
vlog.VI(2).Infof("But other can still access shared.")
- binary = repository.BinaryClient("bini/shared")
- if _, _, err := binary.Stat(otherCtx); err != nil {
+ if _, _, err := b("bini/shared").Stat(otherCtx); err != nil {
t.Fatalf("Stat() should not have failed but did: %v", err)
}
vlog.VI(2).Infof("Self petulantly blacklists other's binary too.")
- binary = repository.BinaryClient("bini/shared")
- acl, tag, err = binary.GetACL(selfCtx)
+ acl, tag, err = b("bini/shared").GetACL(selfCtx)
if err != nil {
t.Fatalf("GetACL() failed: %v", err)
}
for _, tag := range access.AllTypicalTags() {
acl.Blacklist("self/other", string(tag))
}
- err = binary.SetACL(selfCtx, acl, tag)
+ err = b("bini/shared").SetACL(selfCtx, acl, tag)
+ if err != nil {
+ t.Fatalf("SetACL() failed: %v", err)
+ }
+ vlog.VI(2).Infof("And now other can't access shared either.")
+ if _, _, err := b("bini/shared").Stat(otherCtx); !verror.Is(err, verror.ErrNoAccess.ID) {
+ t.Fatalf("Stat() should have failed but didn't: %v", err)
+ }
+ // TODO(rjkroege): Extend the test with a third principal and verify that
+ // new principals can be given Admin perimission at the root.
+
+ vlog.VI(2).Infof("Self feels guilty for petulance and disempowers itself")
+ // TODO(rjkroege,caprita): This is a one-way transition for self. Perhaps it
+ // should not be. Consider adding a factory-reset facility.
+ acl, tag, err = b("bini").GetACL(selfCtx)
+ if err != nil {
+ t.Fatalf("GetACL() failed: %v", err)
+ }
+ acl.Clear("self/$", "Admin")
+ err = b("bini").SetACL(selfCtx, acl, tag)
if err != nil {
t.Fatalf("SetACL() failed: %v", err)
}
- vlog.VI(2).Infof("And now other can't access shared either.")
- binary = repository.BinaryClient("bini/shared")
- if _, _, err := binary.Stat(otherCtx); !verror.Is(err, verror.ErrNoAccess.ID) {
- t.Fatalf("Stat() should have failed but didn't: %v", err)
+ vlog.VI(2).Info("Self can't access other's binary now")
+ if _, _, err := b("bini/otherbinary").Stat(selfCtx); err == nil {
+ t.Fatalf("Stat() should have failed but didn't")
}
}
@@ -405,8 +438,7 @@
pid := mgmttest.ReadPID(t, nms)
defer syscall.Kill(pid, syscall.SIGINT)
- binary := repository.BinaryClient("bini")
- acl, tag, err := binary.GetACL(selfCtx)
+ acl, tag, err := b("bini").GetACL(selfCtx)
if err != nil {
t.Fatalf("GetACL failed: %#v", err)
}
@@ -422,12 +454,12 @@
}
acl.Blacklist("self", string("Read"))
- err = binary.SetACL(selfCtx, acl, tag)
+ err = b("bini").SetACL(selfCtx, acl, tag)
if err != nil {
t.Fatalf("SetACL() failed: %v", err)
}
- acl, tag, err = binary.GetACL(selfCtx)
+ acl, tag, err = b("bini").GetACL(selfCtx)
if err != nil {
t.Fatalf("GetACL failed: %#v", err)
}
diff --git a/services/mgmt/lib/acls/hierarchical_authorizer.go b/services/mgmt/lib/acls/hierarchical_authorizer.go
index 86204b9..5a0dece 100644
--- a/services/mgmt/lib/acls/hierarchical_authorizer.go
+++ b/services/mgmt/lib/acls/hierarchical_authorizer.go
@@ -1,8 +1,6 @@
package acls
import (
- "fmt"
-
"v.io/v23/security"
"v.io/v23/services/security/access"
"v.io/x/lib/vlog"
@@ -11,8 +9,8 @@
// hierarchicalAuthorizer manages a pair of authorizers for two-level
// inheritance of ACLs.
type hierarchicalAuthorizer struct {
- root security.Authorizer
- child security.Authorizer
+ child security.Authorizer
+ rootACL access.ACL
}
// TAMGetter defines an abstract interface that a customer of
@@ -26,6 +24,15 @@
TAMForPath(path string) (access.TaggedACLMap, bool, error)
}
+func mkRootAuth(rootTam access.TaggedACLMap) (security.Authorizer, error) {
+ rootAuth, err := access.TaggedACLAuthorizer(rootTam, access.TypicalTagType())
+ if err != nil {
+ vlog.Errorf("Successfully obtained an ACL from the filesystem but TaggedACLAuthorizer couldn't use it: %v", err)
+ return nil, err
+ }
+ return rootAuth, nil
+}
+
// NewHierarchicalAuthorizer creates a new hierarchicalAuthorizer
func NewHierarchicalAuthorizer(rootDir, childDir string, get TAMGetter) (security.Authorizer, error) {
rootTam, intentionallyEmpty, err := get.TAMForPath(rootDir)
@@ -36,15 +43,9 @@
return nil, nil
}
- rootAuth, err := access.TaggedACLAuthorizer(rootTam, access.TypicalTagType())
- if err != nil {
- vlog.Errorf("Successfully obtained an ACL from the filesystem but TaggedACLAuthorizer couldn't use it: %v", err)
- return nil, err
- }
-
// We are at the root so exit early.
if rootDir == childDir {
- return rootAuth, nil
+ return mkRootAuth(rootTam)
}
// This is not fatal: the childDir may not exist if we are invoking
@@ -53,7 +54,7 @@
if err != nil {
return nil, err
} else if intentionallyEmpty {
- return rootAuth, nil
+ return mkRootAuth(rootTam)
}
childAuth, err := access.TaggedACLAuthorizer(childTam, access.TypicalTagType())
@@ -63,22 +64,27 @@
}
return &hierarchicalAuthorizer{
- root: rootAuth,
- child: childAuth,
+ child: childAuth,
+ rootACL: rootTam[string(access.Admin)],
}, nil
}
-// Authorize provides two-levels of authorization. Permissions on "Roots"
-// in the namespace will apply to children paths regardless of accesses
-// set on the children. Conversely, ACL exclusions are not inherited.
+// Authorize provides two-levels of authorization. Admin permission
+// on the root provides a "superuser"-like power for administering the
+// server using an instance of hierarchicalAuthorizer. Otherwise, the
+// default permissions of the named path apply.
func (ha *hierarchicalAuthorizer) Authorize(call security.Call) error {
childErr := ha.child.Authorize(call)
if childErr == nil {
return nil
}
- rootErr := ha.root.Authorize(call)
- if rootErr == nil {
+
+ // Maybe the invoking principal can invoke this method because
+ // it has root permissions.
+ names, _ := call.RemoteBlessings().ForCall(call)
+ if len(names) > 0 && ha.rootACL.Includes(names...) {
return nil
}
- return fmt.Errorf("Both root acls (%v) and child acls (%v) deny access.", rootErr, childErr)
+
+ return childErr
}
