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

Add documentation for Serving Encryption #5804

Merged
merged 8 commits into from
Apr 30, 2024

Conversation

ReToCode
Copy link
Member

@ReToCode ReToCode commented Jan 3, 2024

Fixes #5805
Fixes #5800

Proposed Changes

  • Adds documentation for new Serving encryption features: https://github.com/orgs/knative/projects/63/views/1
  • Consolidates existing pages to new structure
  • Adds an overview of Serving encryption parts
  • Renames the features to align to the parts in overview
  • Delegates the documentation to certbot, certmanager and trust-manager instead of repeating it inline
  • Fixes broken documentation links

@knative-prow knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 3, 2024
Copy link

netlify bot commented Jan 3, 2024

Deploy Preview for knative ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 5474f29
🔍 Latest deploy log https://app.netlify.com/sites/knative/deploys/6630ceddd3150a000873d03a
😎 Deploy Preview https://deploy-preview-5804--knative.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@knative-prow knative-prow bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 3, 2024
Copy link

knative-prow bot commented Jan 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ReToCode

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

@knative-prow knative-prow bot requested review from psschwei and skonto January 3, 2024 14:45
@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 3, 2024
@ReToCode ReToCode changed the title [WIP] add documentation for Serving Encryption Add documentation for Serving Encryption Jan 4, 2024
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 4, 2024
@ReToCode
Copy link
Member Author

ReToCode commented Jan 4, 2024

/hold we still need to wait for all the feature PRs to be merged, but it should be fine for a first round of reviews:

/assign @KauzClay
/assign @skonto
/assign @dprotaso

@ReToCode
Copy link
Member Author

ReToCode commented Jan 4, 2024

/cc @pierDipi

Copy link
Contributor

@KauzClay KauzClay 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 been away from this for a bit, so mostly just questions for my understanding, but it's cool to see this all coming together!

to automatically distribute the CA bundles. Please refer to their documentation for more information on how to do this.


### Trust during rotation
Copy link
Contributor

Choose a reason for hiding this comment

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

question:

just want to verify that I understand this page.

  • cluster-local clients must be implemented to consume the CA data (this isn't Knative's business).
  • The Knative system components, though, have been implemented so that, provided the right configmap exists, it can pull in and populate the right components with the right CA data.
  • With the trust bundle, there is one "place" to put CA data so it can be consumed by many.
  • Even with that, when rotating, it is still up to the user to coordinate adding in the new CA data/removing the old CA data and giving enough time for things to switch over

Does that sound correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is exactly how it is 👍

## Trust

!!! warning
A quick note on trust, Knative will automatically trust the CA that signed the Certificates, if the cert-manager issuer allows
Copy link
Contributor

Choose a reason for hiding this comment

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

question:

I'm not sure I understand this line.

Is it saying that if your Secret has a ca.crt field, then Knative will trust it?

Could you please help me understand what the implications of that are?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that's it. Cert-Manager has different issuers. Some of them add the CA to ca.crt in the Secrets, other do not (like an external ACME CA will never populate the CA there). For convenience, Knative will also trust the ca.crt to make a simple setup work out of the box (without an additional CA bundle in a ConfigMap).

@ReToCode
Copy link
Member Author

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2024
@ReToCode
Copy link
Member Author

With the last Serving PR merged, mind taking a look at this?
@skonto @dprotaso

@ReToCode
Copy link
Member Author

ReToCode commented Apr 4, 2024

Updated the documentation to reflect that net-certmanager is now integrated in Serving. @skonto PTAL.

@psschwei
Copy link
Contributor

/unassign

@psschwei psschwei removed their request for review April 18, 2024 23:18
@ReToCode
Copy link
Member Author

@skonto mind taking another look? We can use downstream enablement of this feature to test if the doc is complete.

@@ -0,0 +1,50 @@
# Serving Encryption Overview
Copy link
Contributor

@skonto skonto Apr 30, 2024

Choose a reason for hiding this comment

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

Should we call it native encryption or something. I think we need to distinguish between what Istio offers with a full mesh (which we support) and what it is documented here (ingress only + tls in Knative components.)
As a new Knative user it would be nice to know the options I have for encryption and security in general.
Maybe a table would help with key features/capabilities for each option.

Currently, all control-plane traffic (including Kubernetes PreStopHooks and metadata like metrics) is not encrypted.

## The parts in detail
The different parts are independent of each other and (can) use different Certificate Authorities to sign certificates.
Copy link
Contributor

@skonto skonto Apr 30, 2024

Choose a reason for hiding this comment

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

From a user perspective what is the value of allowing part 2, without part 3? What setup do we recommend here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it is really. It just gives the user the option to do the transition in steps, e.g. maybe they already have 1, they could start with 3 and add 2 once they have the CA stuff figured out. There is not really a recommendation, this highly depends on the environment and the requirements of a company.

* Accessing it from a `Secret`/`ConfigMap` via K8s API

If reloading certificates without downtime is important for your client, the workload must either watch changes on the K8s resource (Secret/ConfigMap) or watch the filesystem.
If the workload is watching the filesystem, it is important to note that using `ionotify` to catch changing Secrets/ConfigMaps is not very reliable on K8s. Tests have shown that it is more reliable to regularly poll and check the certificate on the filesystem for changes.
Copy link
Contributor

@skonto skonto Apr 30, 2024

Choose a reason for hiding this comment

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

We will need to have an example on how to do the rotation. I think a minimum example should include manual rotation based on workload restarting (as others have). That would be appreciated by operators I suppose to start with something.

@skonto
Copy link
Contributor

skonto commented Apr 30, 2024

I am ok merging this to get early feedback, also we will test it downstream anyway. I have some comments for follow up work.

@ReToCode
Copy link
Member Author

@skonto updated as discussed.

@skonto
Copy link
Contributor

skonto commented Apr 30, 2024

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2024
@knative-prow knative-prow bot merged commit 59339cd into knative:main Apr 30, 2024
19 checks passed
spec:
ca:
secretName: knative-selfsigned-ca
---
Copy link
Contributor

Choose a reason for hiding this comment

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

In this example, I would add some comments to the two ClusterIssuer to explain what they are for and where they are used. Also, I probably would align the naming so that 'selfsigned' is on the same positions (so either selfsigned-cluster-issuer/selfsigned-knative-issuer or cluster-selfsigned-issuer/knative-selfsigned-issuer

@rhuss
Copy link
Contributor

rhuss commented Apr 30, 2024

Great documentation! Thanks a lot, @ReToCode !

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. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add documentation for Serving Encryption Routing across multiple Knative services for HTTPS/TLS
6 participants