search

knative-extensions / control-protocol

Control protocol to enable interaction between control plane and data plane without redeploy
Apache License 2.0
2 stars 26 forks source link

Adding namespace to SAN #265

Closed davidhadas closed 1 year ago

davidhadas commented 1 year ago

Fixes #264

This PR aims at a minimalistic change

  1. Add namespace SANs to certificates + modify tests accordingly.
  2. Since it is meant to be used also for mTLS, the produced certificates need to serve dual use - both as a client certificate and as a server certificate. For example, the Activator will use this certificate as both client and server. This resulted in merging the creation of a client template with that of a server template.

Note: While working on this PR many other opportunities to merge code poped up. The terminology used by the code (data-plane, control-plane) is somewhat unclear and it is hard to see the relationship between the naming and the certificates created and their use. These are left for a separate PR after a discussion of the correct terminology and the intent for the separation between control-plane and data-plane certificates.

Items for review:

codecov[bot] commented 1 year ago

Codecov Report

Merging #265 (4ed838c) into main (e5eadf2) will decrease coverage by 0.44%. The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #265      +/-   ##
==========================================
- Coverage   73.11%   72.68%   -0.44%     
==========================================
  Files          23       23              
  Lines        1380     1380              
==========================================
- Hits         1009     1003       -6     
- Misses        315      319       +4     
- Partials       56       58       +2
Impacted Files Coverage Δ
pkg/network/tls.go 0.00% <0.00%> (ø)
pkg/certificates/reconciler/certificates.go 69.52% <100.00%> (ø)
pkg/reconciler/tls_dialer_factory.go 85.36% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

davidhadas commented 1 year ago

/retest

davidhadas commented 1 year ago

Current code terminology is a bit confusing. Is there a clear distinction between "control-plane" certificates "data-plane" certificates? Why are they separated in code? Does it really matter from TLS and certificate point of view if the entity (client or server) is classified as a control or data?

I would like to first have a fix for SANs in this PR, then consider if we need to open an issue and create a second PR to rearrange the code by removing the current separation of "control-plane" vs. "data-plane" certificates if this is not needed. Alternatively, we may need to add some remarks and make it more clear or improve naming in this project to make it clearer when we refer to clients or server etc..

davidhadas commented 1 year ago

/retest

davidhadas commented 1 year ago

/cc @ReToCode @nak3 @evankanderson

davidhadas commented 1 year ago

CommonName: "control-plane" I think its not ideal, as this is also used to sign data-plane certs and potentially more in the future. Could it be something like knative-internal-ca?

This requires changes in other packages as well. If this is ok, I prefer to do that in a separate PR after we make sure that the new names will work system-wide. Or, that we can add the new names (e.g. knative-internal-ca) now and delete the old names later in a separate PR after making the system work with the new names.

ReToCode commented 1 year ago

This requires changes in other packages as well. If this is ok, I prefer to do that in a separate PR after we make sure that the new names will work system-wide

I'm fine with that. But I think we will, for a short period of time, break some stuff in other repos anyways. We should just make sure that the PRs to fix it are available in a timely manner.

davidhadas commented 1 year ago

  • Bite the bullet and add a new CreateServerCert method which takes a list of SANs. Added CreateCert as it creates cert for both client and server

  • Expose fakeDnsName as LegacyFakeDnsName Already exposed as FakeDnsName, changed to LegacyFakeDnsName

  • Migrate callers to CreateServerCert, and add LegacyFakeDnsName to the list of SANs provided at the caller site. Migrated all internal callers, we will need to now migrate any other caller from other packages.

  • (Add a flag at the caller site to be able to drop LegacyFakeDnsName in the future) I think a PR will do it.

davidhadas commented 1 year ago

Do we want the namespace SAN to be \<namespace> or \<prefix>-\<namespace>? For some reason, the author of this PR followed knative-\<namespace> but can't justify why this prefix is helpful... Should we leave it? or change it?

evankanderson commented 1 year ago

Do we want the SAN to be or -? For some reason, the author of this PR followed knative- but cant justify why this prefix is helpful.

I think the prefix is helpful for helping users connect the dots if the prefix is sufficiently specific.

knative-prow[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidhadas, evankanderson, 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: - ~~[OWNERS](https://github.com/knative-sandbox/control-protocol/blob/main/OWNERS)~~ [evankanderson] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
davidhadas commented 1 year ago

I suggest we fix this here - see https://github.com/knative-sandbox/control-protocol/pull/269 And later follow up with a more gradual change in dependencies.