cmd/scion: add one-hop support to ping

[oncilla]

Add support to create one-hop paths in the ping command. This can be used to send SCION pings to direct neighbors over interfaces for which no path exists currently.

---

This change is 

[jiceatscion]

Some non-binding suggestions only.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @oncilla)

---

dispatcher/underlay.go line 357 at r1 (raw file):

		return serrors.WrapStr("reversing path", err)
	}
	pkt.SCION.PathType = pkt.SCION.Path.Type()

A bit obscure to me. Why would PathType need to change after reversal? Add a comment for the naive readers like me?

---

private/keyconf/keyconf.go line 58 at r1 (raw file):

	}
	dbuf = dbuf[:n]
	if strings.ToLower(algo) != RawKey {

Not that this is added by this PR, but...WTF ever was the purpose of the "algo" parameter? Since this was an unexported function, may be it the algo param could go and the function just just become exported. No?

---

scion/cmd/scion/ping.go line 173 at r1 (raw file):

				path.WithEPIC(flags.epic),
			}
			if flags.egress != 0 || flags.nextHop.IP != nil || flags.forwardingKey != "" {

I understood the intent but, along with the next three tests, it reads weird. Could it make more sense to check that all three conditions have the same truth value and, if not, report an error to that effect? Then use whatever that value is to decide on doing the rest. Might not be shorter but a bit more obvious.

[oncilla]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jiceatscion)

---

scion/cmd/scion/ping.go line 173 at r1 (raw file):

Could it make more sense to check that all three conditions have the same truth value and, if not, report an error to that effect? Then use whatever that value is to decide on doing the rest.

I'm not sure I understand exactly what you mean here.

The intention is the following:

• If any of these flags is set, we assume the user wants to run in one-hop mode.
• In one-hop mode, all the flags are required.

I guess I could change it to:

• If any of the flags is set, we assume the user wants to run in one-hop mode.
• Check if all are set, and if not, report the flags that are missing.

Is that what you had in mind?

[jiceatscion]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @oncilla)

---

scion/cmd/scion/ping.go line 173 at r1 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Could it make more sense to check that all three conditions have the same truth value and, if not, report an error to that effect? Then use whatever that value is to decide on doing the rest.

I'm not sure I understand exactly what you mean here.

The intention is the following:

• If any of these flags is set, we assume the user wants to run in one-hop mode.
• In one-hop mode, all the flags are required.

I guess I could change it to:

• If any of the flags is set, we assume the user wants to run in one-hop mode.
• Check if all are set, and if not, report the flags that are missing.

Is that what you had in mind?

Ok, the intent is what I understood. What I had in mind was something like: if a and b and c { one_hop } elif !a and !b and !c { not_one_hop } else { error } ... May be all that's needed is what you propose, only replacing your three different errors with a single one; like: if (!b or !b or !c) { error("all three flags must be set, or none must be") }. It would make everything clearer; both to readers and users.

[matzf]

Reviewed 2 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jiceatscion and @oncilla)

---

dispatcher/underlay.go line 357 at r1 (raw file):

Previously, jiceatscion wrote…

A bit obscure to me. Why would PathType need to change after reversal? Add a comment for the naive readers like me?

It's because Reverse is (too) magic and can return a path of different type. In particular, reversing a one-hop path returns a SCION path. It wouldn't have to, but it does. It's sort of a convenience in higher levels of the inter-AS RPC stack.

---

private/keyconf/keyconf.go line 58 at r1 (raw file):

Previously, jiceatscion wrote…

Not that this is added by this PR, but...WTF ever was the purpose of the "algo" parameter? Since this was an unexported function, may be it the algo param could go and the function just just become exported. No?

Historically, this function was also used to load elliptic curve keys, which now are all stored in PEM and use a different code path. I guess this algorithm could be removed indeed.

---

scion/cmd/scion/ping.go line 397 at r1 (raw file):

	cmd.Flags().StringVar(&flags.format, "format", "human",
		"Specify the output format (human|json|yaml)")
	cmd.Flags().Uint16Var(&flags.egress, "onehop.egress", 0, "Egress interface for one-hop path")

The onehop flags, in particular the egress parameter, is incompatible with any other path option flags, namely

• --interactive
• --refresh
• --healthy-only
• --sequence
• (and also --epic)

A somewhat smoother integration might be to add onehop as only a bool flag instead of specifying the egress interface.
This could create the entire set of one hop paths to the neighboring AS (if there are any), among which the user can then choose with the existing options (interactive prompt, sequence match, arbitrary default).
As epic and onehop are mutually exclusive, perhaps these flags could even be combined in a single enum "path type" parameter.

If this seems too much additional work for only cosmetic benefit, I'd sueggest to mark the new onehop flags as experimental, so that we can tackle this later without too much friction.

---

scion/cmd/scion/ping.go line 398 at r1 (raw file):

		"Specify the output format (human|json|yaml)")
	cmd.Flags().Uint16Var(&flags.egress, "onehop.egress", 0, "Egress interface for one-hop path")
	cmd.Flags().Var(&flags.nextHop, "onehop.next-hop", "NextHop for one-hop path")

The next hop can be inferred from the topology. The daemon's Interfaces() provides the corresponding mapping from the egress interface.
With this, we can make this parameter optional, or perhaps even remove it entirely -- for the other path types, we don't support overriding the next hop either.

---

scion/cmd/scion/ping.go line 400 at r1 (raw file):

	cmd.Flags().Var(&flags.nextHop, "onehop.next-hop", "NextHop for one-hop path")
	cmd.Flags().StringVar(&flags.forwardingKey, "onehop.forwarding-key", "",
		"Forwarding key for one-hop path",

As we don't have a separate user manual for this tool, perhaps add a bit more information to this doc string here.

Suggestion:

	cmd.Flags().StringVar(&flags.forwardingKey, "onehop.forwarding-key", "master0.key",
		"Forwarding secret key file for one-hop path. Note that this is typically only available to the operator of the SCION AS.",

[oncilla]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jiceatscion and @matzf)

---

dispatcher/underlay.go line 357 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

It's because Reverse is (too) magic and can return a path of different type. In particular, reversing a one-hop path returns a SCION path. It wouldn't have to, but it does. It's sort of a convenience in higher levels of the inter-AS RPC stack.

Agreed. The Reverse method is on the wrong level. On the level of the path we do not have enough info. This is also why the EPIC above needs special handling.

I will add a note here, but this is a more fundamental issue with the path itself.

---

private/keyconf/keyconf.go line 58 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Historically, this function was also used to load elliptic curve keys, which now are all stored in PEM and use a different code path. I guess this algorithm could be removed indeed.

Yeah, we could also drop the master1.key 😆

I think we can clean it up when we finally support forwarding key rollover.

---

scion/cmd/scion/ping.go line 397 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

The onehop flags, in particular the egress parameter, is incompatible with any other path option flags, namely

• --interactive
• --refresh
• --healthy-only
• --sequence
• (and also --epic)

A somewhat smoother integration might be to add onehop as only a bool flag instead of specifying the egress interface.
This could create the entire set of one hop paths to the neighboring AS (if there are any), among which the user can then choose with the existing options (interactive prompt, sequence match, arbitrary default).
As epic and onehop are mutually exclusive, perhaps these flags could even be combined in a single enum "path type" parameter.

If this seems too much additional work for only cosmetic benefit, I'd sueggest to mark the new onehop flags as experimental, so that we can tackle this later without too much friction.

Ack, I can do some clean ups here.

---

scion/cmd/scion/ping.go line 398 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

The next hop can be inferred from the topology. The daemon's Interfaces() provides the corresponding mapping from the egress interface.
With this, we can make this parameter optional, or perhaps even remove it entirely -- for the other path types, we don't support overriding the next hop either.

That requires that the scion command has access to the topology file. I would prefer to not spread the topology file any further than what it has already infected.

For regular paths, the next hop is part of the response of the Daemon. We can extend the Daemon API to allow querying the next hop for a certain interface, but that seems like an overkill to me.

[matzf]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jiceatscion and @oncilla)

---

scion/cmd/scion/ping.go line 398 at r1 (raw file):

Previously, oncilla (Dominik Roos) wrote…

That requires that the scion command has access to the topology file. I would prefer to not spread the topology file any further than what it has already infected.

For regular paths, the next hop is part of the response of the Daemon. We can extend the Daemon API to allow querying the next hop for a certain interface, but that seems like an overkill to me.

I fully agree about the topology file, accessing this directly would not be nice.

But the API in the daemon already exists: pkg/daemon.Connector.Interfaces(ctx context.Context) (map[uint16]*net.UDPAddr, error).

[oncilla]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jiceatscion and @matzf)

---

scion/cmd/scion/ping.go line 398 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

I fully agree about the topology file, accessing this directly would not be nice.

But the API in the daemon already exists: pkg/daemon.Connector.Interfaces(ctx context.Context) (map[uint16]*net.UDPAddr, error).

Oh wow, I forgot that even existed 💯

Will do!

[jiceatscion]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @matzf and @oncilla)

---

private/keyconf/keyconf.go line 58 at r1 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Yeah, we could also drop the master1.key 😆

I think we can clean it up when we finally support forwarding key rollover.

Add a todo/file a bug, so it gets done one day?

---

scion/cmd/scion/ping.go line 173 at r1 (raw file):

Previously, jiceatscion wrote…

Ok, the intent is what I understood. What I had in mind was something like: if a and b and c { one_hop } elif !a and !b and !c { not_one_hop } else { error } ... May be all that's needed is what you propose, only replacing your three different errors with a single one; like: if (!b or !b or !c) { error("all three flags must be set, or none must be") }. It would make everything clearer; both to readers and users.

We understood each-other. Not blocking anyway.

[jiceatscion]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @matzf and @oncilla)