feat(x): add `x/internal/outline` package for some shared app logic #39

jyyi1 · 2023-08-09T22:30:45Z

This PR introduces a new package, x/internal/outline, which contains business logic that can be shared among Outline apps such as OutlineCLI, Outline Client and ConnectivityCLI. For now, I added the following three components:

ParseAccessKey parses an Outline access key to a structured config
NewOutlineStreamDialer creates a StreamDialer from the access key
NewOutlinePacketListener creates a PacketListener from the access key
NewOutlineDevice creates an IPDevice with a refreshable UDP handling fro the result of connectivity test

x/outline/outline_device.go

fortuna · 2023-08-09T23:06:31Z

x/outline/outline_device.go

+	ssPktProxy       network.PacketProxy
+}
+
+func NewOutlineDevice(accessKey string) (*OutlineDevice, error) {


This conflates too many things to be in the SDK. It's code that belongs in outline-client instead.

For the SDK, I'd like to see a device others can reuse that is detached from our application logic. And users should be able to provide their own relay logic. We can provide the Outline and relay functionality via composition/layering.

One functionality that any VPN provider will need is the UDP refresh.
It would be helpful to see an IPdevice that provides the UDP update, taking the transport objects.

The relay should be a separate function. So is the Outline/Shadowsocks logic.

removed this and replaced with three new functions: NewOutlineStreamDialer, NewOutlinePacketProxy and NewOutlinePacketListener.

x/outline/access_key.go

fortuna · 2023-08-11T04:16:04Z

x/outline-connectivity/main.go

@@ -149,14 +134,14 @@ func main() {
 				var testDuration time.Duration
 				switch proto {
 				case "tcp":
-					dialer, err := makeStreamDialer(proxyAddress, config.CryptoKey, config.Prefix)
+					dialer, err := outline.NewOutlineStreamDialer(config)


We need to minimize the dependency on configs. In particular, having a config in addition to the access key is a pain.

We can have outline.NewOutlineStreamDialer(accessKey) instead, to bypass the need for a config. We are already committed to supporting accessKeys as a published format. In general, no one should be using the config in code, and doing so here sets a bad example.

This particular app, outline-connectivity, is special though, since it tries different combinations and needs to know how to build the Stream/Packet dialers. So it can't use access keys, unless it builds an access key, which is problematic.

We should never build access keys or configs to create the transports because we can create the transports directly. That's what the original code was doing.

We can keep the outline.ParseAccessKey call, but I'd rather revert back to makeStreamDialer/makePacketDialer. For other apps, we should provide functions that take the accessKey instead.

I may be open to have functions that take the config JSON we actually support, but this app doesn't make use of that config, and access keys are easier because it's what the manager returns.

The only reason config's fields are public is because this outline-connectivity is using them, and it also needs to loop through all IP addresses from config.Hostname.

Another way to completely hide the config is to introduce ResolveOutlineStreamDialers(key string) []StreamDialer and ResolveOutlinePacketListeners(key string) []PacketListener, they will loop through all IP addresses from a hostname and return a slice of StreamDialers and PacketListeners. This interface is also applicable to multi-server strategy / online config as well. Any thoughts about this?

x/internal/outline/outline_packet_proxy.go

fortuna · 2023-08-11T04:29:35Z

x/internal/outline/outline_packet_proxy.go

+	"github.com/Jigsaw-Code/outline-sdk/x/connectivity"
+)
+
+type OutlinePacketProxy interface {


Do we need to expose NewOutlinePacketProxy, NewOutlinePacketListener and NewOutlineStreamDialer?

For VPNs, you just need to expose a NewOutlineIpDevice(accessKey). For apps, perhaps a the stream dialer and a packet dialer make sense.

I agree , the only user would be connectivity test. But as you mentioned above, it contains special logic of looping through all IP addresses, so after I reverted the outline-connectivity there will be no apps using these, and then I can mark all these StreamDialer and PacketProxy private, and only exposes an OutlineDevice.

fortuna · 2023-08-11T04:31:57Z

x/internal/outline/outline_packet_proxy.go

+	return &proxy, nil
+}
+
+func (proxy *outlinePacketProxy) TestConnectivityAndRefresh(resolver, domain string) error {


This leaks implementation details of the connectivity test to the user of the outlinePacketProxy. If we no longer use DNS resolution, the user will need to change. That's the kind of dependency that makes it hard to change things.

Can you provide just a Refresh() with no arguments?

Perhaps you pass the connectivity test as a function in the constructor?
That way you also won't need to hold the remotePktListener, which is a bit awkward.

Instead of passing a connectivity test function, I'm thinking of whether a PacketTransportOptions object would be more readable?

type PacketTransportOptions struct { PacketListener transport.PacketListener TestConnectivity func() error } func NewOutlinePacketProxy(options PacketTransportOptions) (network.PacketProxy, error) { // ... }

Why have an "options" structure if you can have the actual object directly?
Also, if we are talking about the app-specific Outline Device, we can probably just hardcode the connectivity test, since our app doesn't customize it..

x/internal/outline/outline_stream_dialer.go

fortuna · 2023-08-11T04:35:08Z

x/internal/outline/access_key.go

-type sessionConfig struct {
+type Prefix []byte
+
+type SessionConfig struct {


This doesn't match our dynamic key config JSON. If we want to expose a format, we should use either the access key or the config we support.

You are right, however outline-connectivity is referencing these fields. I think ideally the type should be:

type SessionConfig struct { // all private, no public fields } func (SessionConfig) String() string { // this method can be used by connectivity test to print the structured config object }

I would still like to keep this intermediate config private and only provide methods that take an access key and return objects.

That doesn't work for outline-connectivity, so let's revert outline-connectivity. It's ok if it duplicates code for now, we can figure that out later.

We may want to provide some sort of shadowsocks-specific public library to parse Shadowsocks URLs, but let's do that later. It needs more discussion. The config area is a whole can of worms.

fortuna · 2023-08-11T23:10:52Z

x/internal/outline/outline_packet_listener.go

+	"github.com/Jigsaw-Code/outline-sdk/transport/shadowsocks"
+)
+
+func NewOutlinePacketListener(accessKey string) (transport.PacketListener, error) {


Argh, I just realized that there's a semantic issue here, uncovered by your change.

Is an access key the config for a VPN service, or just the transport for a tun2socks-based VPN?

I think it makes more sense to treat it as a config for the VPN service, so we can augment it with things like, the dns resolvers, the IP address to use, the routes to include/exclude....

However, we've been treating it as the transport for a tun2socks-based VPN. Kind of, since it also includes the service name.

We can pick one or the other, or say it's a VPN config that just includes the transport, but we can't say it's both.

What would happen if an accesskey encoded a wireguard config, for instance?

I'm not sure how to disentangle that right now. It needs some more thought and design.

Thoughts?

If you want something just to use in your cli, perhaps just define the NewOutlineClientDevice(accessKey string) for now?

I'm not sure if it helps, but I'll point out that the server is configured in a completely different way. That's transport only, and it uses a different format and there's no prefix or host.

It depends, because a VPN config would also contain something that is not related to OutlineClientDevice, such as the TUN device name, routing table configuration, etc. But OutlineClientDevice may also need something more than a key, for example, the DNS resolver for connectivity test. I feel we may introduce both an OutlineTransportOptions NewOutlineClientDevice(options OutlineTransportOptions) and an OutlineVPNOptions:

type OutlineTransportOptions struct { OutlineServerAccessKey string ConnectivityTestDNSResolver string ... } type OutlineVPNOptions struct { Transport *OutlineTransportOptions Routing *RoutingTableOptions ... }

For now, let's just rename accessKey to transportConfig, so it's clear it must be a transport config, in case we extend the access key later.

The assumption is that the existing accessKey format is a transport config, but that can change.

fortuna · 2023-08-16T17:37:19Z

x/internal/outline/access_key.go

-type sessionConfig struct {
+type Prefix []byte
+
+type SessionConfig struct {


I would still like to keep this intermediate config private and only provide methods that take an access key and return objects.

That doesn't work for outline-connectivity, so let's revert outline-connectivity. It's ok if it duplicates code for now, we can figure that out later.

fortuna · 2023-08-16T17:38:14Z

x/internal/outline/access_key.go

-// TODO(fortuna): provide this as a reusable library. Perhaps as x/shadowsocks or x/outline.
-func parseAccessKey(accessKey string) (*sessionConfig, error) {
-	var config sessionConfig
+func ParseAccessKey(accessKey string) (*SessionConfig, error) {


Change it to private.

fortuna · 2023-08-16T17:42:13Z

x/internal/outline/access_key.go

-// TODO(fortuna): provide this as a reusable library. Perhaps as x/shadowsocks or x/outline.
-func parseAccessKey(accessKey string) (*sessionConfig, error) {
-	var config sessionConfig
+func ParseAccessKey(accessKey string) (*SessionConfig, error) {


Note that this function is not ready for production. The old function supports parsing the legacy Shadowsocks format (code).

We will need to make sure it's on par with the existing code. A good way to do that is by processing the examples from https://github.com/Jigsaw-Code/outline-shadowsocksconfig/blob/master/src/shadowsocks_config.spec.ts.

You will have to address that before we can use it in the Outline client.

fortuna · 2023-08-16T17:43:13Z

x/internal/outline/access_key.go

-type sessionConfig struct {
+type Prefix []byte
+
+type SessionConfig struct {


We may want to provide some sort of shadowsocks-specific public library to parse Shadowsocks URLs, but let's do that later. It needs more discussion. The config area is a whole can of worms.

fortuna · 2023-08-16T17:52:50Z

x/internal/outline/outline_packet_proxy.go

+	remote, fallback  network.PacketProxy
+}
+
+func newOutlinePacketProxy(accessKey string) (opp *outlinePacketProxy, err error) {


The DNS fallback logic is really valuable, and others will need it too. Let's decouple it from the transport.

Can you provide a NewOutlineDevice that takes the StreamDialer and the PacketListener?

That way we can layer functionality. People will be able to provide their own transports and leverage the fallback functionality.

fortuna · 2023-08-16T17:53:06Z

x/internal/outline/outline_packet_proxy.go

+	return &proxy, nil
+}
+
+func (proxy *outlinePacketProxy) testConnectivityAndRefresh(resolver, domain string) error {


This can live in the Outline device instead.

fortuna · 2023-08-16T17:54:00Z

x/internal/outline/outline_packet_listener.go

+	"github.com/Jigsaw-Code/outline-sdk/transport/shadowsocks"
+)
+
+func NewOutlinePacketListener(accessKey string) (transport.PacketListener, error) {


For now, let's just rename accessKey to transportConfig, so it's clear it must be a transport config, in case we extend the access key later.

The assumption is that the existing accessKey format is a transport config, but that can change.

fortuna · 2023-08-16T17:54:48Z

x/outline-connectivity/main.go

Let's revert outline-connectivity.

jyyi1 · 2023-10-18T22:30:30Z

Not needed any more. Code is included in #15 .

feat(x): introduce OutlineDevice handling Outline VPN traffic

3b48d44

jyyi1 mentioned this pull request Aug 9, 2023

feat(example): add Outline CLI app for Linux #15

Merged

go mod tidy

616204b

jyyi1 requested a review from fortuna August 9, 2023 22:44

fortuna requested changes Aug 9, 2023

View reviewed changes

move x/outline to x/internal/outline

1955bf3

jyyi1 changed the title ~~feat(x): introduce OutlineDevice handling Outline VPN traffic~~ feat(x): add x/internal/outline package for some shared app logic Aug 11, 2023

add more tests for parsing Outline access key

3c7475f

jyyi1 requested a review from fortuna August 11, 2023 04:10

adjust some parse access key test cases

20094ff

fortuna requested changes Aug 11, 2023

View reviewed changes

jyyi1 added 2 commits August 11, 2023 16:21

bring back the OutlineDevice implementation

84cf12c

extract connectivity test constants

30ae0fb

jyyi1 marked this pull request as ready for review August 11, 2023 20:24

jyyi1 requested a review from fortuna August 11, 2023 20:24

fortuna reviewed Aug 11, 2023

View reviewed changes

fortuna requested changes Aug 16, 2023

View reviewed changes

jyyi1 closed this Oct 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(x): add `x/internal/outline` package for some shared app logic #39

feat(x): add `x/internal/outline` package for some shared app logic #39

jyyi1 commented Aug 9, 2023 •

edited

Loading

fortuna Aug 9, 2023

jyyi1 Aug 11, 2023

fortuna Aug 11, 2023

jyyi1 Aug 11, 2023 •

edited

Loading

fortuna Aug 11, 2023

jyyi1 Aug 11, 2023

fortuna Aug 11, 2023

jyyi1 Aug 11, 2023 •

edited

Loading

fortuna Aug 11, 2023

fortuna Aug 11, 2023

jyyi1 Aug 11, 2023

fortuna Aug 16, 2023

fortuna Aug 16, 2023

fortuna Aug 11, 2023

fortuna Aug 11, 2023

fortuna Aug 11, 2023

jyyi1 Aug 11, 2023

fortuna Aug 16, 2023

fortuna Aug 16, 2023

fortuna Aug 16, 2023

fortuna Aug 16, 2023

fortuna Aug 16, 2023

fortuna Aug 16, 2023

fortuna Aug 16, 2023

fortuna Aug 16, 2023

fortuna Aug 16, 2023

jyyi1 commented Oct 18, 2023

feat(x): add x/internal/outline package for some shared app logic #39

feat(x): add x/internal/outline package for some shared app logic #39

Conversation

jyyi1 commented Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 commented Oct 18, 2023

feat(x): add `x/internal/outline` package for some shared app logic #39

feat(x): add `x/internal/outline` package for some shared app logic #39

jyyi1 commented Aug 9, 2023 •

edited

Loading

jyyi1 Aug 11, 2023 •

edited

Loading

jyyi1 Aug 11, 2023 •

edited

Loading