pan: Remove global state, simplify API, and improve path management. Remove RAINS. #287
pan: Remove global state, simplify API, and improve path management. Remove RAINS. #287rolbk wants to merge 11 commits intonetsec-ethz:masterfrom
Conversation
ba14c16 to
cdadbcf
Compare
Remove support for RAINS name resolution, since it is incompatible with the SCION v0.13.0 API
Mark MangleSCIONAddrURL as deprecated since it relies on URL parsing behavior that will be fixed in Go 1.24.8 as part of CVE-2025-47912.
Key changes: - Add ASContext interface, pass to Dial/Listen functions - Remove global host context singleton, require explicit initialization - Export PathPool with PathPoolInit() for explicit lifecycle - Use addr.IA from snet directly instead of local type alias - Simplify ReplySelector.Path() (remove context parameter) - Fix Path.String() format, improve PathFingerprint readability - Add periodic refresh for subscribers without cached paths - Handle SCMP PacketTooBig errors gracefully
JordiSubira
left a comment
There was a problem hiding this comment.
@JordiSubira made 4 comments.
Reviewable status: 0 of 75 files reviewed, 4 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained.
pkg/pan/selector.go line 200 at r1 (raw file):
s.pingerCtx, s.pingerCancel = context.WithCancel(context.Background()) local := s.local.snetUDPAddr() pinger, err := ping.NewPinger(s.pingerCtx, s.ASCtx.Topology(), local)
ASCtx could be nil and this would panic internally. For now, I would panic early with some clear message, if an application want to use the PingingSelector and it cannot ping, I guess returning gracifully does not make a lot of sense. Btw, this is happening in ssh/client/ssh/selector.go in this repo. In the future, we will revisit the API for this selector and we can handle this differently.
pkg/pan/raw.go line 195 at r1 (raw file):
"SCMP error encountered", "type", scmp.Type(), "code", scmp.Code(), "host", pkt.Source.Host, "source", pkt.Source.IA) return nil
We should think whether we want in the Default handler to bubble up or not SCMP error message in the default case. Right now, interface down is handled by the selectors and "packetTooBig" just logs for not making QUIC crash (I guess those are sensible cases for the default handler). If we decide not to bubble up any SCMP error, we should perhaps remove SCMPError entirely.
pkg/shttp/transport.go line 37 at r1 (raw file):
// This is equivalent to net/http.DefaultTransport with DialContext overridden // to use shttp.Dialer, which dials connections over SCION/QUIC. var DefaultTransport = &http.Transport{
Clients that use DefaultTransport will fail. It doesn't init ASCtx. We have two options here, either removing DefaultTransport breaking the API and forcing clients to use NewTransport(AsCtx,...) or adding "auto-init" on Dial, e.g.,:
func (d *Dialer) DialContext(ctx context.Context, network, addr string) (net.Conn, error) {
asCtx := d.ASContext
if asCtx == nil {
asCtx = pan.MustLoadDefaultASContext()
}
...
pkg/shttp3/transport.go line 32 at r1 (raw file):
// DefaultTransport is the default RoundTripper that can be used for HTTP/3 // over SCION. var DefaultTransport = &http3.Transport{
Same as with shttp
rolbk
left a comment
There was a problem hiding this comment.
@rolbk made 4 comments.
Reviewable status: 0 of 75 files reviewed, 4 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained.
pkg/pan/raw.go line 195 at r1 (raw file):
Previously, JordiSubira wrote…
We should think whether we want in the Default handler to bubble up or not SCMP error message in the default case. Right now, interface down is handled by the selectors and "packetTooBig" just logs for not making QUIC crash (I guess those are sensible cases for the default handler). If we decide not to bubble up any SCMP error, we should perhaps remove
SCMPErrorentirely.
I think it's better to just log and remove the struct. For an easy-to-use library it's probably better to handle this internally (most SCMP errors aren't connection-terminating anyway i think?). Also Anapaya deliberately removed it so I'd go with that.
pkg/pan/selector.go line 200 at r1 (raw file):
Previously, JordiSubira wrote…
ASCtx could be nil and this would panic internally. For now, I would panic early with some clear message, if an application want to use the PingingSelector and it cannot ping, I guess returning gracifully does not make a lot of sense. Btw, this is happening in
ssh/client/ssh/selector.goin this repo. In the future, we will revisit the API for this selector and we can handle this differently.
Good catch! I implemented it as a nil check with panic("PingingSelector requires ASCtx to be set") in ensureRunning(). The alternative would be to just return, effectively not starting the pinger - avoids the panic, but kinda intransparent, so probably not the best idea.
pkg/shttp/transport.go line 37 at r1 (raw file):
Previously, JordiSubira wrote…
Clients that use
DefaultTransportwill fail. It doesn't initASCtx. We have two options here, either removingDefaultTransportbreaking the API and forcing clients to use NewTransport(AsCtx,...) or adding "auto-init" on Dial, e.g.,:func (d *Dialer) DialContext(ctx context.Context, network, addr string) (net.Conn, error) { asCtx := d.ASContext if asCtx == nil { asCtx = pan.MustLoadDefaultASContext() } ...
I think it should be ok if we break the API, but I think an auto-init is better for now, implemented that.
pkg/shttp3/transport.go line 32 at r1 (raw file):
Previously, JordiSubira wrote…
Same as with
shttp
ack
JordiSubira
left a comment
There was a problem hiding this comment.
@JordiSubira made 2 comments and resolved 2 discussions.
Reviewable status: 0 of 76 files reviewed, 2 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained.
pkg/pan/raw.go line 195 at r1 (raw file):
Previously, rolbk (Emanuel Mairoll) wrote…
I think it's better to just log and remove the struct. For an easy-to-use library it's probably better to handle this internally (most SCMP errors aren't connection-terminating anyway i think?). Also Anapaya deliberately removed it so I'd go with that.
Did they also remove the SCMPError, I do not think they are using it either internally, e.g., redefining the handler.
pkg/shttp/transport.go line 37 at r1 (raw file):
Previously, rolbk (Emanuel Mairoll) wrote…
I think it should be ok if we break the API, but I think an auto-init is better for now, implemented that.
Using this pan.MustLoadDefaultASContext() in the libraries isn't a good idea. It calls PathPoolInit() which starts a new refresher and clears path every time. We do not want to do that, especially on Dial() which may be called several times by the application. In Listen() isn't that bad but probably not ideal. Notice that in web-gateway, for instance, you call pan.MustLoadContext() from the application and internally when doing shttp.ListenAndServer() this already hints at it not being a good idea either. I guess we could postpone this "auto-init" discussion until we have the "Client object" and we reconsider then if it is a good idea.
rolbk
left a comment
There was a problem hiding this comment.
@rolbk made 2 comments.
Reviewable status: 0 of 79 files reviewed, 2 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained.
pkg/pan/raw.go line 195 at r1 (raw file):
Previously, JordiSubira wrote…
Did they also remove the
SCMPError, I do not think they are using it either internally, e.g., redefining the handler.
Yes, they removed all usages but left in the struct.
pkg/shttp/transport.go line 37 at r1 (raw file):
Previously, JordiSubira wrote…
Using this
pan.MustLoadDefaultASContext()in the libraries isn't a good idea. It callsPathPoolInit()which starts a new refresher and clears path every time. We do not want to do that, especially onDial()which may be called several times by the application. InListen()isn't that bad but probably not ideal. Notice that inweb-gateway, for instance, you callpan.MustLoadContext()from the application and internally when doingshttp.ListenAndServer()this already hints at it not being a good idea either. I guess we could postpone this "auto-init" discussion until we have the "Client object" and we reconsider then if it is a good idea.
Yes you are right, calling it in shttp is suboptimal, even with the temporary API. I moved all calls to pan.MustLoadDefaultASContext to their respective main functions.
This PR refactors the pan package to eliminate some global state, introduces explicit lifecycle management upgrades and simplifies the API. These improvments originate in Anapaya's internal fork. Besides, it upgrades to SCION v0.14.0, and removes legacy RAINS support.
Improvements
We upstream the following improvements originated in Anapaya's internal fork of PAN.
ASContextinterface, now passed to Dial/Listen functions for better context managementPathPoolwithPathPoolInit()for explicit lifecycle managementaddr.IAfrom snet directly instead of maintaining a local type aliasReplySelector.Path()by removing unnecessary context parameterPath.String()format andPathFingerprintreadabilitySCION upgrade to v0.14.0
Upgrades from SCION v0.11.0 to v0.14.0, bringing compatibility with the latest SCION APIs and improvements.
Remove RAINS support
pkg/pan/rains.goand related code)github.com/netsec-ethz/rainsAPI changes across scion-apps
Since
panis used by most applications in scion-apps, the newASContext-based API required updates across many files. Given that a broader API rework is in progress (see #285), this PR preserves the exact API from the Anapaya fork rather than introducing additional changes. To reduce code duplication and simplify application code, a helper methodMustLoadDefaultASContext()was added that handles the common initialization pattern used by all apps.Breaking Changes
ASContext-based interfaceThis change is