-
-
Notifications
You must be signed in to change notification settings - Fork 76
feat: add TLS and RFC 1929 auth to KumoProxy #459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
wez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this!
I have some comments about code duplication and suggestions for how to avoid it.
There's a decent amount of work to do to get things into the right shape for this to fit the kumo-way of doing things and be in better shape to maintain in the long term.
One thing that is missing here is integration tests of the new proxy functionality; since we don't have any already, that's not a harsh critique of this PR, but this PR does increase the complexity enough that we really should have integration tests before we go forwards with this.
One of my suggestions is to adopt kumo-server-common and its lua config stuff. A side-effect of that is that it helps to facilitate integration testing. If you follow the pattern that we use in the http and esmtp listeners of logging the endpoint that we're listening to:
kumomta/crates/kumod/src/smtp_server.rs
Lines 684 to 692 in c5266e2
| pub async fn run(self) -> anyhow::Result<()> { | |
| let connection_gauge = connection_gauge(); | |
| let listener = TcpListener::bind(&self.listen) | |
| .await | |
| .with_context(|| format!("failed to bind to {}", self.listen))?; | |
| let addr = listener.local_addr()?; | |
| tracing::info!("smtp listener on {addr:?}"); |
then you can clone crates/integration-tests/src/tsa.rs (which is a test fixture for an isolated tsa-daemon) as crates/integration-tests/src/proxy.rs and this region of code:
kumomta/crates/integration-tests/src/tsa.rs
Lines 98 to 107 in c5266e2
| if line.contains("listener on") { | |
| let mut fields: Vec<&str> = line.trim().split(' ').collect(); | |
| while fields.len() > 4 { | |
| fields.remove(0); | |
| } | |
| let proto = fields[0]; | |
| let addr = fields[3]; | |
| let addr: SocketAddr = addr.parse()?; | |
| listeners.insert(proto.to_string(), addr); | |
| } |
will detect the listener port. That allows defining integration tests that listen on port 0 to have the kernel pick an isolated port for the purpose of running parallel/concurrent integration tests.
There's more that I could say about integration testing, but I suggest looking at all the other points first, then we can regroup and discuss testing in a bit more detail.
I know that what I've mentioned here in this review is a decent amount of additional work, but these things are necessary in order for things to be maintainable and feel more cohesive with the rest of the kumo functionality!
| @@ -0,0 +1,104 @@ | |||
| //! TLS configuration helpers for the proxy server. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by switching to using kumo-server-common as proposed above, we can then simply reference its make_server_config function directly rather than copying the code into this file.
crates/kumod/src/egress_source.rs
Outdated
| anyhow::ensure!( | ||
| pass.len() < 256, | ||
| "username is too long for SOCKS5 protocol" | ||
| "password is too long for SOCKS5 protocol" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
144be61 to
8855270
Compare
|
Hey @wez , thanks for the thorough review. Pushed updates addressing everything:
Used --policy instead of --proxy-config to match kumod/tsa-daemon conventions. Integration tests currently just verify startup. Let me know if you want more coverage (actual SOCKS5 connections, auth flows, etc.) before merging. |
wez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this is really shaping up nicely!
|
Hey @wez, Pushed the fixes. Renamed things as requested, swapped --policy to --proxy-config with proper conflicts_with, ditched the TLS CLI options for temp Lua config generation, used declare_event!, fixed the auth flow, and added a bunch of e2e tests. All green. |
wez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work on this, I'm excited to see this get merged!
It took me a little while to get back to you on this next round of review because I was building out the new ACL system with this in mind and wanted to have that in a better state before suggesting here how to integrate those changes here.
4ed1b0f to
0698f81
Compare
|
Hey @wez, I've addressed all your feedback from today's review: Switched to AuthInfo return type with the new ACL system integration I squashed it down to 2 clean commits. Ready for another look when you get a chance! |
wez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think we're almost there!
0698f81 to
8f4af96
Compare
|
Hey @wez , thanks for the thorough review! I've addressed all your feedback: Bumped the test timeouts to 50s All tests passing. Let me know if there's anything else. |
Move TLS configuration and async stream traits from rfc5321 into a new shared kumo-tls-helper crate for reuse in proxy-server and other crates. Refs KumoCorp#451
Add TLS encryption and username/password authentication support for the KumoProxy SOCKS5 server, with full Lua configuration capabilities. Proxy Server Changes: - Add kumo.start_proxy_listener() Lua function with TLS support - Add proxy_server_auth_rfc1929 event for Lua-based auth validation - Return AuthInfo from auth for ACL system integration - Support optional and required authentication modes - Maintain backwards-compatible legacy CLI mode (--listen, --timeout-seconds) Client Changes (kumod): - Add socks5_proxy_username/password fields to egress source - Add socks5_proxy_tls options for connecting to TLS-enabled proxies Breaking Changes: - Cache renamed from rfc5321_rustls_config to rustls_client_config Fixes KumoCorp#451 Closes: 459
We must only config.put in the success case, otherwise we might leave a broken lua context in the cache refs: KumoCorp#459
8f4af96 to
3e535eb
Compare
wez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I noticed when checking it out and building locally that splice(2) wasn't hooked up, so I added some code that should work to enable that again. It compiles, but I haven't explicitly verified that it it is operating correctly. I force-pushed my updates to your branch.
Could you take a look to verify that splice is operating correctly? This doesn't strictly require an integration test (although I would appreciate it if you feel like making one!). To verify I suppose that you could inspect the log output to see that splice is being used, and then also check to make sure that we're not leaking any additional file descriptors while the service is running and handling connections. I would suggest looking at lsof -np $(pgrep proxy-server) and comparing splice to no-splice mode.
With that verified, the only thing remaining is to write up a doc page for the new kumo.start_proxy_listener function, then add an entry to the changelog for it!
Thank you again for your work and attention here!
|
Hey @wez, Added the docs and changelog entry. Verified splice logic looks correct. Ready for merge when you get a chance! Kevin |
Summary
Fixes #451
Changes
Proxy Server (crates/proxy-server):
--tls-certificate,--tls-private-keyoptions for server TLS--tls-client-caoption for mTLS client verification--auth-fileoption for TOML-based user credentialsKumoMTA Client (crates/kumod):
socks5_proxy_usernameandsocks5_proxy_passwordfieldssocks5_proxy_tls,socks5_proxy_tls_ca,socks5_proxy_tls_insecurefieldsDocumentation:
make_egress_sourceoptionsTest plan