Skip to content
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

proxy: backend TLS support #1140

Merged
merged 1 commit into from
May 24, 2024
Merged

proxy: backend TLS support #1140

merged 1 commit into from
May 24, 2024

Conversation

dormando
Copy link
Member

@dormando dormando commented May 21, 2024

WIP. Connects, handshakes, and routes requests as of this commit. Needs further work before mergeable.

TODO:

  • Flesh out error handling.
  • parameter for write buffer size de-scoping
  • Reduce the size of the code change in proxy_network.c by refactoring
  • Configure option for enabling support instead of hard coding
  • proxy_tls.h to blank out functions if TLS not enabled at compile time
  • enum for common return codes, vs the 0/-1/-2 currently in use
  • add some specific counters for handshake/tls errors de-scoping
  • SSL_shutdown and SSL_free() calls from gc are missing.
  • Move some initialization code to config thread.
  • Per-backend TLS option
  • Ensure all new TODO/FIXME notes are dealt with.

TODO MAYBE:

  • Ship error detail to proxyevents log stream (not useful right now, may punt to next PR)

ETA: maybe a week? It does currently work, so it might not take long to finish.

GOALS:

  • Enable TLS on a global or per-backend basis. Propagate errors properly.
  • Minimize #ifdef soup and testing burden by using a shim for accessing OpenSSL and leaving some TLS related code always compiled in. The shim nulls itself out if TLS support is not compiled in.

FOR FUTURE PR's:

  • Load and reload client certificates/keys
  • Peer certificate validation
  • sessions / session tickets
  • maybe: sidethread for handshake calcs (if not reusing a session)
  • io_uring preparedness.

I'm separating certificate work out of this PR so I can keep the change focused for initial validation. Tired of trying to waterfall every individual feature.

NOTES:
There is a companion PR to this work over in mcshredder: memcached/mcshredder#5 - this is a much more complicated approach that would be necessary to add io_uring back to the proxy. Since we're not currently using io_uring I used a simpler approach that lets OpenSSL handle the socket fd and run syscalls itself.

Since proxy_network.c was already structured to abstract out the read and write socket calls into just two small functions, this allowed the PR to be much less invasive than it would be otherwise.

However, if you are naive you might think: "why aren't you using a function ptr for read/write and modify even less?" - well, OpenSSL is way more complicated than that and we need to change how we interact with the event loop based on what the library returns vs what any underlying socket thinks. It sucks.

@dormando dormando added WIP proxy worklogs and issues related to proxy labels May 21, 2024
@dormando
Copy link
Member Author

dormando commented May 22, 2024

Debating... looks like I could really cut down on the dedicated code by using a few function pointers, but:

  • would require deeper change to existing code. (ie; making functions for the main recv/writev calls)
  • they have to be stored/fetched from somewhere.

Not sure it's worth it vs trying to slim down the existing functions slightly and just accepting some small code duplication with the benefit of not potentially breaking anything.

Should remove the duplication from the _set_event stuff though.

edit: new plan, maybe: do some other work and come back to refactoring. since new connection related stuff isn't a hot path I could merge those codepaths with an if/else check for tls specific code. then leave the split stuff or use maybe one function ptr for the hot paths.

@dormando
Copy link
Member Author

might leave the error handling with correct structure but not very communicative. until we do certificates and peer checks there aren't many reasons outside of network for things to fail... that would significantly cut down the time to merge this initial pass. it already has more correct error handling than the main mc code :(

@dormando
Copy link
Member Author

last things I'm considering doing is some missing init code... then just run tests and merge as-is with experimental tags.

@dormando
Copy link
Member Author

Fixing this last bit sucks a little...

Either need to add a mcp.tls_init() func (which could optionally take certs and shit later) that must be called before any backends are created, or a -o commandline option that does similar. I can't run the init after mcp_config_pools is run since that creates backends inline.

guess I'll wrap this up tomorrow at this point.

@dormando dormando force-pushed the proxy_tls branch 2 times, most recently from 789b254 to 5e8fac8 Compare May 23, 2024 23:57
@dormando dormando removed the WIP label May 23, 2024
@dormando
Copy link
Member Author

this is as far as I care to take this right now; will leave it experimental and merge. it may or may not work.

Has not been extensively tested or validated under benchmarks. Please
let us know if you intend to use the feature, but feel free to try it
out yourself since it will likely work.

To use, within `mcp_config_pools`:
mcp.init_tls() -- before making any backends
mcp.backend_use_tls(true)
or pass 'tls = true' as an argument to `mcp.backend`

Does not currently support client certificates or peer verification. Let
us know if you need this support and we will prioritize it.
@dormando dormando merged commit 8a9b709 into memcached:next May 24, 2024
1 check passed
@dormando dormando deleted the proxy_tls branch May 24, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/fixed for next proxy worklogs and issues related to proxy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant