security: Make Discharge a struct (instead of an interface).
This commit accompanies:
https://vanadium-review.googlesource.com/#/c/5432/
Motivation:
(1) Will allow us to eventually use VDL support for automatic
translation between wire and native types, thereby relieving
developers of the need to manually convert to the WireDischarge format
(2) Discharges are not meant to be implemented outside the v23/security
package anyway as all processes must be able to understand them
(i.e., no user-defined ThirdPartyCaveats/Discharges)
MultiPart: 2/2
Change-Id: Ib145ff7d9f4c3a1786e2a9c876c22552a5fb99d6
diff --git a/runtimes/google/ipc/discharges.go b/runtimes/google/ipc/discharges.go
index dab6861..8a858a0 100644
--- a/runtimes/google/ipc/discharges.go
+++ b/runtimes/google/ipc/discharges.go
@@ -58,10 +58,11 @@
}
// Gather discharges from cache.
- discharges := make([]security.Discharge, len(caveats))
+ // (Collect a set of pointers, where nil implies a missing discharge)
+ discharges := make([]*security.Discharge, len(caveats))
if d.cache.Discharges(caveats, discharges) > 0 {
- // Fetch discharges for caveats for which no discharges were found
- // in the cache.
+ // Fetch discharges for caveats for which no discharges were
+ // found in the cache.
if ctx == nil {
ctx = d.defaultCtx
}
@@ -74,7 +75,7 @@
}
for _, d := range discharges {
if d != nil {
- ret = append(ret, d)
+ ret = append(ret, *d)
}
}
return
@@ -83,23 +84,24 @@
d.cache.invalidate(discharges...)
}
-// fetchDischarges fills in out by fetching discharges for caveats from the
+// fetchDischarges fills out by fetching discharges for caveats from the
// appropriate discharge service. Since there may be dependencies in the
// caveats, fetchDischarges keeps retrying until either all discharges can be
// fetched or no new discharges are fetched.
// REQUIRES: len(caveats) == len(out)
// REQUIRES: caveats[i].ThirdPartyDetails() != nil for 0 <= i < len(caveats)
-func (d *dischargeClient) fetchDischarges(ctx *context.T, caveats []security.Caveat, impetus security.DischargeImpetus, out []security.Discharge) {
+func (d *dischargeClient) fetchDischarges(ctx *context.T, caveats []security.Caveat, impetus security.DischargeImpetus, out []*security.Discharge) {
var wg sync.WaitGroup
for {
type fetched struct {
idx int
- discharge security.Discharge
+ discharge *security.Discharge
}
discharges := make(chan fetched, len(caveats))
want := 0
for i := range caveats {
if out[i] != nil {
+ // Already fetched
continue
}
want++
@@ -118,18 +120,15 @@
vlog.VI(3).Infof("Discharge fetch for %v failed: (%v)", cav, err)
return
}
- d, err := security.NewDischarge(wire)
- if err != nil {
- vlog.Errorf("fetchDischarges: server at %s sent a malformed discharge: %v", tp.Location(), err)
- }
- discharges <- fetched{i, d}
+ d := security.NewDischarge(wire)
+ discharges <- fetched{i, &d}
}(i, ctx, caveats[i])
}
wg.Wait()
close(discharges)
var got int
for fetched := range discharges {
- d.cache.Add(fetched.discharge)
+ d.cache.Add(*fetched.discharge)
out[fetched.idx] = fetched.discharge
got++
}
@@ -145,7 +144,7 @@
// dischargeCache is a concurrency-safe cache for third party caveat discharges.
type dischargeCache struct {
mu sync.RWMutex
- cache map[string]security.Discharge // GUARDED_BY(RWMutex)
+ cache map[string]security.Discharge // GUARDED_BY(mu)
}
// Add inserts the argument to the cache, possibly overwriting previous
@@ -159,18 +158,20 @@
}
// Discharges takes a slice of caveats and a slice of discharges of the same
-// length and fills in nil entries in the discharges slice with discharges
-// from the cache (if there are any).
+// length and fills in nil entries in the discharges slice with pointers to
+// cached discharges (if there are any).
//
// REQUIRES: len(caveats) == len(out)
// REQUIRES: caveats[i].ThirdPartyDetails() != nil, for all 0 <= i < len(caveats)
-func (dcc *dischargeCache) Discharges(caveats []security.Caveat, out []security.Discharge) (remaining int) {
+func (dcc *dischargeCache) Discharges(caveats []security.Caveat, out []*security.Discharge) (remaining int) {
dcc.mu.Lock()
for i, d := range out {
if d != nil {
continue
}
- if out[i] = dcc.cache[caveats[i].ThirdPartyDetails().ID()]; out[i] == nil {
+ if cached, exists := dcc.cache[caveats[i].ThirdPartyDetails().ID()]; exists {
+ out[i] = &cached
+ } else {
remaining++
}
}
@@ -181,9 +182,15 @@
func (dcc *dischargeCache) invalidate(discharges ...security.Discharge) {
dcc.mu.Lock()
for _, d := range discharges {
- if dcc.cache[d.ID()] == d {
- delete(dcc.cache, d.ID())
- }
+ // TODO(ashankar,ataly): The cached discharge might have been
+ // replaced by the time invalidate is called.
+ // Should we have an "Equals" function defined on "Discharge"
+ // and use that? (Could use reflect.DeepEqual as well, but
+ // that will likely be expensive)
+ // if cached := dcc.cache[d.ID()]; cached.Equals(d) {
+ // delete(dcc.cache, d.ID())
+ // }
+ delete(dcc.cache, d.ID())
}
dcc.mu.Unlock()
}
diff --git a/runtimes/google/ipc/server.go b/runtimes/google/ipc/server.go
index 36545a0..2d3a414 100644
--- a/runtimes/google/ipc/server.go
+++ b/runtimes/google/ipc/server.go
@@ -1177,11 +1177,8 @@
fs.ackBlessings = true
}
- for i, d := range req.Discharges {
- dis, err := security.NewDischarge(d)
- if err != nil {
- return verror.New(verror.ErrBadProtocol, fs.T, newErrBadDischarge(fs.T, uint64(i), err))
- }
+ for _, d := range req.Discharges {
+ dis := security.NewDischarge(d)
fs.discharges[dis.ID()] = dis
}
return nil
diff --git a/runtimes/google/ipc/stream/vc/auth.go b/runtimes/google/ipc/stream/vc/auth.go
index 73ae50c..e7c12f2 100644
--- a/runtimes/google/ipc/stream/vc/auth.go
+++ b/runtimes/google/ipc/stream/vc/auth.go
@@ -168,10 +168,7 @@
if len(wired) > 0 {
discharges = make(map[string]security.Discharge)
for _, w := range wired {
- d, err := security.NewDischarge(w)
- if err != nil {
- return nil, nil, err
- }
+ d := security.NewDischarge(w)
discharges[d.ID()] = d
}
}
diff --git a/runtimes/google/ipc/stream/vc/vc_test.go b/runtimes/google/ipc/stream/vc/vc_test.go
index 8d401b8..ef35895 100644
--- a/runtimes/google/ipc/stream/vc/vc_test.go
+++ b/runtimes/google/ipc/stream/vc/vc_test.go
@@ -247,8 +247,8 @@
func TestConnect(t *testing.T) { testConnect(t, SecurityNone) }
func TestConnectTLS(t *testing.T) { testConnect(t, SecurityTLS) }
-func testConnect_Version6(t *testing.T, security options.VCSecurityLevel) {
- h, vc := New(security, version.IPCVersion6, tsecurity.NewPrincipal("client"), tsecurity.NewPrincipal("server"))
+func testConnect_Version7(t *testing.T, security options.VCSecurityLevel) {
+ h, vc := New(security, version.IPCVersion7, tsecurity.NewPrincipal("client"), tsecurity.NewPrincipal("server"))
defer h.Close()
flow, err := vc.Connect()
if err != nil {
@@ -256,8 +256,8 @@
}
testFlowEcho(t, flow, 10)
}
-func TestConnect_Version6(t *testing.T) { testConnect_Version6(t, SecurityNone) }
-func TestConnect_Version6TLS(t *testing.T) { testConnect_Version6(t, SecurityTLS) }
+func TestConnect_Version7(t *testing.T) { testConnect_Version7(t, SecurityNone) }
+func TestConnect_Version7TLS(t *testing.T) { testConnect_Version7(t, SecurityTLS) }
// helper function for testing concurrent operations on multiple flows over the
// same VC. Such tests are most useful when running the race detector.
diff --git a/security/agent/client.go b/security/agent/client.go
index 85f379a..e32e886 100644
--- a/security/agent/client.go
+++ b/security/agent/client.go
@@ -134,9 +134,9 @@
func (c *client) MintDischarge(forCaveat, caveatOnDischarge security.Caveat, additionalCaveatsOnDischarge ...security.Caveat) (security.Discharge, error) {
var discharge security.WireDischarge
if err := c.caller.call("MintDischarge", results(&discharge), forCaveat, caveatOnDischarge, additionalCaveatsOnDischarge); err != nil {
- return nil, err
+ return security.Discharge{}, err
}
- return security.NewDischarge(discharge)
+ return security.NewDischarge(discharge), nil
}
func (c *client) PublicKey() security.PublicKey {
diff --git a/security/audit/principal.go b/security/audit/principal.go
index 530b1b2..f1095b8 100644
--- a/security/audit/principal.go
+++ b/security/audit/principal.go
@@ -50,7 +50,7 @@
d, err := p.principal.MintDischarge(forCaveat, caveatOnDischarge, additionalCaveatsOnDischarge...)
// No need to log the discharge
if err = p.audit(err, "MintDischarge", addCaveats(args{forCaveat, caveatOnDischarge}, additionalCaveatsOnDischarge...), nil); err != nil {
- return nil, err
+ return security.Discharge{}, err
}
return d, nil
}
