Upstream some patches

[klzgrad]

As always, it would be beneficial to upstream some of the patches to reduce churn on new versions.

Altough I'm seeing some big patches that make structural changes to cronet part. In principle this creates big maintenance burden for the future. I don't know if these changes are necessary or it is so just because the API additions have not matured? (Example: the old code for using NetworkIsolationKey klzgrad/naiveproxy@87b03cd was designed for minimizing patch size and API exposure (smuggling parameters via HTTP headers), but the new code SagerNet/naiveproxy@7ed3537 has wide patch surface and may be hard to maintain in the future.)

I'm also seeing a possible future that this outbound can be upgraded to something that is not so naive, which you have translated in naive_conn.go. There has been some attempts to add more sophisticated traffic shaping and behaviors in C++, which has been been taking more time than desired. It's possible these behaviors are easier to implement in Go, if the Cronet API proves to be capable enough to handle the needed changes for these behaviors.

[klzgrad]

I'll add new items as I review some patches:

• SagerNet/naiveproxy@525b7c1 Configure cronet build flags: These are changed from my build.sh and some are recorded in cmd_build.go. It's best not to patch upstream BUILD.gn.
• SagerNet/naiveproxy@c5f2b70 Fix HTTP/2 ALPN negotiation for BidirectionalStream: how did it break H2 bidi stream I'm not sure.
• SagerNet/naiveproxy@865e1c7 Add cronet static library and custom certificate verification API: A big commit with many different things mixed. Should be split before further inspection.
• SagerNet/naiveproxy@65c1120 Remove cronet shared library target: cronet.dll is still shared library being used? Should be merged with later commits adding back shared libraries.
• SagerNet/naiveproxy@4a58898 Fix assertion failure when StartWithParams called twice: I don't think this is correct.
• SagerNet/naiveproxy@185a3e0 Add DnsServerOverride experimental option: DnsConfigOverrides
• SagerNet/naiveproxy@d5127d8 Add public key SHA256 certificate pinning support: Why? https://source.chromium.org/chromium/chromium/src/+/main:components/cronet/native/cronet.idl;l=468;bpv=1;bpt=0 already has this.
• SagerNet/naiveproxy@1cb0cd6 This must be wrong at some level. This is not the root fix.
• SagerNet/naiveproxy@dc4428f Simplify cronet_global_state_stubs and use fixed proxy config: Should split.
• SagerNet/naiveproxy@1ad28f3 Add custom TCP dialer API for connection interception: How does this work?

[klzgrad]

Some other comments

cronet-go/cmd/build-naive/cmd_build.go

Line 194 in 341e56b

"use_thin_lto=false", // Disable ThinLTO so static lib can be linked with system clang

Hopefully this will work out because thinlto makes the binary better.

cronet-go/cmd/build-naive/cmd_build.go

Line 318 in 341e56b

// Windows: only build DLL (static linking not supported - Chromium uses MSVC, Go CGO only supports MinGW)

Windows static build worked. Probably need new work to fix it.

[nekohasekai]

SagerNet/naiveproxy@d5127d8 Add public key SHA256 certificate pinning support: Why? https://source.chromium.org/chromium/chromium/src/+/main:components/cronet/native/cronet.idl;l=468;bpv=1;bpt=0 already has this.

Cronet's built-in Public Key Pins provide supplemental verification, requiring the certificate to be trusted first. However, in self-signed use cases, we use public key pins to verify the certificate, which is also the original behavior of the relevant configuration in the sing-box TLS options. This, along with my added custom Root CA authentication, is a compromise for self-signed users. I don't know your opinion, but if you believe self-signing is incorrect behavior, I will consider removing these two APIs.

[nekohasekai]

Hopefully this will work out because thinlto makes the binary better.

This is a compromise for now, as I'm not ready to have all Android and Apple builds fully support ThinLTO. In my testing, I seem to be encountering some obstacles on the Apple platform because after gomobile creates the xcframework, it ultimately requires Xcode to link, and I'm stuck on this step.

I believe it will eventually be supported, but I'm currently working on getting QUIC and ECH working.

[nekohasekai]

Windows static build worked. Probably need new work to fix it.

Naive's Windows static build works, but Go doesn't seem to. I haven't found a way to get CGO to compile with the naive toolchain before. In the toughest case, we'd need a custom version of Go to make it work, and I don't have any ideas at the moment.

[klzgrad]

in self-signed use cases

Self-signed certificate or self-signed root CA? The latter case is probably ok as-is in Chromium.

[klzgrad]

if you believe self-signing is incorrect behavior

After further reflection, I believe self-signing is against the premise of naiveproxy to parrot Chrome. Chrome does not allow self-signing by default and self-signing changes traffic behavior significantly.

Also public key pinning when not self-signing is entirely unsustainable because it merely introduces massive maintenance burden of fixing broken key pins.

[nekohasekai]

if you believe self-signing is incorrect behavior

After further reflection, I believe self-signing is against the premise of naiveproxy to parrot Chrome. Chrome does not allow self-signing by default and self-signing changes traffic behavior significantly.

Also public key pinning when not self-signing is entirely unsustainable because it merely introduces massive maintenance burden of fixing broken key pins.

I'd like to remove the ability to trust SHA256 while retaining the ability to trust the root CA, and only warn users in the documentation and changelog against using self-signed certificates. Is this acceptable? Because our test system relies on self-signed certificates.

[klzgrad]

Cronet_CreateCertVerifierWithRootCerts? src/net/cert/internal/system_trust_store.cc currently contains an implementation to read certs from SSL_CERT_DIR and SSL_CERT_FILE. Does this not work?

[nekohasekai]

Cronet_CreateCertVerifierWithRootCerts? src/net/cert/internal/system_trust_store.cc currently contains an implementation to read certs from SSL_CERT_DIR and SSL_CERT_FILE. Does this not work?

Because this is a global variable, while sing-box and cronet-go are instance-based recyclable structures, this can cause race conditions. Additionally, sing-box has a feature to set a global list of trusted CA certificates, which can be implemented in naive outbound. (https://sing-box.sagernet.org/configuration/certificate/)

[klzgrad]

Add custom TCP dialer API for connection interception
This enables use cases like routing connections through external proxies or custom network stacks.

I can't imagine an example of this.

[klzgrad]

SagerNet/naiveproxy@dc3b465 Add request-level QUIC forcing mechanism: is this necessary? There is already way to force quic on origin basis.

[nekohasekai]

For locally initiated connections, sing-box handles the connection configuration (connecting to the correct upstream network interface, instead of the transparent proxy configured by sing-box; configuring conn marks; and even configuring connections to different networks in the GUI client), see https://sing-box.sagernet.org/configuration/route/. Additionally, sing-box supports detours, allowing connections to naive servers via other outbound routes. This is extremely useful in restricted network environments, such as a corporate network where access is only possible through a specific HTTP or Socks proxy.

[nekohasekai]

SagerNet/naiveproxy@dc3b465 Add request-level QUIC forcing mechanism: is this necessary? There is already way to force quic on origin basis.

SetQuicEnabled does not enforce QUIC. When QUIC fails, it falls back to HTTP/2. I think that when a user configures to use QUIC, it is expected that if an error occurs, it will be exposed directly, rather than masking the error and using HTTP/2. Naive itself seems to be doing the same thing.

[nekohasekai]

I'm currently organizing the cronet configuration changes I've made, dividing them into porting and compile fixes for cronet, and new features I've added. The latter part doesn't seem to need to be committed to naiveproxy, as these are features only required by cronet-go.

It's currently at

https://github.com/SagerNet/naiveproxy/commits/cronet-go-dev/

Next, I will analyze and try removing some of the changes in the first part to verify if they're necessary, or you can continue reviewing.