AGENT-1526: disable htpasswd auth on IRI registry, keep read-only#6109
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@andfasano: This pull request references AGENT-1526 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test ? |
WalkthroughThe PR removes htpasswd-based config from the iri-registry systemd unit, adds HTTP/TLS and storage env vars to the ExecStart, and updates unit tests and e2e tests to verify unauthenticated registry reads (including a new curl-based unauthenticated read test). ChangesRegistry Service Authentication Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
/test e2e-agent-compact-ipv4-iso-no-registry |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.go (1)
38-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistency: htpasswd file is verified but authentication is disabled.
The test verifies that the htpasswd file exists on disk (Line 38), but also verifies that the registry is not configured to use it (lines 31-32 confirm
REGISTRY_AUTH_HTPASSWD_*environment variables are absent). This creates an inconsistent security posture: credentials are deployed but unused.Consider whether the htpasswd file should:
- Be removed entirely if authentication is truly disabled, or
- Remain in place with a comment explaining it's for future re-enablement per AGENT-1393
🤖 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 `@pkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.go` at line 38, The test is inconsistent: it asserts the htpasswd file via verifyIgnitionFile(t, &ignCfg, "/etc/iri-registry/auth/htpasswd", "...") while the environment vars REGISTRY_AUTH_HTPASSWD_* are intentionally absent; either remove the htpasswd verification from the test (delete the verifyIgnitionFile call) so the test reflects authentication-disabled state, or keep the verification but add a clarifying comment referencing AGENT-1393 and ensure the test also asserts that the registry config does not enable htpasswd auth (check the code path that reads REGISTRY_AUTH_HTPASSWD_*); update the test near verifyIgnitionFile and any related assertions to make the intended behavior explicit.
🧹 Nitpick comments (1)
pkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.go (1)
31-32: ⚡ Quick winConsider adding positive assertions for TLS configuration.
While correctly verifying that htpasswd environment variables are absent, the test should also verify that the TLS configuration that replaced htpasswd authentication is present. Based on the service template, the registry now relies on TLS.
✅ Proposed additions to verify TLS configuration
assert.NotContains(t, *ignCfg.Systemd.Units[0].Contents, "REGISTRY_AUTH_HTPASSWD_REALM") assert.NotContains(t, *ignCfg.Systemd.Units[0].Contents, "REGISTRY_AUTH_HTPASSWD_PATH") + assert.Contains(t, *ignCfg.Systemd.Units[0].Contents, "REGISTRY_HTTP_ADDR=0.0.0.0:22625") + assert.Contains(t, *ignCfg.Systemd.Units[0].Contents, "REGISTRY_HTTP_TLS_CERTIFICATE=/certs/tls.crt") + assert.Contains(t, *ignCfg.Systemd.Units[0].Contents, "REGISTRY_HTTP_TLS_KEY=/certs/tls.key")🤖 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 `@pkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.go` around lines 31 - 32, Add positive assertions that the registry TLS environment variables are present in the systemd unit content to complement the existing NotContains checks for htpasswd; specifically, in the same test where ignCfg.Systemd.Units[0].Contents is inspected, add assert.Contains checks for the TLS keys (e.g., "REGISTRY_HTTP_TLS_CERTIFICATE" and "REGISTRY_HTTP_TLS_KEY") against *ignCfg.Systemd.Units[0].Contents so the test verifies the switch from htpasswd to TLS configuration.
🤖 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.
Outside diff comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.go`:
- Line 38: The test is inconsistent: it asserts the htpasswd file via
verifyIgnitionFile(t, &ignCfg, "/etc/iri-registry/auth/htpasswd", "...") while
the environment vars REGISTRY_AUTH_HTPASSWD_* are intentionally absent; either
remove the htpasswd verification from the test (delete the verifyIgnitionFile
call) so the test reflects authentication-disabled state, or keep the
verification but add a clarifying comment referencing AGENT-1393 and ensure the
test also asserts that the registry config does not enable htpasswd auth (check
the code path that reads REGISTRY_AUTH_HTPASSWD_*); update the test near
verifyIgnitionFile and any related assertions to make the intended behavior
explicit.
---
Nitpick comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.go`:
- Around line 31-32: Add positive assertions that the registry TLS environment
variables are present in the systemd unit content to complement the existing
NotContains checks for htpasswd; specifically, in the same test where
ignCfg.Systemd.Units[0].Contents is inspected, add assert.Contains checks for
the TLS keys (e.g., "REGISTRY_HTTP_TLS_CERTIFICATE" and "REGISTRY_HTTP_TLS_KEY")
against *ignCfg.Systemd.Units[0].Contents so the test verifies the switch from
htpasswd to TLS configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c1f89fc0-8d60-4a01-8384-ce897f8958bc
📒 Files selected for processing (2)
pkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.gopkg/controller/internalreleaseimage/templates/master/units/iri-registry.service.yaml
💤 Files with no reviewable changes (1)
- pkg/controller/internalreleaseimage/templates/master/units/iri-registry.service.yaml
|
/test e2e-agent-compact-ipv4-iso-no-registry |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e-iri/iri_test.go (1)
360-372:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIPv6 URL construction will produce invalid URLs.
The URL construction on line 361 uses
fmt.Sprintf("https://%s:%d/v2/", ipAddr, ...)which does not handle IPv6 addresses correctly. IPv6 addresses must be enclosed in brackets in URLs (e.g.,https://[::1]:22625/v2/).Use
net.JoinHostPort()for proper IPv6 support:🐛 Proposed fix for IPv6 compatibility
+import "net" + func pingIRIRegistry(t *testing.T, client *http.Client, ipAddr string) { - iriRegistryUrl := fmt.Sprintf("https://%s:%d/v2/", ipAddr, ctrlcommon.IRIRegistryPort) + iriRegistryUrl := fmt.Sprintf("https://%s/v2/", net.JoinHostPort(ipAddr, fmt.Sprintf("%d", ctrlcommon.IRIRegistryPort))) req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, iriRegistryUrl, nil)As per coding guidelines: "verify IPv6 and disconnected network compatibility by checking for...URL construction without net.JoinHostPort() for IPv6"
🤖 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 `@test/e2e-iri/iri_test.go` around lines 360 - 372, The URL built in pingIRIRegistry currently uses fmt.Sprintf("https://%s:%d/v2/", ipAddr, ctrlcommon.IRIRegistryPort) which breaks for IPv6; change construction to use net.JoinHostPort(ipAddr, strconv.Itoa(ctrlcommon.IRIRegistryPort)) (or equivalent) to produce a properly bracketed host:port, then prefix with "https://" and append "/v2/" to form iriRegistryUrl; update imports as needed and keep the rest of pingIRIRegistry (request creation, client.Do, and assertions) unchanged.
🤖 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 `@test/e2e-iri/iri_test.go`:
- Around line 384-394: The new test TestIRIRegistry_UnauthenticatedReadSucceeds
is missing the baremetal guard and will fail on non-baremetal platforms; add a
call to skipIfNoBaremetal(t) at the start of that test (and if needed also
skipIfOpenShiftCI(t) to mirror other registry tests) so the test is skipped on
unsupported environments, ensuring behavior matches
TestIRIController_VerifyIRIRegistryOnAllTheMasterNodes_NoCert and
TestIRIController_VerifyIRIRegistryOnApiInt_WithCert.
---
Outside diff comments:
In `@test/e2e-iri/iri_test.go`:
- Around line 360-372: The URL built in pingIRIRegistry currently uses
fmt.Sprintf("https://%s:%d/v2/", ipAddr, ctrlcommon.IRIRegistryPort) which
breaks for IPv6; change construction to use net.JoinHostPort(ipAddr,
strconv.Itoa(ctrlcommon.IRIRegistryPort)) (or equivalent) to produce a properly
bracketed host:port, then prefix with "https://" and append "/v2/" to form
iriRegistryUrl; update imports as needed and keep the rest of pingIRIRegistry
(request creation, client.Do, and assertions) unchanged.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 372c99f7-ec79-4c1f-9ca1-2c3e6375b68b
📒 Files selected for processing (1)
test/e2e-iri/iri_test.go
| func TestIRIRegistry_UnauthenticatedReadSucceeds(t *testing.T) { | ||
| cs := framework.NewClientSet("") | ||
| ctx := context.Background() | ||
|
|
||
| authSecret, err := cs.Secrets(ctrlcommon.MCONamespace).Get(ctx, ctrlcommon.InternalReleaseImageAuthSecretName, v1.GetOptions{}) | ||
| if k8serrors.IsNotFound(err) { | ||
| t.Skip("IRI auth secret not found, authentication is not enabled") | ||
| } | ||
| require.NoError(t, err) | ||
|
|
||
| password := string(authSecret.Data["password"]) | ||
| require.NotEmpty(t, password) | ||
|
|
||
| baseDomain := getBaseDomain(t, cs) | ||
| node := helpers.GetRandomNode(t, cs, "master") | ||
| authHeader := "Basic " + base64.StdEncoding.EncodeToString([]byte(ctrlcommon.IRIRegistryUsername+":"+password)) | ||
|
|
||
| statusCode := curlIRIRegistry(t, cs, node, baseDomain, "-H", "Authorization: "+authHeader) | ||
| require.Equal(t, "200", statusCode, "authenticated request should succeed") | ||
| url := fmt.Sprintf("https://api-int.%s:%d/v2/", baseDomain, ctrlcommon.IRIRegistryPort) | ||
| args := []string{"curl", "-s", "--cacert", iriRootCAPath, "-o", "/dev/null", "-w", "%{http_code}", url} | ||
| statusCode := strings.TrimSpace(helpers.ExecCmdOnNode(t, cs, node, args...)) | ||
| require.Equal(t, "200", statusCode, "unauthenticated read request should succeed") | ||
| } |
There was a problem hiding this comment.
Missing skipIfNoBaremetal(t) will cause test failures on non-baremetal platforms.
Every other IRI test in this file includes skipIfNoBaremetal(t) because the IRI registry is a baremetal-specific feature. This new test accesses the IRI registry but lacks this skip, so it will fail on non-baremetal platforms (e.g., AWS, GCP, vSphere).
Additionally, other registry connectivity tests (TestIRIController_VerifyIRIRegistryOnAllTheMasterNodes_NoCert, TestIRIController_VerifyIRIRegistryOnApiInt_WithCert) also use skipIfOpenShiftCI(t) due to proxy issues—consider whether this test has similar constraints.
🐛 Proposed fix to add skip conditions
func TestIRIRegistry_UnauthenticatedReadSucceeds(t *testing.T) {
+ skipIfOpenShiftCI(t)
+ skipIfNoBaremetal(t)
+
cs := framework.NewClientSet("")
baseDomain := getBaseDomain(t, cs)🤖 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 `@test/e2e-iri/iri_test.go` around lines 384 - 394, The new test
TestIRIRegistry_UnauthenticatedReadSucceeds is missing the baremetal guard and
will fail on non-baremetal platforms; add a call to skipIfNoBaremetal(t) at the
start of that test (and if needed also skipIfOpenShiftCI(t) to mirror other
registry tests) so the test is skipped on unsupported environments, ensuring
behavior matches TestIRIController_VerifyIRIRegistryOnAllTheMasterNodes_NoCert
and TestIRIController_VerifyIRIRegistryOnApiInt_WithCert.
There was a problem hiding this comment.
@andfasano Looks like a genuine suggestion. Will it really fail for none platfoem?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Yes, but for the moment that was intended, since in the MCO repo we only have the default compact (baremetal) iso-no-registry job. We don't have any plan to add more kind of jobs here, but in case that part will need to be bumped (but definitely not as part of this PR)
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
/verified by @andfasano |
openshift-ci-robot
added
the
verified
|
@andfasano: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano, bfournie The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/hold Revision 5da9577 was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
Tests are blocked by #6121 |
|
/retest-required |
|
|
@andfasano: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
7 participants
- What I did
Temporary disabling of the IRI registry auth. To be re-enabled once https://redhat.atlassian.net/browse/AGENT-1393 will be fully completed
- How to verify it
openshift/release#79640 rehearsal must pass
Summary by CodeRabbit
Bug Fixes
Tests