TrustStore: Sanity check resolved TRCs (#3456)

Check that the reply for a TRC rpc contains a TRC with the correct
ISD and version.
Additionally, refactor the test to use actual TRC updates instead of
fake ones.

go/lib/infra/modules/trust/v2/main_test.go

@@ -46,6 +46,9 @@ func (desc TRCDesc) File() string {
 
 var (
 	trc1v1 = TRCDesc{ISD: 1, Version: 1}
+	trc1v2 = TRCDesc{ISD: 1, Version: 2}
+	trc1v3 = TRCDesc{ISD: 1, Version: 3}
+	trc1v4 = TRCDesc{ISD: 1, Version: 4}
 
 	// primary ASes
 	ia110 = xtest.MustParseIA("1-ff00:0:110")

go/lib/infra/modules/trust/v2/resolver.go

@@ -32,6 +32,8 @@ var (
 	// ErrResolveSuperseded indicates that the latest locally available TRC
 	// supersedes the TRC to resolve.
 	ErrResolveSuperseded = serrors.New("latest locally available is newer")
+	// ErrInvalidResponse indicates an invalid response to an RPC call.
+	ErrInvalidResponse = serrors.New("invalid RPC response")
 )
 
 // Resolver resolves verified trust material.
@@ -72,10 +74,11 @@ func (r *resolver) TRC(ctx context.Context, req TRCReq, server net.Addr) (decode
 	if err == nil {
 		min = prev.Version + 1
 	}
-	results := make([]chan rawOrErr, req.Version-min+1)
+	// Use slice of channels because the ordering of TRC replies matters.
+	results := make([]chan resOrErr, req.Version-min+1)
 	for i := range results {
 		// buffered channel to avoid go routine leak.
-		results[i] = make(chan rawOrErr, 1)
+		results[i] = make(chan resOrErr, 1)
 		ireq := req.withVersion(min + scrypto.Version(i))
 		r.startFetchTRC(ctx, results[i], ireq, server)
 	}
@@ -87,9 +90,7 @@ func (r *resolver) TRC(ctx context.Context, req TRCReq, server net.Addr) (decode
 		if res.Err != nil {
 			return decoded.TRC{}, serrors.WrapStr("unable to fetch parts of TRC chain", err)
 		}
-		if decTRC, err = decoded.DecodeTRC(res.Raw); err != nil {
-			return decoded.TRC{}, serrors.WrapStr("failed to parse parts of TRC chain", err)
-		}
+		decTRC = res.Decoded
 		if err = r.inserter.InsertTRC(ctx, decTRC, w.TRC); err != nil {
 			return decoded.TRC{}, serrors.WrapStr("unable to insert parts of TRC chain", err,
 				"trc", decTRC)
@@ -112,34 +113,57 @@ func (r *resolver) resolveLatestVersion(ctx context.Context, req TRCReq,
 	if err != nil {
 		return 0, serrors.WrapStr("error parsing latest TRC", err)
 	}
+	if err := r.trcCheck(req, decTRC.TRC); err != nil {
+		return 0, serrors.Wrap(ErrInvalidResponse, err)
+	}
 	return decTRC.TRC.Version, nil
 }
 
-func (r *resolver) startFetchTRC(ctx context.Context, res chan<- rawOrErr,
+func (r *resolver) startFetchTRC(ctx context.Context, res chan<- resOrErr,
 	req TRCReq, server net.Addr) {
 
 	go func() {
 		defer log.LogPanicAndExit()
 		rawTRC, err := r.rpc.GetTRC(ctx, req, server)
 		if err != nil {
-			res <- rawOrErr{Err: serrors.WrapStr("error requesting TRC", err,
+			res <- resOrErr{Err: serrors.WrapStr("error requesting TRC", err,
+				"isd", req.ISD, "version", req.Version)}
+			return
+		}
+		decTRC, err := decoded.DecodeTRC(rawTRC)
+		if err != nil {
+			res <- resOrErr{Err: serrors.WrapStr("failed to parse TRC", err,
 				"isd", req.ISD, "version", req.Version)}
 			return
 		}
-		res <- rawOrErr{Raw: rawTRC}
+		if err := r.trcCheck(req, decTRC.TRC); err != nil {
+			res <- resOrErr{Err: serrors.Wrap(ErrInvalidResponse, err)}
+			return
+		}
+		res <- resOrErr{Decoded: decTRC}
 	}()
 }
 
+func (r *resolver) trcCheck(req TRCReq, t *trc.TRC) error {
+	switch {
+	case req.ISD != t.ISD:
+		return serrors.New("wrong isd", "expected", req.ISD, "actual", t.ISD)
+	case !req.Version.IsLatest() && req.Version != t.Version:
+		return serrors.New("wrong version", "expected", req.Version, "actual", t.Version)
+	}
+	return nil
+}
+
 func (r *resolver) Chain(ctx context.Context, req ChainReq,
 	server net.Addr) (decoded.Chain, error) {
 
 	// TODO(roosd): implement
 	return decoded.Chain{}, serrors.New("not implemented")
 }
 
-type rawOrErr struct {
-	Raw []byte
-	Err error
+type resOrErr struct {
+	Decoded decoded.TRC
+	Err     error
 }
 
 // prevWrap provides one single previous TRC. It is used in the TRC chain

go/lib/infra/modules/trust/v2/resolver_test.go

@@ -21,15 +21,14 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"golang.org/x/xerrors"
 
 	"github.com/scionproto/scion/go/lib/addr"
 	trust "github.com/scionproto/scion/go/lib/infra/modules/trust/v2"
 	"github.com/scionproto/scion/go/lib/infra/modules/trust/v2/internal/decoded"
 	"github.com/scionproto/scion/go/lib/infra/modules/trust/v2/mock_v2"
 	"github.com/scionproto/scion/go/lib/scrypto"
-	"github.com/scionproto/scion/go/lib/scrypto/trc/v2"
 	"github.com/scionproto/scion/go/lib/serrors"
+	"github.com/scionproto/scion/go/lib/xtest"
 )
 
 func TestResolverTRC(t *testing.T) {
@@ -46,27 +45,26 @@ func TestResolverTRC(t *testing.T) {
 	}{
 		"Fetch missing links successfully": {
 			Expect: func(t *testing.T, m mocks) decoded.TRC {
-				trcs := testTRCs(t, 1)
 				m.DB.EXPECT().GetTRC(gomock.Any(), addr.ISD(1), scrypto.LatestVer).Return(
-					trcs[1].TRC, nil,
+					loadTRC(t, trc1v1).TRC, nil,
 				)
 				req := trust.TRCReq{ISD: 1, Version: scrypto.LatestVer}
-				m.RPC.EXPECT().GetTRC(gomock.Any(), req, nil).Return(trcs[4].Raw, nil)
-				for i := scrypto.Version(2); i <= scrypto.Version(4); i++ {
-					v := i
-					req := trust.TRCReq{ISD: 1, Version: v}
-					m.RPC.EXPECT().GetTRC(gomock.Any(), req, nil).Return(trcs[req.Version].Raw, nil)
-					m.Inserter.EXPECT().InsertTRC(gomock.Any(), trcs[v], gomock.Any()).DoAndReturn(
+				m.RPC.EXPECT().GetTRC(gomock.Any(), req, nil).Return(loadTRC(t, trc1v4).Raw, nil)
+				for _, desc := range []TRCDesc{trc1v2, trc1v3, trc1v4} {
+					req := trust.TRCReq{ISD: desc.ISD, Version: desc.Version}
+					dec := loadTRC(t, desc)
+					m.RPC.EXPECT().GetTRC(gomock.Any(), req, nil).Return(dec.Raw, nil)
+					m.Inserter.EXPECT().InsertTRC(gomock.Any(), dec, gomock.Any()).DoAndReturn(
 						func(_ interface{}, decTRC decoded.TRC, p trust.TRCProviderFunc) error {
-							prev, err := p(nil, 1, v-1)
+							prev, err := p(nil, 1, req.Version-1)
 							require.NoError(t, err)
-							assert.Equal(t, trcs[v-1].TRC, prev)
-							assert.Equal(t, trcs[v], decTRC)
+							assert.Equal(t, req.Version-1, prev.Version)
+							assert.Equal(t, dec, decTRC)
 							return nil
 						},
 					)
 				}
-				return trcs[4]
+				return loadTRC(t, trc1v4)
 			},
 			TRCReq: trust.TRCReq{ISD: 1, Version: scrypto.LatestVer},
 		},
@@ -82,9 +80,8 @@ func TestResolverTRC(t *testing.T) {
 		},
 		"Superseded": {
 			Expect: func(t *testing.T, m mocks) decoded.TRC {
-				trcs := testTRCs(t, 1)
 				m.DB.EXPECT().GetTRC(gomock.Any(), addr.ISD(1), scrypto.LatestVer).Return(
-					trcs[3].TRC, nil,
+					loadTRC(t, trc1v3).TRC, nil,
 				)
 				return decoded.TRC{}
 			},
@@ -112,35 +109,8 @@ func TestResolverTRC(t *testing.T) {
 			expected := test.Expect(t, m)
 			r := trust.NewResolver(m.DB, m.Inserter, m.RPC)
 			decTRC, err := r.TRC(context.Background(), test.TRCReq, nil)
+			xtest.AssertErrorsIs(t, err, test.ExpectedErr)
 			assert.Equal(t, expected, decTRC)
-			if test.ExpectedErr != nil {
-				require.Error(t, err)
-				assert.Truef(t, xerrors.Is(err, test.ExpectedErr),
-					"actual: %s\nexpected: %s", err, test.ExpectedErr)
-			} else {
-				require.NoError(t, err)
-			}
-		})
-	}
-}
-
-// testTRCs returns a map of parsable TRCs. They are not verifiable.
-// FIXME(roosd): Replace this with actual verifiable TRCs in the crypto tar.
-func testTRCs(t *testing.T, isd addr.ISD) map[scrypto.Version]decoded.TRC {
-	baseTRC := loadTRC(t, TRCDesc{ISD: isd, Version: 1})
-	trcs := map[scrypto.Version]decoded.TRC{1: baseTRC}
-	for i := scrypto.Version(2); i < scrypto.Version(5); i++ {
-		update := *baseTRC.TRC
-		update.Version = i
-		enc, err := trc.Encode(&update)
-		require.NoError(t, err)
-		raw, err := trc.EncodeSigned(trc.Signed{
-			Signatures: baseTRC.Signed.Signatures,
-			EncodedTRC: enc,
 		})
-		require.NoError(t, err)
-		trcs[i], err = decoded.DecodeTRC(raw)
-		require.NoError(t, err)
 	}
-	return trcs
 }

go/lib/infra/modules/trust/v2/testdata/gen_crypto_tar.sh

@@ -11,6 +11,15 @@ TMP=`mktemp -d`
 
 $1 v2 tmpl topo -d $TMP ./topology/Default.topo > /dev/null
 $1 v2 keys private -d $TMP "*-*" > /dev/null
+
 $1 v2 trcs gen -d $TMP "*" > /dev/null
+for i in {2..4}
+do
+    sed -e "s/^version = 1/version = $i/g" \
+        -e 's/^votes = \[\]/votes = \["ff00:0:110", "ff00:0:120"\]/g' \
+        -e 's/^grace_period = "0s"/grace_period = "1h"/g' \
+        $TMP/ISD1/trc-v1.toml > $TMP/ISD1/trc-v$i.toml
+    $1 v2 trcs gen -d $TMP --version $i "1" > /dev/null
+done
 
 tar -C $TMP -cf $2 .