LCORE-1253: Add networking configuration and TLS security profiles#1405
Closed
max-svistunov wants to merge 3 commits intolightspeed-core:mainfrom
Closed
LCORE-1253: Add networking configuration and TLS security profiles#1405max-svistunov wants to merge 3 commits intolightspeed-core:mainfrom
max-svistunov wants to merge 3 commits intolightspeed-core:mainfrom
Conversation
Introduce infrastructure for configurable proxy, TLS, and CA certificate settings for all outgoing connections from the Lightspeed Stack. New source modules: - src/utils/tls.py: OpenShift-compatible TLS security profile definitions (OldType, IntermediateType, ModernType, Custom) with cipher suites, protocol version mappings, and helper functions. - src/utils/ssl_context.py: SSL context builder that constructs ssl.SSLContext from TLS profile settings with proper hostname verification, cipher filtering (TLS 1.3 auto-negotiation aware), and optional custom CA certificate loading. - src/utils/certificates.py: CA certificate merging utility that combines the system trust store (certifi) with user-provided extra CA certs into a single bundle, with error handling and duplicate detection. New configuration models in src/models/config.py: - TLSSecurityProfile: Pydantic model with profile type, min TLS version, cipher list, CA cert path, and skip-verification flag. Validates profile types and ciphers against OpenShift spec. - ProxyConfiguration: HTTP/HTTPS proxy URLs and no-proxy bypass list. - NetworkingConfiguration: Top-level container for proxy, TLS, extra CA, and certificate directory settings. The 'networking' field is added as an optional top-level section in the Configuration class, defaulting to None for full backward compatibility. Improvements over the road-core (service) implementation: - Hostname verification stays enabled with custom CA certs (road-core silently disabled it). - TLS 1.3 ciphers are correctly identified and filtered from set_ciphers() calls (they are auto-negotiated by OpenSSL). - Cipher separator uses colon (OpenSSL standard) instead of comma. - Certificate merging returns status enum and handles errors per-cert instead of crashing on the first malformed file. - Certificate output directory is configurable (not hardcoded to /tmp). - Cipher lists use immutable tuples instead of mutable lists. - SSLContext construction is centralized in one function instead of duplicated across three files. Unit tests: 75 new tests covering all utilities and config models. Documentation: Updated docs/config.md with new configuration sections.
Add @field_validator on ProxyConfiguration.http_proxy and https_proxy that verifies URLs have a scheme and hostname at config load time. Catches typos like "htpp://proxy:8080" early. Unit tests for valid and invalid proxy URLs.
Remove unused logger import from tls.py (no logging in this module). Remove unused DEFAULT_TLS_PROFILE and DEFAULT_MIN_TLS_VERSION constants from constants.py (code uses enums directly).
Contributor
WalkthroughThis pull request introduces networking configuration support for outgoing connections. It adds configuration models for proxy settings, TLS security profiles, and CA certificate management, along with utility modules for SSL context construction, TLS profile handling, and custom CA bundle generation. Documentation and comprehensive unit tests accompany all changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/config.md`:
- Around line 385-401: Update the NetworkingConfiguration docs to explicitly
state this section is schema-only and not yet enforced: note that the current
implementation only introduces the schema and helper functions (referencing
NetworkingConfiguration and Configuration.networking) and that enforcement of
proxy, tls_security_profile, extra_ca, and certificate_directory will be
implemented in follow-up PRs; add a brief sentence directing operators not to
rely on these settings for production until those follow-ups land.
In `@src/models/config.py`:
- Around line 169-198: The validator currently only ensures min_tls_version is a
known TLSProtocolVersion; update the validation in the Config model (the block
handling self.profile_type and self.min_tls_version) to also reject any
min_tls_version that would weaken a non-CUSTOM TLSProfiles profile: when profile
!= TLSProfiles.CUSTOM_TYPE, compute the profile's minimum allowed
TLSProtocolVersion (based on the profile enum or its known mapping used by
build_ssl_context()) and raise ValueError if
TLSProtocolVersion(self.min_tls_version) is lower than that floor, with a clear
message referencing the profile and allowed minimum; keep existing unknown-value
checks and the custom-profile bypass intact.
- Around line 254-260: The proxy URL validation currently checks parsed.scheme
and parsed.hostname but ignores malformed ports like "http://proxy:not-a-port";
update the validator that parses v (the code using urlparse and variable parsed)
to access parsed.port inside a try/except ValueError and raise a ValueError with
a clear message if parsed.port access raises (or is None when a port was present
but invalid), ensuring invalid port strings are rejected at config load time
instead of at request time.
In `@src/utils/certificates.py`:
- Around line 107-110: The code copies the system certifi bundle using
certifi_location and shutil.copyfile which can raise filesystem errors; wrap the
copy call (the shutil.copyfile(...) call that uses certifi_location and
destination) in a try/except that catches OSError (or Exception) and on failure
logs the error via logger (including the exception message and the destination)
and returns None (or otherwise signals unusable output) instead of letting the
exception propagate; ensure the function that invokes this code returns the
documented None for unusable locations.
- Around line 65-67: When appending new_cert_data to the certificate store at
store_path, ensure a PEM separator is inserted if the previous content doesn't
end with a newline: open the file for read/append (or seek to end), check the
last byte (or final character) of the existing file, and if it is not a newline
write a single b'\n' (or '\n' for text mode) before calling
store_file.write(new_cert_data); update the try block around
store_path/new_cert_data/write to perform this guard so two PEM blocks are never
concatenated.
In `@src/utils/ssl_context.py`:
- Around line 65-78: The code silently falls back to OpenSSL defaults when
context.set_ciphers(tls12_ciphers) raises ssl.SSLError, which is unsafe for
explicit user overrides; update the block where cipher_string is computed
(resolve_ciphers) and filtered (filter_tls12_ciphers) so that on ssl.SSLError
you re-raise the exception when the ciphers argument was user-supplied (e.g.,
profile_type indicates a custom override or ciphers param is non-default), but
keep the existing warning-and-fallback behavior only when profile_type
corresponds to built-in profiles/defaults; use the presence/value of
profile_type or the original ciphers argument to differentiate and either
logger.warning + continue for built-ins or raise the caught ssl.SSLError for
explicit user policies.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b63e242d-982b-407c-b35b-ddfce3233fdc
📒 Files selected for processing (10)
docs/config.mdsrc/models/config.pysrc/utils/certificates.pysrc/utils/ssl_context.pysrc/utils/tls.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_tls_security_profile.pytests/unit/utils/test_certificates.pytests/unit/utils/test_ssl_context.pytests/unit/utils/test_tls.py
Comment on lines
+385
to
+401
| ## NetworkingConfiguration | ||
|
|
||
|
|
||
| Networking configuration for outgoing connections. | ||
|
|
||
| Central configuration for all outgoing network connections from the | ||
| Lightspeed Stack, including proxy settings, TLS security profiles, | ||
| and custom CA certificates. These settings apply to connections to | ||
| Llama Stack, MCP servers, Splunk, and other external services. | ||
|
|
||
|
|
||
| | Field | Type | Description | | ||
| |-------|------|-------------| | ||
| | proxy | | HTTP proxy settings for outgoing connections. | | ||
| | tls_security_profile | | TLS security settings for outgoing connections. | | ||
| | extra_ca | array | List of paths to additional CA certificate files to trust. | | ||
| | certificate_directory | string | Directory where the merged CA bundle will be stored. | |
Contributor
There was a problem hiding this comment.
Document this section as schema-only for now.
These paragraphs read as if networking already affects outbound traffic, but the provided implementation only adds schema and helper functions; no production call site in this PR consumes Configuration.networking yet. Please call out that enforcement lands in the follow-up PRs so operators do not rely on inert settings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/config.md` around lines 385 - 401, Update the NetworkingConfiguration
docs to explicitly state this section is schema-only and not yet enforced: note
that the current implementation only introduces the schema and helper functions
(referencing NetworkingConfiguration and Configuration.networking) and that
enforcement of proxy, tls_security_profile, extra_ca, and certificate_directory
will be implemented in follow-up PRs; add a brief sentence directing operators
not to rely on these settings for production until those follow-ups land.
Comment on lines
+169
to
+198
| if self.profile_type is not None: | ||
| try: | ||
| profile = TLSProfiles(self.profile_type) | ||
| except ValueError as e: | ||
| valid = [p.value for p in TLSProfiles] | ||
| raise ValueError( | ||
| f"Invalid TLS profile type '{self.profile_type}'. " | ||
| f"Valid types: {valid}" | ||
| ) from e | ||
|
|
||
| # Validate ciphers against profile when not Custom | ||
| if self.ciphers is not None and profile != TLSProfiles.CUSTOM_TYPE: | ||
| supported = TLS_CIPHERS.get(profile, ()) | ||
| for cipher in self.ciphers: | ||
| if cipher not in supported: | ||
| raise ValueError( | ||
| f"Unsupported cipher '{cipher}' for profile " | ||
| f"'{self.profile_type}'. " | ||
| f"Use profile 'Custom' for arbitrary ciphers." | ||
| ) | ||
|
|
||
| if self.min_tls_version is not None: | ||
| try: | ||
| TLSProtocolVersion(self.min_tls_version) | ||
| except ValueError as e: | ||
| valid = [v.value for v in TLSProtocolVersion] | ||
| raise ValueError( | ||
| f"Invalid TLS version '{self.min_tls_version}'. " | ||
| f"Valid versions: {valid}" | ||
| ) from e |
Contributor
There was a problem hiding this comment.
Reject minTLSVersion values that weaken predefined profiles.
Lines 190-198 only check that the version name is recognized. ModernType + VersionTLS12 currently passes validation, but build_ssl_context() will then leave TLS 1.2 enabled and skip set_ciphers() because the Modern profile only contains TLS 1.3 suites. That silently broadens the policy to OpenSSL defaults. Enforce the profile floor for every non-Custom type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/models/config.py` around lines 169 - 198, The validator currently only
ensures min_tls_version is a known TLSProtocolVersion; update the validation in
the Config model (the block handling self.profile_type and self.min_tls_version)
to also reject any min_tls_version that would weaken a non-CUSTOM TLSProfiles
profile: when profile != TLSProfiles.CUSTOM_TYPE, compute the profile's minimum
allowed TLSProtocolVersion (based on the profile enum or its known mapping used
by build_ssl_context()) and raise ValueError if
TLSProtocolVersion(self.min_tls_version) is lower than that floor, with a clear
message referencing the profile and allowed minimum; keep existing unknown-value
checks and the custom-profile bypass intact.
Comment on lines
+254
to
+260
| parsed = urlparse(v) | ||
| if not parsed.scheme: | ||
| raise ValueError( | ||
| f"Proxy URL '{v}' is missing a scheme (e.g., http:// or https://)" | ||
| ) | ||
| if not parsed.hostname: | ||
| raise ValueError(f"Proxy URL '{v}' is missing a hostname") |
Contributor
There was a problem hiding this comment.
Force urlparse to validate the port.
http://proxy:not-a-port gets a scheme and hostname, so Lines 255-260 accept it today. Touch parsed.port in a try/except ValueError so malformed ports fail during config load instead of at request time.
✅ Suggested guard
parsed = urlparse(v)
if not parsed.scheme:
raise ValueError(
f"Proxy URL '{v}' is missing a scheme (e.g., http:// or https://)"
)
if not parsed.hostname:
raise ValueError(f"Proxy URL '{v}' is missing a hostname")
+ try:
+ _ = parsed.port
+ except ValueError as e:
+ raise ValueError(f"Proxy URL '{v}' has an invalid port") from e
return v🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/models/config.py` around lines 254 - 260, The proxy URL validation
currently checks parsed.scheme and parsed.hostname but ignores malformed ports
like "http://proxy:not-a-port"; update the validator that parses v (the code
using urlparse and variable parsed) to access parsed.port inside a try/except
ValueError and raise a ValueError with a clear message if parsed.port access
raises (or is None when a port was present but invalid), ensuring invalid port
strings are rejected at config load time instead of at request time.
Comment on lines
+65
to
+67
| try: | ||
| with open(store_path, "ab") as store_file: | ||
| store_file.write(new_cert_data) |
Contributor
There was a problem hiding this comment.
Add a PEM separator before appending new certificates.
If the source PEM does not end with \n, Line 67 can concatenate two PEM blocks into an invalid bundle. Normalize the boundary before writing.
✅ Suggested guard
try:
- with open(store_path, "ab") as store_file:
- store_file.write(new_cert_data)
+ append_data = (
+ new_cert_data if new_cert_data.endswith(b"\n") else new_cert_data + b"\n"
+ )
+ with open(store_path, "rb+") as store_file:
+ store_file.seek(0, 2)
+ if store_file.tell() > 0:
+ store_file.seek(-1, 2)
+ if store_file.read(1) != b"\n":
+ store_file.seek(0, 2)
+ store_file.write(b"\n")
+ store_file.seek(0, 2)
+ store_file.write(append_data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/certificates.py` around lines 65 - 67, When appending new_cert_data
to the certificate store at store_path, ensure a PEM separator is inserted if
the previous content doesn't end with a newline: open the file for read/append
(or seek to end), check the last byte (or final character) of the existing file,
and if it is not a newline write a single b'\n' (or '\n' for text mode) before
calling store_file.write(new_cert_data); update the try block around
store_path/new_cert_data/write to perform this guard so two PEM blocks are never
concatenated.
Comment on lines
+107
to
+110
| # Start with the system certifi bundle | ||
| certifi_location = certifi.where() | ||
| logger.info("Copying certifi bundle from %s to %s", certifi_location, destination) | ||
| shutil.copyfile(certifi_location, destination) |
Contributor
There was a problem hiding this comment.
Catch bundle creation failures here instead of leaking an exception.
Line 110 can still raise on filesystem errors, even though this helper documents None for unusable output locations. That turns a config problem into an unhandled startup failure.
✅ Suggested guard
certifi_location = certifi.where()
logger.info("Copying certifi bundle from %s to %s", certifi_location, destination)
- shutil.copyfile(certifi_location, destination)
+ try:
+ shutil.copyfile(certifi_location, destination)
+ except (OSError, shutil.SameFileError) as e:
+ logger.error("Failed to create CA bundle '%s': %s", destination, e)
+ return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Start with the system certifi bundle | |
| certifi_location = certifi.where() | |
| logger.info("Copying certifi bundle from %s to %s", certifi_location, destination) | |
| shutil.copyfile(certifi_location, destination) | |
| # Start with the system certifi bundle | |
| certifi_location = certifi.where() | |
| logger.info("Copying certifi bundle from %s to %s", certifi_location, destination) | |
| try: | |
| shutil.copyfile(certifi_location, destination) | |
| except (OSError, shutil.SameFileError) as e: | |
| logger.error("Failed to create CA bundle '%s': %s", destination, e) | |
| return None |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/certificates.py` around lines 107 - 110, The code copies the system
certifi bundle using certifi_location and shutil.copyfile which can raise
filesystem errors; wrap the copy call (the shutil.copyfile(...) call that uses
certifi_location and destination) in a try/except that catches OSError (or
Exception) and on failure logs the error via logger (including the exception
message and the destination) and returns None (or otherwise signals unusable
output) instead of letting the exception propagate; ensure the function that
invokes this code returns the documented None for unusable locations.
Comment on lines
+65
to
+78
| cipher_string = resolve_ciphers(ciphers, profile_type) | ||
| if cipher_string is not None: | ||
| # TLS 1.3 ciphers are auto-negotiated and cannot be set via | ||
| # set_ciphers(). Filter them out to avoid SSLError. | ||
| tls12_ciphers = filter_tls12_ciphers(cipher_string) | ||
| if tls12_ciphers: | ||
| try: | ||
| context.set_ciphers(tls12_ciphers) | ||
| logger.info("SSL context ciphers set: %s", tls12_ciphers) | ||
| except ssl.SSLError: | ||
| logger.warning( | ||
| "Could not set ciphers '%s'. Falling back to OpenSSL defaults.", | ||
| tls12_ciphers, | ||
| ) |
Contributor
There was a problem hiding this comment.
Don't silently ignore invalid custom cipher policies.
If context.set_ciphers() fails for a user-supplied ciphers list, Lines 74-78 fall back to OpenSSL defaults and may negotiate suites the operator explicitly tried to disable. Re-raise for explicit overrides; keep the warning-only fallback only for built-in profile defaults if you still want that behavior.
✅ Suggested guard
- except ssl.SSLError:
- logger.warning(
- "Could not set ciphers '%s'. Falling back to OpenSSL defaults.",
- tls12_ciphers,
- )
+ except ssl.SSLError:
+ if ciphers is not None:
+ raise
+ logger.warning(
+ "Could not set ciphers '%s'. Falling back to OpenSSL defaults.",
+ tls12_ciphers,
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/ssl_context.py` around lines 65 - 78, The code silently falls back
to OpenSSL defaults when context.set_ciphers(tls12_ciphers) raises ssl.SSLError,
which is unsafe for explicit user overrides; update the block where
cipher_string is computed (resolve_ciphers) and filtered (filter_tls12_ciphers)
so that on ssl.SSLError you re-raise the exception when the ciphers argument was
user-supplied (e.g., profile_type indicates a custom override or ciphers param
is non-default), but keep the existing warning-and-fallback behavior only when
profile_type corresponds to built-in profiles/defaults; use the presence/value
of profile_type or the original ciphers argument to differentiate and either
logger.warning + continue for built-ins or raise the caught ssl.SSLError for
explicit user policies.
Contributor
Author
|
Will submit only e2e tests targeting Llama Stack's run.yaml NetworkConfig; this PR's lightspeed-stack implementation is out of scope. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add networking configuration infrastructure for proxy, TLS security profiles, and custom CA certificates. This is PR 1 of 3 for LCORE-1253.
What: Introduces config models (
NetworkingConfiguration,ProxyConfiguration,TLSSecurityProfile) and supporting utilities (tls.py,ssl_context.py,certificates.py) for configuring outgoing connection security. No behavioral changes — the newnetworkingconfig field defaults toNone.Why: LCORE-1178 (proxy support) had not landed, and PR #869 (TLS profiles by @onmete) has been stale since Dec 2025. This implements the infrastructure needed for proxy and TLS support, porting and improving upon the road-core service's approach.
Improvements over road-core:
check_hostname)set_ciphers()(road-core would crash withSSLError)/tmp)All improvements are strictly additive — no road-core feature was dropped. Cipher lists, profile names, and TLS version identifiers match the OpenShift API specification exactly.
Reviewer guidance:
TLSSecurityProfilevalidator imports fromutils.tlsat module level inconfig.py— this is a deliberate coupling between config and utils layers, chosen over deferred imports for clarity.TLSSecurityProfileusespopulate_by_name=Truefor YAML alias support (typevsprofile_type). No other config model in the codebase uses this — it's needed becausetypeis a Python builtin.Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
Verify backward compatibility — load existing config without
networkingsection:Summary by CodeRabbit
Release Notes
New Features
Documentation