Add TLS and certificate-management contracts - #583
Open
RonaldHensbergen wants to merge 6 commits into
Open
Conversation
Co-authored-by: RonaldHensbergen <22539616+RonaldHensbergen@users.noreply.github.com>
Co-authored-by: RonaldHensbergen <22539616+RonaldHensbergen@users.noreply.github.com>
Co-authored-by: RonaldHensbergen <22539616+RonaldHensbergen@users.noreply.github.com>
Self-review of #583 surfaced two issues, both fixed here: 1. _plan_has_tls_reverse_proxy() suppressed CDS-SEC-074 if *any* module in the plan provided a reverse-proxy/https contract, without checking it was actually wired (via consumes/mappedFrom) to the specific plaintext module. An unrelated TLS-terminating module anywhere in the plan would incorrectly clear the finding for a plaintext endpoint it has no relationship to. Replaced with _plaintext_module_ids_fronted_by_tls_reverse_proxy(), which walks the plan's resolved consumes wiring and only treats a plaintext module as fronted when another module's consumed contract actually resolves to that module's http-service contract. Updated the plaintext-exposure test fixtures: secure-ingress now declares a `backend` consumes entry (mappedFrom spec.config.backend) and profile-with-tls wires it to `api.http-service`, matching the stricter check. Added profile-unrelated-tls (and a companion secure-ingress-unwired module with no consumes) as a regression fixture proving an unwired https reverse-proxy no longer suppresses the finding. 2. Adding "reason" to _NON_SECRET_PATH_SUFFIXES exempted *any* key ending in "reason" anywhere in a profile/env from high-entropy secret detection, not just the waiver field. Reverted that suffix addition and instead added a path-scoped exemption (spec.security.waivers.*.reason) using the existing pathPatterns matcher, so only the documented waiver field is exempted. Added tests/test_security.py::test_entropy_rule_still_flags_other_keys_ending_in_reason as a regression guard for the narrower exemption. make check: 617 tests pass, lint clean.
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
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.
Summary
Implements the Compose-side scope of #205: a
cds securityrule that flags production profiles exposing plaintext HTTP endpoints without a TLS reverse-proxy in front of them, plus an explicit waiver mechanism.cli/resources/profile.schema.json: addsspec.security.waivers.plaintextEndpointExposure.reason(required, non-empty string) for profiles that intentionally accept plaintext exposure.cli/security.py:_module_provides_plaintext_http()/_plan_has_tls_reverse_proxy()detect, from the plan, whether any module provides anhttp-servicecontract withprotocol: httpand whether areverse-proxycontract withprotocol: httpsis present._port_is_non_local_host_exposure()treats any non-127.0.0.1/localhost/::1published port as an external exposure._check_production_plaintext_exposure()reports a newCDS-SEC-074(high) finding for production profiles with plaintext exposure and no TLS reverse-proxy; ifspec.security.waivers.plaintextEndpointExposure.reasonis set instead, it emits aW098warning diagnostic with the waiver reason instead of failing._try_render_compose_for_scan()now also returns the resolvedplanso the new rule can inspect module contracts, not just the rendered Compose output.reasonadded to_NON_SECRET_PATH_SUFFIXESso waiver-reason text isn't mistaken for a secret value by the entropy/secret-matching rules.tests/fixtures/security/plaintext-exposure/: new fixture modules (apps/plain-api,network/secure-ingress) and three profiles (profile-with-tls,profile-missing-tls,profile-waived-plaintext) covering the valid/missing/waived TLS cases end-to-end.tests/test_cds_workflow.py/tests/test_security.py: new end-to-end and unit coverage for all three cases.Out of scope (tracked separately, per #205's runtime delivery boundary)
reverse-proxy/TLS contract inshared/contracts/, TLS requirement fields on existing HTTP/database/cache contracts, and the certificate/key reference mechanism described in Add TLS and certificate-management contracts #205's remaining work itemsTesting
python -m unittest discover -s tests -p "test_*.py" -v— 609 tests, all pass (1 skipped)make lint— passescoverage run -m unittest discover -s tests -p "test_*.py" && coverage report -m— 85% (gate is 65%)Resolves #205