From bcd8775b5b0c83ee0d3e34f32be402193ffe2b08 Mon Sep 17 00:00:00 2001 From: robertdavidsmith <34475852+robertdavidsmith@users.noreply.github.com> Date: Tue, 20 Apr 2021 09:44:32 +0100 Subject: [PATCH] Accept client certs based on SAN, not just CN (#514) * first attempt at SAN-based VerifyNodeName * Update docs on verify-node-name (cherry picked from commit 77bd4e4cf6035cc39b191203a341093fafeabe8b) --- cmd/nfd-master/main.go | 2 +- .../templates/master.yaml | 2 +- docs/advanced/developer-guide.md | 2 +- docs/advanced/master-commandline-reference.md | 9 +++----- docs/get-started/deployment-and-usage.md | 7 ++---- nfd-master.yaml.template | 2 +- pkg/nfd-master/nfd-master.go | 22 +++++++++++++++---- 7 files changed, 27 insertions(+), 19 deletions(-) diff --git a/cmd/nfd-master/main.go b/cmd/nfd-master/main.go index 1e994b3c7..ca2a576dc 100644 --- a/cmd/nfd-master/main.go +++ b/cmd/nfd-master/main.go @@ -103,7 +103,7 @@ func initFlags(flagset *flag.FlagSet) *master.Args { flagset.Var(&args.ResourceLabels, "resource-labels", "Comma separated list of labels to be exposed as extended resources.") flagset.BoolVar(&args.VerifyNodeName, "verify-node-name", false, - "Verify worker node name against CN from the TLS certificate. "+ + "Verify worker node name against the worker's TLS certificate. "+ "Only takes effect when TLS authentication has been enabled.") return args diff --git a/deployment/node-feature-discovery/templates/master.yaml b/deployment/node-feature-discovery/templates/master.yaml index 3521bcd3b..67d7e5da2 100644 --- a/deployment/node-feature-discovery/templates/master.yaml +++ b/deployment/node-feature-discovery/templates/master.yaml @@ -53,7 +53,7 @@ spec: ## a ConfigMap named nfd-ca-cert, and, the TLS authentication credentials stored ## in a TLS Secret named nfd-master-cert. ## Additional hardening can be enabled by specifying --verify-node-name in -## args, in which case every nfd-worker requires a individual node-specific +## args, in which case node name will be checked against the worker's ## TLS certificate. # - "--ca-file=/etc/kubernetes/node-feature-discovery/trust/ca.crt" # - "--key-file=/etc/kubernetes/node-feature-discovery/certs/tls.key" diff --git a/docs/advanced/developer-guide.md b/docs/advanced/developer-guide.md index 5912f5a5b..e028c6be7 100644 --- a/docs/advanced/developer-guide.md +++ b/docs/advanced/developer-guide.md @@ -196,7 +196,7 @@ Usage of nfd-master: -resource-labels value Comma separated list of labels to be exposed as extended resources. -verify-node-name - Verify worker node name against CN from the TLS certificate. Only takes effect when TLS authentication has been enabled. + Verify worker node name against the worker's TLS certificate. Only takes effect when TLS authentication has been enabled. -version Print version and exit. ``` diff --git a/docs/advanced/master-commandline-reference.md b/docs/advanced/master-commandline-reference.md index 89574950b..a1aa8e876 100644 --- a/docs/advanced/master-commandline-reference.md +++ b/docs/advanced/master-commandline-reference.md @@ -124,14 +124,11 @@ nfd-master -key-file=/opt/nfd/master.key -cert-file=/opt/nfd/master.crt -ca-file The `-verify-node-name` flag controls the NodeName based authorization of incoming requests and only has effect when mTLS authentication has been enabled (with `-ca-file`, `-cert-file` and `-key-file`). If enabled, the worker node -name of the incoming must match with the CN in its TLS certificate. Thus, +name of the incoming must match with the CN or a SAN in its TLS certificate. Thus, workers are only able to label the node they are running on (or the node whose -certificate they present), and, each worker must have an individual -certificate. +certificate they present). -Node Name based authorization is disabled by default and thus it is possible -for all nfd-worker pods in the cluster to use one shared certificate, making -NFD deployment much easier. +Node Name based authorization is disabled by default. Default: *false* diff --git a/docs/get-started/deployment-and-usage.md b/docs/get-started/deployment-and-usage.md index e53b5ea7d..1064b6785 100644 --- a/docs/get-started/deployment-and-usage.md +++ b/docs/get-started/deployment-and-usage.md @@ -310,8 +310,8 @@ the nfd-master Service of the cluster. By default, nfd-master only check that the nfd-worker has been signed by the specified root certificate (-ca-file). Additional hardening can be enabled by specifying -verify-node-name in nfd-master args, in which case nfd-master verifies that the NodeName presented -by nfd-worker matches the Common Name (CN) of its certificate. This means that -each nfd-worker requires a individual node-specific TLS certificate. +by nfd-worker matches the Common Name (CN) or a Subject Alternative Name (SAN) +of its certificate. #### Automated TLS certificate management using cert-manager @@ -334,9 +334,6 @@ $ kubectl apply -f nfd-cert-manager.yaml Finally, deploy `nfd-master.yaml` and `nfd-worker-daemonset.yaml` with the Secrets (`nfd-master-cert` and `nfd-worker-cert`) mounts enabled. -**Note:** the automated setup to support `--verify-node-name` hardening cannot -be configured currently. - ## Worker configuration NFD-Worker supports dynamic configuration through a configuration file. The diff --git a/nfd-master.yaml.template b/nfd-master.yaml.template index fd08f038d..818c2b851 100644 --- a/nfd-master.yaml.template +++ b/nfd-master.yaml.template @@ -97,7 +97,7 @@ spec: ## the TLS authentication credentials and a root certificate named ca.crt created. ## cert-manager can be used to automate the Secret creation and updates. ## Additional hardening can be enabled by specifying --verify-node-name in -## args, in which case every nfd-worker requires a individual node-specific +## args, in which case node name will be checked against the worker's ## TLS certificate. # args: # - "--ca-file=/etc/kubernetes/node-feature-discovery/certs/ca.crt" diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go index fc6e16861..2b9d1d9c6 100644 --- a/pkg/nfd-master/nfd-master.go +++ b/pkg/nfd-master/nfd-master.go @@ -18,6 +18,7 @@ package nfdmaster import ( "crypto/tls" + "crypto/x509" "fmt" "net" "os" @@ -347,6 +348,18 @@ func filterFeatureLabels(labels Labels, extraLabelNs map[string]struct{}, labelW return outLabels, extendedResources } +func verifyNodeName(cert *x509.Certificate, nodeName string) error { + if cert.Subject.CommonName == nodeName { + return nil + } + + err := cert.VerifyHostname(nodeName) + if err != nil { + return fmt.Errorf("Certificate %q not valid for node %q: %v", cert.Subject.CommonName, nodeName, err) + } + return nil +} + // SetLabels implements LabelerServer func (m *nfdMaster) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*pb.SetLabelsReply, error) { if m.args.VerifyNodeName { @@ -366,10 +379,11 @@ func (m *nfdMaster) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*pb.Se klog.Errorf("gRPC request error: client certificate verification for '%v' failed", client.Addr) return &pb.SetLabelsReply{}, fmt.Errorf("client certificate verification failed") } - cn := tlsAuth.State.VerifiedChains[0][0].Subject.CommonName - if cn != r.NodeName { - klog.Errorf("gRPC request error: authorization for %v failed: cert valid for %q, requested node name %q", client.Addr, cn, r.NodeName) - return &pb.SetLabelsReply{}, fmt.Errorf("request authorization failed: cert valid for %q, requested node name %q", cn, r.NodeName) + + err := verifyNodeName(tlsAuth.State.VerifiedChains[0][0], r.NodeName) + if err != nil { + klog.Errorf("gRPC request error: authorization for %v failed: %v", client.Addr, err) + return &pb.SetLabelsReply{}, err } } if klog.V(1).Enabled() {