From b8cfdd4e975e5e04832725352b1258585e939f7a Mon Sep 17 00:00:00 2001 From: Oncilla Date: Fri, 6 Dec 2019 08:53:30 +0100 Subject: [PATCH] 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 | 3 + go/lib/infra/modules/trust/v2/resolver.go | 46 +++++++++++---- .../infra/modules/trust/v2/resolver_test.go | 58 +++++-------------- .../trust/v2/testdata/gen_crypto_tar.sh | 9 +++ 4 files changed, 61 insertions(+), 55 deletions(-) diff --git a/go/lib/infra/modules/trust/v2/main_test.go b/go/lib/infra/modules/trust/v2/main_test.go index cae7e5f63b..f8fec45e3c 100644 --- a/go/lib/infra/modules/trust/v2/main_test.go +++ b/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") diff --git a/go/lib/infra/modules/trust/v2/resolver.go b/go/lib/infra/modules/trust/v2/resolver.go index 5526c0ce6b..513053beab 100644 --- a/go/lib/infra/modules/trust/v2/resolver.go +++ b/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,24 +113,47 @@ 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) { @@ -137,9 +161,9 @@ func (r *resolver) Chain(ctx context.Context, req ChainReq, 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 diff --git a/go/lib/infra/modules/trust/v2/resolver_test.go b/go/lib/infra/modules/trust/v2/resolver_test.go index eec0c57981..614c7353e9 100644 --- a/go/lib/infra/modules/trust/v2/resolver_test.go +++ b/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 } diff --git a/go/lib/infra/modules/trust/v2/testdata/gen_crypto_tar.sh b/go/lib/infra/modules/trust/v2/testdata/gen_crypto_tar.sh index d99c69a38a..1e2b38eee4 100755 --- a/go/lib/infra/modules/trust/v2/testdata/gen_crypto_tar.sh +++ b/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 .