Add AdditionalTrustBundle to InstallConfig #2055

jcpowermac · 2019-07-19T19:09:59Z

To support MITM (ssl) proxy and registries add an optional
AdditionalTrustBundle string to InstallConfig.

Modified InstallConfig to include AdditionalTrustBundle
Add validation of certificate that is provided via InstallConfig
Modify BootstrapTemplateData to include AdditionalTrustBundle
Add a template for ca.crt in /etc/pki/ca-trust/source/anchors
Add update-trust.sh script to update-ca-trust if ca.crt file exists in /etc/pki/ca-trust/source/anchors
Add systemd unit to run update-trust.sh script

patrickdillon · 2019-07-19T19:23:18Z

Is there a case where user might have one cert for proxy and a different one for registry, so they need 2 bundles?

jcpowermac · 2019-07-19T19:39:33Z

Is there a case where user might have one cert for proxy and a different one for registry, so they need 2 bundles?

should be able to combine the certificate files together and use that string in the install-config.yaml.

patrickdillon · 2019-07-19T19:53:05Z

data/data/bootstrap/systemd/units/update-trust.service

maybe this systemd service is not needed if coreos/fedora-coreos-config#120 merges?

@patrickdillon nope you're right I don't think we would need it. Maybe it will be a timing issue when it makes it into RHCOS?

DanyC97 · 2019-07-21T21:21:31Z

@jcpowermac curious, MITM stands for ?

danehans · 2019-07-22T16:49:17Z

@DanyC97 MITM = Man in the Middle.

jcpowermac · 2019-07-24T16:35:24Z

@abhinavdahiya when you have a chance can you PTAL, thanks!

abhinavdahiya · 2019-07-24T16:35:24Z

data/data/bootstrap/files/etc/pki/ca-trust/source/anchors/ca.crt.template

you can add newline.

abhinavdahiya · 2019-07-24T16:37:25Z

pkg/asset/manifests/additionaltrustbundleconfig.go

these needs to be ca-bundle.crt

abhinavdahiya · 2019-07-24T16:37:33Z

pkg/asset/manifests/additionaltrustbundleconfig.go

should rename to something like user-ca-bundle

abhinavdahiya · 2019-07-24T16:38:08Z

pkg/asset/manifests/additionaltrustbundleconfig.go

name is somethings like user-ca-bundle

@abhinavdahiya that was the name based on the google doc.

so looking at
https://github.com/openshift/api/blob/d75a161a0f4ded1815ec24321fc1f8609c255cdb/config/v1/types_proxy.go#L46-L49

namespace is required to be openshift-config-managed
the key should be ca-bundle.crt

so restriction on name, and since we use this for more than proxy, user-ca-bundle like is more appropriate.

abhinavdahiya · 2019-07-24T16:48:47Z

pkg/validate/validate.go

this should probably be CABundle as that's what's is validating ie that the string is ca bundle.

abhinavdahiya · 2019-07-24T16:50:26Z

pkg/validate/validate.go

this error is most appropriate in pkg/types/validate where we are validating the field..
invalid block should be enough here.

abhinavdahiya · 2019-07-24T16:50:35Z

pkg/validate/validate.go

jcpowermac · 2019-07-24T19:50:58Z

pkg/asset/manifests/additionaltrustbundleconfig.go

@danehans do we want all the certificates in the ConfigMap or just the CA certificates?

abhinavdahiya · 2019-07-24T21:23:16Z

pkg/asset/manifests/additionaltrustbundleconfig.go

nit: extra line

abhinavdahiya · 2019-07-24T21:25:16Z

pkg/asset/manifests/additionaltrustbundleconfig.go

nit: extra line.

To support MITM (ssl) proxy and registries add an optional AdditionalTrustBundle string to InstallConfig. Modified InstallConfig to include AdditionalTrustBundle Add validation of certificate that is provided via InstallConfig Modify BootstrapTemplateData to include AdditionalTrustBundle Add a template for ca.crt in /etc/pki/ca-trust/source/anchors Add update-trust.sh script to update-ca-trust if ca.crt file exists in /etc/pki/ca-trust/source/anchors Add machineconfig ForAdditionalTrustBundle function Modified master and worker asset machines for AdditionalTrustBundle Add Tests for validate AdditionalTrustBundle Add additionaltrustbundleconfig that creates a ConfigMap that contains the Proxy CA files.

abhinavdahiya · 2019-07-25T16:35:43Z

/lgtm

openshift-ci-robot · 2019-07-25T16:36:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, jcpowermac

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya]

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

openshift-bot · 2019-07-25T18:13:48Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2019-07-25T19:39:06Z

@jcpowermac: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	`fdc518a`	link	`/test e2e-aws-scaleup-rhel7`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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/test-infra repository. I understand the commands that are listed here.

danehans · 2019-08-06T19:35:40Z

pkg/asset/manifests/additionaltrustbundleconfig.go

+			Kind:       "ConfigMap",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "openshift-config-managed",


@abhinavdahiya @jcpowermac according to the proxy workflow doc and after talking with @bparees, it appears the installer is creating the configmap in the wrong namespace. The namespace should be openshift-config instead of openshift-config-managed.

No i think it's supposed to be openshift-config-managed https://github.com/openshift/api/blob/9ef0612c775d8571388e16d1bc68c4b3df83e7fb/config/v1/types_proxy.go#L49

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 19, 2019

openshift-ci-robot requested review from jstuever and patrickdillon July 19, 2019 19:10

patrickdillon reviewed Jul 19, 2019

View reviewed changes

danehans mentioned this pull request Jul 22, 2019

Add Trusted CA to Proxy API type openshift/api#377

Merged

jcpowermac changed the title ~~[WIP] Add AdditionalTrustBundle to InstallConfig~~ Add AdditionalTrustBundle to InstallConfig Jul 24, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 24, 2019

abhinavdahiya reviewed Jul 24, 2019

View reviewed changes

data/data/bootstrap/files/etc/pki/ca-trust/source/anchors/ca.crt.template Outdated

Copy link

Contributor

abhinavdahiya Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add newline.

abhinavdahiya reviewed Jul 24, 2019

View reviewed changes

pkg/asset/manifests/additionaltrustbundleconfig.go Outdated

Copy link

Contributor

abhinavdahiya Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these needs to be ca-bundle.crt

abhinavdahiya reviewed Jul 24, 2019

View reviewed changes

pkg/asset/manifests/additionaltrustbundleconfig.go Outdated

Copy link

Contributor

abhinavdahiya Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should rename to something like user-ca-bundle

abhinavdahiya reviewed Jul 24, 2019

View reviewed changes

pkg/validate/validate.go Outdated

Copy link

Contributor

abhinavdahiya Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra line

jcpowermac force-pushed the CORS-1158 branch from b48c527 to 12a158f Compare July 24, 2019 19:17

jcpowermac commented Jul 24, 2019

View reviewed changes

abhinavdahiya reviewed Jul 24, 2019

View reviewed changes

pkg/asset/manifests/additionaltrustbundleconfig.go Outdated

Copy link

Contributor

abhinavdahiya Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra line

abhinavdahiya reviewed Jul 24, 2019

View reviewed changes

pkg/asset/manifests/additionaltrustbundleconfig.go Outdated

Copy link

Contributor

abhinavdahiya Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra line.

jcpowermac added 2 commits July 24, 2019 17:34

Updated RHCOS

9137e9a

Update vendor

fdc518a

jcpowermac force-pushed the CORS-1158 branch from 08fda78 to fdc518a Compare July 24, 2019 21:36

openshift-ci-robot assigned abhinavdahiya Jul 25, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2019

openshift-merge-robot merged commit df92892 into openshift:master Jul 25, 2019

danehans reviewed Aug 6, 2019

View reviewed changes

Add AdditionalTrustBundle to InstallConfig #2055

Add AdditionalTrustBundle to InstallConfig #2055

Uh oh!

Conversation

jcpowermac commented Jul 19, 2019

Uh oh!

patrickdillon commented Jul 19, 2019

Uh oh!

jcpowermac commented Jul 19, 2019

Uh oh!

patrickdillon Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanyC97 commented Jul 21, 2019

Uh oh!

danehans commented Jul 22, 2019

Uh oh!

jcpowermac commented Jul 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Jul 25, 2019

Uh oh!

openshift-ci-robot commented Jul 25, 2019

Uh oh!

openshift-bot commented Jul 25, 2019

Uh oh!

openshift-ci-robot commented Jul 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

patrickdillon Jul 19, 2019 •

edited

Loading