Conversation
skamenan7
left a comment
There was a problem hiding this comment.
Nice security features, Doug! Looks good except these comments.
|
|
||
| # Apply CLI security mode overrides BEFORE model validation, | ||
| # so --insecure can downgrade production mode without triggering the validator. | ||
| if args.insecure or args.security_mode: |
There was a problem hiding this comment.
Seems CLI overrides get lost along the way. _run_stack_run_cmd() validates the original file before this branch, and then create_app() re-reads LLAMA_STACK_CONFIG from disk anyway. so --insecure can't actually rescue a production config missing certs, and --security-mode production never reaches the verify_tls=False guard on the app side. happy to walk through the call chain if that's helpful.
| raise ValueError( | ||
| "Production security mode requires TLS: set 'tls_certfile' and 'tls_keyfile' in server config, " | ||
| "or use '--insecure' / security_mode='development' to run without TLS." | ||
| ) |
There was a problem hiding this comment.
current validator leaves a hole in the FIPS-only guarantee. We only auto-fill the allowlist when tls_config.ciphers is None, and _uvicorn_run() forwards any explicit list as-is. Would it make sense to validate that production-mode ciphers are a non-empty subset of FIPS_APPROVED_CIPHERS, or reject custom overrides entirely in production?
|
|
||
|
|
||
| class TestProductionVerifyTlsFalse: | ||
| def test_production_mode_rejects_verify_tls_false(self): |
There was a problem hiding this comment.
this test doesn't exercise the production startup guard. it builds an OAuth2TokenAuthConfig and asserts verify_tls stayed False, but never calls create_app(), so deleting the SystemExit branch in server.py would still leave it green. worth driving create_app() or extracting that guard into a testable helper so the test covers the real failure path.
Signed-off-by: Doug Edgar <dedgar@redhat.com>
Signed-off-by: Doug Edgar <dedgar@redhat.com>
|
Ok, I've added a new commit, which should address the comments on original commit. |
What does this PR do?
Providing an optional mode to enforce secure connections for llama-stack, using only FIPS-approved TLS cipher suites.
allowlist (RS256/PS256/ES256 families)
Closes RHAIENG-1865
If accepted, I expect to open a couple follow-up PRs for things like backend URL validation and HTTP-to-HTTPS redirect + some remaining config hardening. I split this effort into a couple segments to make it a more manageable size to review at one time.
Test Plan
Additional manual checks