Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Oct 16, 2018

This allows you to connect your cluster to external services whose X.509 certificates are signed by a certificate authority that doesn't belong to the OSes list of certificate authorities. For example, it allows you to use a local registry with a self-signed certificate.

While this might seem like a use-case for a custom root CA:

  • We don't currently install the root CA for general use on the bootstrap node, so the bootstrap node would still have trouble pulling images from a local registry with a nonstandard CA.

  • The root CA for the cluster and the CA for a local registry are really completely independent. I don't want to make a caller have to provide a complete set of custom certs and keys for all of our cluster-specific roles if all they need to do is authenticate a local registry.

The commit is a work in progress, because:

  • This may be something of a niche use-case, I don't really know. If it is, it may not belong in the install-config. I guess users could edit generated ignition files and inject these directly once pkg/asset: Introduce Load() into the Asset interface that loads assets (from disk) #374 lands. Or they could edit the generated install config to inject these there. However this shakes out, the environment variable I'm using here is almost certainly not going to be the final UI.

  • I'm currently ignoring installConfig.Machines[*].CertificateAuthorities while I play around with mirroring. If it works out, I'll go over this again and clean up the pointerIgnitionConfig API to allow for selecting the appropriate installConfig.Machines entry.

This allows you to connect your cluster to external services whose
X.509 certificates are signed by a certificate authority that doesn't
belong to the OSes list of certificate authorities.  For example, it
allows you to use a local registry with a self-signed certificate.

While this might seem like a use-case for a custom root CA:

* We don't currently install the root CA for general use on the
  bootstrap node, so the bootstrap node would still have trouble
  pulling images from a local registry with a nonstandard CA.

* The root CA for the cluster and the CA for a local registry are
  really completely independent.  I don't want to make a caller have
  to provide a complete set of custom certs and keys for all of our
  cluster-specific roles if all they need to do is authenticate a
  local registry.

The commit is a work in progress, because:

* This may be something of a niche use-case, I don't really know.  If
  it is, it may not belong in the install-config.  I guess users could
  edit generated ignition files and inject these directly once openshift#374
  lands.  Or they could edit the generated install config to inject
  these there.  However this shakes out, the environment variable I'm
  using here is almost certainly not going to be the final UI.

* I'm currently ignoring
  `installConfig.Machines[*].CertificateAuthorities` while I play
  around with mirroring [1].  If it works out, I'll go over this again
  and clean up the `pointerIgnitionConfig` API to allow for selecting
  the appropriate `installConfig.Machines` entry.

[1]: https://github.com/wking/openshift-installer/blob/mirror-docs/docs/user/local-mirror.md
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2018
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 16, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wking

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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2018
@wking
Copy link
Member Author

wking commented Oct 16, 2018

Hmm, the CertificateAuthorities Ignition setting is not going to do what I want. The "when fetching over https" docs are actually "when fetching ignition input over https", because the backing code uses SystemCertPool which has:

SystemCertPool returns a copy of the system cert pool.

Any mutations to the returned pool are not written to disk and do not affect any other pool.

So installing a new system cert where CRI-O will find it will probably need custom scripting. Or maybe we can set append to stuff the cert(s) into /etc/pki/tls/certs/ca-bundle.crt (which is where curl, at least, is looking)?

@crawford
Copy link
Contributor

@wking that's correct. Everything in the ignition section of the config just affects the current invocation of Ignition. If you want to persist the root CA to disk, you'll need to write it there.

@ericavonb
Copy link

@openshift/sig-security

@mrogers950
Copy link

This may be something of a niche use-case, I don't really know.

The ansible installer's openshift_additional_ca option did this, so we should definitely keep the ability to add extra CAs on install time. But we should take care that the additional CAs do not get confused with the root CA that corresponds with the root signing key, and that when we get around to doing rotation of the root CA we can keep the added ones available.

Machines []MachinePool `json:"machines"`

// DefaultCertificateAuthorities is the default slice of additional
// PEM-encoded certificated to add to machines (in addition to the

Choose a reason for hiding this comment

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

typo: certificated -> certificates

@crawford
Copy link
Contributor

crawford commented Nov 2, 2018

@wking is this abandoned? Can I carry this across the finish line?

@wking
Copy link
Member Author

wking commented Nov 2, 2018

@wking is this abandoned? Can I carry this across the finish line?

I still want this, it just seems like low priority to support faster, mostly-offline libvirt registries for devs. If you want to pick it up and get cert injection working, that's fine with me. Otherwise, I hope to get back to this next week.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2018
@openshift-bot
Copy link
Contributor

@wking: PR needs rebase.

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.

@crawford
Copy link
Contributor

crawford commented Dec 3, 2018

@wking I'm going to close this for now. We can reopen once one of us have time to take another crack.

/close

@openshift-ci-robot
Copy link
Contributor

@crawford: Closed this PR.

Details

In response to this:

@wking I'm going to close this for now. We can reopen once one of us have time to take another crack.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants