Repository: scionproto/scion
Pull request #3464 (2019-12-06 10:04:36)

trust: start using snet.UDPAddr
which replaces the snet.Addr

Contributes: #3344

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/scionproto/scion/3464)
<!-- Reviewable:end -->



Patch:
diff --git a/go/lib/infra/modules/trust/handlers.go b/go/lib/infra/modules/trust/handlers.go
index 179af67ce..fe8470213 100644
--- a/go/lib/infra/modules/trust/handlers.go
+++ b/go/lib/infra/modules/trust/handlers.go
@@ -16,6 +16,7 @@ package trust
 
 import (
 	"context"
+	"fmt"
 	"net"
 
 	"github.com/scionproto/scion/go/lib/addr"
@@ -28,6 +29,7 @@ import (
 	"github.com/scionproto/scion/go/lib/scrypto"
 	"github.com/scionproto/scion/go/lib/scrypto/cert"
 	"github.com/scionproto/scion/go/lib/scrypto/trc"
+	"github.com/scionproto/scion/go/lib/serrors"
 	"github.com/scionproto/scion/go/lib/snet"
 	"github.com/scionproto/scion/go/proto"
 )
@@ -46,11 +48,20 @@ type trcReqHandler struct {
 
 func (h *trcReqHandler) Handle() *infra.HandlerResult {
 	logger := log.FromCtx(h.request.Context())
+
 	l := metrics.HandlerLabels{
-		Client:  addrLocation(h.request.Peer, h.store.ia),
 		ReqType: metrics.TRCReq,
 		Result:  metrics.ErrInternal,
 	}
+
+	peer, err := redirectToUDP(h.request.Peer)
+	if err != nil {
+		logger.Error("[TrustStore:trcReqHandler] wrong address type", "err", err)
+		metrics.Handler.Request(l).Inc()
+		return infra.MetricsErrInternal
+	}
+	l.Client = addrLocation(peer, h.store.ia)
+
 	trcReq, ok := h.request.Message.(*cert_mgmt.TRCReq)
 	if !ok {
 		logger.Error("[TrustStore:trcReqHandler] wrong message type, expected cert_mgmt.TRCReq",
@@ -73,7 +84,6 @@ func (h *trcReqHandler) Handle() *infra.HandlerResult {
 	subCtx = metrics.CtxWith(subCtx, metrics.TRCReq)
 
 	var trcObj *trc.TRC
-	var err error
 	// Only allow network traffic to be sent out if recursion is enabled and
 	// CacheOnly is not requested.
 	// FIXME(scrye): when protocol support is in, the below needs to select
@@ -91,8 +101,8 @@ func (h *trcReqHandler) Handle() *infra.HandlerResult {
 			TrustStoreOpts: infra.TrustStoreOpts{LocalOnly: !h.recurse},
 			AllowInactive:  true,
 		}
-		trcObj, err = h.store.getTRC(subCtx, trcReq.ISD,
-			scrypto.Version(trcReq.Version), opts, h.request.Peer)
+
+		trcObj, err = h.store.getTRC(subCtx, trcReq.ISD, trcReq.Version, opts, peer)
 		if err != nil {
 			logger.Error("[TrustStore:trcReqHandler] Unable to retrieve TRC", "err", err)
 			metrics.Handler.Request(l).Inc()
@@ -135,11 +145,20 @@ type chainReqHandler struct {
 
 func (h *chainReqHandler) Handle() *infra.HandlerResult {
 	logger := log.FromCtx(h.request.Context())
+
 	l := metrics.HandlerLabels{
-		Client:  addrLocation(h.request.Peer, h.store.ia),
 		ReqType: metrics.ChainReq,
 		Result:  metrics.ErrInternal,
 	}
+
+	peer, err := redirectToUDP(h.request.Peer)
+	if err != nil {
+		logger.Error("[TrustStore:trcReqHandler] wrong address type", "err", err)
+		metrics.Handler.Request(l).Inc()
+		return infra.MetricsErrInternal
+	}
+	l.Client = addrLocation(peer, h.store.ia)
+
 	chainReq, ok := h.request.Message.(*cert_mgmt.ChainReq)
 	if !ok {
 		logger.Error("[TrustStore:chainReqHandler] wrong message type, expected cert_mgmt.ChainReq",
@@ -149,7 +168,7 @@ func (h *chainReqHandler) Handle() *infra.HandlerResult {
 	}
 	l.CacheOnly = chainReq.CacheOnly
 	logger.Debug("[TrustStore:chainReqHandler] Received request", "chainReq", chainReq,
-		"peer", h.request.Peer)
+		"peer", peer)
 	rw, ok := infra.ResponseWriterFromContext(h.request.Context())
 	if !ok {
 		logger.Warn("[TrustStore:chainReqHandler] Unable to service request, no Messenger found")
@@ -162,7 +181,6 @@ func (h *chainReqHandler) Handle() *infra.HandlerResult {
 	subCtx = metrics.CtxWith(subCtx, metrics.ChainReq)
 
 	var chain *cert.Chain
-	var err error
 	// Only allow network traffic to be sent out if recursion is enabled and
 	// CacheOnly is not requested.
 	// FIXME(scrye): when protocol support is in, the below needs to select
@@ -180,8 +198,9 @@ func (h *chainReqHandler) Handle() *infra.HandlerResult {
 			TrustStoreOpts:   infra.TrustStoreOpts{LocalOnly: !h.recurse},
 			AllowInactiveTRC: true,
 		}
+
 		chain, err = h.store.getChain(subCtx, chainReq.IA(), scrypto.Version(chainReq.Version),
-			opts, h.request.Peer)
+			opts, peer)
 		if err != nil {
 			logger.Error("[TrustStore:chainReqHandler] Unable to retrieve Chain", "err", err)
 			metrics.Handler.Request(l).Inc()
@@ -219,11 +238,20 @@ type trcPushHandler struct {
 
 func (h *trcPushHandler) Handle() *infra.HandlerResult {
 	logger := log.FromCtx(h.request.Context())
+
 	l := metrics.HandlerLabels{
-		Client:  addrLocation(h.request.Peer, h.store.ia),
 		ReqType: metrics.TRCPush,
 		Result:  metrics.ErrInternal,
 	}
+
+	peer, err := redirectToUDP(h.request.Peer)
+	if err != nil {
+		logger.Error("[TrustStore:trcReqHandler] wrong address type", "err", err)
+		metrics.Handler.Request(l).Inc()
+		return infra.MetricsErrInternal
+	}
+	l.Client = addrLocation(peer, h.store.ia)
+
 	// FIXME(scrye): In case a TRC update will invalidate the local certificate
 	// chain after the gracePeriod, CSes must use this gracePeriod to fetch a
 	// new chain from the issuer. If a chain is not obtained within the
@@ -289,11 +317,20 @@ type chainPushHandler struct {
 
 func (h *chainPushHandler) Handle() *infra.HandlerResult {
 	logger := log.FromCtx(h.request.Context())
+
 	l := metrics.HandlerLabels{
-		Client:  addrLocation(h.request.Peer, h.store.ia),
 		ReqType: metrics.ChainPush,
 		Result:  metrics.ErrInternal,
 	}
+
+	peer, err := redirectToUDP(h.request.Peer)
+	if err != nil {
+		logger.Error("[TrustStore:trcReqHandler] wrong address type", "err", err)
+		metrics.Handler.Request(l).Inc()
+		return infra.MetricsErrInternal
+	}
+	l.Client = addrLocation(peer, h.store.ia)
+
 	chainPush, ok := h.request.Message.(*cert_mgmt.Chain)
 	if !ok {
 		logger.Error("[TrustStore:chainPushHandler] Wrong message type, expected cert_mgmt.Chain",
@@ -350,19 +387,37 @@ func (h *chainPushHandler) Handle() *infra.HandlerResult {
 
 // addrLocation extracts the client from an infra request. If the request is nil,
 // "app" is returned.
-func addrLocation(client net.Addr, localIA addr.IA) string {
-	if client == nil {
+func addrLocation(address net.Addr, localIA addr.IA) string {
+	switch p := address.(type) {
+	case *snet.Addr:
+		return addrLocation(p.ToXAddr(), localIA)
+	case *snet.UDPAddr:
+		if p == nil {
+			return metrics.App
+		}
+
+		if localIA.Equal(p.IA) {
+			return infra.PromSrcASLocal
+		}
+		if localIA.I == p.IA.I {
+			return infra.PromSrcISDLocal
+		}
+		return infra.PromSrcISDRemote
+
+	default:
 		return metrics.App
 	}
-	sAddr, ok := client.(*snet.Addr)
-	if !ok {
-		return infra.PromSrcUnknown
-	}
-	if localIA.Equal(sAddr.IA) {
-		return infra.PromSrcASLocal
-	}
-	if localIA.I == sAddr.IA.I {
-		return infra.PromSrcISDLocal
+}
+
+func redirectToUDP(a net.Addr) (*snet.UDPAddr, error) {
+	switch p := a.(type) {
+	case nil:
+		return nil, nil
+	case *snet.Addr:
+		return redirectToUDP(p.ToXAddr())
+	case *snet.UDPAddr:
+		return p, nil
+	default:
+		return nil, serrors.New("invalid address type", "address", fmt.Sprintf("%v(%T)", p, p))
 	}
-	return infra.PromSrcISDRemote
 }
diff --git a/go/lib/infra/modules/trust/trust.go b/go/lib/infra/modules/trust/trust.go
index d1eb51a16..ca4ddd6f7 100644
--- a/go/lib/infra/modules/trust/trust.go
+++ b/go/lib/infra/modules/trust/trust.go
@@ -198,7 +198,7 @@ func (store *Store) GetTRC(ctx context.Context, isd addr.ISD, version scrypto.Ve
 // client contains the node that caused the function to be called, or nil if the
 // function was called due to a local feature.
 func (store *Store) getTRC(ctx context.Context, isd addr.ISD, version scrypto.Version,
-	opts infra.TRCOpts, client net.Addr) (*trc.TRC, error) {
+	opts infra.TRCOpts, client *snet.UDPAddr) (*trc.TRC, error) {
 
 	l := metrics.LookupLabels{
 		Client:    addrLocation(client, store.ia),
@@ -256,7 +256,7 @@ func (store *Store) getTRC(ctx context.Context, isd addr.ISD, version scrypto.Ve
 	return trcObj, err
 }
 
-func (store *Store) getTRClabels(ctx context.Context, client, server net.Addr,
+func (store *Store) getTRClabels(ctx context.Context, client *snet.UDPAddr, server net.Addr,
 	err error) metrics.SentLabels {
 
 	l := metrics.SentLabels{
@@ -356,7 +356,7 @@ func (store *Store) GetChain(ctx context.Context, ia addr.IA, version scrypto.Ve
 }
 
 func (store *Store) getChain(ctx context.Context, ia addr.IA, version scrypto.Version,
-	opts infra.ChainOpts, client net.Addr) (*cert.Chain, error) {
+	opts infra.ChainOpts, client *snet.UDPAddr) (*cert.Chain, error) {
 
 	l := metrics.LookupLabels{
 		Client:    addrLocation(client, store.ia),
@@ -500,7 +500,7 @@ func verifyChain(validator *trc.TRC, chain *cert.Chain) error {
 	return nil
 }
 
-func (store *Store) getChainLabels(ctx context.Context, client, server net.Addr,
+func (store *Store) getChainLabels(ctx context.Context, client *snet.UDPAddr, server net.Addr,
 	err error) metrics.SentLabels {
 
 	l := metrics.SentLabels{
@@ -726,19 +726,18 @@ func (store *Store) NewChainPushHandler() infra.Handler {
 
 // isLocal returns an error if address is not part of the local AS (or if the
 // check cannot be made).
-func (store *Store) isLocal(address net.Addr) error {
+func (store *Store) isLocal(address *snet.UDPAddr) error {
 	// We need to send out a network request, but only do so if we're
 	// servicing a request coming from our own AS.
-	if address != nil {
-		switch saddr, ok := address.(*snet.Addr); {
-		case !ok:
-			return common.NewBasicError("Unable to determine AS of address",
-				nil, "addr", address)
-		case !store.ia.Equal(saddr.IA):
-			return common.NewBasicError("Object not found in DB, and recursion not "+
-				"allowed for clients outside AS", nil, "addr", address)
-		}
+	if address == nil {
+		return nil
 	}
+
+	if !store.ia.Equal(address.IA) {
+		return common.NewBasicError("Object not found in DB, and recursion not "+
+			"allowed for clients outside AS", nil, "addr", address)
+	}
+
 	return nil
 }
 
diff --git a/go/lib/infra/modules/trust/v2/recurser.go b/go/lib/infra/modules/trust/v2/recurser.go
index 42919dfb7..ad424c04e 100644
--- a/go/lib/infra/modules/trust/v2/recurser.go
+++ b/go/lib/infra/modules/trust/v2/recurser.go
@@ -44,15 +44,18 @@ type ASLocalRecurser struct {
 // AllowRecursion returns an error if address is not part of the local AS (or if
 // the check cannot be made).
 func (r *ASLocalRecurser) AllowRecursion(peer net.Addr) error {
-	if peer != nil {
-		switch saddr, ok := peer.(*snet.Addr); {
-		case !ok:
-			return serrors.WrapStr("unable to determine AS of peer", ErrRecursionNotAllowed,
-				"addr", peer)
-		case !r.IA.Equal(saddr.IA):
-			return serrors.WrapStr("client outside local AS", ErrRecursionNotAllowed,
-				"addr", peer)
-		}
+	if peer == nil {
+		return nil
+	}
+
+	a, ok := peer.(*snet.UDPAddr)
+	if !ok {
+		return serrors.WrapStr("unable to determine AS of peer", ErrRecursionNotAllowed,
+			"addr", peer)
+	}
+	if !r.IA.Equal(a.IA) {
+		return serrors.WrapStr("client outside local AS", ErrRecursionNotAllowed,
+			"addr", peer)
 	}
 	return nil
 }
diff --git a/go/lib/infra/modules/trust/v2/router.go b/go/lib/infra/modules/trust/v2/router.go
index c0c874b8f..ac67d7341 100644
--- a/go/lib/infra/modules/trust/v2/router.go
+++ b/go/lib/infra/modules/trust/v2/router.go
@@ -44,7 +44,7 @@ func (r *localRouter) ChooseServer(_ context.Context, _ addr.ISD) (net.Addr, err
 }
 
 func (r *localRouter) chooseServer() net.Addr {
-	return &snet.Addr{IA: r.ia, Host: addr.NewSVCUDPAppAddr(addr.SvcCS)}
+	return snet.NewSVCAddr(r.ia, nil, nil, addr.SvcCS)
 }
 
 type csRouter struct {
@@ -66,13 +66,8 @@ func (r *csRouter) ChooseServer(ctx context.Context, subjectISD addr.ISD) (net.A
 	if err != nil {
 		return nil, serrors.WrapStr("unable to find path to any core AS", err, "isd", dstISD)
 	}
-	a := &snet.Addr{
-		IA:      path.Destination(),
-		Host:    addr.NewSVCUDPAppAddr(addr.SvcCS),
-		Path:    path.Path(),
-		NextHop: path.OverlayNextHop(),
-	}
-	return a, nil
+	ret := snet.NewSVCAddr(path.Destination(), path.Path(), path.OverlayNextHop(), addr.SvcCS)
+	return ret, nil
 }
 
 // dstISD selects the CS to ask for crypto material, using the following strategy:
diff --git a/go/lib/snet/addr.go b/go/lib/snet/addr.go
index 7868a9f18..65887d23f 100644
--- a/go/lib/snet/addr.go
+++ b/go/lib/snet/addr.go
@@ -66,7 +66,7 @@ func (a *Addr) ToXAddr() net.Addr {
 		u := &net.UDPAddr{IP: a.Host.L3.IP(), Port: int(a.Host.L4)}
 		return NewUDPAddr(a.IA, a.Path, a.NextHop, u)
 	}
-	return &net.TCPAddr{} //TODO(karampok). discuss: return nil
+	return nil
 }
 
 func (a *Addr) Network() string {


Test patch:
diff --git a/go/lib/infra/modules/trust/v2/recurser_test.go b/go/lib/infra/modules/trust/v2/recurser_test.go
index 60be28549..14f50ef39 100644
--- a/go/lib/infra/modules/trust/v2/recurser_test.go
+++ b/go/lib/infra/modules/trust/v2/recurser_test.go
@@ -33,11 +33,11 @@ func TestASLocalRecurserAllowRecursion(t *testing.T) {
 			Assertion: assert.NoError,
 		},
 		"AS local": {
-			Addr:      &snet.Addr{IA: ia110},
+			Addr:      snet.NewUDPAddr(ia110, nil, nil, nil),
 			Assertion: assert.NoError,
 		},
 		"remote AS": {
-			Addr:      &snet.Addr{IA: ia120},
+			Addr:      snet.NewUDPAddr(ia120, nil, nil, nil),
 			Assertion: assert.Error,
 		},
 		"invalid Address": {
@@ -63,11 +63,11 @@ func TestLocalOnlyRecurserAllowRecursion(t *testing.T) {
 			Assertion: assert.NoError,
 		},
 		"AS local": {
-			Addr:      &snet.Addr{IA: ia110},
+			Addr:      snet.NewUDPAddr(ia110, nil, nil, nil),
 			Assertion: assert.Error,
 		},
 		"remote AS": {
-			Addr:      &snet.Addr{IA: ia120},
+			Addr:      snet.NewUDPAddr(ia120, nil, nil, nil),
 			Assertion: assert.Error,
 		},
 		"invalid Address": {
diff --git a/go/lib/infra/modules/trust/v2/router_test.go b/go/lib/infra/modules/trust/v2/router_test.go
index f97fe91ad..7160ca193 100644
--- a/go/lib/infra/modules/trust/v2/router_test.go
+++ b/go/lib/infra/modules/trust/v2/router_test.go
@@ -42,7 +42,7 @@ func TestLocalRouterChooseServer(t *testing.T) {
 	}
 	for name, isd := range tests {
 		t.Run(name, func(t *testing.T) {
-			localCS := &snet.Addr{IA: ia122, Host: addr.NewSVCUDPAppAddr(addr.SvcCS)}
+			localCS := snet.NewSVCAddr(ia122, nil, nil, addr.SvcCS)
 			router := trust.NewLocalRouter(localCS.IA)
 			routed, err := router.ChooseServer(context.Background(), isd)
 			require.NoError(t, err)
@@ -136,12 +136,8 @@ func TestCSRouterChooseServer(t *testing.T) {
 				xtest.AssertErrorsIs(t, err, test.ExpectedErr)
 			} else {
 				require.NoError(t, err)
-				expected := &snet.Addr{
-					IA:      p.Destination(),
-					Host:    addr.NewSVCUDPAppAddr(addr.SvcCS),
-					Path:    p.Path(),
-					NextHop: p.OverlayNextHop(),
-				}
+				expected := snet.NewSVCAddr(p.Destination(), p.Path(),
+					p.OverlayNextHop(), addr.SvcCS)
 				assert.Equal(t, expected, res)
 			}
 		})