Adds Support for Control Plane TLS by danehans · Pull Request #208 · envoyproxy/gateway

danehans · 2022-08-18T21:26:49Z

Adds a package for generating control plane auth certs.
Adds a const to the EG config pkg to represent the default "envoy-gateway-system" ns.
Updates ObjectName() method to get/set the default name of the ProxyInfra type.

fixes: #70

Signed-off-by: danehans daneyonhansen@gmail.com

LukeShu · 2022-08-23T21:28:27Z

To communicate: I haven't been looking at this because it is failing CI.

danehans · 2022-08-25T02:18:42Z

@LukeShu I've been side tracked with other PR. I marked the PR as WIP.

Signed-off-by: danehans <daneyonhansen@gmail.com>

lizan · 2022-09-22T06:25:37Z

internal/crypto/certgen_test.go

+		caCertPEM:  caCert,
+		caKeyPEM:   caKey,
+		expiry:     expiry,
+		commonName: "envoy-gateway",


Common names are deprecated in RFC 2818 (in 2000) in favor of SANs. Major softwares (Golang, Chrome etc) stopped supporting them, so we should just issue certs without common names.

Something to consider instead of just not setting the CN (having it be an empty string) is to have it be a human-friendly string; like "Envoy Gateway xDS server" or something.

But yeah, we shouldn't be issuing certs with hostnames in the CN.

danehans · 2022-09-22T16:28:16Z

xref #404 will causes this PR to support a TLS secret per managed Envoy deployment.

LukeShu

Some nit-picks, but on the whole LGTM.

LukeShu · 2022-09-22T19:09:56Z

internal/crypto/certgen.go

+	DefaultNamespace = config.EnvoyGatewayNamespace
+
+	// DefaultCertificateLifetime holds the default certificate lifetime (in days).
+	DefaultCertificateLifetime = 365


Can we just have this be DefaultCertificateLifetime = 365*24*time.Hour so it's typed as a time.Duration and we don't need to worry about converting it?

LukeShu · 2022-09-22T19:19:19Z

internal/crypto/certgen_test.go

+		caCertPEM:  caCert,
+		caKeyPEM:   caKey,
+		expiry:     expiry,
+		commonName: "envoy-gateway",


Something to consider instead of just not setting the CN (having it be an empty string) is to have it be a human-friendly string; like "Envoy Gateway xDS server" or something.

But yeah, we shouldn't be issuing certs with hostnames in the CN.

LukeShu · 2022-09-22T19:31:49Z

internal/crypto/certgen.go

+// bigIntHash generates a SubjectKeyId by hashing the modulus of the private
+// key. This isn't one of the methods listed in RFC 5280 4.2.1.2, but that also
+// notes that other methods are acceptable.
+//
+// gosec makes a blanket claim that SHA-1 is unacceptable, which is
+// false here. The core Go method of generations the SubjectKeyId (see
+// https://github.com/golang/go/issues/26676) also uses SHA-1, as recommended
+// by RFC 5280.
+func bigIntHash(n *big.Int) []byte {
+	h := sha1.New()    // nolint:gosec
+	h.Write(n.Bytes()) // nolint:errcheck
+	return h.Sum(nil)
+}


Why have a separate .Write(); hy not skip the .Write() and just return h.Sum(n.Bytes)? Or even skip creating an object at all and

h := sha1.Sum(n.Bytes()) return h[:]

(this is what the mentioned code the core crypto/x509 does)

But also, why are we even bothering with this? It's optional in non-CA certificates, and the crypto/x509 code will handle it for us in CA certificates.

LukeShu · 2022-09-22T19:34:43Z

internal/crypto/certgen.go

+func newSerial(now time.Time) *big.Int {
+	return big.NewInt(int64(now.Nanosecond()))
+}


Using the time feels weird to me. Perhaps crypto/rand would be better?

serialNumberLimit := new(big.Int).Lsh(big.NewInt(1), 128) serialNumber, err := rand.Int(rand.Reader, serialNumberLimit)

LukeShu · 2022-09-22T19:42:57Z

internal/infrastructure/kubernetes/deployment.go

+					Name:      "sds",
+					MountPath: "/sds",
+				},
+			},


Should we put them in /mnt or /etc or somewhere? Putting them straight in the root feels weird to me.

LukeShu · 2022-09-22T19:46:46Z

internal/provider/kubernetes/config/envoy-gateway/certgen-job.yaml

+        runAsUser: 65534
+        runAsGroup: 65534


#200 (comment)

65534 is the magic UID that users outside of your UID namespace show up as. So using it here is questionable. (nss-systemd calls this special user "nobody", but IMO a better name would have been "unknown")

I'm not necessarily asking you to change it, I'm asking you to justify it.

LukeShu · 2022-09-22T20:05:45Z

internal/crypto/certgen.go

+		Type:  "RSA PRIVATE KEY",
+		Bytes: x509.MarshalPKCS1PrivateKey(newKey),


Getting in to bike-shed territory, but any reason to not use "PRIVATE KEY" and x509.MarshalPKCS8PrivateKey? It'd have fewer things to change if we ever want to switch away from RSA.

LukeShu · 2022-09-22T20:10:47Z

If anyone cares, here's some very similar code in Emissary: https://github.com/emissary-ingress/emissary/blob/master/cmd/apiext/ca.go

danehans requested a review from a team as a code owner August 18, 2022 21:26

danehans changed the title ~~Adds Crypto Pkg for Generating Certs~~ WIP: Adds Crypto Pkg for Generating Certs Aug 25, 2022

danehans added the provider/kubernetes Issues related to the Kubernetes provider label Aug 30, 2022

danehans added this to the 0.2.0-rc2 milestone Aug 30, 2022

danehans marked this pull request as draft September 13, 2022 18:53

danehans force-pushed the issue_70 branch 4 times, most recently from 8eeccaf to f2849cd Compare September 20, 2022 14:54

danehans changed the title ~~WIP: Adds Crypto Pkg for Generating Certs~~ Adds Support for Control Plane TLS Sep 20, 2022

danehans marked this pull request as ready for review September 20, 2022 15:19

danehans mentioned this pull request Sep 20, 2022

Use Case - Front Proxy with mTLS #88

Closed

Adds Control Plane Auth Support

1f3eaa0

Signed-off-by: danehans <daneyonhansen@gmail.com>

danehans force-pushed the issue_70 branch from f2849cd to 1f3eaa0 Compare September 20, 2022 18:54

lizan reviewed Sep 22, 2022

View reviewed changes

LukeShu approved these changes Sep 22, 2022

View reviewed changes

danehans merged commit e210323 into envoyproxy:main Sep 22, 2022

danehans deleted the issue_70 branch September 22, 2022 20:30

This was referenced Sep 22, 2022

Fix Feedback in https://github.com/envoyproxy/gateway/pull/208 #410

Closed

Refine Control Plane Auth #440

Closed

danehans mentioned this pull request Oct 12, 2022

Support EG local running #256

Closed

danehans mentioned this pull request Oct 28, 2022

Provide a Simple and Native Certificate Management Solution #82

Closed

arkodg mentioned this pull request Jan 6, 2023

docs: allow users to configure custom certs for control plane auth #866

Closed

		Type: "RSA PRIVATE KEY",
		Bytes: x509.MarshalPKCS1PrivateKey(newKey),

Conversation

danehans commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LukeShu commented Aug 23, 2022

Uh oh!

danehans commented Aug 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danehans commented Sep 22, 2022

Uh oh!

LukeShu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukeShu commented Sep 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danehans commented Aug 18, 2022 •

edited

Loading