feat(x): Parse psiphon config format as part of smart-proxy #394

ohnorobo · 2025-03-25T12:36:28Z

This is not yet setting up the psiphon connection, but I figured it would be important to hash out exactly what format we want to use in config.yaml for the fallbacks since this is modifying the format slightly from #384.

I've broken the parsing of the config out into parseConfig to deal with the string and object parsing.

Don't worry too much about the json parsing/logging around :331. It will go away in the next PR. I just wanted to make sure this parsing approach would work for getting from YAML->rawJson without actually putting the format validation here.

Tested

go run -C ./x/examples/smart-proxy/ . -v -localAddr=localhost:1080 --transport="" --domain rt.com  --config=[...]/outline-sdk/x/examples/smart-proxy/config_broken.yaml
Finding strategy
...
[DNS/TLS output omitted]
...
🏃 running test: 'ss://Y2hhY2hhMjAtaWV0Zi1wb2x5MTMwNTprSzdEdHQ0MkJLOE9hRjBKYjdpWGFK@1.2.3.4:9999/?outline=1' (domain: rt.com.)
❌ Psiphon is not yet supported, skipping: {"psiphon":{"LocalHttpProxyPort":8081,"LocalSocksProxyPort":1081,"PropagationChannelId":"FFFFFFFFFFFFFFFF","RemoteServerListDownloadFilename":"remote_server_list","RemoteServerListSignaturePublicKey":"MIICIDANBgkqhkiG9w0BAQEFAAOCAg0AMIICCAKCAgEAt7Ls+/39r+T6zNW7GiVpJfzq/xvL9SBH5rIFnk0RXYEYavax3WS6HOD35eTAqn8AniOwiH+DOkvgSKF2caqk/y1dfq47Pdymtwzp9ikpB1C5OfAysXzBiwVJlCdajBKvBZDerV1cMvRzCKvKwRmvDmHgphQQ7WfXIGbRbmmk6opMBh3roE42KcotLFtqp0RRwLtcBRNtCdsrVsjiI1Lqz/lH+T61sGjSjQ3CHMuZYSQJZo/KrvzgQXpkaCTdbObxHqb6/+i1qaVOfEsvjoiyzTxJADvSytVtcTjijhPEV6XskJVHE1Zgl+7rATr/pDQkw6DPCNBS1+Y6fy7GstZALQXwEDN/qhQI9kWkHijT8ns+i1vGg00Mk/6J75arLhqcodWsdeG/M/moWgqQAnlZAGVtJI1OgeF5fsPpXu4kctOfuZlGjVZXQNW34aOzm8r8S0eVZitPlbhcPiR4gT/aSMz/wd8lZlzZYsje/Jr8u/YtlwjjreZrGRmG8KMOzukV3lLmMppXFMvl4bxv6YFEmIuTsOhbLTwFgh7KYNjodLj/LsqRVfwz31PgWQFTEPICV7GCvgVlPRxnofqKSjgTWI4mxDhBpVcATvaoBl1L/6WLbFvBsoAUBItWwctO2xalKxF5szhGm8lccoc5MZr8kfE0uxMgsxz4er68iCID+rsCAQM=","RemoteServerListUrl":"https://s3.amazonaws.com//psiphon/web/mjr4-p23r-puwl/server_list_compressed","SponsorId":"FFFFFFFFFFFFFFFF","UseIndistinguishableTLS":true}}
🏃 running test: 'socks5://192.168.1.10:1080' (domain: rt.com.)
2025/03/25 17:08:00 Failed to find dialer: could not find a working fallback: all tests failed
exit status 1

Also tested a failing psiphon config followed by a successful outline server.

x/examples/smart-proxy/config_broken.yaml

x/smart/stream_dialer.go

ohnorobo · 2025-03-25T15:00:43Z

Alright, I made an attempt at changing for format. But I don't particularly like it. It seems like it introduces a lot of complication in parsing the config.

Kludging around any types instead of being able to rely on reasonable type safety from the yaml unmarshal
now there's parsing logic in findFallback instead of just business logic
confusing errors message if you write the config wrong (I mis-edited the config while testing, and my configurls started getting parsed as psiphon configs.) I think this kind of error will happen more in the future as we make yet-unknown changes to this config.
we no longer have the psiphon key in the config, which I think makes it easier to read for someone who doesn't already know what the big json chunk is.

All for the dubious pleasure of mixing strings and objects in a list. You're sure it wouldn't be cleaner just to switch to a keyed map while there's no one relying on this?

I do really like just copying the json from the psiphon config directly. Seems much nicer.

x/examples/smart-proxy/config_broken.yaml

fortuna · 2025-03-25T15:40:42Z

x/smart/stream_dialer.go

+
+			return &SearchResult{dialer, fallbackConfig}, nil
+		case map[string]interface{}:
+			psiphonJSON, err := json.Marshal(v)


We'll need a switch here based on what type field was set.

I've added a check for the string, but I'm not sure I'm understanding what you mean.

We need to do the same we do for DNS, like:

if cfg := entry.Psiphon; cfg != nil { // Process Psiphon config }

doing it like

type psiphonEntryConfig struct { Psiphon any `yaml:"psiphon,omitempty"` } ... psiphonCfg, ok := fallbackConfig.(psiphonEntryConfig)

doesn't succeed. ok = false and it fails to coerce the type.

case psiphonEntryConfig:

also fails

The point of the map[string]interface{} type was to get enough of a toehold on the type that I'm able to extract the child object.

Attempting to call fallbackConfig.Psiphon fails to build because it's not certain the field exists.

Because there's no union type yaml.Unmarshal can't do all the work for us of mapping the yaml into structs while we're mixing structs and strings.

We could force the conversion here with something like https://pkg.go.dev/reflect, but that seems heavyweight when we're just trying to check and will be passing on to the psiphon lib in a moment.

That type should be called something else, since it should allow other types. Example:

type fallbackEntryConfig struct { Psiphon any FooBar SomeType }

To parse, you can change the list entry to be ast.Node (from github.com/goccy/go-yaml).

Then you can use NodeToValue.

Or do what we do in the client with mapToAny.

broke out a parseConfig function that parses strings and objects directly from the ast. I would love ideas on how to make this part shorter though.

x/smart/stream_dialer.go

fortuna · 2025-03-26T14:51:22Z

x/smart/stream_dialer.go

+
+			return &SearchResult{dialer, fallbackConfig}, nil
+		case map[string]interface{}:
+			psiphonJSON, err := json.Marshal(v)


You need to extract the config from the psiphon field, like we do for DNS.
Perhaps define a fallbackEntryConfig with the Psiphon: any field.

fortuna · 2025-03-26T14:53:49Z

x/smart/stream_dialer.go

+
+			return &SearchResult{dialer, fallbackConfig}, nil
+		case map[string]interface{}:
+			psiphonJSON, err := json.Marshal(v)


We need to do the same we do for DNS, like:

if cfg := entry.Psiphon; cfg != nil { // Process Psiphon config }

fortuna · 2025-03-26T20:25:19Z

x/smart/stream_dialer.go

+
+			return &SearchResult{dialer, fallbackConfig}, nil
+		case map[string]interface{}:
+			psiphonJSON, err := json.Marshal(v)


That type should be called something else, since it should allow other types. Example:

type fallbackEntryConfig struct { Psiphon any FooBar SomeType }

To parse, you can change the list entry to be ast.Node (from github.com/goccy/go-yaml).

Then you can use NodeToValue.

Or do what we do in the client with mapToAny.

x/smart/stream_dialer.go

fortuna · 2025-03-31T23:53:06Z

x/smart/stream_dialer.go

-		return &SearchResult{dialer, fallbackUrl}, nil
+			return &SearchResult{dialer, fallbackConfig}, nil
+		case fallbackEntryStructConfig:
+			psiphonCfg := v.Psiphon


Please test if Psiphon is not nil. I want the Psiphon code to be in it's own block. Think about how it would look with more types.
Also, you shouldn't call Marshall on nil.

fortuna · 2025-03-31T23:53:30Z

x/smart/stream_dialer.go

-			return nil, fmt.Errorf("getStreamDialer failed: %w", err)
-		}
+	fallback, err := raceTests(ctx, 250*time.Millisecond, fallbackConfigs, func(fallbackConfig fallbackEntryConfig) (*SearchResult, error) {
+		switch v := fallbackConfig.(type) {


This parsing logic probably deserves a unit test.

Yeah, I think this specifically and the selection logic in general would benefit from more unit testing. I'm also a bit nervous about the correctness of where we're returning errors vs logging error output.

To do it as a unit test will require some refactoring though, since currently the network calls are intertwined. Can I do it in a follow up?

I did add a few tests, but just for ParseConfig, since it's currently well isolated.

One thing that may help for testing is replacing the base dialers (though we can't do that for Psiphon). But that can be done some other time.

fortuna · 2025-03-31T23:55:46Z

x/smart/stream_dialer.go

@@ -365,14 +394,58 @@ func (f *StrategyFinder) newProxylessDialer(ctx context.Context, testDomains []s
 	return f.findTLS(ctx, testDomains, dnsDialer, config.TLS)
 }

+func (f *StrategyFinder) parseConfig(configBytes []byte) (configConfig, error) {


Why are you doing this manual parsing instead of unmarshalling into the output directly?

I got rid of the manual parsing and switched this to use mapToAny + Unmarshal.

I've just copied mapToAny in wholesale from outline-apps. I figure we'll want it anyway eventually. But let me know if you'd like me to put it somewhere other than this file in anticipation of eventually moving over more code.

I've also added a bit of unit testing, partially just to enumerate the allowed config fields in a single readable place besides x/examples/smart_proxy.

fortuna · 2025-04-01T16:18:36Z

x/smart/stream_dialer.go

-			return nil, fmt.Errorf("getStreamDialer failed: %w", err)
-		}
+	fallback, err := raceTests(ctx, 250*time.Millisecond, fallbackConfigs, func(fallbackConfig fallbackEntryConfig) (*SearchResult, error) {
+		switch v := fallbackConfig.(type) {


One thing that may help for testing is replacing the base dialers (though we can't do that for Psiphon). But that can be done some other time.

ohnorobo force-pushed the psiphon-fallback branch 2 times, most recently from 4394b98 to 7a8888e Compare March 25, 2025 12:49

ohnorobo requested a review from fortuna March 25, 2025 13:00

fortuna requested changes Mar 25, 2025

View reviewed changes

x/examples/smart-proxy/config_broken.yaml Outdated Show resolved Hide resolved

x/examples/smart-proxy/config_broken.yaml Show resolved Hide resolved

fortuna reviewed Mar 25, 2025

View reviewed changes

x/smart/stream_dialer.go Show resolved Hide resolved

x/smart/stream_dialer.go Show resolved Hide resolved

ohnorobo requested a review from fortuna March 25, 2025 15:01

fortuna reviewed Mar 25, 2025

View reviewed changes

ohnorobo requested a review from fortuna March 25, 2025 16:57

ohnorobo force-pushed the psiphon-fallback branch from c6d9a3a to 4ac6bd3 Compare March 26, 2025 10:46

fortuna reviewed Mar 26, 2025

View reviewed changes

ohnorobo requested a review from fortuna March 26, 2025 17:41

fortuna reviewed Mar 26, 2025

View reviewed changes

ohnorobo force-pushed the psiphon-fallback branch from 4ac6bd3 to 1b11975 Compare March 27, 2025 11:16

ohnorobo changed the base branch from main to update-yaml March 27, 2025 11:16

ohnorobo force-pushed the psiphon-fallback branch from 1b11975 to 5cc6097 Compare March 27, 2025 13:34

ohnorobo requested a review from fortuna March 27, 2025 13:41

Base automatically changed from update-yaml to main March 27, 2025 14:41

ohnorobo force-pushed the psiphon-fallback branch from 5cc6097 to c0a72b2 Compare March 27, 2025 14:42

fortuna reviewed Mar 31, 2025

View reviewed changes

ohnorobo force-pushed the psiphon-fallback branch 2 times, most recently from 4cd69e3 to 58822c9 Compare April 1, 2025 14:37

ohnorobo requested a review from fortuna April 1, 2025 14:48

ohnorobo force-pushed the psiphon-fallback branch from 58822c9 to b9c7805 Compare April 1, 2025 14:52

ohnorobo marked this pull request as ready for review April 1, 2025 14:52

fortuna approved these changes Apr 1, 2025

View reviewed changes

parse psiphon config wihout connecting

93cc839

ohnorobo force-pushed the psiphon-fallback branch from b9c7805 to 9eefc82 Compare April 1, 2025 17:13

reviewer comments

49d77cb

ohnorobo force-pushed the psiphon-fallback branch from 9eefc82 to 49d77cb Compare April 1, 2025 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(x): Parse psiphon config format as part of smart-proxy #394

feat(x): Parse psiphon config format as part of smart-proxy #394

ohnorobo commented Mar 25, 2025 •

edited

Loading

ohnorobo commented Mar 25, 2025 •

edited

Loading

fortuna Mar 25, 2025

ohnorobo Mar 25, 2025

fortuna Mar 26, 2025

ohnorobo Mar 26, 2025

fortuna Mar 26, 2025

ohnorobo Mar 27, 2025

fortuna Mar 26, 2025

ohnorobo Mar 27, 2025

fortuna Mar 26, 2025

fortuna Mar 26, 2025

fortuna Mar 31, 2025

ohnorobo Apr 1, 2025

fortuna Mar 31, 2025

ohnorobo Apr 1, 2025 •

edited

Loading

fortuna Apr 1, 2025

fortuna Mar 31, 2025

ohnorobo Apr 1, 2025 •

edited

Loading

fortuna Apr 1, 2025

feat(x): Parse psiphon config format as part of smart-proxy #394

Are you sure you want to change the base?

feat(x): Parse psiphon config format as part of smart-proxy #394

Conversation

ohnorobo commented Mar 25, 2025 • edited Loading

Tested

ohnorobo commented Mar 25, 2025 • 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

ohnorobo Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohnorobo Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohnorobo commented Mar 25, 2025 •

edited

Loading

ohnorobo commented Mar 25, 2025 •

edited

Loading

ohnorobo Apr 1, 2025 •

edited

Loading

ohnorobo Apr 1, 2025 •

edited

Loading