Add TLS support for incoming MCP server connections#8
Conversation
📝 WalkthroughWalkthroughThis PR adds optional incoming TLS configuration: a new ChangesTLS Server Support
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 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 `@src/rhos_ls_mcps/main.py`:
- Around line 112-118: When config.tls.ssl_ca_certs is provided we currently add
it to ssl_kwargs but never set ssl_cert_reqs, so mutual TLS is not enforced;
update the block that builds ssl_kwargs (the code that reads
config.tls.ssl_certfile, ssl_keyfile, ssl_keyfile_password, ssl_ca_certs) to
also set ssl_kwargs["ssl_cert_reqs"] = ssl.CERT_REQUIRED whenever
config.tls.ssl_ca_certs is present (import ssl if not already), ensuring that
ssl_ca_certs + ssl_cert_reqs=ssl.CERT_REQUIRED are both set to enable mTLS
enforcement.
🪄 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
Run ID: 9795455a-7cd3-4ddc-aa00-469a1658b2a6
📒 Files selected for processing (3)
config.yaml.samplesrc/rhos_ls_mcps/main.pysrc/rhos_ls_mcps/settings.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/rhos_ls_mcps/main.py`:
- Around line 112-121: The current TLS handling builds ssl_kwargs only when
config.tls.ssl_certfile is set, so if operators set ssl_keyfile or ssl_ca_certs
without ssl_certfile the server silently starts without TLS; validate the TLS
config and fail fast by adding explicit checks (ideally in TLSSettings
validation) to detect partial configurations: when any of
config.tls.ssl_keyfile, config.tls.ssl_keyfile_password, or
config.tls.ssl_ca_certs is provided but ssl_certfile is missing (or other
invalid combinations) raise an error and do not start, and/or perform the checks
before constructing ssl_kwargs in main.py (references: ssl_kwargs,
config.tls.ssl_certfile, config.tls.ssl_keyfile,
config.tls.ssl_keyfile_password, config.tls.ssl_ca_certs, TLSSettings).
In `@src/rhos_ls_mcps/settings.py`:
- Around line 68-70: The README docs still show the old default for
allowed_origins; update any documentation that mentions the default origins to
match the new runtime default in the settings (the allowed_origins Field now
defaults to ["http://*:*", "https://*:*"]). Search for references to
allowed_origins in docs/README and examples and change the listed default
origins to include both http and https, and ensure any explanatory text reflects
that both schemes are permitted by default.
🪄 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
Run ID: bbed3c17-2895-46b0-be8a-c5e37eed2690
📒 Files selected for processing (3)
config.yaml.samplesrc/rhos_ls_mcps/main.pysrc/rhos_ls_mcps/settings.py
✅ Files skipped from review due to trivial changes (1)
- config.yaml.sample
umago
left a comment
umago
left a comment
There was a problem hiding this comment.
Code looks good. I would just implement the suggestion from coderabbitai. It's a quick win and feels right to address
There was a problem hiding this comment.
🧹 Nitpick comments (1)
README.md (1)
74-104: ⚡ Quick winMissing documentation for the new TLS configuration section.
The README documents four configuration sections (General, MCP Security, OpenStack, OpenShift), but the PR introduces a new top-level
tls: TLSSettingsfield in the Settings model (as shown in the relevant code snippets) with optional TLS settings for incoming HTTPS support. This configuration section should be documented in the README alongside the other sections for discoverability and completeness.Based on the settings validation, the TLS section should document:
ssl_certfile: Path to SSL certificate file (required if other TLS fields are set)ssl_keyfile: Path to SSL private key file (optional)ssl_keyfile_password: Password for encrypted SSL key file (optional)ssl_ca_certs: Path to CA certs file for mutual TLS (optional)This mirrors the sample configuration that is documented elsewhere but should be reflected here.
🤖 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 `@README.md` around lines 74 - 104, The README is missing documentation for the new TLS configuration section that was added to support HTTPS. Add a new documentation section titled "TLS" (or similar) in the README after the General section and before the MCP Security section that documents the following TLS fields: ssl_certfile (path to SSL certificate file, required if other TLS fields are set), ssl_keyfile (path to SSL private key file, optional), ssl_keyfile_password (password for encrypted SSL key file, optional), and ssl_ca_certs (path to CA certs file for mutual TLS, optional). Each field should follow the same documentation format as the other configuration options with descriptions and default values where applicable.
🤖 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 `@README.md`:
- Around line 74-104: The README is missing documentation for the new TLS
configuration section that was added to support HTTPS. Add a new documentation
section titled "TLS" (or similar) in the README after the General section and
before the MCP Security section that documents the following TLS fields:
ssl_certfile (path to SSL certificate file, required if other TLS fields are
set), ssl_keyfile (path to SSL private key file, optional), ssl_keyfile_password
(password for encrypted SSL key file, optional), and ssl_ca_certs (path to CA
certs file for mutual TLS, optional). Each field should follow the same
documentation format as the other configuration options with descriptions and
default values where applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca9324d7-6be1-4479-8900-879793dd7b84
📒 Files selected for processing (4)
README.mdconfig.yaml.samplesrc/rhos_ls_mcps/main.pysrc/rhos_ls_mcps/settings.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/rhos_ls_mcps/settings.py
- src/rhos_ls_mcps/main.py
- config.yaml.sample
|
PR is been open for awhile, going ahead with it. Small change and looks good! |
3 participants
Summary by CodeRabbit