Propagate OpenShift TLS security profile to Authorino#2091
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe controller now detects the OpenShift APIServer CRD, watches APIServer resources, resolves their TLS profiles, and applies the resulting settings to Authorino. RBAC, fake-manager compatibility, dependency versions, generated metadata, and an OIDC policy test expectation are also updated. ChangesOpenShift TLS profile propagation
OIDC policy test adjustment
Build and release metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ControllerBootstrap
participant RESTMapper
participant APIServerWatcher
participant AuthorinoReconciler
participant APIServer
participant Authorino
ControllerBootstrap->>RESTMapper: check APIServer CRD
RESTMapper-->>ControllerBootstrap: availability
ControllerBootstrap->>APIServerWatcher: register watcher when available
APIServerWatcher->>AuthorinoReconciler: deliver APIServer topology event
AuthorinoReconciler->>APIServer: read selected TLS profile
APIServer-->>AuthorinoReconciler: return TLSSecurityProfile
AuthorinoReconciler->>Authorino: apply TLS version and cipher suites
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
philbrookes
force-pushed
the
ocp5-tls-compliance
branch
from
59ae86f to
e7b63a5
Compare
![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 (1)
internal/openshift/tls_profile.go (1)
45-77: 📐 Maintainability & Code Quality | 🔵 TrivialConsider a completeness test for the cipher translation table.
opensslToIANAis a manually maintained map. If OpenShift renames/adds ciphers to a predefined profile (Old/Intermediate/Modern) in a futureopenshift/apibump, ciphers not yet added here will silently disappear fromResolveTLSProfile's output with no compile-time or test signal. Consider a test that iteratesconfigv1.TLSProfiles[...].Ciphersfor all built-in profile types and asserts every cipher is either mapped or explicitly documented/allow-listed as an intentionally-skipped DHE cipher.Also applies to: 99-103
🤖 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 `@internal/openshift/tls_profile.go` around lines 45 - 77, Add a completeness test around the opensslToIANA translation table in tls_profile.go so future openshift/api profile cipher changes are caught. In the test, iterate all built-in entries from configv1.TLSProfiles and verify every cipher in each profile’s Ciphers list is either present in opensslToIANA or explicitly allow-listed as an intentionally skipped DHE cipher. Use ResolveTLSProfile and opensslToIANA as the primary symbols to locate the behavior, and fail the test when a new cipher would otherwise be silently dropped.
🤖 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 `@internal/controller/authorino_reconciler.go`:
- Around line 239-254: The chained type assertion in
resolveTLSProfileFromTopology can panic if the child’s underlying object is not
the expected APIServer type. Update the lookup in
AuthorinoReconciler.resolveTLSProfileFromTopology to use safe type assertions
when casting child to controller.RuntimeObject and then to configv1.APIServer,
and if either cast fails, skip that child and fall back to
openshift.ResolveTLSProfile(nil) instead of panicking.
---
Nitpick comments:
In `@internal/openshift/tls_profile.go`:
- Around line 45-77: Add a completeness test around the opensslToIANA
translation table in tls_profile.go so future openshift/api profile cipher
changes are caught. In the test, iterate all built-in entries from
configv1.TLSProfiles and verify every cipher in each profile’s Ciphers list is
either present in opensslToIANA or explicitly allow-listed as an intentionally
skipped DHE cipher. Use ResolveTLSProfile and opensslToIANA as the primary
symbols to locate the behavior, and fail the test when a new cipher would
otherwise be silently dropped.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bac19e49-aea4-4f10-93ca-7b6332c251b4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
Dockerfileconfig/rbac/role.yamlgo.modhack/test-tls-profile.shinternal/controller/authorino_reconciler.gointernal/controller/fake/manager.gointernal/controller/state_of_the_world.gointernal/openshift/tls_profile.gointernal/openshift/tls_profile_test.go
philbrookes
changed the title
philbrookes
force-pushed
the
ocp5-tls-compliance
branch
from
b6ff143 to
ecdcd4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
hack/test-tls-profile.sh (4)
180-204: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse
mktempinstead of hardcoded/tmppaths for TLS key and certificate.Hardcoded
/tmp/tls.keyand/tmp/tls.crtare vulnerable to symlink/TOCTOU attacks and can collide with concurrent runs.mktempis trivially safer and avoids stale-file issues.🛡️ Suggested fix
setup_tls() { log "Setting up TLS for deployment args verification" - openssl req -x509 -newkey rsa:2048 -keyout /tmp/tls.key -out /tmp/tls.crt \ + local tls_key tls_crt + tls_key="$(mktemp)" + tls_crt="$(mktemp)" + openssl req -x509 -newkey rsa:2048 -keyout "$tls_key" -out "$tls_crt" \ -days 1 -nodes -subj '/CN=authorino' 2>/dev/null kubectl create secret tls "$TLS_SECRET_NAME" -n "$AUTHORINO_NS" \ - --cert=/tmp/tls.crt --key=/tmp/tls.key 2>/dev/null || true + --cert="$tls_crt" --key="$tls_key" 2>/dev/null || trueAnd in
teardown_tls:teardown_tls() { log "Tearing down TLS" kubectl patch "$AUTHORINO_GVR" "$AUTHORINO_NAME" -n "$AUTHORINO_NS" \ --type=merge -p '{"spec":{ "listener":{"tls":{"enabled":false,"certSecretRef":null}}, "oidcServer":{"tls":{"enabled":false,"certSecretRef":null}} }}' 2>/dev/null || true kubectl delete secret "$TLS_SECRET_NAME" -n "$AUTHORINO_NS" 2>/dev/null || true - rm -f /tmp/tls.key /tmp/tls.crt }Note: with
mktemp, cleanup should ideally use a trap or store the paths in variables accessible toteardown_tls. Alternatively, usemktemp -dto create a temporary directory and remove it on exit.🤖 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 `@hack/test-tls-profile.sh` around lines 180 - 204, Use mktemp for the TLS key and certificate in setup_tls instead of hardcoded /tmp/tls.key and /tmp/tls.crt, and store the generated paths in variables that teardown_tls can access for cleanup. Update the openssl req and kubectl create secret tls calls to use those variables, and ensure teardown_tls removes the same temporary files or a temporary directory created with mktemp -d.
121-137: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against missing
python3.The script silently depends on
python3for JSON arg parsing but never checks for its availability. On minimal environments (e.g., some CI images), this will produce a confusing error.🛡️ Suggested pre-flight check
if ! wait_for_authorino 5; then echo "Authorino CR not found in $AUTHORINO_NS. Run 'make local-setup' first." exit 1 fi +if ! command -v python3 &>/dev/null; then + echo "python3 is required but not found in PATH." + exit 1 +fi +🤖 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 `@hack/test-tls-profile.sh` around lines 121 - 137, The get_deployment_arg_value helper currently assumes python3 is installed, which can fail on minimal CI or local environments. Add a pre-flight dependency check near the top of the script or before get_deployment_arg_value is used, and make it fail fast with a clear message if python3 is unavailable. Keep the change scoped to the TLS profile test script and the functions get_deployment_args/get_deployment_arg_value so the dependency is validated before JSON parsing starts.
17-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
failfunction to avoid shadowing thefailcounter variable.While bash resolves
failas a command (function) vs$fail(variable) in separate namespaces, the identical name is confusing and fragile — a future contributor might easily break the arithmetic or introduce a recursive call.♻️ Suggested rename
pass=0 -fail=0 +fail_count=0 log() { echo -e "${YELLOW}--- $1${NC}"; } ok() { echo -e "${GREEN}PASS: $1${NC}"; pass=$((pass + 1)); } -fail() { echo -e "${RED}FAIL: $1${NC}"; fail=$((fail + 1)); } +fail_test() { echo -e "${RED}FAIL: $1${NC}"; fail_count=$((fail_count + 1)); }Note: all call sites and the final
exit "$fail"on line 450 would need updating accordingly.🤖 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 `@hack/test-tls-profile.sh` around lines 17 - 21, The `fail` helper function in `test-tls-profile.sh` is named the same as the `fail` counter variable, which is confusing and fragile. Rename the function to a distinct name (for example, a failure-reporting helper) and update every call site in the script, including the final `exit "$fail"`-related references if they rely on the old name, so the counter and helper are clearly separated.
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe hardcoded
openshift/apiversion in the GOMODCACHE path is fragile.If
go.modis updated to a differentgithub.com/openshift/apiversion, this path silently points to a non-existent file and the script fails at runtime with a confusing error. Consider deriving the version fromgo.modor at minimum adding a guard:🛡️ Suggested guard
APISERVER_CRD="$(go env GOMODCACHE)/github.com/openshift/api@v0.0.0-20240926211938-f89ab92f1597/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers.crd.yaml" +if [[ ! -f "$APISERVER_CRD" ]]; then + echo "APIServer CRD manifest not found at: $APISERVER_CRD" >&2 + echo "Verify that github.com/openshift/api is in go.mod and downloaded to GOMODCACHE." >&2 + exit 1 +ficmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go (1)
483-564: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
notExpectedInRulefield is declared but never populated.The test struct includes
notExpectedInRule []stringand the test loop iterates over it (lines 555-559), but none of the three test cases populate it. Consider adding negative assertions — e.g., in case 2 verify thatbaseURLdoes not appear in location 2 — or remove the field if not needed.🤖 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 `@cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go` around lines 483 - 564, The test table in TestBuildOpaAuthorizationRule_UsesCorrectBaseURL declares notExpectedInRule but never uses it, so either add real negative assertions for buildOpaAuthorizationRule (for example, verify the custom baseURL is not used where igwURL should be) or remove the unused field and loop entirely. Keep the fix localized to the test cases and the assertion block so the struct shape matches the actual checks being performed.
🤖 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
`@cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go`:
- Around line 226-343: `TestBuildTargetCookieExpression_Examples` is asserting
the wrong CEL substring for the query-string branch. Update the expected pattern
in that test to match the actual output from `buildTargetCookieExpression`,
including the `has(request.query) && request.query != ""` guard used elsewhere
in the same test file. Keep the check aligned with the expression built for
`request.url_path` so the subtests compare against the exact generated form.
---
Nitpick comments:
In
`@cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go`:
- Around line 483-564: The test table in
TestBuildOpaAuthorizationRule_UsesCorrectBaseURL declares notExpectedInRule but
never uses it, so either add real negative assertions for
buildOpaAuthorizationRule (for example, verify the custom baseURL is not used
where igwURL should be) or remove the unused field and loop entirely. Keep the
fix localized to the test cases and the assertion block so the struct shape
matches the actual checks being performed.
In `@hack/test-tls-profile.sh`:
- Around line 180-204: Use mktemp for the TLS key and certificate in setup_tls
instead of hardcoded /tmp/tls.key and /tmp/tls.crt, and store the generated
paths in variables that teardown_tls can access for cleanup. Update the openssl
req and kubectl create secret tls calls to use those variables, and ensure
teardown_tls removes the same temporary files or a temporary directory created
with mktemp -d.
- Around line 121-137: The get_deployment_arg_value helper currently assumes
python3 is installed, which can fail on minimal CI or local environments. Add a
pre-flight dependency check near the top of the script or before
get_deployment_arg_value is used, and make it fail fast with a clear message if
python3 is unavailable. Keep the change scoped to the TLS profile test script
and the functions get_deployment_args/get_deployment_arg_value so the dependency
is validated before JSON parsing starts.
- Around line 17-21: The `fail` helper function in `test-tls-profile.sh` is
named the same as the `fail` counter variable, which is confusing and fragile.
Rename the function to a distinct name (for example, a failure-reporting helper)
and update every call site in the script, including the final `exit
"$fail"`-related references if they rely on the old name, so the counter and
helper are clearly separated.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6d140ce-5af6-4d95-b634-e87351d4094e
⛔ Files ignored due to path filters (8)
go.sumis excluded by!**/*.sumpkg/extension/grpc/v1/common.pb.gois excluded by!**/*.pb.gopkg/extension/grpc/v1/descriptor_service.pb.gois excluded by!**/*.pb.gopkg/extension/grpc/v1/descriptor_service_grpc.pb.gois excluded by!**/*.pb.gopkg/extension/grpc/v1/gateway_api.pb.gois excluded by!**/*.pb.gopkg/extension/grpc/v1/kuadrant.pb.gois excluded by!**/*.pb.gopkg/extension/grpc/v1/kuadrant_grpc.pb.gois excluded by!**/*.pb.gopkg/extension/grpc/v1/policy.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
Dockerfilecmd/extensions/oidc-policy/api/v1alpha1/oidcpolicy_types.gocmd/extensions/oidc-policy/api/v1alpha1/oidcpolicy_types_test.gocmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.gocmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.goconfig/rbac/role.yamlgo.modhack/test-tls-profile.shinternal/controller/authorino_reconciler.gointernal/controller/fake/manager.gointernal/controller/state_of_the_world.gointernal/extension/reconciler.gointernal/openshift/tls_profile.gointernal/openshift/tls_profile_test.gopkg/extension/grpc/v1/gateway_api.proto
🚧 Files skipped from review as they are similar to previous changes (8)
- config/rbac/role.yaml
- Dockerfile
- internal/openshift/tls_profile_test.go
- internal/controller/fake/manager.go
- internal/openshift/tls_profile.go
- internal/controller/state_of_the_world.go
- go.mod
- internal/controller/authorino_reconciler.go
philbrookes
force-pushed
the
ocp5-tls-compliance
branch
2 times, most recently
from
248f3d4 to
e7949a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
hack/test-tls-profile.sh (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded openshift/api pseudo-version may drift from
go.mod.
APISERVER_CRDpins an exact pseudo-version ofgithub.com/openshift/apiindependently ofgo.mod. If that dependency is bumped elsewhere in this PR/stack, this path will stop resolving andkubectl apply -f "$APISERVER_CRD"(line 256) will fail, aborting the whole script underset -e.♻️ Suggested approach
-APISERVER_CRD="$(go env GOMODCACHE)/github.com/openshift/api@v0.0.0-20240926211938-f89ab92f1597/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers.crd.yaml" +APISERVER_API_DIR="$(go list -m -f '{{.Dir}}' github.com/openshift/api)" +APISERVER_CRD="${APISERVER_API_DIR}/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers.crd.yaml"Please confirm whether the
openshift/apiversion pinned ingo.modmatchesv0.0.0-20240926211938-f89ab92f1597, as this script will silently break otherwise.🤖 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 `@hack/test-tls-profile.sh` at line 4, Update APISERVER_CRD in hack/test-tls-profile.sh to derive the openshift/api module version from go.mod or the Go module tooling instead of hardcoding v0.0.0-20240926211938-f89ab92f1597, while preserving the existing generated CRD path used by kubectl apply.Dockerfile (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning the builder image to an exact patch/digest.
golang:1.26is a floating tag that will silently pick up newer patch releases (e.g. currently resolves to 1.26.5) over time, which can produce non-reproducible builds across CI runs. Consider pinning to the exact patch used ingo.mod(golang:1.26.4) or, better, a digest, consistent with theratchet:pinning already used elsewhere in the repo's GitHub Actions workflows.🤖 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 `@Dockerfile` at line 6, Pin the builder image in the Dockerfile’s FROM declaration to the exact Go patch version required by go.mod, golang:1.26.4, or preferably to its immutable digest; retain the existing BUILDPLATFORM and builder stage.
🤖 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 `@hack/test-tls-profile.sh`:
- Line 242: Guard both wait_for_authorino 30 calls in the Scenario 1 and
Scenario 2 flows so a timeout does not terminate the script under set -e.
Preserve the subsequent assert/ok/fail handling by allowing each wait failure to
continue execution.
---
Nitpick comments:
In `@Dockerfile`:
- Line 6: Pin the builder image in the Dockerfile’s FROM declaration to the
exact Go patch version required by go.mod, golang:1.26.4, or preferably to its
immutable digest; retain the existing BUILDPLATFORM and builder stage.
In `@hack/test-tls-profile.sh`:
- Line 4: Update APISERVER_CRD in hack/test-tls-profile.sh to derive the
openshift/api module version from go.mod or the Go module tooling instead of
hardcoding v0.0.0-20240926211938-f89ab92f1597, while preserving the existing
generated CRD path used by kubectl apply.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12c12f68-dfd0-47a7-8736-a43255a6291a
⛔ Files ignored due to path filters (8)
go.sumis excluded by!**/*.sumpkg/extension/grpc/v1/common.pb.gois excluded by!**/*.pb.gopkg/extension/grpc/v1/descriptor_service.pb.gois excluded by!**/*.pb.gopkg/extension/grpc/v1/descriptor_service_grpc.pb.gois excluded by!**/*.pb.gopkg/extension/grpc/v1/gateway_api.pb.gois excluded by!**/*.pb.gopkg/extension/grpc/v1/kuadrant.pb.gois excluded by!**/*.pb.gopkg/extension/grpc/v1/kuadrant_grpc.pb.gois excluded by!**/*.pb.gopkg/extension/grpc/v1/policy.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (17)
Dockerfilebundle.Dockerfilebundle/manifests/kuadrant-operator.clusterserviceversion.yamlcmd/extensions/oidc-policy/api/v1alpha1/oidcpolicy_types.gocmd/extensions/oidc-policy/api/v1alpha1/oidcpolicy_types_test.gocmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.gocmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.goconfig/rbac/role.yamlgo.modhack/test-tls-profile.shinternal/controller/authorino_reconciler.gointernal/controller/fake/manager.gointernal/controller/state_of_the_world.gointernal/extension/reconciler.gointernal/openshift/tls_profile.gointernal/openshift/tls_profile_test.gopkg/extension/grpc/v1/gateway_api.proto
💤 Files with no reviewable changes (1)
- bundle.Dockerfile
🚧 Files skipped from review as they are similar to previous changes (11)
- cmd/extensions/oidc-policy/api/v1alpha1/oidcpolicy_types.go
- pkg/extension/grpc/v1/gateway_api.proto
- internal/openshift/tls_profile_test.go
- cmd/extensions/oidc-policy/api/v1alpha1/oidcpolicy_types_test.go
- config/rbac/role.yaml
- internal/controller/fake/manager.go
- internal/controller/state_of_the_world.go
- cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go
- internal/openshift/tls_profile.go
- internal/controller/authorino_reconciler.go
- cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go
philbrookes
force-pushed
the
ocp5-tls-compliance
branch
2 times, most recently
from
682b3ea to
bab4a9c
Compare
philbrookes
changed the title
philbrookes
force-pushed
the
ocp5-tls-compliance
branch
from
bab4a9c to
e5eadb8
Compare
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
`@cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go`:
- Line 336: Update the comment describing the CEL expression in the OIDC policy
reconciler test to include the request.query != "" guard, matching the corrected
assertion and production behavior. Change only the stale comment near the
relevant assertion.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54f86acd-ce1c-4ea5-a249-d7224a7a5011
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
bundle/manifests/kuadrant-operator.clusterserviceversion.yamlcharts/kuadrant-operator/templates/manifests.yamlcmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.goconfig/rbac/role.yamlgo.modinternal/controller/authorino_reconciler.gointernal/controller/fake/manager.gointernal/controller/state_of_the_world.gointernal/openshift/tls_profile.gointernal/openshift/tls_profile_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- bundle/manifests/kuadrant-operator.clusterserviceversion.yaml
- config/rbac/role.yaml
- internal/openshift/tls_profile_test.go
- internal/openshift/tls_profile.go
- internal/controller/authorino_reconciler.go
- go.mod
- internal/controller/fake/manager.go
- internal/controller/state_of_the_world.go
philbrookes
force-pushed
the
ocp5-tls-compliance
branch
from
e5eadb8 to
0271cb4
Compare
Read the cluster's APIServer CR (apiserver.config.openshift.io/cluster) and propagate its TLS security profile to the Authorino CR on both the Listener and OIDCServer TLS configurations. On non-OpenShift clusters, falls back to Intermediate profile defaults. - Navigate the topology graph from Kuadrant CR to find the APIServer CR instead of filtering all objects; CR name defaults to "cluster" and can be overridden via APISERVER_CR_NAME env var - Map OpenShift VersionTLS12 format to 1.2 short form to match the updated Authorino CLI flag format - Add integration test script (hack/test-tls-profile.sh) covering 9 scenarios with both Authorino CR spec and deployment args verification Signed-off-by: Phil Brookes <pbrookes@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
philbrookes
force-pushed
the
ocp5-tls-compliance
branch
from
0271cb4 to
6d477c3
Compare
| // The CEL expression uses a ternary: request.url_path + (has(request.query) && request.query != "" ? "?" + request.query : "") | ||
| // This should construct the full path with query when query is present | ||
| if !strings.Contains(expression, `request.url_path + (has(request.query) ? "?" + request.query : "")`) { | ||
| if !strings.Contains(expression, `request.url_path + (has(request.query) && request.query != "" ? "?" + request.query : "")`) { |
There was a problem hiding this comment.
This is a "pre-existing" part, but does this test check for nothing? And the comment seems useless.
| apiServer, ok := rObj.Object.(*configv1.APIServer) | ||
| if !ok { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Why do we have two casts/checks?
| if profile == nil || profile.Type == "" { | ||
| spec = configv1.TLSProfiles[configv1.TLSProfileIntermediateType] | ||
| } else if profile.Type == configv1.TLSProfileCustomType && profile.Custom != nil { | ||
| spec = &profile.Custom.TLSProfileSpec | ||
| } else { | ||
| spec = configv1.TLSProfiles[profile.Type] | ||
| } | ||
|
|
||
| if spec == nil { | ||
| spec = configv1.TLSProfiles[configv1.TLSProfileIntermediateType] | ||
| } |
There was a problem hiding this comment.
This defines what the default profile is in two places: the first if block and this nil check. It should work the same if we remove the first if and would leave us with the single "default" source of truth 🤔
| t.Run("Custom profile with unsupported DHE cipher skips it", func(t *testing.T) { | ||
| profile := &configv1.TLSSecurityProfile{ | ||
| Type: configv1.TLSProfileCustomType, | ||
| Custom: &configv1.CustomTLSProfile{ | ||
| TLSProfileSpec: configv1.TLSProfileSpec{ | ||
| Ciphers: []string{"DHE-RSA-AES128-GCM-SHA256", "ECDHE-RSA-AES128-GCM-SHA256"}, | ||
| MinTLSVersion: configv1.VersionTLS12, | ||
| }, | ||
| }, | ||
| } | ||
| _, ciphers := ResolveTLSProfile(profile) | ||
| if len(ciphers) != 1 { | ||
| t.Fatalf("expected 1 cipher (DHE skipped), got %d: %v", len(ciphers), ciphers) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Intermediate profile excludes DHE ciphers", func(t *testing.T) { | ||
| _, ciphers := ResolveTLSProfile(nil) | ||
| for _, c := range ciphers { | ||
| if c == "DHE-RSA-AES128-GCM-SHA256" || c == "DHE-RSA-AES256-GCM-SHA384" { | ||
| t.Errorf("DHE cipher should not appear in translated output: %s", c) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
If someone were to add DHE ciphers to the opensslToIANA map this will not catch it. We should be looking for "TLS_DHE_*" or matching the count to what we expect
| } | ||
|
|
||
| func apiServerCRName() string { | ||
| if name := os.Getenv("APISERVER_CR_NAME"); name != "" { |
There was a problem hiding this comment.
How will people know that this envar exists?
If there is any documentation that we expect to produce, we would benefit from including this envar in there.
| } | ||
| } | ||
|
|
||
| return minVersion, cipherSuites |
There was a problem hiding this comment.
There is a mention of a test script in the description and a few test scenarios. Are those manual testing steps?
We could have an integration test case that would register a new CRD/CR and verify this logic. Should we have it?
| return name | ||
| } | ||
| return openshift.DefaultAPIServerCRName | ||
| } |
There was a problem hiding this comment.
Wondering if this func belongs in the openshift package too 🤔
| } | ||
|
|
||
| func LinkKuadrantToAPIServer(objs controller.Store) machinery.LinkFunc { | ||
| kuadrants := lo.Map(objs.FilterByGroupKind(kuadrantv1beta1.KuadrantGroupKind), controller.ObjectAs[machinery.Object]) |
There was a problem hiding this comment.
Since the cluster APIServer CR is a cluster-scoped singleton, I wonder if the link function couldn't be filtering it by name already. I.e., linking this one and only, well-known CR to the Kuadrant CR. Then, later, the reconcilers could always navigate to the first child of the Kuadrant CR of this type, with no need to filter by name again. WDYT?
| } | ||
|
|
||
| func (b *BootOptionsBuilder) getOpenShiftServerConfigOptions() ([]controller.ControllerOption, error) { | ||
| opts := make([]controller.ControllerOption, 0, 2) |
There was a problem hiding this comment.
Line 504 appends 3 elements to the slice, but the capacity is preset to 2. Won't this require a realloc?
| return openshift.DefaultAPIServerCRName | ||
| } | ||
|
|
||
| func (r *AuthorinoReconciler) resolveTLSProfileFromTopology(topology *machinery.Topology, kobj *v1beta1.Kuadrant) (string, []string) { |
There was a problem hiding this comment.
Maybe add a unit test for this func?
| for _, ex := range examples { | ||
| t.Run(ex.scenario, func(t *testing.T) { | ||
| // The CEL expression uses a ternary: request.url_path + (has(request.query) ? "?" + request.query : "") | ||
| // The CEL expression uses a ternary: request.url_path + (has(request.query) && request.query != "" ? "?" + request.query : "") |
There was a problem hiding this comment.
Brought from an unrelated and uncommitted local change perhaps, or was it @CodeRabbit complaining about it?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh pr view 2091 --repo Kuadrant/kuadrant-operator --json comments,reviews -q '.reviews[] | select(.body != "") | .body' 2>/dev/null | head -100
echo "---review comments---"
gh api repos/Kuadrant/kuadrant-operator/pulls/2091/comments --paginate -q '.[] | {user: .user.login, body: .body, path: .path, line: .line}' 2>/dev/nullLength of output: 23282
Confirmed it — I flagged this comment.
@guicassolato it was CodeRabbit: I posted the review comment on Line 336 (cr-comment:v1:4f7a4605c9a13a684743c91c) pointing out the stale comment still described the old ternary has(request.query) ? "?" + request.query : "" while the assertion on Line 338 and the actual production code already had the && request.query != "" guard. That comment was subsequently addressed in commit 0271cb4, which is the diff you're looking at now — so this fix is directly tied to my review, not an unrelated local change.
There was a problem hiding this comment.
It's unrelated to the main issue addressed by this PR though. For the sake of cohesion, I personally think it would be better addressed in a separate PR.
There was a problem hiding this comment.
@guicassolato, fair point — noted. Since the fix is already included in this PR's diff and is a trivial one-line comment/documentation correction (no functional impact), I'll leave it as is unless you'd like it reverted and reopened separately. Either way works from my end.
| CertSecret: clusterAuthorino.Spec.Listener.Tls.CertSecret, | ||
| MinVersion: tlsMinVersion, | ||
| CipherSuites: tlsCipherSuites, |
There was a problem hiding this comment.
Should we avoid setting these options when tls.enabled is false? Thinking about reducing the noise in the CR spec (and command-line args). WDYT?
3 participants
Summary
APIServerCR (apiserver.config.openshift.io/cluster) and resolves its TLS security profiletlsMinVersionandtlsCipherSuitesto the Authorino CRapiservers(get/list/watch) and test script covering 9 scenariosContext
Part of OCP5 TLS profile compliance. Authorino is the only Kuadrant component serving TLS. This change ensures its TLS configuration matches the cluster-wide TLS security profile.
The operator looks up the
APIServerCR namedcluster— this follows the OpenShift convention where allconfig.openshift.ioresources are singletons with a canonical instance name of "cluster". See:Depends on: Kuadrant/authorino-operator#335
Test plan
ResolveTLSProfile()(all profile types, nil input, OpenSSL→IANA mapping)Summary by CodeRabbit
New Features
APIServerresource when available.Bug Fixes
Compatibility
APIServerresources.