Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds design doc for bundle CRD #2

Merged
merged 4 commits into from Nov 23, 2021
Merged

Adds design doc for bundle CRD #2

merged 4 commits into from Nov 23, 2021

Conversation

JoshVanL
Copy link
Collaborator

Signed-off-by: joshvanl vleeuwenjoshua@gmail.com

Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
@jetstack-bot jetstack-bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Jul 16, 2021
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoshVanL

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:

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

@jetstack-bot jetstack-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 16, 2021
Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

I've left a bunch of comments for discussion; this is a great starting point and it's an interesting problem space.

(Feel free to ignore the nitpicks, they're not important but I thought it would be worth highlighting them)

design/20210715.bundle-crd.md Outdated Show resolved Hide resolved
design/20210715.bundle-crd.md Outdated Show resolved Hide resolved
design/20210715.bundle-crd.md Show resolved Hide resolved
Comment on lines +121 to +123
While sources currently support `ConfigMap`, `Secret` and `InLine`, it could be
extended to other resources in future, or even remote servers in the aid of
custom informers.
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: I think Bundle would be tremendously improved by supporting the ability to include publicly trusted CAs, such as Mozilla's list. I worry that if we don't support a publicly trusted list, people will hack around it and get themselves into all kinds of trouble by statically including a bundle which will inevitably never get updated.

I don't know if such a publicly trusted list is required for a v1 release, but I think it would be a huge improvement to have it in place; it'd enable Bundle to essentially be the one-stop-shop for all trust needs for the vast majority of workloads.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In two minds about this.

While I absolutely agree this would be a great addition, I feel we run the risk of;

  1. The baked in trust bundles becoming out of date meaning the tool has an expiry data if left unmaintained,
  2. Open a flood gate of "also add this default trust store" feature creep.

design/20210715.bundle-crd.md Outdated Show resolved Hide resolved
design/20210715.bundle-crd.md Outdated Show resolved Hide resolved
design/20210715.bundle-crd.md Outdated Show resolved Hide resolved
design/20210715.bundle-crd.md Outdated Show resolved Hide resolved
design/20210715.bundle-crd.md Outdated Show resolved Hide resolved
design/20210715.bundle-crd.md Show resolved Hide resolved
@JoshVanL JoshVanL closed this Jul 16, 2021
@JoshVanL JoshVanL reopened this Jul 16, 2021
@JoshVanL
Copy link
Collaborator Author

Thank you for the review @SgtCoDFish! Hopefully have covered all your comments

is a non-goal

Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
@SgtCoDFish
Copy link
Member

Replying to this here because it'll be easier to track. This is quite long; maybe we should have a call to discuss this.

While I absolutely agree this would be a great addition, I feel we run the risk of;

  1. The baked in trust bundles becoming out of date meaning the tool has an expiry data if left unmaintained,
  2. Open a flood gate of "also add this default trust store" feature creep.

I definitely understand why you'd be in two minds, but I'm not hugely concerned by either of these things.

  1. The baked in trust bundles becoming out of date meaning the tool has an expiry [date] if left unmaintained,

I think there's a case to be made for allowing the tool to dynamically fetch updated trust stores at runtime, although there are definitely questions which would need to be answered to implement that (where to fetch from, how often, how to surface an updated trust bundle to cluster admins, etc).

Whether or not we implement dynamic updating at runtime, I don't think most cluster operators would be overly concerned by using a baked trust store, since a lot of them are just going to be trusting whatever's provided in their container images, be that alpine, centos, debian, whatever.

By "centralising" on cert-manager/trust, they potentially only have to update one trust store (via a Bundle) rather than having to keep track of the disparate trust stores in their running applications on different distros.

Plus if we stopped maintaining cert-manager/trust, I think they'd be fine; it's open source and they could easily create new versions with updated baked trust stores.

  1. Open a flood gate of "also add this default trust store" feature creep.

In theory I'd be concerned about this, but I don't think in practice there are actually many lists of publicly trusted roots that people would want in the first place. Most Linux distros that I know of just use Mozilla's list, and the only other major list I'd expect to hear about is Chrome/Chromium's. If in the worst case people start requesting a bunch of stuff we don't want to add, I'm personally quite comfortable saying "no" 😉

UX Without Publicly Trusted Roots

My fear is that if we don't add support for a publicly trusted list of roots, the UX will suffer massively. The ideal UX for TLS clients connecting to servers IMO is:

  • Create Bundle resource + mount resulting target ConfigMap into their containers
  • Use TLS as normal both with public and private CAs

That is, they could mount the trust store and replace the system trust store for their container - so they'd not need to modify RootCAs in tls.Config, --ca-file in curl or whatever the equivalent is for their language / tool.

They could combine the mounted ConfigMap and the system trust store in the container, but that requires them to run something like this at container start-up time, which is a pain and is error prone.

Basically, my vision here is that cert-manager/trust could be the one-stop-shop for trust everywhere in a cluster, providing visibility and a central place to update trust stores.

@JoshVanL
Copy link
Collaborator Author

/retest

@SgtCoDFish
Copy link
Member

/lgtm

I don't think holding this design doc PR open is providing any value - we can always discuss potential features in their own issues. Specifically I'll create an issue from this comment.

In any case, this doc is really good and well written as-is, so let's merge!

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2021
@jetstack-bot jetstack-bot merged commit a542ad7 into main Nov 23, 2021
@SgtCoDFish SgtCoDFish deleted the design-bundle-crd branch June 1, 2023 16:37
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. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants