syncbase: Authorization helpers for recursively checking Resolve.
Added helpers for authorization with verifying Resolve on all
ancestors and returning fuzzy errors (ErrNoExistOrNoAccess) when
caller is not authorized for Exists(). Authorization now uses
explicitly listed tags, providing more flexibility than RPC method
attached tags (e.g. allowing multiple tags, checking different tags
on Database and Collection, etc.).
Updated Exists() RPCs to use the new authorization helpers.
Expanded tests to cover Exists() permission checking. More tests
will be added in subsequent CLs to cover error fuzzifying.
MultiPart: 1/3
Change-Id: I027014fe179b060de7ad838a90c307624bed68b0
diff --git a/services/syncbase/service.vdl b/services/syncbase/service.vdl
index 4ad7d57..45e5d10 100644
--- a/services/syncbase/service.vdl
+++ b/services/syncbase/service.vdl
@@ -16,7 +16,15 @@
// - Valid Collection permissions tags are Admin, Read, Write.
// - Valid Syncgroup permissions tags are Admin, Read.
// Other tags are not allowed and are reserved for future use.
-// TODO(ivanpi): Add and implement other security notes.
+// Unless stated otherwise, each permissions tag requirement on a method also
+// implies requiring Resolve on all levels of hierarchy up to, but excluding,
+// the level requiring the tag.
+// TODO(ivanpi): Implemented on Exists, implement elsewhere.
+// ErrNoAccess, Err[No]Exist, ErrUnknownBatch are only returned if the caller
+// is allowed to call Exists on the receiver of the RPC (or the first missing
+// component of the hierarchy to the receiver); otherwise, the returned error
+// is ErrNoExistOrNoAccess.
+// TODO(ivanpi): Implement.
package syncbase
import (
@@ -72,9 +80,10 @@
// TODO(sadovsky): Specify what happens to syncgroups.
Destroy() error {access.Write}
- // Exists returns true only if this Database exists. Insufficient permissions
- // cause Exists to return false instead of an error.
- Exists() (bool | error) {access.Resolve}
+ // Exists returns true only if this Database exists.
+ // Requires: at least one tag on Database, or Read or Write on Service.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
+ Exists() (bool | error)
// ListCollections returns an unsorted list of all Collection ids that the
// caller is allowed to see.
@@ -160,13 +169,12 @@
// TODO(sadovsky): Specify what happens to syncgroups.
Destroy(bh BatchHandle) error {access.Write}
- // Exists returns true only if this Collection exists. Insufficient
- // permissions cause Exists to return false instead of an error.
- // TODO(ivanpi): Exists may fail with an error if higher levels of hierarchy
- // do not exist.
- // TODO(ivanpi): Temporarily set to Read access because Resolve is now invalid
- // on Collection.
- Exists(bh BatchHandle) (bool | error) {access.Read}
+ // Exists returns true only if this Collection exists.
+ // Requires: at least one tag on Collection, or Read or Write on Database.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
+ // If Database does not exist, returned value is identical to
+ // Database.Exists().
+ Exists(bh BatchHandle) (bool | error)
// GetPermissions returns the current Permissions for the Collection.
GetPermissions(bh BatchHandle) (access.Permissions | error) {access.Admin}
@@ -190,14 +198,15 @@
// Row represents a single row in a Collection.
// All access checks are performed against the Collection ACL.
type Row interface {
- // Exists returns true only if this Row exists. Insufficient permissions
- // cause Exists to return false instead of an error.
- // Note, Exists on Row requires read permissions, unlike higher levels of
- // hierarchy which require resolve, because Row existence usually carries
- // more information.
- // TODO(ivanpi): Exists may fail with an error if higher levels of hierarchy
- // do not exist.
- Exists(bh BatchHandle) (bool | error) {access.Read}
+ // Exists returns true only if this Row exists.
+ // Requires: Read or Write on Collection.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
+ // If Collection does not exist, returned value is identical to
+ // Collection.Exists().
+ // Note, write methods on Row do not leak information whether the Row existed
+ // before, but Write is sufficient to call Exists. Therefore, Read protects
+ // Row data and listing, but not Row existence.
+ Exists(bh BatchHandle) (bool | error)
// Get returns the value for this Row.
Get(bh BatchHandle) (any | error) {access.Read}
diff --git a/services/syncbase/syncbase.vdl.go b/services/syncbase/syncbase.vdl.go
index 4b4525e..f0ff63a 100644
--- a/services/syncbase/syncbase.vdl.go
+++ b/services/syncbase/syncbase.vdl.go
@@ -19,7 +19,15 @@
// - Valid Collection permissions tags are Admin, Read, Write.
// - Valid Syncgroup permissions tags are Admin, Read.
// Other tags are not allowed and are reserved for future use.
-// TODO(ivanpi): Add and implement other security notes.
+// Unless stated otherwise, each permissions tag requirement on a method also
+// implies requiring Resolve on all levels of hierarchy up to, but excluding,
+// the level requiring the tag.
+// TODO(ivanpi): Implemented on Exists, implement elsewhere.
+// ErrNoAccess, Err[No]Exist, ErrUnknownBatch are only returned if the caller
+// is allowed to call Exists on the receiver of the RPC (or the first missing
+// component of the hierarchy to the receiver); otherwise, the returned error
+// is ErrNoExistOrNoAccess.
+// TODO(ivanpi): Implement.
package syncbase
import (
@@ -5441,8 +5449,9 @@
// Destroy destroys this Database, permanently removing all of its data.
// TODO(sadovsky): Specify what happens to syncgroups.
Destroy(*context.T, ...rpc.CallOpt) error
- // Exists returns true only if this Database exists. Insufficient permissions
- // cause Exists to return false instead of an error.
+ // Exists returns true only if this Database exists.
+ // Requires: at least one tag on Database, or Read or Write on Service.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
Exists(*context.T, ...rpc.CallOpt) (bool, error)
// ListCollections returns an unsorted list of all Collection ids that the
// caller is allowed to see.
@@ -5740,8 +5749,9 @@
// Destroy destroys this Database, permanently removing all of its data.
// TODO(sadovsky): Specify what happens to syncgroups.
Destroy(*context.T, rpc.ServerCall) error
- // Exists returns true only if this Database exists. Insufficient permissions
- // cause Exists to return false instead of an error.
+ // Exists returns true only if this Database exists.
+ // Requires: at least one tag on Database, or Read or Write on Service.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
Exists(*context.T, rpc.ServerCall) (bool, error)
// ListCollections returns an unsorted list of all Collection ids that the
// caller is allowed to see.
@@ -5893,8 +5903,9 @@
// Destroy destroys this Database, permanently removing all of its data.
// TODO(sadovsky): Specify what happens to syncgroups.
Destroy(*context.T, rpc.ServerCall) error
- // Exists returns true only if this Database exists. Insufficient permissions
- // cause Exists to return false instead of an error.
+ // Exists returns true only if this Database exists.
+ // Requires: at least one tag on Database, or Read or Write on Service.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
Exists(*context.T, rpc.ServerCall) (bool, error)
// ListCollections returns an unsorted list of all Collection ids that the
// caller is allowed to see.
@@ -6062,11 +6073,10 @@
},
{
Name: "Exists",
- Doc: "// Exists returns true only if this Database exists. Insufficient permissions\n// cause Exists to return false instead of an error.",
+ Doc: "// Exists returns true only if this Database exists.\n// Requires: at least one tag on Database, or Read or Write on Service.\n// Otherwise, ErrNoExistOrNoAccess is returned.",
OutArgs: []rpc.ArgDesc{
{"", ``}, // bool
},
- Tags: []*vdl.Value{vdl.ValueOf(access.Tag("Resolve"))},
},
{
Name: "ListCollections",
@@ -6184,12 +6194,11 @@
// Destroy destroys this Collection, permanently removing all of its data.
// TODO(sadovsky): Specify what happens to syncgroups.
Destroy(_ *context.T, bh BatchHandle, _ ...rpc.CallOpt) error
- // Exists returns true only if this Collection exists. Insufficient
- // permissions cause Exists to return false instead of an error.
- // TODO(ivanpi): Exists may fail with an error if higher levels of hierarchy
- // do not exist.
- // TODO(ivanpi): Temporarily set to Read access because Resolve is now invalid
- // on Collection.
+ // Exists returns true only if this Collection exists.
+ // Requires: at least one tag on Collection, or Read or Write on Database.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
+ // If Database does not exist, returned value is identical to
+ // Database.Exists().
Exists(_ *context.T, bh BatchHandle, _ ...rpc.CallOpt) (bool, error)
// GetPermissions returns the current Permissions for the Collection.
GetPermissions(_ *context.T, bh BatchHandle, _ ...rpc.CallOpt) (access.Permissions, error)
@@ -6342,12 +6351,11 @@
// Destroy destroys this Collection, permanently removing all of its data.
// TODO(sadovsky): Specify what happens to syncgroups.
Destroy(_ *context.T, _ rpc.ServerCall, bh BatchHandle) error
- // Exists returns true only if this Collection exists. Insufficient
- // permissions cause Exists to return false instead of an error.
- // TODO(ivanpi): Exists may fail with an error if higher levels of hierarchy
- // do not exist.
- // TODO(ivanpi): Temporarily set to Read access because Resolve is now invalid
- // on Collection.
+ // Exists returns true only if this Collection exists.
+ // Requires: at least one tag on Collection, or Read or Write on Database.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
+ // If Database does not exist, returned value is identical to
+ // Database.Exists().
Exists(_ *context.T, _ rpc.ServerCall, bh BatchHandle) (bool, error)
// GetPermissions returns the current Permissions for the Collection.
GetPermissions(_ *context.T, _ rpc.ServerCall, bh BatchHandle) (access.Permissions, error)
@@ -6376,12 +6384,11 @@
// Destroy destroys this Collection, permanently removing all of its data.
// TODO(sadovsky): Specify what happens to syncgroups.
Destroy(_ *context.T, _ rpc.ServerCall, bh BatchHandle) error
- // Exists returns true only if this Collection exists. Insufficient
- // permissions cause Exists to return false instead of an error.
- // TODO(ivanpi): Exists may fail with an error if higher levels of hierarchy
- // do not exist.
- // TODO(ivanpi): Temporarily set to Read access because Resolve is now invalid
- // on Collection.
+ // Exists returns true only if this Collection exists.
+ // Requires: at least one tag on Collection, or Read or Write on Database.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
+ // If Database does not exist, returned value is identical to
+ // Database.Exists().
Exists(_ *context.T, _ rpc.ServerCall, bh BatchHandle) (bool, error)
// GetPermissions returns the current Permissions for the Collection.
GetPermissions(_ *context.T, _ rpc.ServerCall, bh BatchHandle) (access.Permissions, error)
@@ -6492,14 +6499,13 @@
},
{
Name: "Exists",
- Doc: "// Exists returns true only if this Collection exists. Insufficient\n// permissions cause Exists to return false instead of an error.\n// TODO(ivanpi): Exists may fail with an error if higher levels of hierarchy\n// do not exist.\n// TODO(ivanpi): Temporarily set to Read access because Resolve is now invalid\n// on Collection.",
+ Doc: "// Exists returns true only if this Collection exists.\n// Requires: at least one tag on Collection, or Read or Write on Database.\n// Otherwise, ErrNoExistOrNoAccess is returned.\n// If Database does not exist, returned value is identical to\n// Database.Exists().",
InArgs: []rpc.ArgDesc{
{"bh", ``}, // BatchHandle
},
OutArgs: []rpc.ArgDesc{
{"", ``}, // bool
},
- Tags: []*vdl.Value{vdl.ValueOf(access.Tag("Read"))},
},
{
Name: "GetPermissions",
@@ -6593,13 +6599,14 @@
// Row represents a single row in a Collection.
// All access checks are performed against the Collection ACL.
type RowClientMethods interface {
- // Exists returns true only if this Row exists. Insufficient permissions
- // cause Exists to return false instead of an error.
- // Note, Exists on Row requires read permissions, unlike higher levels of
- // hierarchy which require resolve, because Row existence usually carries
- // more information.
- // TODO(ivanpi): Exists may fail with an error if higher levels of hierarchy
- // do not exist.
+ // Exists returns true only if this Row exists.
+ // Requires: Read or Write on Collection.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
+ // If Collection does not exist, returned value is identical to
+ // Collection.Exists().
+ // Note, write methods on Row do not leak information whether the Row existed
+ // before, but Write is sufficient to call Exists. Therefore, Read protects
+ // Row data and listing, but not Row existence.
Exists(_ *context.T, bh BatchHandle, _ ...rpc.CallOpt) (bool, error)
// Get returns the value for this Row.
Get(_ *context.T, bh BatchHandle, _ ...rpc.CallOpt) (*vom.RawBytes, error)
@@ -6650,13 +6657,14 @@
// Row represents a single row in a Collection.
// All access checks are performed against the Collection ACL.
type RowServerMethods interface {
- // Exists returns true only if this Row exists. Insufficient permissions
- // cause Exists to return false instead of an error.
- // Note, Exists on Row requires read permissions, unlike higher levels of
- // hierarchy which require resolve, because Row existence usually carries
- // more information.
- // TODO(ivanpi): Exists may fail with an error if higher levels of hierarchy
- // do not exist.
+ // Exists returns true only if this Row exists.
+ // Requires: Read or Write on Collection.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
+ // If Collection does not exist, returned value is identical to
+ // Collection.Exists().
+ // Note, write methods on Row do not leak information whether the Row existed
+ // before, but Write is sufficient to call Exists. Therefore, Read protects
+ // Row data and listing, but not Row existence.
Exists(_ *context.T, _ rpc.ServerCall, bh BatchHandle) (bool, error)
// Get returns the value for this Row.
Get(_ *context.T, _ rpc.ServerCall, bh BatchHandle) (*vom.RawBytes, error)
@@ -6736,14 +6744,13 @@
Methods: []rpc.MethodDesc{
{
Name: "Exists",
- Doc: "// Exists returns true only if this Row exists. Insufficient permissions\n// cause Exists to return false instead of an error.\n// Note, Exists on Row requires read permissions, unlike higher levels of\n// hierarchy which require resolve, because Row existence usually carries\n// more information.\n// TODO(ivanpi): Exists may fail with an error if higher levels of hierarchy\n// do not exist.",
+ Doc: "// Exists returns true only if this Row exists.\n// Requires: Read or Write on Collection.\n// Otherwise, ErrNoExistOrNoAccess is returned.\n// If Collection does not exist, returned value is identical to\n// Collection.Exists().\n// Note, write methods on Row do not leak information whether the Row existed\n// before, but Write is sufficient to call Exists. Therefore, Read protects\n// Row data and listing, but not Row existence.",
InArgs: []rpc.ArgDesc{
{"bh", ``}, // BatchHandle
},
OutArgs: []rpc.ArgDesc{
{"", ``}, // bool
},
- Tags: []*vdl.Value{vdl.ValueOf(access.Tag("Read"))},
},
{
Name: "Get",
diff --git a/syncbase/model.go b/syncbase/model.go
index 943dfe0..bb0d989 100644
--- a/syncbase/model.go
+++ b/syncbase/model.go
@@ -115,8 +115,9 @@
// TODO(sadovsky): Specify what happens to syncgroups.
Destroy(ctx *context.T) error
- // Exists returns true only if this Database exists. Insufficient permissions
- // cause Exists to return false instead of an error.
+ // Exists returns true only if this Database exists.
+ // Requires: at least one tag on Database, or Read or Write on Service.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
Exists(ctx *context.T) (bool, error)
// BeginBatch creates a new batch. Instead of calling this function directly,
@@ -228,10 +229,9 @@
// FullName returns the object name (encoded) of this Collection.
FullName() string
- // Exists returns true only if this Collection exists. Insufficient
- // permissions cause Exists to return false instead of an error.
- // TODO(ivanpi): Exists may fail with an error if higher levels of hierarchy
- // do not exist.
+ // Exists returns true only if this Collection exists.
+ // Requires: at least one tag on Collection, or Read or Write on Database.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
Exists(ctx *context.T) (bool, error)
// Create creates this Collection.
@@ -301,10 +301,9 @@
// FullName returns the object name (encoded) of this Row.
FullName() string
- // Exists returns true only if this Row exists. Insufficient permissions cause
- // Exists to return false instead of an error.
- // TODO(ivanpi): Exists may fail with an error if higher levels of hierarchy
- // do not exist.
+ // Exists returns true only if this Row exists.
+ // Requires: Read or Write on Collection.
+ // Otherwise, ErrNoExistOrNoAccess is returned.
Exists(ctx *context.T) (bool, error)
// Get loads the value stored in this Row into the given value. If the given
diff --git a/syncbase/permissions_test.go b/syncbase/permissions_test.go
index b1b29ca..1b73600 100644
--- a/syncbase/permissions_test.go
+++ b/syncbase/permissions_test.go
@@ -105,6 +105,15 @@
},
{
layer: databaseTest{f: func(ctx *context.T, d syncbase.Database) error {
+ _, err := d.Exists(ctx)
+ return err
+ }},
+ name: "database.Exists",
+ patterns: []string{"XX__", "XR__", "XW__", "XA__", "R___", "W___"},
+ mutating: false,
+ },
+ {
+ layer: databaseTest{f: func(ctx *context.T, d syncbase.Database) error {
_, _, err := d.GetPermissions(ctx)
return err
}},
@@ -123,6 +132,15 @@
// Collection tests.
{
layer: collectionTest{f: func(ctx *context.T, c syncbase.Collection) error {
+ _, err := c.Exists(ctx)
+ return err
+ }},
+ name: "collection.Exists",
+ patterns: []string{"XXR_", "XXW_", "XXA_", "XR__", "XW__"},
+ mutating: false,
+ },
+ {
+ layer: collectionTest{f: func(ctx *context.T, c syncbase.Collection) error {
_, err := c.GetPermissions(ctx)
return err
}},
@@ -133,6 +151,15 @@
// Row tests.
{
layer: rowTest{f: func(ctx *context.T, r syncbase.Row) error {
+ _, err := r.Exists(ctx)
+ return err
+ }},
+ name: "row.Exists",
+ patterns: []string{"XXR_", "XXW_"},
+ mutating: false,
+ },
+ {
+ layer: rowTest{f: func(ctx *context.T, r syncbase.Row) error {
var value string
return r.Get(ctx, &value)
}},
@@ -162,9 +189,9 @@
// Allowed tests are generated by splitting the list of security patterns
// of the test group into one pattern per test.
// Denied tests are generated the following way: we take each allowed pattern
-// and generate all patterns that differ in one character, preserving all '_'.
-// This way we make sure that every character in a security pattern is
-// important.
+// and generate all patterns that differ in one character, preserving all '_'
+// (excluding patterns that are a subset of an allowed pattern). This way we
+// make sure that every character in a security pattern is important.
// Allowed tests are split in two categories: mutating tests and static tests.
// Mutating tests change the state of Syncbase, static don't.
// Denied tests are always static.
@@ -214,7 +241,7 @@
patternBytes := []byte(pattern)
for _, c := range []byte{'X', 'R', 'W', 'A'} {
patternBytes[i] = c
- if !allowedPatterns[string(patternBytes)] {
+ if !isSubsetOfAny(string(patternBytes), allowedPatterns) {
deniedPatterns[string(patternBytes)] = true
}
}
@@ -234,6 +261,23 @@
return
}
+func isSubsetOfAny(needle string, haystack map[string]bool) bool {
+ for p := range haystack {
+ isSubset := true
+ for i := 0; i < len(p); i++ {
+ if p[i] != '_' && p[i] != needle[i] {
+ isSubset = false
+ break
+ }
+ }
+ if isSubset {
+ // All perms in needle correspond to the same perm or '_' in p.
+ return true
+ }
+ }
+ return false
+}
+
func runTests(t *testing.T, expectSuccess bool, tests ...securitySpecTest) {
// Create permissions.
servicePerms := makePerms("root:admin", 0, access.AllTypicalTags(), tests...)
@@ -275,8 +319,9 @@
}
if expectSuccess && err != nil {
tu.Fatalf(t, "test %v failed with non-nil error: %v", test, err)
- } else if !expectSuccess && verror.ErrorID(err) != verror.ErrNoAccess.ID {
- tu.Fatalf(t, "test %v didn't fail with ErrNoAccess error: %v", test, err)
+ } else if !expectSuccess && verror.ErrorID(err) != verror.ErrNoAccess.ID && verror.ErrorID(err) != verror.ErrNoExistOrNoAccess.ID {
+ // TODO(ivanpi): Allow specifying which error is expected. Also test on nonexistent layers.
+ tu.Fatalf(t, "test %v didn't fail with ErrNoAccess or ErrNoExistOrNoAccess error: %v", test, err)
}
}
}
