MGMT-20207: avoid adding system CA bundle to AdditionalTrustBundle #7448

danielerez · 2025-03-24T12:59:02Z

When generating the install config, it should not include the entire system CA bundle. I.e. when setting the AdditionalTrustBundle[1]. However, when using a mirror regsitry (e.g. with MirrorRegistryRef[2] in ASC), the installConfigBuilder is adding the content of tls-ca-bundle.pem[3]. This pem file is created[4] by ASC controller, and includes the full system CA bundle since the bundle[5] is injected into 'cluster-trusted-ca-bundle'.

Therefore, the suggestion solution is to create a separate pem file just for the certificates specified in MirrorRegistryRef. I.e. generate a new 'user-registry-ca-bundle.pem' file, that will be included as part of AdditionalTrustBundle. These certificates will propagate into the 'user-ca-bundle' CM during cluster installation.
This will ensure that 'user-ca-bundle' CM indeed includes only custom certificates mandatory for the user, instead of the system CA bundle.

Note: as backwards compatibility, for flows like ABI, we keep a fallback to the current behaviour (i.e. include 'tls-ca-bundle.pem' only when 'user-registry-ca-bundle.pem' doesn't exist).

[1]

assisted-service/internal/installcfg/builder/builder.go

Line 275 in 40ab10d

cfg.AdditionalTrustBundle = caContent

[2]

assisted-service/vendor/github.com/openshift/assisted-service/api/v1beta1/agentserviceconfig_types.go

Line 82 in cb169a2

    
           MirrorRegistryRef *corev1.LocalObjectReference `json:"mirrorRegistryRef,omitempty"`

[3] https://github.com/openshift/assisted-service/blob/a1c3229afee1f0f774b286283fb0d0098b9eac03/internal/common/common.go#L35
[4]

assisted-service/internal/controller/controllers/agentserviceconfig_controller.go

Line 1848 in 341f986

Path: common.MirrorRegistriesCertificateFile,

[5]

assisted-service/internal/controller/controllers/agentserviceconfig_controller.go

Line 1059 in 341f986

metav1.SetMetaDataLabel(&cm.ObjectMeta, injectTrustedCALabel, "true")

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

openshift-ci-robot · 2025-03-24T12:59:07Z

@danielerez: This pull request references MGMT-20207 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 bug to target the "4.19.0" version, but no target version was set.

In response to this:

When generating the install config, it should not include the entire system CA bundle. I.e. when setting the AdditionalTrustBundle[1]. However, when using a mirror regsitry (e.g. with MirrorRegistryRef[2] in ASC), the installConfigBuilder is adding the content of tls-ca-bundle.pem[3]. This pem file is created[4] by ASC controller, and includes the full system CA bundle since the bundle[5] is injected into 'cluster-trusted-ca-bundle'.

Therefore, the suggestion solution is to create a separate pem file just for the certificates specified in MirrorRegistryRef. I.e. generate a new 'user-registry-ca-bundle.pem' file, that will be included as part of AdditionalTrustBundle. These certificates will propagate into the 'user-ca-bundle' CM during cluster installation.
This will ensure that 'user-ca-bundle' CM indeed includes only custom certificates mandatory for the user, instead of the system CA bundle.

Note: as backwards compatibility, for flows as ABI, we keep a fallback to the current behaviour (i.e. include 'tls-ca-bundle.pem' only when 'user-registry-ca-bundle.pem' doesn't exist).

[1]

assisted-service/internal/installcfg/builder/builder.go

Line 275 in 40ab10d

cfg.AdditionalTrustBundle = caContent

[2]

assisted-service/vendor/github.com/openshift/assisted-service/api/v1beta1/agentserviceconfig_types.go

Line 82 in cb169a2

MirrorRegistryRef *corev1.LocalObjectReference `json:"mirrorRegistryRef,omitempty"`

[3] https://github.com/openshift/assisted-service/blob/a1c3229afee1f0f774b286283fb0d0098b9eac03/internal/common/common.go#L35
[4]

assisted-service/internal/controller/controllers/agentserviceconfig_controller.go

Line 1848 in 341f986

Path: common.MirrorRegistriesCertificateFile,

[5]

assisted-service/internal/controller/controllers/agentserviceconfig_controller.go

Line 1059 in 341f986

metav1.SetMetaDataLabel(&cm.ObjectMeta, injectTrustedCALabel, "true")

List all the issues related to this PR

New Feature

Enhancement

Bug fix

Tests

Documentation

CI/CD

What environments does this code impact?

Automation (CI, tools, etc)

Cloud

Operator Managed Deployments

None

How was this code tested?

assisted-test-infra environment

dev-scripts environment

Reviewer's test appreciated

Waiting for CI to do a full test run

Manual (Elaborate on how it was tested)

No tests needed

Checklist

Title and description added to both, commit and PR.

Relevant issues have been associated (see CONTRIBUTING guide)

This change does not require a documentation update (docstring, docs, README, etc)

Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?

Is there a bug required (and linked) for this change?

Should this PR be backported?

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.

openshift-ci · 2025-03-24T12:59:07Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

danielerez · 2025-03-24T13:00:11Z

/test all

openshift-ci-robot · 2025-03-24T13:37:44Z

@danielerez: This pull request references MGMT-20207 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 bug to target the "4.19.0" version, but no target version was set.

In response to this:

When generating the install config, it should not include the entire system CA bundle. I.e. when setting the AdditionalTrustBundle[1]. However, when using a mirror regsitry (e.g. with MirrorRegistryRef[2] in ASC), the installConfigBuilder is adding the content of tls-ca-bundle.pem[3]. This pem file is created[4] by ASC controller, and includes the full system CA bundle since the bundle[5] is injected into 'cluster-trusted-ca-bundle'.

Therefore, the suggestion solution is to create a separate pem file just for the certificates specified in MirrorRegistryRef. I.e. generate a new 'user-registry-ca-bundle.pem' file, that will be included as part of AdditionalTrustBundle. These certificates will propagate into the 'user-ca-bundle' CM during cluster installation.
This will ensure that 'user-ca-bundle' CM indeed includes only custom certificates mandatory for the user, instead of the system CA bundle.

Note: as backwards compatibility, for flows like ABI, we keep a fallback to the current behaviour (i.e. include 'tls-ca-bundle.pem' only when 'user-registry-ca-bundle.pem' doesn't exist).

[1]

assisted-service/internal/installcfg/builder/builder.go

Line 275 in 40ab10d

cfg.AdditionalTrustBundle = caContent

[2]

assisted-service/vendor/github.com/openshift/assisted-service/api/v1beta1/agentserviceconfig_types.go

Line 82 in cb169a2

MirrorRegistryRef *corev1.LocalObjectReference `json:"mirrorRegistryRef,omitempty"`

[3] https://github.com/openshift/assisted-service/blob/a1c3229afee1f0f774b286283fb0d0098b9eac03/internal/common/common.go#L35
[4]

assisted-service/internal/controller/controllers/agentserviceconfig_controller.go

Line 1848 in 341f986

Path: common.MirrorRegistriesCertificateFile,

[5]

assisted-service/internal/controller/controllers/agentserviceconfig_controller.go

Line 1059 in 341f986

metav1.SetMetaDataLabel(&cm.ObjectMeta, injectTrustedCALabel, "true")

List all the issues related to this PR

New Feature

Enhancement

Bug fix

Tests

Documentation

CI/CD

What environments does this code impact?

Automation (CI, tools, etc)

Cloud

Operator Managed Deployments

None

How was this code tested?

assisted-test-infra environment

dev-scripts environment

Reviewer's test appreciated

Waiting for CI to do a full test run

Manual (Elaborate on how it was tested)

No tests needed

Checklist

Title and description added to both, commit and PR.

Relevant issues have been associated (see CONTRIBUTING guide)

This change does not require a documentation update (docstring, docs, README, etc)

Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?

Is there a bug required (and linked) for this change?

Should this PR be backported?

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.

danielerez · 2025-03-24T15:18:40Z

/test all

danielerez · 2025-03-24T15:29:36Z

/cc @carbonin

danielerez · 2025-03-24T23:49:50Z

/retest

codecov · 2025-03-25T02:44:25Z

Codecov Report

Attention: Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.29%. Comparing base (f5c6846) to head (c99b438).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/mirrorregistries/generator.go	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7448      +/-   ##
==========================================
+ Coverage   67.26%   67.29%   +0.02%     
==========================================
  Files         334      334              
  Lines       42265    42309      +44     
==========================================
+ Hits        28431    28470      +39     
- Misses      11261    11265       +4     
- Partials     2573     2574       +1

Files with missing lines	Coverage Δ
internal/common/common.go	`30.98% <ø> (ø)`
...oller/controllers/agentserviceconfig_controller.go	`84.10% <100.00%> (+0.14%)`	⬆️
internal/controller/controllers/common.go	`77.12% <ø> (ø)`
pkg/mirrorregistries/generator.go	`67.56% <80.00%> (+0.90%)`	⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

danielerez · 2025-03-26T12:32:24Z

/retest

carbonin

Just one comment, otherwise looks good.

carbonin · 2025-03-26T13:23:11Z

pkg/mirrorregistries/mirror_registry_test.go

@@ -129,6 +130,19 @@ var _ = Describe("Generator tests", func() {
 				res := m.IsMirrorRegistriesConfigured()
 				Expect(res).Should(Equal(true))
 			})
+
+			It("returns true when only the system CA exists (and registry.conf has mirrors)", func() {


I don't understand this test. Why is there a conditional and why does IsMirrorRegistriesConfigured have anything to do with the system CA, shouldn't it be based on the user one?

Oh, that's just for testing the fallback: https://github.com/openshift/assisted-service/pull/7448/files#diff-7cc7e8a2e1a64d1393f8dfcf02e682c3286f987e2391b11f9a2b05919912be54R65
I.e. to ensure we use common.SystemCertificateBundlePath when m.MirrorRegistriesCertificatePath doesn't exist. I can check the GetMirrorCA func directly if it's clearer.
(Added it here just since IsMirrorRegistriesConfigured is calling GetMirrorCA first)
The condition is only to avoid writing to the local common.SystemCertificateBundlePath.

Okay, but for this test we should know if the file is going to exist or not and expect a result accordingly. I'm just saying that there shouldn't be a conditional in the test around the Expect lines.

Yeah, fair enough. So added a new set of tests for GetMirrorCA.

carbonin · 2025-03-28T20:06:24Z

pkg/mirrorregistries/generator.go

+	bytes, err := m.fileReader(m.MirrorRegistriesCertificatePath)
+	if err != nil {
+		// fallback to tls-ca-bundle.pem (used by ABI)
+		return m.fileReader(common.SystemCertificateBundlePath)


Instead of injecting the reader function, can we set this path on the struct?
It will be less likely to confuse folks in the future that don't know why this fileReader exists

Sure, good idea.

When generating the install config, it should not include the entire system CA bundle. I.e. when setting the AdditionalTrustBundle[1]. However, when using a mirror regsitry (e.g. with MirrorRegistryRef[2] in ASC), the installConfigBuilder is adding the content of tls-ca-bundle.pem[3]. This pem file is created[4] by ASC controller, and includes the full system CA bundle since the bundle[5] is injected into 'cluster-trusted-ca-bundle'. Therefore, the suggestion solution is to create a separate pem file just for the certificates specified in MirrorRegistryRef. I.e. generate a new 'user-registry-ca-bundle.pem' file, that will be included as part of AdditionalTrustBundle. These certificates will propagate into the 'user-ca-bundle' CM during cluster installation. This will ensure that 'user-ca-bundle' CM indeed includes only custom certificates mandatory for the user, instead of the system CA bundle. Note: as backwards compatibility, for flows as ABI, we keep a fallback to the current behaviour (i.e. include 'tls-ca-bundle.pem' only when 'user-registry-ca-bundle.pem' doesn't exist). [1] https://github.com/openshift/assisted-service/blob/40ab10db5e872e519ab0a97e82fc318423feeaba/internal/installcfg/builder/builder.go#L275 [2] https://github.com/openshift/assisted-service/blob/cb169a2d2c97bb3dccd06ad4b75f2937e01f78f4/vendor/github.com/openshift/assisted-service/api/v1beta1/agentserviceconfig_types.go#L82 [3] https://github.com/openshift/assisted-service/blob/a1c3229afee1f0f774b286283fb0d0098b9eac03/internal/common/common.go#L35 [4] https://github.com/openshift/assisted-service/blob/341f9860c455cccc42741f350024d05aa72755f8/internal/controller/controllers/agentserviceconfig_controller.go#L1848 [5] https://github.com/openshift/assisted-service/blob/341f9860c455cccc42741f350024d05aa72755f8/internal/controller/controllers/agentserviceconfig_controller.go#L1059

openshift-ci · 2025-03-31T12:49:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, danielerez

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [carbonin,danielerez]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

danielerez · 2025-03-31T16:41:40Z

/cherry-pick release-ocm-2.12

openshift-cherrypick-robot · 2025-03-31T16:41:42Z

@danielerez: once the present PR merges, I will cherry-pick it on top of release-ocm-2.12 in a new PR and assign it to you.

In response to this:

/cherry-pick release-ocm-2.12

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 kubernetes-sigs/prow repository.

danielerez · 2025-03-31T16:41:47Z

/cherry-pick release-ocm-2.13

openshift-cherrypick-robot · 2025-03-31T16:41:50Z

@danielerez: once the present PR merges, I will cherry-pick it on top of release-ocm-2.13 in a new PR and assign it to you.

In response to this:

/cherry-pick release-ocm-2.13

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 kubernetes-sigs/prow repository.

openshift-ci-robot · 2025-03-31T16:59:18Z

/retest-required

Remaining retests: 0 against base HEAD 1de1416 and 2 for PR HEAD c99b438 in total

openshift-ci-robot · 2025-03-31T19:50:43Z

/retest-required

Remaining retests: 0 against base HEAD 1de1416 and 2 for PR HEAD c99b438 in total

openshift-ci-robot · 2025-03-31T23:54:31Z

/retest-required

Remaining retests: 0 against base HEAD 03b3c1c and 1 for PR HEAD c99b438 in total

danielerez · 2025-04-01T19:35:51Z

/retest

gamli75 · 2025-04-03T09:45:38Z

/retest

openshift-ci-robot · 2025-04-03T13:00:33Z

/retest-required

Remaining retests: 0 against base HEAD 1dc6632 and 2 for PR HEAD c99b438 in total

danmanor · 2025-04-03T15:59:51Z

@giladravid16 @pastequo Do you have an idea why these 2 konflux jobs are not triggering for a long time ?

openshift-ci · 2025-04-03T16:17:04Z

@danielerez: all tests passed!

Full PR test history. Your PR dashboard.

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 24, 2025

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 24, 2025

openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 24, 2025

danielerez force-pushed the user-registry-ca-bundle branch from 488e114 to 3228d83 Compare March 24, 2025 13:31

openshift-ci bot requested a review from carbonin March 24, 2025 15:29

danielerez marked this pull request as ready for review March 25, 2025 09:06

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 25, 2025

openshift-ci bot requested review from giladravid16 and linoyaslan March 25, 2025 09:06

carbonin reviewed Mar 26, 2025

View reviewed changes

danielerez force-pushed the user-registry-ca-bundle branch from 3228d83 to d2837d1 Compare March 27, 2025 11:22

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 27, 2025

carbonin reviewed Mar 28, 2025

View reviewed changes

danielerez force-pushed the user-registry-ca-bundle branch from d2837d1 to c99b438 Compare March 30, 2025 09:34

carbonin approved these changes Mar 31, 2025

View reviewed changes

openshift-ci bot assigned carbonin Mar 31, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2025

MGMT-20207: avoid adding system CA bundle to AdditionalTrustBundle #7448

Are you sure you want to change the base?

MGMT-20207: avoid adding system CA bundle to AdditionalTrustBundle #7448

Conversation

danielerez commented Mar 24, 2025 • edited Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Mar 24, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Mar 24, 2025

danielerez commented Mar 24, 2025

openshift-ci-robot commented Mar 24, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

danielerez commented Mar 24, 2025

danielerez commented Mar 24, 2025

danielerez commented Mar 24, 2025

codecov bot commented Mar 25, 2025 • edited Loading

Codecov Report

danielerez commented Mar 26, 2025

carbonin left a comment

Choose a reason for hiding this comment

carbonin Mar 26, 2025

Choose a reason for hiding this comment

danielerez Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

carbonin Mar 26, 2025

Choose a reason for hiding this comment

danielerez Mar 27, 2025

Choose a reason for hiding this comment

carbonin Mar 28, 2025

Choose a reason for hiding this comment

danielerez Mar 30, 2025

Choose a reason for hiding this comment

openshift-ci bot commented Mar 31, 2025

danielerez commented Mar 31, 2025

openshift-cherrypick-robot commented Mar 31, 2025

danielerez commented Mar 31, 2025

openshift-cherrypick-robot commented Mar 31, 2025

openshift-ci-robot commented Mar 31, 2025

openshift-ci-robot commented Mar 31, 2025

openshift-ci-robot commented Mar 31, 2025

danielerez commented Apr 1, 2025

gamli75 commented Apr 3, 2025

openshift-ci-robot commented Apr 3, 2025

danmanor commented Apr 3, 2025

openshift-ci bot commented Apr 3, 2025

danielerez commented Mar 24, 2025 •

edited

Loading

openshift-ci-robot commented Mar 24, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 24, 2025 •

edited by openshift-ci bot

Loading

codecov bot commented Mar 25, 2025 •

edited

Loading

danielerez Mar 26, 2025 •

edited

Loading