Support TLS on the xmlrpc server and client - #189
Conversation
939b092 to
f9bdc3f
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@coderabbitai ptal |
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughChangesTLS support is added to the XML-RPC server and client, including certificate configuration, client authentication, TLS error handling, documentation, and integration tests covering accepted and rejected connections. XML-RPC TLS
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerProxy
participant XMLRPCServer
participant ServerTLS
Client->>ServerProxy: create with SSL context
ServerProxy->>XMLRPCServer: send XML-RPC request over TLS
XMLRPCServer->>ServerTLS: use configured TLS-wrapped socket
ServerTLS-->>XMLRPCServer: authenticate server and client certificates
XMLRPCServer-->>ServerProxy: return XML-RPC response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
config/server.yaml (1)
23-31: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocument that omitting
ssl_clientsaccepts any client.With
ssl_certfileset butssl_clientsunset,verify_modestaysCERT_NONE, so the connection is encrypted but unauthenticated. Worth stating here since it is the security-relevant default.📝 Suggested wording
-# Certificate and private key for the xmlrpc server. When ssl_certfile is not -# set, the server speaks HTTP. When ssl_keyfile is not -# set, the private key is expected in the ssl_certfile itself. +# Certificate and private key for the xmlrpc server. When ssl_certfile is not +# set, the server speaks HTTP. When ssl_keyfile is not set, the private key is +# expected in the ssl_certfile itself. `#ssl_certfile`: '/etc/resallocserver/server.crt' `#ssl_keyfile`: '/etc/resallocserver/server.key' # PEM file with the concatenated certificates of all the clients allowed to -# connect. Requires ssl_certfile. +# connect. Requires ssl_certfile. When unset, the traffic is encrypted but +# any client is accepted (no client certificate is requested). `#ssl_clients`: '/etc/resallocserver/clients.pem'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/server.yaml` around lines 23 - 31, Update the SSL configuration comments near ssl_clients to state that leaving ssl_clients unset accepts any client: connections remain encrypted when ssl_certfile is configured, but client authentication is disabled because verify_mode remains CERT_NONE.resallocserver/tls.py (1)
56-73: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueTLS handshake runs on the accept loop, not on the worker threads.
Wrapping the listening socket means
accept()performs the handshake inline (do_handshake_on_connect=True), so theserve_forever()thread blocks for the duration of every handshake. A slow or stalled client can therefore delay all other connections even thoughdaemon_threadsis set. Handshake errors themselves are fine —ssl.SSLErrorsubclassesOSErrorandsocketserverswallows those in_handle_request_noblock, which matches whatshelltests/tests/tls.shobserves.If you want to keep the accept loop responsive, set a socket timeout / defer the handshake to the request thread; otherwise this is acceptable for the expected client count.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resallocserver/tls.py` around lines 56 - 73, The build_tls_socket method currently performs TLS handshakes during accept, blocking the server loop on slow clients. Configure the wrapped socket to use a finite timeout or defer the handshake until request handling so serve_forever remains responsive; preserve the existing plain-socket behavior when TLS is disabled.resalloc/client.py (1)
43-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse exception chaining instead of disabling the lint in both TLS error paths. Both sites re-raise as
ResallocClientExceptionwhile suppressingraise-missing-from, discarding the originalssl.SSLErrorcontext that makes TLS failures diagnosable. The project is Python 3 only (TEST_PYTHONS := python3), soraise ... fromis available and also clears the Ruff B904 warnings.
resalloc/client.py#L43-L44: drop the pylint disable and useraise ResallocClientException(...) from err.resalloc/client.py#L66-L68: drop the pylint disable and useraise ResallocClientException(...) from ssl_err.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resalloc/client.py` around lines 43 - 44, Update both TLS exception handlers in resalloc/client.py at lines 43-44 and 66-68: remove the pylint disables and chain each ResallocClientException with its caught exception using “from err” in the first path and “from ssl_err” in the second, preserving the existing messages.Source: Linters/SAST tools
shelltests/tests/tls.sh (1)
167-169: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFixed 20s
timeoutalways burns the full 20 seconds.The misconfigured server never exits on its own, so
timeoutmust fire every time — repeated for each python/database combination in the matrix. The warning is written during startup, so polling the log and killing early is equivalent and much faster.⏱️ Proposed fix
-CONFIG_DIR=$WORKDIR/bad-etc timeout 20 "$SERVER_BIN" &>/dev/null || : -grep -q "ssl_clients is set but ssl_certfile is not" "$badlog/main.log" \ - || fatal "no warning about ssl_clients without ssl_certfile" +CONFIG_DIR=$WORKDIR/bad-etc "$SERVER_BIN" &>/dev/null & +bad_server_pid=$! +cleanup_actions+=( "kill $bad_server_pid 2>/dev/null" ) +counter=20 +while ! grep -q "ssl_clients is set but ssl_certfile is not" \ + "$badlog/main.log" 2>/dev/null; do + counter=$(( counter - 1 )) + test $counter -gt 0 || fatal "no warning about ssl_clients without ssl_certfile" + sleep 1 +done +kill "$bad_server_pid" 2>/dev/null || :🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shelltests/tests/tls.sh` around lines 167 - 169, Replace the fixed 20-second timeout around the misconfigured SERVER_BIN startup with polling for the expected warning in badlog/main.log, then terminate the server as soon as the warning appears. Preserve the existing fatal failure when the warning is not observed and ensure the background server process is cleaned up after polling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shelltests/tests/tls.sh`:
- Around line 115-119: Add a short sleep inside the startup retry loop
surrounding maint resource-list, after each failed attempt or counter decrement,
so retries are paced and the full timeout remains available for server startup.
---
Nitpick comments:
In `@config/server.yaml`:
- Around line 23-31: Update the SSL configuration comments near ssl_clients to
state that leaving ssl_clients unset accepts any client: connections remain
encrypted when ssl_certfile is configured, but client authentication is disabled
because verify_mode remains CERT_NONE.
In `@resalloc/client.py`:
- Around line 43-44: Update both TLS exception handlers in resalloc/client.py at
lines 43-44 and 66-68: remove the pylint disables and chain each
ResallocClientException with its caught exception using “from err” in the first
path and “from ssl_err” in the second, preserving the existing messages.
In `@resallocserver/tls.py`:
- Around line 56-73: The build_tls_socket method currently performs TLS
handshakes during accept, blocking the server loop on slow clients. Configure
the wrapped socket to use a finite timeout or defer the handshake until request
handling so serve_forever remains responsive; preserve the existing plain-socket
behavior when TLS is disabled.
In `@shelltests/tests/tls.sh`:
- Around line 167-169: Replace the fixed 20-second timeout around the
misconfigured SERVER_BIN startup with polling for the expected warning in
badlog/main.log, then terminate the server as soon as the warning appears.
Preserve the existing fatal failure when the warning is not observed and ensure
the background server process is cleaned up after polling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 21b7c6f9-380c-4b96-905d-cabc6bdbc172
📒 Files selected for processing (6)
Makefileconfig/server.yamlresalloc/client.pyresallocserver/main.pyresallocserver/tls.pyshelltests/tests/tls.sh
| counter=30 | ||
| while ! maint resource-list &>/dev/null; do | ||
| counter=$(( counter - 1 )) | ||
| test $counter -gt 0 | ||
| done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How do the other shelltests wait for the server, and how does maint connect?
fd -t f . shelltests | head -50
rg -n -C3 'maint\s*\(\)|maint resource-list|MAINT_BIN' shelltestsRepository: praiskup/resalloc
Length of output: 10528
🏁 Script executed:
#!/bin/bash
sed -n '1,180p' shelltests/tests/tls.sh
printf '\n--- testlib ---\n'
sed -n '1,120p' shelltests/testlibRepository: praiskup/resalloc
Length of output: 8442
🏁 Script executed:
#!/bin/bash
rg -n -C2 'resalloc-maint|--connection|ssl_certfile|ssl_clients|CONFIG_DIR|resource-list' -g '!*.o' -g '!*.a' .Repository: praiskup/resalloc
Length of output: 20744
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' bin/resalloc-maint
printf '\n--- maintainer ---\n'
rg -n -C2 'class Maintainer|def resource_list|def ticket_list|requests|http|xmlrpc|sqlite|sqlalchemy' resallocserver tests bin/resalloc-maintRepository: praiskup/resalloc
Length of output: 16283
Add a sleep to the startup wait loop.
It retries as fast as possible, so the 30-iteration timeout can expire before the server finishes coming up.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shelltests/tests/tls.sh` around lines 115 - 119, Add a short sleep inside the
startup retry loop surrounding maint resource-list, after each failed attempt or
counter decrement, so retries are paced and the full timeout remains available
for server startup.
Source: Linters/SAST tools
f9bdc3f to
5ed9eab
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
resalloc/client.py (1)
41-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the original TLS exception causes.
Both wrappers suppress exception chaining, making TLS failures harder to diagnose.
resalloc/client.py#L41-L44: raiseResallocClientExceptionfromerr.resalloc/client.py#L64-L68: raiseResallocClientExceptionfromssl_err.Proposed fix
- raise ResallocClientException( - "Invalid client TLS configuration: {0}".format(err)) + raise ResallocClientException( + "Invalid client TLS configuration: {0}".format(err)) from err ... - raise ResallocClientException( - "TLS error while talking to the server: {0}".format( - ssl_err)) + raise ResallocClientException( + "TLS error while talking to the server: {0}".format( + ssl_err)) from ssl_err🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resalloc/client.py` around lines 41 - 44, Preserve exception chaining in both TLS error wrappers: update the ResallocClientException raises in resalloc/client.py lines 41-44 and 64-68 to explicitly raise from the caught exceptions err and ssl_err respectively, removing the suppression workaround while keeping the existing messages and exception handling.Source: Linters/SAST tools
shelltests/tests/tls.sh (1)
167-169: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the unconditional 20-second wait.
The server is expected to remain running, so
timeout 20delays this test by the full 20 seconds before checking the log. Start it in the background, poll for the warning with a bounded retry loop, then terminate and reap it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shelltests/tests/tls.sh` around lines 167 - 169, Update the server startup check around CONFIG_DIR and badlog/main.log to remove the unconditional timeout wait: launch SERVER_BIN in the background, poll for the expected “ssl_clients is set but ssl_certfile is not” warning using a bounded retry loop, then terminate and reap the server before asserting failure if the warning was not observed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@resalloc/client.py`:
- Around line 41-44: Preserve exception chaining in both TLS error wrappers:
update the ResallocClientException raises in resalloc/client.py lines 41-44 and
64-68 to explicitly raise from the caught exceptions err and ssl_err
respectively, removing the suppression workaround while keeping the existing
messages and exception handling.
In `@shelltests/tests/tls.sh`:
- Around line 167-169: Update the server startup check around CONFIG_DIR and
badlog/main.log to remove the unconditional timeout wait: launch SERVER_BIN in
the background, poll for the expected “ssl_clients is set but ssl_certfile is
not” warning using a bounded retry loop, then terminate and reap the server
before asserting failure if the warning was not observed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b1038a2-584c-4954-a7f0-e5f75a47e463
📒 Files selected for processing (6)
Makefileconfig/server.yamlresalloc/client.pyresallocserver/main.pyresallocserver/tls.pyshelltests/tests/tls.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- config/server.yaml
- Makefile
- resallocserver/tls.py
- resallocserver/main.py
|
ptal @praiskup |
3 participants
Relates #7
Added 3 new options in
config/server.yaml:As per #7 (comment), no config for client is implemented in this PR.
Both sides use
ssl.create_default_context(which is the recommended way), more about it hereSummary by CodeRabbit