From 9bdd29ccfe7994e50a689d4d0fe9f7b8c43de3ca Mon Sep 17 00:00:00 2001 From: jiceatscion <139873336+jiceatscion@users.noreply.github.com> Date: Tue, 29 Aug 2023 09:45:45 +0200 Subject: [PATCH] hidden_paths: enable registration of hiddenpaths both locally and remotely. Fixes #4364 There were a number of issues: * Because the normal case is that the registry is remote, the registerer makes no attempt at using the intraAS network to reach the registry (and the registry isn't listening on TCP). However, quic/scion path resolution to an AS-local host was broken: the resulting paths are expected to be complete paths with Metadata, but intraAS paths were meta-data-less paths, resulting in nill-ptr dereference. * By default the control service port (shared with the hidden_segment registry) was a dynamic port. The discovery service cannot only knows of statically configured port (configured in the topo file). So the only way to make the hidden_segments service discoverable is to give the control service a static port. While possible, this would result in a confusing and conter-intuitive configuration: * the control_service quic address would be configured by the cs yaml file and be different from what appears in the topo file for the CS but identical to the hidden_segment service address. * the CS address in the topo file was acually the TCP address and could never be mirrored by the quic address (because that quic address was occupied by the service resolver). * the address of the hidden_segments registry in the topo file would be different from that of the CS, eventhough they share the same port. * This was all invisible to the integration test which was skillfully predicting the dynamic port and configuring the discovery service for it. To correct these infelicities: * Give the Svc resolver a dynamic port * By default, give to the CS and the hidden_segment registry the quic address that has the same IP and port as the control service's TCP address. * Produce complete Path objects to represent intraAS paths. In passing: * A handful of AssertEqual() had arguments in the wrong order. * In one case we were constructing a full SVC address with a nil path instead of an empty path. --- acceptance/common/scion.py | 37 +++++++++++++++++- acceptance/hidden_paths/test.py | 19 +++------- control/cmd/control/main.go | 9 ++++- control/hiddenpaths.go | 3 +- control/tasks.go | 5 +++ pkg/experimental/hiddenpath/BUILD.bazel | 1 + pkg/experimental/hiddenpath/discovery.go | 16 +++++--- pkg/snet/path.go | 4 -- pkg/snet/router.go | 13 ------- private/app/appnet/BUILD.bazel | 2 + private/app/appnet/addr.go | 1 + private/app/appnet/addr_test.go | 29 +++++++------- private/app/appnet/infraenv.go | 17 +++++++-- private/app/appnet/intra_as.go | 48 ++++++++++++++++++++++++ 14 files changed, 148 insertions(+), 56 deletions(-) create mode 100644 private/app/appnet/intra_as.go diff --git a/acceptance/common/scion.py b/acceptance/common/scion.py index 9ad00ae034..c330fb20c3 100644 --- a/acceptance/common/scion.py +++ b/acceptance/common/scion.py @@ -14,7 +14,7 @@ import logging import json -from typing import Any, Dict, List, MutableMapping +from typing import Any, Dict, List, MutableMapping, Mapping import toml import yaml @@ -69,6 +69,29 @@ def update_json(change_dict: Dict[str, Any], files: LocalPath): json.dump(t, f, indent=2) +def load_from_json(key: str, files: LocalPath) -> Any: + """ Reads the value associated with the given key from the given json files. + + The first value found is returned. If not found, None is returned. + + Args: + key: dot separated path of the JSON key. + files: names of file or files to read. + + Returns: + The value. None if the path doesn't exist in the dictionary tree. + + Raises: + IOError / FileNotFoundError: File path is not valid + """ + for file in files: + with open(file, "r") as f: + t = json.load(f) + v = val_at_path(t, key) + if v is not None: + return v + + class ASList: """ ASList is a list of AS separated by core and non-core ASes. It can be loaded @@ -117,6 +140,18 @@ def path_to_dict(path: str, val: Any) -> Dict: return d +def val_at_path(d: Mapping[str, Any], path: str) -> Any: + """ + Walks nested dictionaries by following the given path and returns the value + associated with the leaf key. + """ + v = d + for k in path.split('.'): + v = v.get(k, None) + if not isinstance(v, Mapping): + return v + + def merge_dict(change_dict: Dict[str, Any], orig_dict: MutableMapping[str, Any]): """ Merge changes into the original dictionary. Leaf values in the change dict diff --git a/acceptance/hidden_paths/test.py b/acceptance/hidden_paths/test.py index c3144c9e41..3a9f1af7f9 100755 --- a/acceptance/hidden_paths/test.py +++ b/acceptance/hidden_paths/test.py @@ -61,16 +61,6 @@ def setup_prepare(self): "4": "172.20.0.65", "5": "172.20.0.73", } - # XXX(lukedirtwalker): The ports below are the dynamic QUIC server - # ports. Thanks to the docker setup they are setup consistently so we - # can use them. Optimally we would define a static server port inside - # the CS and use that one instead. - control_addresses = { - "2": "172.20.0.51:32768", - "3": "172.20.0.59:32768", - "4": "172.20.0.67:32768", - "5": "172.20.0.75:32768", - } # Each AS participating in hidden paths has their own hidden paths configuration file. hp_configs = { "2": "hp_groups_as2_as5.yml", @@ -98,13 +88,16 @@ def setup_prepare(self): # even though some don't need the registration service. as_dir_path = self.artifacts / "gen" / ("ASff00_0_%s" % as_number) + # The hidden_segment services are behind the same server as the control_service. + topology_file = as_dir_path / "topology.json" + control_service_addr = scion.load_from_json( + 'control_service.%s.addr' % control_id, [topology_file]) topology_update = { "hidden_segment_lookup_service.%s.addr" % control_id: - control_addresses[as_number], + control_service_addr, "hidden_segment_registration_service.%s.addr" % control_id: - control_addresses[as_number], + control_service_addr, } - topology_file = as_dir_path / "topology.json" scion.update_json(topology_update, [topology_file]) def setup_start(self): diff --git a/control/cmd/control/main.go b/control/cmd/control/main.go index d481cd607c..d5de9e08fb 100644 --- a/control/cmd/control/main.go +++ b/control/cmd/control/main.go @@ -200,11 +200,17 @@ func realMain(ctx context.Context) error { return err } + // FIXME: readability would be improved if we could be consistent with address + // representations in NetworkConfig (string or cooked, chose one). nc := infraenv.NetworkConfig{ - IA: topo.IA(), + IA: topo.IA(), + // Public: (Historical name) The TCP/IP:port address for the control service. Public: topo.ControlServiceAddress(globalCfg.General.ID), ReconnectToDispatcher: globalCfg.General.ReconnectToDispatcher, QUIC: infraenv.QUIC{ + // Address: the QUIC/SCION address of this service. If not + // configured, QUICStack() uses the same IP and port as + // for the public address. Address: globalCfg.QUIC.Address, TLSVerifier: trust.NewTLSCryptoVerifier(trustDB), GetCertificate: cs.NewTLSCertificateLoader( @@ -221,6 +227,7 @@ func realMain(ctx context.Context) error { }, SCIONNetworkMetrics: metrics.SCIONNetworkMetrics, SCIONPacketConnMetrics: metrics.SCIONPacketConnMetrics, + MTU: topo.MTU(), } quicStack, err := nc.QUICStack() if err != nil { diff --git a/control/hiddenpaths.go b/control/hiddenpaths.go index f0036c1f10..299d265871 100644 --- a/control/hiddenpaths.go +++ b/control/hiddenpaths.go @@ -84,7 +84,8 @@ func (c HiddenPathConfigurator) Setup(location string) (*HiddenPathRegistrationC &hpgrpc.AuthoritativeSegmentServer{ Lookup: c.localAuthServer(groups), Verifier: c.Verifier, - }) + }, + ) hspb.RegisterHiddenSegmentRegistrationServiceServer(c.InterASQUICServer, &hpgrpc.RegistrationServer{ Registry: hiddenpath.RegistryServer{ diff --git a/control/tasks.go b/control/tasks.go index 4b8a8ab755..fedae5e905 100644 --- a/control/tasks.go +++ b/control/tasks.go @@ -196,6 +196,11 @@ func (t *TasksConfig) segmentWriter(segType seg.Type, Writer: writer, Tick: beaconing.NewTick(t.RegistrationInterval), } + // The period of the task is short because we want to retry quickly + // if we fail fast. So during one interval we'll make as many attempts + // as we can until we succeed. After succeeding, the task does nothing + // until the end of the interval. The interval itself is used as a + // timeout. If we fail slow we give up at the end of the cycle. return periodic.Start(r, 500*time.Millisecond, t.RegistrationInterval) } diff --git a/pkg/experimental/hiddenpath/BUILD.bazel b/pkg/experimental/hiddenpath/BUILD.bazel index ee33cdb962..3962a4665e 100644 --- a/pkg/experimental/hiddenpath/BUILD.bazel +++ b/pkg/experimental/hiddenpath/BUILD.bazel @@ -25,6 +25,7 @@ go_library( "//pkg/private/serrors:go_default_library", "//pkg/segment:go_default_library", "//pkg/snet:go_default_library", + "//pkg/snet/path:go_default_library", "//private/config:go_default_library", "//private/pathdb:go_default_library", "//private/pathdb/query:go_default_library", diff --git a/pkg/experimental/hiddenpath/discovery.go b/pkg/experimental/hiddenpath/discovery.go index 8418debb9c..f336a152c0 100644 --- a/pkg/experimental/hiddenpath/discovery.go +++ b/pkg/experimental/hiddenpath/discovery.go @@ -21,6 +21,7 @@ import ( "github.com/scionproto/scion/pkg/addr" "github.com/scionproto/scion/pkg/private/serrors" "github.com/scionproto/scion/pkg/snet" + "github.com/scionproto/scion/pkg/snet/path" ) // Servers is a list of discovered remote hidden segment server. @@ -80,19 +81,22 @@ func (r LookupResolver) Resolve(ctx context.Context, ia addr.IA) (net.Addr, erro func resolve(ctx context.Context, ia addr.IA, discoverer Discoverer, router snet.Router, extractAddr func(Servers) (*net.UDPAddr, error)) (net.Addr, error) { - path, err := router.Route(ctx, ia) + p, err := router.Route(ctx, ia) if err != nil { return nil, serrors.WrapStr("looking up path", err) } - if path == nil { + if p == nil { return nil, serrors.WrapStr("no path found to remote", err) } dsAddr := &snet.SVCAddr{ IA: ia, - NextHop: path.UnderlayNextHop(), - Path: path.Dataplane(), + NextHop: p.UnderlayNextHop(), + Path: p.Dataplane(), SVC: addr.SvcDS, } + if dsAddr.Path == nil { + dsAddr.Path = path.Empty{} + } hps, err := discoverer.Discover(ctx, dsAddr) if err != nil { return nil, serrors.WrapStr("discovering hidden path server", err) @@ -104,7 +108,7 @@ func resolve(ctx context.Context, ia addr.IA, discoverer Discoverer, router snet return &snet.UDPAddr{ IA: ia, Host: a, - NextHop: path.UnderlayNextHop(), - Path: path.Dataplane(), + NextHop: dsAddr.NextHop, + Path: dsAddr.Path, }, nil } diff --git a/pkg/snet/path.go b/pkg/snet/path.go index 8fa12fc036..1a19ad5760 100644 --- a/pkg/snet/path.go +++ b/pkg/snet/path.go @@ -249,10 +249,6 @@ func (p *partialPath) Dataplane() DataplanePath { return p.dataplane } -func (p *partialPath) Interfaces() []PathInterface { - return nil -} - func (p *partialPath) Source() addr.IA { return p.source } diff --git a/pkg/snet/router.go b/pkg/snet/router.go index f763a529ba..a11f78c1a7 100644 --- a/pkg/snet/router.go +++ b/pkg/snet/router.go @@ -55,16 +55,3 @@ func (r *BaseRouter) Route(ctx context.Context, dst addr.IA) (Path, error) { func (r *BaseRouter) AllRoutes(ctx context.Context, dst addr.IA) ([]Path, error) { return r.Querier.Query(ctx, dst) } - -// IntraASPathQuerier implements the PathQuerier interface. It will only provide -// AS internal paths, i.e., empty paths with only the IA as destination. This -// should only be used in places where you know that you only need to -// communicate inside the AS. -type IntraASPathQuerier struct { - IA addr.IA -} - -// Query implements PathQuerier. -func (q IntraASPathQuerier) Query(_ context.Context, _ addr.IA) ([]Path, error) { - return []Path{&partialPath{source: q.IA, destination: q.IA}}, nil -} diff --git a/private/app/appnet/BUILD.bazel b/private/app/appnet/BUILD.bazel index a65d75e38d..68f7650183 100644 --- a/private/app/appnet/BUILD.bazel +++ b/private/app/appnet/BUILD.bazel @@ -5,6 +5,7 @@ go_library( srcs = [ "addr.go", "infraenv.go", + "intra_as.go", ], importpath = "github.com/scionproto/scion/private/app/appnet", visibility = ["//visibility:public"], @@ -13,6 +14,7 @@ go_library( "//pkg/daemon:go_default_library", "//pkg/log:go_default_library", "//pkg/private/serrors:go_default_library", + "//pkg/slayers/path:go_default_library", "//pkg/snet:go_default_library", "//pkg/snet/path:go_default_library", "//pkg/snet/squic:go_default_library", diff --git a/private/app/appnet/addr.go b/private/app/appnet/addr.go index 4bbd733037..2d5514c40b 100644 --- a/private/app/appnet/addr.go +++ b/private/app/appnet/addr.go @@ -161,6 +161,7 @@ func (r AddressRewriter) buildFullAddress(ctx context.Context, return nil, serrors.WrapStr("Unable to resolve underlay", err) } ret.NextHop = ov + ret.Path = path.Empty{} } return ret, nil diff --git a/private/app/appnet/addr_test.go b/private/app/appnet/addr_test.go index 27bba692cc..dd6a009d3d 100644 --- a/private/app/appnet/addr_test.go +++ b/private/app/appnet/addr_test.go @@ -80,8 +80,8 @@ func TestRedirectQUIC(t *testing.T) { a, r, err := aw.RedirectToQUIC(context.Background(), tc.input) tc.assertErr(t, err) - assert.Equal(t, a, tc.wantAddr) - assert.Equal(t, r, tc.wantRedirect) + assert.Equal(t, tc.wantAddr, a) + assert.Equal(t, tc.wantRedirect, r) }) } @@ -100,7 +100,6 @@ func TestRedirectQUIC(t *testing.T) { path.EXPECT().Metadata().Return(&snet.PathMetadata{ Interfaces: make([]snet.PathInterface, 1), // just non-empty }) - aw := infraenv.AddressRewriter{ Router: router, Resolver: resolver, @@ -137,7 +136,6 @@ func TestRedirectQUIC(t *testing.T) { path.EXPECT().Metadata().Return(&snet.PathMetadata{ Interfaces: make([]snet.PathInterface, 1), // just non-empty }) - aw := infraenv.AddressRewriter{ Router: router, Resolver: resolver, @@ -182,7 +180,7 @@ func TestRedirectQUIC(t *testing.T) { want := &snet.SVCAddr{ SVC: addr.SvcCS, NextHop: &net.UDPAddr{IP: net.ParseIP("10.1.1.1")}, - Path: snetpath.SCION{}, + Path: snetpath.Empty{}, } a, r, err := aw.RedirectToQUIC(context.Background(), input) assert.NoError(t, err) @@ -225,7 +223,7 @@ func TestBuildFullAddress(t *testing.T) { SVC: addr.SvcCS, } a, err := aw.BuildFullAddress(context.Background(), input) - assert.Equal(t, a, input) + assert.Equal(t, input, a) assert.NoError(t, err) }) @@ -255,7 +253,7 @@ func TestBuildFullAddress(t *testing.T) { NextHop: &net.UDPAddr{}, SVC: addr.SvcCS, } - assert.Equal(t, a, want) + assert.Equal(t, want, a) assert.NoError(t, err) }) @@ -285,8 +283,13 @@ func TestBuildFullAddress(t *testing.T) { input := &snet.SVCAddr{IA: localIA, SVC: addr.SvcCS, Path: snetpath.Empty{}} a, err := aw.BuildFullAddress(context.Background(), input) - want := &snet.SVCAddr{IA: localIA, NextHop: underlayAddr, SVC: addr.SvcCS} - assert.Equal(t, a, want) + want := &snet.SVCAddr{ + IA: localIA, + NextHop: underlayAddr, + SVC: addr.SvcCS, + Path: snetpath.Empty{}, + } + assert.Equal(t, want, a) assert.NoError(t, err) }) @@ -388,9 +391,9 @@ func TestResolve(t *testing.T) { } initResolver(resolver, tc.ResolverSetup) p, a, redirect, err := aw.ResolveSVC(context.Background(), path, tc.input) - assert.Equal(t, p, tc.wantPath) - assert.Equal(t, a.String(), tc.want.String()) - assert.Equal(t, redirect, tc.wantQUICRedirect) + assert.Equal(t, tc.wantPath, p) + assert.Equal(t, tc.want.String(), a.String()) + assert.Equal(t, tc.wantQUICRedirect, redirect) tc.assertErr(t, err) }) } @@ -448,7 +451,7 @@ func TestParseReply(t *testing.T) { if err != nil { return } - assert.Equal(t, a.String(), tc.want.String()) + assert.Equal(t, tc.want.String(), a.String()) }) } } diff --git a/private/app/appnet/infraenv.go b/private/app/appnet/infraenv.go index 595cd1cc01..c9fdd7646c 100644 --- a/private/app/appnet/infraenv.go +++ b/private/app/appnet/infraenv.go @@ -83,6 +83,8 @@ type NetworkConfig struct { SCIONNetworkMetrics snet.SCIONNetworkMetrics // Metrics injected into DefaultPacketDispatcherService. SCIONPacketConnMetrics snet.SCIONPacketConnMetrics + // MTU of the local AS + MTU uint16 } // QUICStack contains everything to run a QUIC based RPC stack. @@ -103,8 +105,9 @@ func (nc *NetworkConfig) TCPStack() (net.Listener, error) { func (nc *NetworkConfig) QUICStack() (*QUICStack, error) { if nc.QUIC.Address == "" { - nc.QUIC.Address = net.JoinHostPort(nc.Public.IP.String(), "0") + nc.QUIC.Address = nc.Public.String() } + client, server, err := nc.initQUICSockets() if err != nil { return nil, err @@ -221,7 +224,7 @@ func (nc *NetworkConfig) AddressRewriter( } } return &AddressRewriter{ - Router: &snet.BaseRouter{Querier: snet.IntraASPathQuerier{IA: nc.IA}}, + Router: &snet.BaseRouter{Querier: IntraASPathQuerier{IA: nc.IA, MTU: nc.MTU}}, SVCRouter: nc.SVCResolver, Resolver: &svc.Resolver{ LocalIA: nc.IA, @@ -266,9 +269,15 @@ func (nc *NetworkConfig) initSvcRedirect(quicAddress string) (func(), error) { Dispatcher: packetDispatcher, Metrics: nc.SCIONNetworkMetrics, } - conn, err := network.Listen(context.Background(), "udp", nc.Public, addr.SvcWildcard) + + // The service resolution address gets a dynamic port. In reality, neither the + // address nor the port are needed to address the resolver, but the dispatcher still + // requires them and checks unicity. At least a dynamic port is allowed. + srAddr := &net.UDPAddr{IP: nc.Public.IP, Port: 0} + conn, err := network.Listen(context.Background(), "udp", srAddr, addr.SvcWildcard) if err != nil { - return nil, serrors.WrapStr("listening on SCION", err, "addr", nc.Public) + log.Info("Listen failed", "err", err) + return nil, serrors.WrapStr("listening on SCION", err, "addr", srAddr) } ctx, cancel := context.WithCancel(context.Background()) diff --git a/private/app/appnet/intra_as.go b/private/app/appnet/intra_as.go new file mode 100644 index 0000000000..6735dce534 --- /dev/null +++ b/private/app/appnet/intra_as.go @@ -0,0 +1,48 @@ +// Copyright 2020 ETH Zurich +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package appnet + +import ( + "context" + "time" + + "github.com/scionproto/scion/pkg/addr" + rawpath "github.com/scionproto/scion/pkg/slayers/path" + "github.com/scionproto/scion/pkg/snet" + "github.com/scionproto/scion/pkg/snet/path" +) + +// IntraASPathQuerier implements the PathQuerier interface. It will only provide +// AS-internal paths, i.e., zero-hops paths with only the IA as destination. This +// should only be used in places where you know that you only need to +// communicate inside the AS. The type of Path returned is a complete +// implementation with proper metadata. +type IntraASPathQuerier struct { + IA addr.IA + MTU uint16 +} + +// Query implements PathQuerier. +func (q IntraASPathQuerier) Query(_ context.Context, _ addr.IA) ([]snet.Path, error) { + return []snet.Path{path.Path{ + Src: q.IA, + Dst: q.IA, + DataplanePath: path.Empty{}, + Meta: snet.PathMetadata{ + MTU: q.MTU, + Expiry: time.Now().Add(rawpath.MaxTTL * time.Second), + }, + }}, nil +}