Add internal trust flag to config #778

davidhadas · 2023-03-03T17:58:02Z

Fixes #777

Signed-off-by: David Hadas <david.hadas@gmail.com>

codecov · 2023-03-03T18:03:25Z

Codecov Report

Patch coverage: 81.81% and project coverage change: -0.23 ⚠️

Comparison is base (68725bd) 94.80% compared to head (3e484ba) 94.57%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #778      +/-   ##
==========================================
- Coverage   94.80%   94.57%   -0.23%     
==========================================
  Files          41       41              
  Lines        1231     1253      +22     
==========================================
+ Hits         1167     1185      +18     
- Misses         52       56       +4     
  Partials       12       12

Impacted Files	Coverage Δ
pkg/config/config.go	`84.44% <81.81%> (-0.37%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: David Hadas <david.hadas@gmail.com>

evankanderson

I'm going to continue to object to the idea that verifying a server SAN and CA does not provide MITM protection, but I'm happy to enable the internal-trust boolean for future mutual TLS.

config/config-network.yaml

pkg/config/config.go

evankanderson · 2023-03-16T15:29:49Z

config/config-network.yaml

-    # NOTE: This flag is in an alpha state and is mostly here to enable internal testing
-    #       for now. Use with caution.
-    internal-encryption: "false"
+  _example: "################################\n#                              #\n#    EXAMPLE CONFIGURATION     #\n#                              #\n################################\n\n# This block is not actually functional configuration,\n# but serves to illustrate the available configuration\n# options and document them in a way that is accessible\n# to users that `kubectl edit` this config map.\n#\n# These sample configuration options may be copied out of\n# this example block and unindented to be in the data block\n# to actually change the configuration.\n\n# ingress-class specifies the default ingress class\n# to use when not dictated by Route annotation.\n#\n# If not specified, will use the Istio ingress.\n#\n# Note that changing the Ingress class of an existing Route\n# will result in undefined behavior.  Therefore it is best to only\n# update this value during the setup of Knative, to avoid getting\n# undefined behavior.\ningress-class: \"istio.ingress.networking.knative.dev\"\n\n# certificate-class specifies the default Certificate class\n# to use when not dictated by Route annotation.\n#\n# If not specified, will use the Cert-Manager Certificate.\n#\n# Note that changing the Certificate class of an existing Route\n# will result in undefined behavior.  Therefore it is best to only\n# update this value during the setup of Knative, to avoid getting\n# undefined behavior.\ncertificate-class: \"cert-manager.certificate.networking.knative.dev\"\n\n# namespace-wildcard-cert-selector specifies a LabelSelector which\n# determines which namespaces should have a wildcard certificate\n# provisioned.\n#\n# Use an empty value to disable the feature (this is the default):\n#   namespace-wildcard-cert-selector: \"\"\n#\n# Use an empty object to enable for all namespaces\n#   namespace-wildcard-cert-selector: {}\n#\n# Useful labels include the \"kubernetes.io/metadata.name\" label to\n# avoid provisioning a certifcate for the \"kube-system\" namespaces.\n# Use the following selector to match pre-1.0 behavior of using\n# \"networking.knative.dev/disableWildcardCert\" to exclude namespaces:\n#\n# matchExpressions:\n# - key: \"networking.knative.dev/disableWildcardCert\"\n#   operator: \"NotIn\"\n#   values: [\"true\"]\nnamespace-wildcard-cert-selector: \"\"\n\n# domain-template specifies the golang text template string to use\n# when constructing the Knative service's DNS name. The default\n# value is \"{{.Name}}.{{.Namespace}}.{{.Domain}}\".\n#\n# Valid variables defined in the template include Name, Namespace, Domain,\n# Labels, and Annotations. Name will be the result of the tagTemplate\n# below, if a tag is specified for the route.\n#\n# Changing this value might be necessary when the extra levels in\n# the domain name generated is problematic for wildcard certificates\n# that only support a single level of domain name added to the\n# certificate's domain. In those cases you might consider using a value\n# of \"{{.Name}}-{{.Namespace}}.{{.Domain}}\", or removing the Namespace\n# entirely from the template. When choosing a new value be thoughtful\n# of the potential for conflicts - for example, when users choose to use\n# characters such as `-` in their service, or namespace, names.\n# {{.Annotations}} or {{.Labels}} can be used for any customization in the\n# go template if needed.\n# We strongly recommend keeping namespace part of the template to avoid\n# domain name clashes:\n# eg. '{{.Name}}-{{.Namespace}}.{{ index .Annotations \"sub\"}}.{{.Domain}}'\n# and you have an annotation {\"sub\":\"foo\"}, then the generated template\n# would be {Name}-{Namespace}.foo.{Domain}\ndomain-template: \"{{.Name}}.{{.Namespace}}.{{.Domain}}\"\n\n# tagTemplate specifies the golang text template string to use\n# when constructing the DNS name for \"tags\" within the traffic blocks\n# of Routes and Configuration.  This is used in conjunction with the\n# domainTemplate above to determine the full URL for the tag.\ntag-template: \"{{.Tag}}-{{.Name}}\"\n\n# Controls whether TLS certificates are automatically provisioned and\n# installed in the Knative ingress to terminate external TLS connection.\n# 1. Enabled: enabling auto-TLS feature.\n# 2. Disabled: disabling auto-TLS feature.\nauto-tls: \"Disabled\"\n\n# Controls the behavior of the HTTP endpoint for the Knative ingress.\n# It requires autoTLS to be enabled.\n# 1. Enabled: The Knative ingress will be able to serve HTTP connection.\n# 2. Redirected: The Knative ingress will send a 301 redirect for all\n# http connections, asking the clients to use HTTPS.\n#\n# \"Disabled\" option is deprecated.\nhttp-protocol: \"Enabled\"\n\n# rollout-duration contains the minimal duration in seconds over which the\n# Configuration traffic targets are rolled out to the newest revision.\nrollout-duration: \"0\"\n\n# autocreate-cluster-domain-claims controls whether ClusterDomainClaims should\n# be automatically created (and deleted) as needed when DomainMappings are\n# reconciled.\n#\n# If this is \"false\" (the default), the cluster administrator is\n# responsible for creating ClusterDomainClaims and delegating them to\n# namespaces via their spec.Namespace field. This setting should be used in\n# multitenant environments which need to control which namespace can use a\n# particular domain name in a domain mapping.\n#\n# If this is \"true\", users are able to associate arbitrary names with their\n# services via the DomainMapping feature.\nautocreate-cluster-domain-claims: \"false\"\n\n# If true, networking plugins can add additional information to deployed\n# applications to make their pods directly accessible via their IPs even if mesh is\n# enabled and thus direct-addressability is usually not possible.\n# Consumers like Knative Serving can use this setting to adjust their behavior\n# accordingly, i.e. to drop fallback solutions for non-pod-addressable systems.\n#\n# NOTE: This flag is in an alpha state and is mostly here to enable internal testing\n#       for now. Use with caution.\nenable-mesh-pod-addressability: \"false\"\n\n# mesh-compatibility-mode indicates whether consumers of network plugins\n# should directly contact Pod IPs (most efficient), or should use the\n# Cluster IP (less efficient, needed when mesh is enabled unless\n# `enable-mesh-pod-addressability`, above, is set).\n# Permitted values are:\n#  - \"auto\" (default): automatically determine which mesh mode to use by trying Pod IP and falling back to Cluster IP as needed.\n#  - \"enabled\": always use Cluster IP and do not attempt to use Pod IPs.\n#  - \"disabled\": always use Pod IPs and do not fall back to Cluster IP on failure.\nmesh-compatibility-mode: \"auto\"\n\n# Defines the scheme used for external URLs if autoTLS is not enabled.\n# This can be used for making Knative report all URLs as \"HTTPS\" for example, if you're\n# fronting Knative with an external loadbalancer that deals with TLS termination and\n# Knative doesn't know about that otherwise.\ndefault-external-scheme: \"http\"\n\n# internal-encryption indicates whether internal traffic is encrypted or not.\n# Using internal-encryption does not protect against MITM attacks\n# If this is \"true\", the following traffic are encrypted:\n#  - ingress to activator\n#  - ingress to queue-proxy\n#  - activator to queue-proxy\n#\n# NOTE: This flag is in an alpha state and is mostly here to enable internal testing\n#       for now. Use with caution.\ninternal-encryption: \"false\"\n\n# internal-trust indicates whether internal traffic is encrypted and mutual trust is achieved.\n# Using internal-trust protect against MITM attacks and ensures that all requests received\n# will go via the ingress and will come from an approved set of sources \n# (e.g. KServ from recognized namespaces)\n# If this is \"true\", the following traffic is encrypted and client authentication is verified with mutual TLS:\n#  - ingress to activator\n#  - ingress to queue-proxy\n#  - activator to queue-proxy\n#\n# NOTE: This flag is in an alpha state and is mostly here to enable internal testing\n#       for now. Use with caution.\ninternal-trust: \"false\"\n"


This got corrupted to containing internal newlines...

yes, I am trying to recover

krsna-m · 2023-03-22T20:11:44Z

the yaml lint failure is due to too many fixer warning which will be fixed once this knative/actions#120 lands. The golang linter, I have no idea why it is checking other files not being changed, my local test krsna-m#1 does not have that issue.

davidhadas · 2023-03-22T20:30:56Z

the yaml lint failure is due to too many fixer warning which will be fixed once this knative/actions#120 lands. The golang linter, I have no idea why it is checking other files not being changed, my local test kvmware#1 does not have that issue.

I changed the file that is being reported - but the warnings are not in the lines changed.
Lets wait for the update and hopefully we will be fine

davidhadas · 2023-03-23T07:47:37Z

/retest

davidhadas · 2023-03-23T20:39:56Z

/retest

davidhadas · 2023-03-24T19:38:16Z

/hold
See discussion at knative/serving#13764
The plan is to merge this flag with the flag now discussed there.

davidhadas · 2023-03-27T20:23:12Z

/retest

evankanderson

I'm being picky about names because these are presumably going to be interfaces that administrators will continue to use for years.

I'm 👍 on the general intent of this change at this point.

config/config-network.yaml

evankanderson · 2023-03-28T04:52:54Z

config/config-network.yaml

+    # internal-trust = "identity" same as "mutual" and also adds sender identity verified by the Kingress and forwarded to Kserv
+    #
+    # NOTE: This flag is in an alpha state and is mostly here to enable internal testing for now. Use with caution.
+    internal-trust: "none"


I'm not sure that "trust" is the right word here -- maybe you mean "internal-auth" or "internal-verification"? We're establishing trust, but the variable name could also be seen as declaring trust without an enforcement mechanism.

I'm not sure that "trust" is the right word here -- maybe you mean "internal-auth" or "internal-verification"? We're establishing trust, but the variable name could also be seen as declaring trust without an enforcement mechanism.

It is ok to change trust, but it seems better than auth even verification.
since it is suggested to control if we use any description and if we do use encryption, then, verifications do we do.

pkg/config/config.go

evankanderson · 2023-03-28T04:55:32Z

pkg/config/config.go

@@ -295,6 +304,7 @@ func NewConfigFromMap(data map[string]string) (*Config, error) {
 		cm.AsBool(EnableMeshPodAddressabilityKey, &nc.EnableMeshPodAddressability),
 		cm.AsString(DefaultExternalSchemeKey, &nc.DefaultExternalScheme),
 		cm.AsBool(InternalEncryptionKey, &nc.InternalEncryption),
+		cm.AsString(InternalTrustKey, &nc.InternalTrust),


Not your fault, but I wish we had AsEnum here.

(Don't take action on this gripe in this PR)

davidhadas · 2023-04-30T11:09:28Z

/unhold
We need to complete this to start making changes to different packages that will now depend on the flags

davidhadas · 2023-04-30T11:10:17Z

/assign nak3

nak3 · 2023-05-01T01:35:42Z

config/config-network.yaml

+    # internal-controlplane-trust = "mutual" ensures control messages are encrypted using mTLS (client and server authenticate each other)
+    #
+    # NOTE: This flag is in an alpha state and is mostly here to enable internal testing for now. Use with caution.
+    internal-controlplane-trust: "disabled"


I think I understood about internal-dataplane-trust but I am still not sure how internal-controlplane-trust will be used. Could you share the link docs or github issue for your design/idea where I can refer to?

@nak3
AFAIK, we have little on this right now.
We understand that we need to start building control plane trust (after we complete the more critical data plane trust)
And we define a flag that will allow the introduction of such work moving forward.

The little we do have is now summarized in https://docs.google.com/document/d/19R0YOBAZfed2bl99-fdUnQ89cz3hoPY6KJNxhn6b40A/edit#heading=h.n8a530nnrb - which is incomplete and need more contributions.

config/config-network.yaml

pkg/config/config.go

config/config-network.yaml

nak3 · 2023-05-01T02:02:07Z

config/config-network.yaml

    #
    # NOTE: This flag is in an alpha state and is mostly here to enable internal testing
    #       for now. Use with caution.
    internal-encryption: "false"
+
+    # internal-dataplane-trust indicates the level of trust established in the knative data-plane.


@davidhadas @evankanderson
Do we need internal- prefix for internal-controlplane-trust and internal-dataplane-trust ?
I feel controlplane-trust and dataplane-trust are enough and they implicate "internal".

I am ok with removing 'internal',
I was following the original internal-encryption flag.

nak3 · 2023-05-02T02:54:10Z

/lgtm
/approve

knative-prow · 2023-05-02T02:54:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidhadas, nak3

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~~ [nak3]

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

Add internal trust flag to config

ec8db3a

Signed-off-by: David Hadas <david.hadas@gmail.com>

knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 3, 2023

knative-prow bot requested review from izabelacg and jsanin-vmw March 3, 2023 17:58

nits

07900f2

Signed-off-by: David Hadas <david.hadas@gmail.com>

evankanderson reviewed Mar 16, 2023

View reviewed changes

config/config-network.yaml Outdated Show resolved Hide resolved

config/config-network.yaml Outdated Show resolved Hide resolved

pkg/config/config.go Outdated Show resolved Hide resolved

davidhadas added 2 commits March 16, 2023 17:03

text improvements

c5ada7d

text improvements

c109b6b

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 16, 2023

evankanderson reviewed Mar 16, 2023

View reviewed changes

text improvements

b490085

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 16, 2023

davidhadas requested review from evankanderson and removed request for izabelacg and jsanin-vmw March 16, 2023 21:13

davidhadas closed this Mar 22, 2023

davidhadas reopened this Mar 22, 2023

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 24, 2023

davidhadas mentioned this pull request Mar 24, 2023

Change internal-encryption to use no verification of the certificate knative/serving#13764

Closed

multi-level flag

d8447c9

davidhadas closed this Mar 27, 2023

davidhadas reopened this Mar 27, 2023

repush

e5a021c

evankanderson reviewed Mar 28, 2023

View reviewed changes

davidhadas closed this Mar 29, 2023

davidhadas reopened this Mar 29, 2023

seperate flags for control and data planes

44d7484

davidhadas requested a review from evankanderson March 30, 2023 05:15

davidhadas added 2 commits April 30, 2023 12:05

Merge branch 'knative:main' into internalTrustFlag

8577d25

refreshed

61c3c1b

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 30, 2023

knative-prow bot assigned nak3 Apr 30, 2023

nak3 reviewed May 1, 2023

View reviewed changes

string as enum

ca7681d

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 1, 2023

update-deps

3e484ba

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2023

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 2, 2023

knative-prow bot merged commit 3f4627e into knative:main May 2, 2023

KauzClay mentioned this pull request May 22, 2023

refactor: use new dataplane-trust flag and new certificate name ServingRoutingCertName knative-extensions/net-contour#912

Merged

KauzClay mentioned this pull request Jun 2, 2023

Cannot change TLS minimum version to 1.3 for internal encryption (between Ingress to Activator) knative/serving#14057

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add internal trust flag to config #778

Add internal trust flag to config #778

davidhadas commented Mar 3, 2023

codecov bot commented Mar 3, 2023 •

edited

Loading

evankanderson left a comment

evankanderson Mar 16, 2023

davidhadas Mar 16, 2023

krsna-m commented Mar 22, 2023

davidhadas commented Mar 22, 2023

davidhadas commented Mar 23, 2023

davidhadas commented Mar 23, 2023

davidhadas commented Mar 24, 2023

davidhadas commented Mar 27, 2023

evankanderson left a comment

evankanderson Mar 28, 2023

davidhadas Mar 28, 2023

evankanderson Mar 28, 2023

davidhadas commented Apr 30, 2023

davidhadas commented Apr 30, 2023

nak3 May 1, 2023

davidhadas May 1, 2023

nak3 May 1, 2023

davidhadas May 1, 2023

nak3 commented May 2, 2023

knative-prow bot commented May 2, 2023

Add internal trust flag to config #778

Add internal trust flag to config #778

Conversation

davidhadas commented Mar 3, 2023

codecov bot commented Mar 3, 2023 • edited Loading

Codecov Report

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krsna-m commented Mar 22, 2023

davidhadas commented Mar 22, 2023

davidhadas commented Mar 23, 2023

davidhadas commented Mar 23, 2023

davidhadas commented Mar 24, 2023

davidhadas commented Mar 27, 2023

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhadas commented Apr 30, 2023

davidhadas commented Apr 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nak3 commented May 2, 2023

knative-prow bot commented May 2, 2023

codecov bot commented Mar 3, 2023 •

edited

Loading