FABRID base version #168

marcodermatt · 2024-08-14T12:16:48Z

Control plane features:

Definition of FABRID policies and connection points on which they are available
CS loads policies and appends maps for policy indices and connection points as detachable extensions to PCBs
Remote CSes cache FABRID maps
SD fetches detached extensions on request

Data plane features:

Fabrid and Identifier HBH extensions for sending FABRID traffic
WithFabrid option for choosing paths based on the fabrid query
Fabrid dataplane path which sets HBH extensions
BR fetches DRKey secret values and fabrid config on startup
BR validates and updates FABRID HVFs

Tools:

topology: --fabrid flag enables DRKey and fabrid for local topology
ping: select fabrid policies with --fabridquery flag
end2end: run integration test with --fabrid to test path validation

Demo:

./scion.sh topology --fabrid (docker: ./scion.sh topology -d --endhosts --fabrid)
./scion.sh run
./bin/end2end_integration --fabrid (docker: ./bin/end2end_integration -d --fabrid)
./scion.sh stop

Upstream changes:

snet: add extension header support to DataplanePath and PacketInfo scionproto/scion#4628

This change is

juagargi

Thank you all for this amazing contribution.
My review regards only maintainability, for that purpose I try to have as little "diff conflicts" as possible with other changes coming from scionproto-upstream, or changes initiated in scionlab.\

Since scionproto doesn't have yet a clear boundary between library and applications, nor supports plugins, we have to support the changes via code patches/changesets. That's why I mainly have two types of comments in this PR:

Move this to a new file : Meaning create a new file or use a new file introduced in this PR to write all the functions, types, constants, etc, that could live in the package, but also get the changes out of this file because upstream also modifies or will modify it. If creating a new file just to hold changes pertaining to fabrid, you can use suffixes such as _scionlab, to denote that these files are present only in scionlab.
Can we change this upstream? : When a change can be directly applied upstream, let's try to do it upstream. This includes bug fixes, linting problems, comments, typos, etc.

All in all, these changes should allow easier maintenance of the code, but it is not bulletproof (at all).
I suggest all the maintainers and code owners of FABRID to write a strategy on how to modify upstream to support extensibility, so that FABRID could be implemented by only adding files to the (new and extensible) upstream code base.
An example of necessary changes would include, e.g. how to make ping support extended behavior, dataplane support multiple fields, etc.

daemon/drkey/client_engine.go line 43 at r2 (raw file):

// For all ASHost Keys and for the HostHost Key, it checks whether the keys are in the database.
// If this is the case, those keys are returned. If not, the keys are requested from CS.
func (e *ClientEngine) FabridKeys(ctx context.Context, meta drkey.FabridKeysMeta,

Move this function to a new file

daemon/internal/servers/grpc.go line 124 at r2 (raw file):

}

type tempHopInfo struct {

move this type to a new file

daemon/internal/servers/grpc.go line 134 at r2 (raw file):

// updateFabridInfo updates the FABRID info that is contained in the path Metadata for detached
// hops, by fetching the corresponding FABRID maps from the corresponding AS.
func updateFabridInfo(ctx context.Context, dialer libgrpc.Dialer, detachedHops []tempHopInfo) {

move this to a new file

daemon/internal/servers/grpc.go line 303 at r2 (raw file):

}

func fabridPolicyToPB(fp *fabrid.Policy) *sdpb.FabridPolicy {

move this to a new file

daemon/internal/servers/grpc.go line 313 at r2 (raw file):

}

func fabridInfoToPB(fi *snet.FabridInfo) *sdpb.FabridInfo {

move this to a new file

daemon/internal/servers/grpc.go line 501 at r2 (raw file):

}

func (s *DaemonServer) FabridKeys(ctx context.Context, req *pb_daemon.FabridKeysRequest,

move this to a new file

pkg/addr/isdas.go line 73 at r2 (raw file):

}

func parseAS(as string, sep string) (AS, error) {

Can we change this upstream?

pkg/daemon/grpc.go line 230 at r2 (raw file):

// Returns all the ASHost DRKeys for the ASes inside the meta.PathAS
func (c grpcConn) FabridKeys(ctx context.Context, meta drkey.FabridKeysMeta,

Move this function to a new file

pkg/daemon/grpc.go line 374 at r2 (raw file):

}

func fabridInfoFromPB(fi *sdpb.FabridInfo) snet.FabridInfo {

Move this function to a new file

pkg/drkey/drkey.go line 203 at r2 (raw file):

}

type FabridKeysMeta struct {

Move this to a new file

pkg/grpc/dialer.go line 56 at r2 (raw file):

// IP address. Without this it would not be possible to call IP sensitive endpoints like
// control service drkey.
type FixedLocalIPTCPDialer struct {

Move this to a new file

pkg/private/xtest/graph/graph.go line 421 at r2 (raw file):

// FabridPolicy returns an arbitrary set of policies between between two interfaces of an AS.
func (g *Graph) FabridPolicy(a, b uint16) []*fabrid.Policy {

Move this to a new file

pkg/private/xtest/graph/graph.go line 675 at r2 (raw file):

			}
			if ifid == outIF || g.isPeer[ifid] {
				carbonIntensity.Inter[common.IFIDType(ifid)] =

Can we change this upstream?

pkg/snet/path.go line 90 at r2 (raw file):

// HopInterface represents a single hop on the path
type HopInterface struct {

Move this to a new file

pkg/snet/path.go line 115 at r2 (raw file):

}

type FabridInfo struct {

Move this to a new file

pkg/snet/path.go line 190 at r2 (raw file):

}

func (pm *PathMetadata) Hops() []HopInterface {

Move this to a new file

pkg/snet/reply_pather.go line 61 at r2 (raw file):

	return nil
}
func (p RawReplyPath) SetExtensions(s *slayers.SCION, pi *PacketInfo) error {

Add blank line

pkg/snet/path/empty.go line 30 at r2 (raw file):

}

func (e Empty) SetExtensions(s *slayers.SCION, p *snet.PacketInfo) error {

Move this to a new file

pkg/snet/path/epic.go line 98 at r2 (raw file):

}

func (e *EPIC) SetExtensions(s *slayers.SCION, p *snet.PacketInfo) error {

Move this to a new file

pkg/snet/path/onehop.go line 46 at r2 (raw file):

}

func (p OneHop) SetExtensions(s *slayers.SCION, pi *snet.PacketInfo) error {

Move this to a new file

pkg/snet/path/path.go line 65 at r2 (raw file):

func (p Path) Metadata() *snet.PathMetadata {
	return p.Meta.Copy()

Can we change this upstream?

pkg/snet/path/scion.go line 47 at r2 (raw file):

	return nil
}

Move this to a new file

private/path/combinator/graph.go line 465 at r2 (raw file):

	return auth
}

why remove this line?

proto/control_plane/experimental/v1/seg_detached_extensions.proto line 32 at r2 (raw file):

message FABRIDDetachedExtension {

Move this to one of the new files

proto/daemon/v1/daemon.proto line 130 at r2 (raw file):

}

message FabridInfo {

Move this to a new file

router/connector.go line 166 at r2 (raw file):

}

func (c *Connector) AddDRKeySecret(protocolID int32, sv control.SecretValue) error {

Move this to a new file

router/dataplane.go line 493 at r2 (raw file):

}

func (d *DataPlane) UpdateFabridPolicies(ipRangePolicies map[uint32][]*control.PolicyIPRange,

Move this to a new file

router/dataplane.go line 1080 at r2 (raw file):

}

func (p *scionPacketProcessor) processFabrid(egressIF uint16) error {

Move this to a new file

router/dataplane.go line 1451 at r2 (raw file):

}

type transitType int

Move this to a new file

router/dataplane.go line 2373 at r2 (raw file):

	scmpH.SetNetworkLayerForChecksum(&scionL)

	needsAuth := false

Why remove this change?
Can we change this upstream?

router/dataplane_internal_test.go line 218 at r2 (raw file):

}

func TestFabridPolicies(t *testing.T) {

Move this to a new file

router/dataplane_test.go line 1075 at r2 (raw file):

					}).MinTimes(1)
				mInternal.EXPECT().WriteTo(gomock.Any(), gomock.Any()).Return(0, nil).
					AnyTimes()

Can we change this upstream?

tools/end2end/main.go line 254 at r2 (raw file):

}

func (s server) handlePingFabrid(conn snet.PacketConn) error {

Move this to a new file

tools/end2end/main.go line 648 at r2 (raw file):

}

func readFromFabrid(conn snet.PacketConn, pkt *snet.Packet, ov *net.UDPAddr) error {

Move this to a new file

tools/integration/binary.go line 107 at r2 (raw file):

}

func NewBinaryEndhostIntegration(name string, cmd string, clientArgs,

Move this to a new file

tools/integration/docker.go line 51 at r2 (raw file):

}

type dockerizedEndhostIntegration struct {

Move this to a new file

tools/integration/docker.go line 64 at r2 (raw file):

}

func dockerizeEndhost(bi *binaryIntegration) Integration {

Move this to a new file

tools/integration/docker.go line 73 at r2 (raw file):

}

// StartServer starts a server and blocks until the ReadySignal is received on Stdout.

The comment belonged to the existing function, duplicate?

tools/integration/docker.go line 74 at r2 (raw file):

// StartServer starts a server and blocks until the ReadySignal is received on Stdout.
func (di *dockerizedEndhostIntegration) StartServer(ctx context.Context, dst *snet.UDPAddr) (Waiter,

Move this to a new file

tools/integration/docker.go line 129 at r2 (raw file):

}

func EndhostID(a *snet.UDPAddr) string {

Move this to a new file

tools/integration/integration.go line 251 at r2 (raw file):

// SDAddr reads the endhost (dockerized) or scion daemon (normal) host Addr from the topology
// for the specified IA. If the address cannot be found, the CS address is returned.
var SDAddr HostAddr = func(ia addr.IA) *snet.UDPAddr {

Move this to a new file

tools/topology/common.py line 120 at r2 (raw file):

def sciond_ip(docker, topo_id, networks: Mapping[IPNetwork,
                                                 NetworkDescription]):

Can we change this upstream?

tools/topology/common.py line 128 at r2 (raw file):

def endhost_ip(docker, topo_id,

Move this to a new file

tools/topology/common.py line 130 at r2 (raw file):

def prom_addr_dispatcher(docker, topo_id,
                         networks: Mapping[IPNetwork,
                                           NetworkDescription], port, name):

Can we change this upstream?

juagargi

Additionally, please let me know how to run the integration tests, as they seem to fail for me:

make test-integration
...
...
...

================================================================================
(17:01:00) INFO: Elapsed time: 1344.806s, Critical Path: 927.66s
(17:01:00) INFO: 836 processes: 535 remote cache hit, 269 linux-sandbox, 45 local.
(17:01:00) INFO: Build completed, 12 tests FAILED, 836 total actions
//dist/test:deb_test                                                     PASSED in 13.8s
//dist/test:openwrt_test                                                 PASSED in 93.4s
//scion-pki/trcs:go_integration_test                                     PASSED in 7.5s
//tools/cryptoplayground:trc_ceremony_sensitive_test                     PASSED in 12.5s
//tools/cryptoplayground:trc_ceremony_test                               PASSED in 6.8s
//acceptance/hidden_paths:test                                          TIMEOUT in 901.1s
  bazel-testlogs/acceptance/hidden_paths/test/test.log
//acceptance/app_vs_endhost_br_dispatch:test                             FAILED in 10.0s
  bazel-testlogs/acceptance/app_vs_endhost_br_dispatch/test/test.log
//acceptance/cert_renewal:test                                           FAILED in 8.6s
  bazel-testlogs/acceptance/cert_renewal/test/test.log
//acceptance/router_benchmark:test                                       FAILED in 65.5s
  bazel-testlogs/acceptance/router_benchmark/test/test.log
//acceptance/router_multi:test_bfd                                       FAILED in 4.2s
  bazel-testlogs/acceptance/router_multi/test_bfd/test.log
//acceptance/router_multi:test_nobfd                                     FAILED in 22.0s
  bazel-testlogs/acceptance/router_multi/test_nobfd/test.log
//acceptance/sig_ping:test                                               FAILED in 9.5s
  bazel-testlogs/acceptance/sig_ping/test/test.log
//acceptance/topo_cs_reload:go_default_test                              FAILED in 5.7s
  bazel-testlogs/acceptance/topo_cs_reload/go_default_test/test.log
//acceptance/topo_daemon_reload:go_default_test                          FAILED in 5.6s
  bazel-testlogs/acceptance/topo_daemon_reload/go_default_test/test.log
//acceptance/trc_update:test                                             FAILED in 8.9s
  bazel-testlogs/acceptance/trc_update/test/test.log
//demo/drkey:test                                                        FAILED in 8.8s
  bazel-testlogs/demo/drkey/test/test.log
//demo/file_transfer:file_transfer                                       FAILED in 8.2s
  bazel-testlogs/demo/file_transfer/file_transfer/test.log

Executed 17 out of 17 tests: 5 tests pass and 12 fail locally.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.
make: *** [Makefile:56: test-integration] Error 3

juagargi

Follow up on integration tests:
Running the tests with make test-integration under scionlab won't work because of scionlab's switch from modernc's sqlite library to mattn's one (see #166).
Running manually end2end_integration works well, no problems found there.

rohrerj

daemon/internal/servers/grpc.go line 124 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

daemon/internal/servers/grpc.go line 134 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

daemon/internal/servers/grpc.go line 303 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

daemon/internal/servers/grpc.go line 313 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

daemon/internal/servers/grpc.go line 501 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

pkg/daemon/grpc.go line 230 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

pkg/daemon/grpc.go line 374 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

pkg/drkey/drkey.go line 203 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

pkg/private/xtest/graph/graph.go line 421 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

pkg/snet/path.go line 90 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

pkg/snet/path.go line 115 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

pkg/snet/path.go line 190 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

proto/daemon/v1/daemon.proto line 130 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

router/dataplane.go line 493 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

router/dataplane.go line 1080 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

router/dataplane.go line 1451 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

router/dataplane.go line 2373 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Change was added by a mistake during rebase. Reverted.

tools/end2end/main.go line 254 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

tools/end2end/main.go line 648 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

tools/integration/docker.go line 51 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

tools/integration/docker.go line 64 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

tools/integration/docker.go line 74 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

tools/integration/docker.go line 129 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

tools/integration/integration.go line 251 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

tools/topology/common.py line 128 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

daemon/drkey/client_engine.go line 43 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

pkg/grpc/dialer.go line 56 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

pkg/snet/path/empty.go line 30 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

pkg/snet/path/epic.go line 98 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

pkg/snet/path/onehop.go line 46 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

pkg/snet/path/scion.go line 47 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

proto/control_plane/experimental/v1/seg_detached_extensions.proto line 32 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

router/connector.go line 166 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

router/dataplane_internal_test.go line 218 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

tools/integration/binary.go line 107 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

- FABRID policies are defined with description, identifier, and a set of supported connection points - CSes load policies and validate them - CS appends maps for policy indices and supported connections to beacons - Remote CSes cache policy information from beacons - passes linter

- SD adds FABRID policies to paths

- SD uses correct local address for fetching DRKeys - FabridKeys function fetches required ASHost keys and the HostHost key

Also fixed linting error

SetExtensions method on dataplane paths is called during serialization, setting the HBH and E2E extension headers

- Fabrid and Identifier HBH extensions for sending FABRID traffic - Fabrid crypto library for computing HVFs and the path validator

Passing the WithFabrid option when choosing a path, selects an appropriate path based on the fabrid query and creates a fabrid dataplane path.

- BR fetches DRKey secret values and fabrid config on startup - BR validates and updates FABRID HVFs

- ping: select fabrid policies with --fabridquery flag - end2end: run integration test with --fabrid to test path validation

juagargi

router/dataplane.go line 1069 at r8 (raw file):

}

func (p *scionPacketProcessor) processHbhOptions(egressIF uint16) error {

we should move this to a different file

tools/integration/integration.go line 221 at r8 (raw file):

type HostAddr func(ia addr.IA) *snet.UDPAddr

// CSAddr reads the tester host Addr from the topology for the specified IA.

change upstream

juagargi

tools/integration/docker.go line 73 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Now the comment is missing.

jeltevanbommel · 2024-09-20T11:46:33Z

router/dataplane.go line 1069 at r8 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done

juagargi

Added some changes upstream by PR 4629
scionproto#4629

juagargi

juagargi requested review from juagargi and mawyss August 15, 2024 12:06

juagargi requested changes Sep 12, 2024

View reviewed changes

juagargi reviewed Sep 12, 2024

View reviewed changes

juagargi reviewed Sep 13, 2024

View reviewed changes

rohrerj reviewed Sep 16, 2024

View reviewed changes

marcodermatt mentioned this pull request Sep 20, 2024

snet: add extension header support to DataplanePath and PacketInfo scionproto/scion#4628

Open

Marc Odermatt and others added 23 commits September 20, 2024 12:28

Daemon FABRID support

dd32362

- SD adds FABRID policies to paths

DRKeys for FABRID

ee8a3e2

- SD uses correct local address for fetching DRKeys - FabridKeys function fetches required ASHost keys and the HostHost key

FABRID detachable extensions

c466392

modified fetcher used by daemon for drkey

62c8dc4

Fixing minor issues from last review round.

a3ea69f

Refactored FABRID policy validation

4e57df1

Also fixed linting error

FABRID detachable extensions

d0ccff6

Enable header extensions

fa947fc

SetExtensions method on dataplane paths is called during serialization, setting the HBH and E2E extension headers

FABRID header extensions and crypto

69a6bc5

- Fabrid and Identifier HBH extensions for sending FABRID traffic - Fabrid crypto library for computing HVFs and the path validator

FABRID dataplane path

6c8b559

Passing the WithFabrid option when choosing a path, selects an appropriate path based on the fabrid query and creates a fabrid dataplane path.

FABRID dataplane router

dd1292c

- BR fetches DRKey secret values and fabrid config on startup - BR validates and updates FABRID HVFs

FABRID ping and end2end

174bc03

- ping: select fabrid policies with --fabridquery flag - end2end: run integration test with --fabrid to test path validation

resolved some router and fabrid crypto related PR comments

b68263d

Fixing minor issues from last review round.

2538c15

Now pretty printing the selected policies.

50331c2

Refactor fabrid accumulator

6f9b582

code comments for FABRID dataplane and router

97bf83f

Revert change from rebase + lint

b86ea1e

extraced FABRID functions and moved to new files

a25dc7e

update github upload artifact version

03858e7

Delete moved endhost_ip function

60b9736

Undo unneeded formatting changes

f675c9d

juagargi force-pushed the fabrid/data-plane branch from 42acc25 to f675c9d Compare September 20, 2024 10:34

juagargi requested changes Sep 20, 2024

View reviewed changes

Moved hop by hop processing to separate dataplane_fabrid file.

0eb117a

Revert changes happening upstream.

bf66782

juagargi reviewed Sep 20, 2024

View reviewed changes

juagargi approved these changes Sep 20, 2024

View reviewed changes

juagargi force-pushed the scionlab branch from 469b21f to c323c13 Compare September 20, 2024 12:21

jeltevanbommel merged commit 17d89ad into netsec-ethz:scionlab Sep 20, 2024
1 check passed

jeltevanbommel mentioned this pull request Sep 20, 2024

Revert "FABRID base version" #169

Closed

marcodermatt mentioned this pull request Sep 25, 2024

FABRID expansions marcodermatt/scion#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FABRID base version #168

FABRID base version #168

marcodermatt commented Aug 14, 2024 •

edited

Loading

juagargi left a comment

juagargi left a comment

juagargi left a comment

rohrerj left a comment

juagargi left a comment

juagargi left a comment

jeltevanbommel commented Sep 20, 2024

juagargi left a comment

juagargi left a comment

FABRID base version #168

FABRID base version #168

Conversation

marcodermatt commented Aug 14, 2024 • edited Loading

juagargi left a comment

Choose a reason for hiding this comment

juagargi left a comment

Choose a reason for hiding this comment

juagargi left a comment

Choose a reason for hiding this comment

rohrerj left a comment

Choose a reason for hiding this comment

juagargi left a comment

Choose a reason for hiding this comment

juagargi left a comment

Choose a reason for hiding this comment

jeltevanbommel commented Sep 20, 2024

juagargi left a comment

Choose a reason for hiding this comment

juagargi left a comment

Choose a reason for hiding this comment

marcodermatt commented Aug 14, 2024 •

edited

Loading