veyron2/security: Provide a way to "unmark" blessings in the BlessingStore.
This commit changes the API of the BlessingStore interface from:
Add(blessing, pattern)
to
Set(blessing, pattern).
Without this change, operations on the blessing store's map from
pattern to blessings were always additive, with no way to get rid
of associations.
By using "Set" instead, previous associations can be overridden.
I also briefly considered keeping Add and adding a Remove, but
went with Set for now since it seemed like a simpler API.
Change-Id: I2d042326fb33e3b96ce9ff23af5dca0c59bbd52f
diff --git a/runtimes/google/ipc/stream/sectest/sectest.go b/runtimes/google/ipc/stream/sectest/sectest.go
index 3fbbcec..2cbf560 100644
--- a/runtimes/google/ipc/stream/sectest/sectest.go
+++ b/runtimes/google/ipc/stream/sectest/sectest.go
@@ -43,9 +43,10 @@
k security.PublicKey
}
-func (bs *blessingStore) Add(blessings security.Blessings, peer security.BlessingPattern) error {
+func (bs *blessingStore) Set(blessings security.Blessings, peer security.BlessingPattern) (security.Blessings, error) {
+ old := bs.m[string(peer)]
bs.m[string(peer)] = blessings
- return nil
+ return old, nil
}
func (bs *blessingStore) ForPeer(peers ...string) security.Blessings {
diff --git a/runtimes/google/ipc/stream/vc/vc_test.go b/runtimes/google/ipc/stream/vc/vc_test.go
index c0a2f95..4e19629 100644
--- a/runtimes/google/ipc/stream/vc/vc_test.go
+++ b/runtimes/google/ipc/stream/vc/vc_test.go
@@ -138,8 +138,8 @@
if err != nil {
t.Fatal(err)
}
- client.BlessingStore().Add(forServer1, security.BlessingPattern("server1"))
- client.BlessingStore().Add(forServer2, security.BlessingPattern("server2"))
+ client.BlessingStore().Set(forServer1, security.BlessingPattern("server1"))
+ client.BlessingStore().Set(forServer2, security.BlessingPattern("server2"))
// Make the clients and servers recognize each other as valid root certificate providers.
for _, p := range []security.Principal{client, server1, server2} {
diff --git a/runtimes/google/rt/blessingstore.go b/runtimes/google/rt/blessingstore.go
index 30b14fe..2e87449 100644
--- a/runtimes/google/rt/blessingstore.go
+++ b/runtimes/google/rt/blessingstore.go
@@ -19,17 +19,12 @@
var errStoreAddMismatch = errors.New("blessing's public key does not match store's public key")
-type markedBlessings struct {
- Blessings security.Blessings
- Patterns []security.BlessingPattern
-}
-
type persistentState struct {
- // Store contains a set of Blessings marked with a set of BlessingsPatterns.
- // These blessings are intended to be shared with peers whose blessings
- // match the corresponding pattern. All Blessings in the store must have
- // the same public key.
- Store []markedBlessings
+ // Store maps BlessingPatterns to the Blessings object that is to be shared
+ // with peers which present blessings of their own that match the pattern.
+ //
+ // All blessings bind to the same public key.
+ Store map[security.BlessingPattern]security.Blessings
// Default is the default Blessings to be shared with peers for which
// no other information is available to select blessings.
Default security.Blessings
@@ -44,68 +39,47 @@
mu sync.RWMutex
}
-func (s *blessingStore) Add(blessings security.Blessings, forPeers security.BlessingPattern) error {
- if !reflect.DeepEqual(blessings.PublicKey(), s.publicKey) {
- return errStoreAddMismatch
- }
+func (s *blessingStore) Set(blessings security.Blessings, forPeers security.BlessingPattern) (security.Blessings, error) {
if !forPeers.IsValid() {
- return fmt.Errorf("%q is an invalid BlessingPattern", forPeers)
+ return nil, fmt.Errorf("%q is an invalid BlessingPattern", forPeers)
}
-
+ if blessings != nil && !reflect.DeepEqual(blessings.PublicKey(), s.publicKey) {
+ return nil, errStoreAddMismatch
+ }
s.mu.Lock()
defer s.mu.Unlock()
-
- var entry *markedBlessings
- for i, mb := range s.state.Store {
- if !reflect.DeepEqual(mb.Blessings, blessings) {
- continue
- }
- entry = &s.state.Store[i]
- for _, p := range mb.Patterns {
- if p == forPeers {
- return nil
- }
- }
- break
- }
- if entry == nil {
- s.state.Store = append(s.state.Store, markedBlessings{blessings, []security.BlessingPattern{forPeers}})
+ old, hadold := s.state.Store[forPeers]
+ if blessings != nil {
+ s.state.Store[forPeers] = blessings
} else {
- entry.Patterns = append(entry.Patterns, forPeers)
+ delete(s.state.Store, forPeers)
}
-
if err := s.save(); err != nil {
- if entry == nil {
- s.state.Store = s.state.Store[:len(s.state.Store)-1]
+ if hadold {
+ s.state.Store[forPeers] = old
} else {
- entry.Patterns = entry.Patterns[:len(entry.Patterns)-1]
+ delete(s.state.Store, forPeers)
}
- return err
+ return nil, err
}
- return nil
+ return old, nil
}
func (s *blessingStore) ForPeer(peerBlessings ...string) security.Blessings {
s.mu.RLock()
defer s.mu.RUnlock()
- var matchingBlessings []security.Blessings
- for _, mb := range s.state.Store {
- for _, p := range mb.Patterns {
- if p.MatchedBy(peerBlessings...) {
- matchingBlessings = append(matchingBlessings, mb.Blessings)
- break
+ var ret security.Blessings
+ for pattern, blessings := range s.state.Store {
+ if pattern.MatchedBy(peerBlessings...) {
+ if union, err := security.UnionOfBlessings(ret, blessings); err != nil {
+ vlog.Errorf("UnionOfBlessings(%v, %v) failed: %v, dropping the latter from BlessingStore.ForPeers(%v)", ret, blessings, err, peerBlessings)
+ } else {
+ ret = union
}
}
}
-
- blessings, err := security.UnionOfBlessings(matchingBlessings...)
- if err != nil {
- // This case should never be hit.
- vlog.Errorf("BlessingStore: %s is broken, could not union Blessings obtained from it: %s", s, err)
- return nil
- }
- return blessings
+ return ret
}
func (s *blessingStore) Default() security.Blessings {
@@ -153,6 +127,7 @@
func newInMemoryBlessingStore(publicKey security.PublicKey) security.BlessingStore {
return &blessingStore{
publicKey: publicKey,
+ state: persistentState{Store: make(map[security.BlessingPattern]security.Blessings)},
}
}
@@ -171,6 +146,7 @@
}
s := &blessingStore{
publicKey: publicKey,
+ state: persistentState{Store: make(map[security.BlessingPattern]security.Blessings)},
dir: directory,
signer: signer,
}
@@ -179,9 +155,9 @@
return nil, err
}
- for _, mb := range s.state.Store {
- if !reflect.DeepEqual(mb.Blessings.PublicKey(), publicKey) {
- return nil, fmt.Errorf("directory contains Blessings: %v that are not for the provided PublicKey: %v", mb.Blessings, publicKey)
+ for _, b := range s.state.Store {
+ if !reflect.DeepEqual(b.PublicKey(), publicKey) {
+ return nil, fmt.Errorf("directory contains Blessings: %v that are not for the provided PublicKey: %v", b, publicKey)
}
}
return s, nil
diff --git a/runtimes/google/rt/blessingstore_test.go b/runtimes/google/rt/blessingstore_test.go
index 096fb19..3e78688 100644
--- a/runtimes/google/rt/blessingstore_test.go
+++ b/runtimes/google/rt/blessingstore_test.go
@@ -1,7 +1,6 @@
package rt
import (
- "bytes"
"fmt"
"io/ioutil"
"os"
@@ -9,7 +8,6 @@
"testing"
"veyron.io/veyron/veyron2/security"
- "veyron.io/veyron/veyron2/vom"
)
type storeTester struct {
@@ -17,7 +15,7 @@
bForAll, bForGoogle, bForVeyron, bDefault security.Blessings
}
-func (t *storeTester) testAdd(s security.BlessingStore) error {
+func (t *storeTester) testSet(s security.BlessingStore) error {
var (
p = newPrincipal(t.t)
bOther = blessSelf(t.t, p, "irrelevant")
@@ -37,8 +35,9 @@
{t.bForAll, "foo/.../bar", "invalid BlessingPattern"},
}
for _, d := range testdata {
- if err := matchesError(s.Add(d.blessings, d.pattern), d.wantErr); err != nil {
- return fmt.Errorf("Add(%v, %q): %v", d.blessings, p, err)
+ _, err := s.Set(d.blessings, d.pattern)
+ if merr := matchesError(err, d.wantErr); merr != nil {
+ return fmt.Errorf("Set(%v, %q): %v", d.blessings, p, merr)
}
}
return nil
@@ -114,7 +113,7 @@
func TestInMemoryBlessingStore(t *testing.T) {
tester, pkey := newStoreTester(t)
s := newInMemoryBlessingStore(pkey)
- if err := tester.testAdd(s); err != nil {
+ if err := tester.testSet(s); err != nil {
t.Error(err)
}
if err := tester.testForPeer(s); err != nil {
@@ -145,7 +144,7 @@
t.Fatalf("newPersistingBlessingStore failed: %v", err)
}
- if err := tester.testAdd(s); err != nil {
+ if err := tester.testSet(s); err != nil {
t.Error(err)
}
if err := tester.testForPeer(s); err != nil {
@@ -168,36 +167,81 @@
}
}
-func TestBlessingStoreDuplicates(t *testing.T) {
- roundTrip := func(blessings security.Blessings) security.Blessings {
- var b bytes.Buffer
- if err := vom.NewEncoder(&b).Encode(blessings); err != nil {
- t.Fatalf("could not VOM-Encode Blessings: %v", err)
- }
- var decodedBlessings security.Blessings
- if err := vom.NewDecoder(&b).Decode(&decodedBlessings); err != nil {
- t.Fatalf("could not VOM-Decode Blessings: %v", err)
- }
- return decodedBlessings
- }
- add := func(s security.BlessingStore, blessings security.Blessings, forPeers security.BlessingPattern) {
- if err := s.Add(blessings, forPeers); err != nil {
- t.Fatalf("Add(%v, %q) failed unexpectedly: %v", blessings, forPeers, err)
- }
- }
+func TestBlessingStoreSetOverridesOldSetting(t *testing.T) {
var (
- // root principal
- // test blessings
p = newPrincipal(t)
alice = blessSelf(t, p, "alice")
-
- pkey = p.PublicKey()
+ bob = blessSelf(t, p, "bob")
+ s = newInMemoryBlessingStore(p.PublicKey())
)
- s := newInMemoryBlessingStore(pkey)
- add(s, alice, "...")
- add(s, roundTrip(alice), "...")
+ // Set(alice, "alice")
+ // Set(bob, "alice/...")
+ // So, {alice, bob} is shared with "alice", whilst {bob} is shared with "alice/tv"
+ if _, err := s.Set(alice, "alice"); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := s.Set(bob, "alice/..."); err != nil {
+ t.Fatal(err)
+ }
+ if got, want := s.ForPeer("alice"), unionOfBlessings(t, alice, bob); !reflect.DeepEqual(got, want) {
+ t.Errorf("Got %v, want %v", got, want)
+ }
+ if got, want := s.ForPeer("alice/friend"), bob; !reflect.DeepEqual(got, want) {
+ t.Errorf("Got %v, want %v", got, want)
+ }
- if got, want := s.ForPeer(), alice; !reflect.DeepEqual(got, want) {
- t.Fatalf("ForPeer(): got: %v, want: %v", got, want)
+ // Clear out the blessing associated with "alice".
+ // Now, bob should be shared with both alice and alice/friend.
+ if _, err := s.Set(nil, "alice"); err != nil {
+ t.Fatal(err)
+ }
+ if got, want := s.ForPeer("alice"), bob; !reflect.DeepEqual(got, want) {
+ t.Errorf("Got %v, want %v", got, want)
+ }
+ if got, want := s.ForPeer("alice/friend"), bob; !reflect.DeepEqual(got, want) {
+ t.Errorf("Got %v, want %v", got, want)
+ }
+
+ // Clearing out an association that doesn't exist should have no effect.
+ if _, err := s.Set(nil, "alice/enemy"); err != nil {
+ t.Fatal(err)
+ }
+ if got, want := s.ForPeer("alice"), bob; !reflect.DeepEqual(got, want) {
+ t.Errorf("Got %v, want %v", got, want)
+ }
+ if got, want := s.ForPeer("alice/friend"), bob; !reflect.DeepEqual(got, want) {
+ t.Errorf("Got %v, want %v", got, want)
+ }
+
+ // Clear everything
+ if _, err := s.Set(nil, "alice/..."); err != nil {
+ t.Fatal(err)
+ }
+ if got := s.ForPeer("alice"); got != nil {
+ t.Errorf("Got %v, want nil", got)
+ }
+ if got := s.ForPeer("alice/friend"); got != nil {
+ t.Errorf("Got %v, want nil", got)
+ }
+}
+
+func TestBlessingStoreSetReturnsOldValue(t *testing.T) {
+ var (
+ p = newPrincipal(t)
+ alice = blessSelf(t, p, "alice")
+ bob = blessSelf(t, p, "bob")
+ s = newInMemoryBlessingStore(p.PublicKey())
+ )
+ if old, err := s.Set(alice, "..."); old != nil || err != nil {
+ t.Errorf("Got (%v, %v)", old, err)
+ }
+ if old, err := s.Set(alice, "..."); !reflect.DeepEqual(old, alice) || err != nil {
+ t.Errorf("Got (%v, %v) want (%v, nil)", old, err, alice)
+ }
+ if old, err := s.Set(bob, "..."); !reflect.DeepEqual(old, alice) || err != nil {
+ t.Errorf("Got (%v, %v) want (%v, nil)", old, err, alice)
+ }
+ if old, err := s.Set(nil, "..."); !reflect.DeepEqual(old, bob) || err != nil {
+ t.Errorf("Got (%v, %v) want (%v, nil)", old, err, bob)
}
}
diff --git a/runtimes/google/rt/rt_test.go b/runtimes/google/rt/rt_test.go
index 53d3512..f9a9cf6 100644
--- a/runtimes/google/rt/rt_test.go
+++ b/runtimes/google/rt/rt_test.go
@@ -146,7 +146,7 @@
if err != nil {
t.Fatal(err)
}
- if err := p.BlessingStore().Add(blessing, security.AllPrincipals); err != nil {
+ if _, err := p.BlessingStore().Set(blessing, security.AllPrincipals); err != nil {
t.Fatal(err)
}
if err := p.AddToRoots(blessing); err != nil {
diff --git a/security/audit/principal_test.go b/security/audit/principal_test.go
index ba3d356..487154f 100644
--- a/security/audit/principal_test.go
+++ b/security/audit/principal_test.go
@@ -219,7 +219,7 @@
t.Fatal(err)
}
signer := security.NewInMemoryECDSASigner(key)
- p, err := security.CreatePrincipal(signer, &store{signer.PublicKey()}, &roots{})
+ p, err := security.CreatePrincipal(signer, nil, nil)
if err != nil {
t.Fatal(err)
}
@@ -253,20 +253,3 @@
}
return c, d
}
-
-// TODO(ashankar,ataly): Consider moving these implementations to veyron2/security/test so that various
-// test libraries do not need to implement this.
-type store struct {
- key security.PublicKey
-}
-
-func (*store) Add(blessings security.Blessings, forPeers security.BlessingPattern) error { return nil }
-func (*store) ForPeer(peerBlesssings ...string) security.Blessings { return nil }
-func (*store) SetDefault(blessings security.Blessings) error { return nil }
-func (*store) Default() security.Blessings { return nil }
-func (s *store) PublicKey() security.PublicKey { return s.key }
-
-type roots struct{}
-
-func (*roots) Add(security.PublicKey, security.BlessingPattern) error { return nil }
-func (*roots) Recognized(security.PublicKey, string) error { return nil }
