security: Avoid unnecessarily public key marshaling/unmarshaling.
Most places where "recognition" of a root public key is to be tested
(via BlessingRoots.Recognized), the marshaled form of the key is
available. However, given the BlessingRoots API prior to this commit,
the code ended up unmarshaling the bytes into an object and then the
BlessingRoots implementation would marshal it back into a string
in order to lookup a cache key. This was simply a waste of time.
The BlessingRoots API (in particular the Add and Recognized methods) are
likely to not be directly used outside the Vanadium runtime
implementation. By changing their signature to take serialized keys
instead of objects, we see a significant speedup (3-9x) in
RemoteBlessingNames, which will be used by developers more frequently.
go test v.io/v23/security -bench RemoteBlessingNames
(values are in ns/op):
BEFORE AFTER SPEEDUP
BenchmarkRemoteBlessingNames 75746 8451 9x
BenchmarkRemoteBlessingNames 24741 8081 3x [-tags noopenssl]
It is curious that the OpenSSL implementation is still a bit slower
than a pure Go implementation in this benchmark. I'm looking into that
(and currently creation of an OpenSSL-based public key does involved an
extra Marshaling/Unmarshaling).
MultiPart: 2/3
Change-Id: I2c9776175c51b9fc56d093567d71a8341125f358
diff --git a/cmd/principal/main.go b/cmd/principal/main.go
index e1eaadf..4b604bb 100644
--- a/cmd/principal/main.go
+++ b/cmd/principal/main.go
@@ -505,11 +505,7 @@
if err != nil {
return fmt.Errorf("invalid base64 encoding of public key: %v", err)
}
- key, err := security.UnmarshalPublicKey(der)
- if err != nil {
- return fmt.Errorf("invalid DER encoding of public key: %v", err)
- }
- return p.Roots().Add(key, security.BlessingPattern(args[0]))
+ return p.Roots().Add(der, security.BlessingPattern(args[0]))
}),
}
diff --git a/lib/security/blessingroots.go b/lib/security/blessingroots.go
index 5c0a7b2..a0b60ee 100644
--- a/lib/security/blessingroots.go
+++ b/lib/security/blessingroots.go
@@ -26,23 +26,15 @@
state blessingRootsState // GUARDED_BY(mu)
}
-func stateMapKey(root security.PublicKey) (string, error) {
- rootBytes, err := root.MarshalBinary()
- if err != nil {
- return "", err
- }
- return string(rootBytes), nil
-}
-
-func (br *blessingRoots) Add(root security.PublicKey, pattern security.BlessingPattern) error {
+func (br *blessingRoots) Add(root []byte, pattern security.BlessingPattern) error {
if pattern == security.AllPrincipals {
return verror.New(errRootsAddPattern, nil)
}
- key, err := stateMapKey(root)
- if err != nil {
+ // Sanity check to avoid invalid keys being added.
+ if _, err := security.UnmarshalPublicKey(root); err != nil {
return err
}
-
+ key := string(root)
br.mu.Lock()
defer br.mu.Unlock()
patterns := br.state[key]
@@ -61,20 +53,23 @@
return nil
}
-func (br *blessingRoots) Recognized(root security.PublicKey, blessing string) error {
- key, err := stateMapKey(root)
- if err != nil {
- return err
- }
-
+func (br *blessingRoots) Recognized(root []byte, blessing string) error {
+ key := string(root)
br.mu.RLock()
- defer br.mu.RUnlock()
for _, p := range br.state[key] {
if p.MatchedBy(blessing) {
+ br.mu.RUnlock()
return nil
}
}
- return security.NewErrUnrecognizedRoot(nil, root.String(), nil)
+ br.mu.RUnlock()
+ // Silly to have to unmarshal the public key on an error.
+ // Change the error message to not require that?
+ obj, err := security.UnmarshalPublicKey(root)
+ if err != nil {
+ return err
+ }
+ return security.NewErrUnrecognizedRoot(nil, obj.String(), nil)
}
func (br *blessingRoots) Dump() map[security.BlessingPattern][]security.PublicKey {
diff --git a/lib/security/blessingroots_test.go b/lib/security/blessingroots_test.go
index ad6afd5..47492e5 100644
--- a/lib/security/blessingroots_test.go
+++ b/lib/security/blessingroots_test.go
@@ -16,15 +16,20 @@
"v.io/v23/verror"
)
-type rootsTester [4]security.PublicKey
+type rootsTester [4][]byte
func newRootsTester() *rootsTester {
var tester rootsTester
- var err error
for idx := range tester {
- if tester[idx], _, err = NewPrincipalKey(); err != nil {
+ key, _, err := NewPrincipalKey()
+ if err != nil {
panic(err)
}
+ keybytes, err := key.MarshalBinary()
+ if err != nil {
+ panic(err)
+ }
+ tester[idx] = keybytes
}
return &tester
}
@@ -34,7 +39,7 @@
return fmt.Errorf("Add( , %v) succeeded, expected it to fail", security.AllPrincipals)
}
testdata := []struct {
- root security.PublicKey
+ root []byte
pattern security.BlessingPattern
}{
{t[0], "vanadium"},
@@ -52,7 +57,7 @@
func (t *rootsTester) testRecognized(br security.BlessingRoots) error {
testdata := []struct {
- root security.PublicKey
+ root []byte
recognized []string
notRecognized []string
}{
@@ -99,10 +104,17 @@
func (s pubKeySorter) Swap(i, j int) { s[i], s[j] = s[j], s[i] }
func (t *rootsTester) testDump(br security.BlessingRoots) error {
+ object := func(data []byte) security.PublicKey {
+ ret, err := security.UnmarshalPublicKey(data)
+ if err != nil {
+ panic(err)
+ }
+ return ret
+ }
want := map[security.BlessingPattern][]security.PublicKey{
- "google/foo": []security.PublicKey{t[1], t[2]},
- "google/$": []security.PublicKey{t[0]},
- "vanadium": []security.PublicKey{t[0]},
+ "google/foo": []security.PublicKey{object(t[1]), object(t[2])},
+ "google/$": []security.PublicKey{object(t[0])},
+ "vanadium": []security.PublicKey{object(t[0])},
}
got := br.Dump()
sort.Sort(pubKeySorter(want["google/foo"]))
diff --git a/services/agent/agentlib/agent_test.go b/services/agent/agentlib/agent_test.go
index 03d07fa..0aeb5e2 100644
--- a/services/agent/agentlib/agent_test.go
+++ b/services/agent/agentlib/agent_test.go
@@ -219,7 +219,10 @@
}
func runRecognizedNegativeBenchmark(b *testing.B, p security.Principal) {
- key := p.PublicKey()
+ key, err := p.PublicKey().MarshalBinary()
+ if err != nil {
+ b.Fatal(err)
+ }
b.ResetTimer()
for i := 0; i < b.N; i++ {
if d := p.Roots().Recognized(key, "foobar"); d == nil {
@@ -229,7 +232,10 @@
}
func runRecognizedBenchmark(b *testing.B, p security.Principal) {
- key := p.PublicKey()
+ key, err := p.PublicKey().MarshalBinary()
+ if err != nil {
+ b.Fatal(err)
+ }
blessing, err := p.BlessSelf("foobar")
if err != nil {
b.Fatal(err)
diff --git a/services/agent/agentlib/client.go b/services/agent/agentlib/client.go
index c5a68c8..39ccdc2 100644
--- a/services/agent/agentlib/client.go
+++ b/services/agent/agentlib/client.go
@@ -362,20 +362,12 @@
caller caller
}
-func (b *blessingRoots) Add(root security.PublicKey, pattern security.BlessingPattern) error {
- marshalledKey, err := root.MarshalBinary()
- if err != nil {
- return err
- }
- return b.caller.call("BlessingRootsAdd", results(), marshalledKey, pattern)
+func (b *blessingRoots) Add(root []byte, pattern security.BlessingPattern) error {
+ return b.caller.call("BlessingRootsAdd", results(), root, pattern)
}
-func (b *blessingRoots) Recognized(root security.PublicKey, blessing string) error {
- marshalledKey, err := root.MarshalBinary()
- if err != nil {
- return err
- }
- return b.caller.call("BlessingRootsRecognized", results(), marshalledKey, blessing)
+func (b *blessingRoots) Recognized(root []byte, blessing string) error {
+ return b.caller.call("BlessingRootsRecognized", results(), root, blessing)
}
func (b *blessingRoots) Dump() map[security.BlessingPattern][]security.PublicKey {
diff --git a/services/agent/internal/cache/cache.go b/services/agent/internal/cache/cache.go
index f4500a8..a3d3418 100644
--- a/services/agent/internal/cache/cache.go
+++ b/services/agent/internal/cache/cache.go
@@ -49,11 +49,8 @@
return roots, nil
}
-func (r *cachedRoots) Add(root security.PublicKey, pattern security.BlessingPattern) error {
- cacheKey, err := keyToString(root)
- if err != nil {
- return err
- }
+func (r *cachedRoots) Add(root []byte, pattern security.BlessingPattern) error {
+ cachekey := string(root)
defer r.mu.Unlock()
r.mu.Lock()
@@ -63,22 +60,19 @@
}
if r.cache != nil {
-
- r.cache[cacheKey] = append(r.cache[cacheKey], pattern)
+ r.cache[cachekey] = append(r.cache[cachekey], pattern)
}
return nil
}
-func (r *cachedRoots) Recognized(root security.PublicKey, blessing string) (result error) {
- key, err := keyToString(root)
- if err != nil {
- return err
- }
+func (r *cachedRoots) Recognized(root []byte, blessing string) (result error) {
+ cachekey := string(root)
- r.mu.RLock()
var cacheExists bool
+ var err error
+ r.mu.RLock()
if r.cache != nil {
- err = r.recognizeFromCache(key, root, blessing)
+ err = r.recognizeFromCache(cachekey, root, blessing)
cacheExists = true
}
r.mu.RUnlock()
@@ -89,7 +83,7 @@
r.mu.Unlock()
return err
}
- err = r.recognizeFromCache(key, root, blessing)
+ err = r.recognizeFromCache(cachekey, root, blessing)
r.mu.Unlock()
}
@@ -97,11 +91,11 @@
// to support the 'Dump' method.
r.mu.RLock()
if !r.dumpExists && err != nil {
- negKey := key + blessing
+ negKey := cachekey + blessing
negErr, ok := r.negative.Get(negKey)
if !ok {
r.mu.RUnlock()
- return r.recognizeFromImpl(key, root, blessing)
+ return r.recognizeFromImpl(root, blessing)
}
r.negative.Put(negKey, err)
err = negErr.(error)
@@ -150,13 +144,14 @@
return nil
}
- for p, keys := range dump {
- for _, key := range keys {
- cacheKey, err := keyToString(key)
+ for p, pubkeys := range dump {
+ for _, pubkey := range pubkeys {
+ keybytes, err := pubkey.MarshalBinary()
if err != nil {
return err
}
- r.cache[cacheKey] = append(r.cache[cacheKey], p)
+ cachekey := string(keybytes)
+ r.cache[cachekey] = append(r.cache[cachekey], p)
}
}
r.dumpExists = true
@@ -176,38 +171,44 @@
}
dump := make(map[security.BlessingPattern][]security.PublicKey)
for keyStr, patterns := range r.cache {
- key, err := security.UnmarshalPublicKey([]byte(keyStr))
+ pubkey, err := security.UnmarshalPublicKey([]byte(keyStr))
if err != nil {
logger.Global().Errorf("security.UnmarshalPublicKey(%v) returned error: %v", []byte(keyStr), err)
return nil
}
for _, p := range patterns {
- dump[p] = append(dump[p], key)
+ dump[p] = append(dump[p], pubkey)
}
}
return dump
}
// Must be called while holding mu.
-func (r *cachedRoots) recognizeFromCache(key string, root security.PublicKey, blessing string) error {
- for _, p := range r.cache[key] {
+func (r *cachedRoots) recognizeFromCache(cachekey string, root []byte, blessing string) error {
+ for _, p := range r.cache[cachekey] {
if p.MatchedBy(blessing) {
return nil
}
}
- return security.NewErrUnrecognizedRoot(nil, root.String(), nil)
+ // Silly to do this unmarshaling work on an error. Change the error string?
+ object, err := security.UnmarshalPublicKey(root)
+ if err != nil {
+ return err
+ }
+ return security.NewErrUnrecognizedRoot(nil, object.String(), nil)
}
// TODO(ataly): Get rid of this method once all agents have been updated
// to support the 'Dump' method.
-func (r *cachedRoots) recognizeFromImpl(key string, root security.PublicKey, blessing string) error {
- negKey := key + blessing
+func (r *cachedRoots) recognizeFromImpl(root []byte, blessing string) error {
+ cachekey := string(root)
err := r.impl.Recognized(root, blessing)
r.mu.Lock()
if err == nil {
- r.cache[key] = append(r.cache[key], security.BlessingPattern(blessing))
+ r.cache[cachekey] = append(r.cache[cachekey], security.BlessingPattern(blessing))
} else {
+ negKey := cachekey + blessing
r.negative.Put(negKey, err)
}
r.mu.Unlock()
@@ -218,7 +219,7 @@
// wraps over another implementation and adds caching.
type cachedStore struct {
mu *sync.RWMutex
- key security.PublicKey
+ pubkey security.PublicKey
def security.Blessings
hasDef bool
peers map[security.BlessingPattern]security.Blessings
@@ -302,7 +303,7 @@
}
func (s *cachedStore) PublicKey() security.PublicKey {
- return s.key
+ return s.pubkey
}
func (s *cachedStore) DebugString() string {
@@ -369,7 +370,7 @@
}
type dummySigner struct {
- key security.PublicKey
+ pubkey security.PublicKey
}
func (s dummySigner) Sign(purpose, message []byte) (security.Signature, error) {
@@ -378,7 +379,7 @@
}
func (s dummySigner) PublicKey() security.PublicKey {
- return s.key
+ return s.pubkey
}
func NewCachedPrincipal(ctx *context.T, impl agent.Principal, call rpc.ClientCall) (p agent.Principal, err error) {
@@ -411,9 +412,9 @@
return
}
cachedStore := &cachedStore{
- mu: &mu,
- key: impl.PublicKey(),
- impl: impl.BlessingStore(),
+ mu: &mu,
+ pubkey: impl.PublicKey(),
+ impl: impl.BlessingStore(),
}
flush = func() {
defer mu.Unlock()
@@ -429,11 +430,3 @@
p = &cachedPrincipal{sp, impl}
return
}
-
-func keyToString(key security.PublicKey) (string, error) {
- bytes, err := key.MarshalBinary()
- if err != nil {
- return "", err
- }
- return string(bytes), nil
-}
diff --git a/services/agent/internal/cache/cache_test.go b/services/agent/internal/cache/cache_test.go
index e0c2193..3f669ad 100644
--- a/services/agent/internal/cache/cache_test.go
+++ b/services/agent/internal/cache/cache_test.go
@@ -16,7 +16,7 @@
"v.io/x/ref/test/testutil"
)
-func createRoots() (security.PublicKey, security.BlessingRoots, *cachedRoots) {
+func createRoots() ([]byte, security.BlessingRoots, *cachedRoots) {
var mu sync.RWMutex
ctx, _ := context.RootContext()
ctx = context.WithLogger(ctx, logger.Global())
@@ -26,7 +26,11 @@
if err != nil {
panic(err)
}
- return p.PublicKey(), impl, roots
+ keybytes, err := p.PublicKey().MarshalBinary()
+ if err != nil {
+ panic(err)
+ }
+ return keybytes, impl, roots
}
func TestCreateRoots(t *testing.T) {
@@ -39,17 +43,15 @@
}
}
-func expectRecognized(roots security.BlessingRoots, key security.PublicKey, blessing string) string {
- err := roots.Recognized(key, blessing)
- if err != nil {
+func expectRecognized(roots security.BlessingRoots, key []byte, blessing string) string {
+ if err := roots.Recognized(key, blessing); err != nil {
return fmt.Sprintf("Key (%s, %v) not matched by roots:\n%s, Recognized returns error: %v", key, blessing, roots.DebugString(), err)
}
return ""
}
-func expectNotRecognized(roots security.BlessingRoots, key security.PublicKey, blessing string) string {
- err := roots.Recognized(key, blessing)
- if err == nil {
+func expectNotRecognized(roots security.BlessingRoots, key []byte, blessing string) string {
+ if err := roots.Recognized(key, blessing); err == nil {
return fmt.Sprintf("Key (%s, %s) should not match roots:\n%s", key, blessing, roots.DebugString())
}
return ""
@@ -164,7 +166,7 @@
func createStore(p security.Principal) (security.BlessingStore, *cachedStore) {
var mu sync.RWMutex
impl := p.BlessingStore()
- return impl, &cachedStore{mu: &mu, key: p.PublicKey(), impl: impl}
+ return impl, &cachedStore{mu: &mu, pubkey: p.PublicKey(), impl: impl}
}
func TestDefaultBlessing(t *testing.T) {
diff --git a/services/agent/internal/server/server.go b/services/agent/internal/server/server.go
index ba9c579..a9ccc5c 100644
--- a/services/agent/internal/server/server.go
+++ b/services/agent/internal/server/server.go
@@ -305,23 +305,15 @@
}
func (a *agentd) BlessingRootsAdd(root []byte, pattern security.BlessingPattern) error {
- pkey, err := security.UnmarshalPublicKey(root)
- if err != nil {
- return err
- }
defer a.unlock()
a.mu.Lock()
- return a.principal.Roots().Add(pkey, pattern)
+ return a.principal.Roots().Add(root, pattern)
}
func (a *agentd) BlessingRootsRecognized(root []byte, blessing string) error {
- pkey, err := security.UnmarshalPublicKey(root)
- if err != nil {
- return err
- }
defer a.mu.RUnlock()
a.mu.RLock()
- return a.principal.Roots().Recognized(pkey, blessing)
+ return a.principal.Roots().Recognized(root, blessing)
}
func (a *agentd) BlessingRootsDump() (map[security.BlessingPattern][][]byte, error) {
diff --git a/services/agent/internal/test_principal/main.go b/services/agent/internal/test_principal/main.go
index 40c4f5c..09e03d8 100644
--- a/services/agent/internal/test_principal/main.go
+++ b/services/agent/internal/test_principal/main.go
@@ -99,13 +99,17 @@
errorf("MintDischarge: %v", err)
}
// BlessingRoots
- if err := p.Roots().Recognized(p.PublicKey(), "batman"); err == nil {
+ keybytes, err := p.PublicKey().MarshalBinary()
+ if err != nil {
+ errorf("Failed to marshal public key: %v", err)
+ }
+ if err := p.Roots().Recognized(keybytes, "batman"); err == nil {
errorf("Roots().Recognized returned nil")
}
if err := p.AddToRoots(b); err != nil {
errorf("AddToRoots: %v", err)
}
- if err := p.Roots().Recognized(p.PublicKey(), "batman"); err != nil {
+ if err := p.Roots().Recognized(keybytes, "batman"); err != nil {
errorf("Roots().Recognized: %v", err)
}
// BlessingStore: Defaults
diff --git a/services/device/claimable/main.go b/services/device/claimable/main.go
index 0fd53e4..88aab9a 100644
--- a/services/device/claimable/main.go
+++ b/services/device/claimable/main.go
@@ -70,14 +70,10 @@
if err := json.Unmarshal([]byte(jRoot), &bRoot); err != nil {
ctx.Fatalf("unable to unmarshal the json blessing root: %v", err)
}
- decodedKey, err := base64.URLEncoding.DecodeString(bRoot.PublicKey)
+ key, err := base64.URLEncoding.DecodeString(bRoot.PublicKey)
if err != nil {
ctx.Fatalf("unable to decode public key: %v", err)
}
- key, err := security.UnmarshalPublicKey(decodedKey)
- if err != nil {
- ctx.Fatalf("unable to unmarshal the public key: %v", err)
- }
roots := v23.GetPrincipal(ctx).Roots()
for _, name := range bRoot.Names {
if err := roots.Add(key, security.BlessingPattern(name)); err != nil {
diff --git a/services/wspr/browsprd/main_nacl.go b/services/wspr/browsprd/main_nacl.go
index 08940f7..d8f2561 100644
--- a/services/wspr/browsprd/main_nacl.go
+++ b/services/wspr/browsprd/main_nacl.go
@@ -214,16 +214,16 @@
}
inst.logger.VI(1).Infof("Using blessing roots for identity with key %v and names %v", msg.IdentitydBlessingRoot.PublicKey, msg.IdentitydBlessingRoot.Names)
- key, err := decodeAndUnmarshalPublicKey(msg.IdentitydBlessingRoot.PublicKey)
+ keybytes, err := base64.URLEncoding.DecodeString(msg.IdentitydBlessingRoot.PublicKey)
if err != nil {
- inst.logger.Fatalf("decodeAndUnmarshalPublicKey(%v) failed: %v", msg.IdentitydBlessingRoot.PublicKey, err)
+ inst.logger.Fatalf("failed to decode public key (%v): %v", msg.IdentitydBlessingRoot.PublicKey, err)
}
for _, name := range msg.IdentitydBlessingRoot.Names {
pattern := security.BlessingPattern(name)
// Trust the identity servers blessing root.
- p.Roots().Add(key, pattern)
+ p.Roots().Add(keybytes, pattern)
// Use our blessing to only talk to the identity server.
if _, err := p.BlessingStore().Set(blessing, pattern); err != nil {
diff --git a/test/testutil/security_test.go b/test/testutil/security_test.go
index b13eb6b..11df3c2 100644
--- a/test/testutil/security_test.go
+++ b/test/testutil/security_test.go
@@ -17,10 +17,14 @@
if err := idp.Bless(p, "bar"); err != nil {
t.Fatal(err)
}
- if err := p.Roots().Recognized(idp.PublicKey(), "foo"); err != nil {
+ idpkey, err := idp.PublicKey().MarshalBinary()
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := p.Roots().Recognized(idpkey, "foo"); err != nil {
t.Error(err)
}
- if err := p.Roots().Recognized(idp.PublicKey(), "foo/bar"); err != nil {
+ if err := p.Roots().Recognized(idpkey, "foo/bar"); err != nil {
t.Error(err)
}
def := p.BlessingStore().Default()
