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

Auto approve kubelet server certificate signing requests. #46884

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions cluster/addons/rbac/kubelet-certificate-management.yaml
Expand Up @@ -57,5 +57,6 @@ rules:
- "certificates.k8s.io"
resources:
- certificatesigningrequests/selfnodeclient
- certificatesigningrequests/selfnodeserver
verbs:
- "create"
2 changes: 2 additions & 0 deletions pkg/controller/certificates/approver/BUILD
Expand Up @@ -33,6 +33,8 @@ go_library(
"//pkg/client/clientset_generated/clientset:go_default_library",
"//pkg/client/informers/informers_generated/externalversions/certificates/v1beta1:go_default_library",
"//pkg/controller/certificates:go_default_library",
"//pkg/features:go_default_library",
"//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
],
)

Expand Down
26 changes: 21 additions & 5 deletions pkg/controller/certificates/approver/sarapprove.go
Expand Up @@ -23,11 +23,13 @@ import (
"reflect"
"strings"

utilfeature "k8s.io/apiserver/pkg/util/feature"
authorization "k8s.io/kubernetes/pkg/apis/authorization/v1beta1"
capi "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
certificatesinformers "k8s.io/kubernetes/pkg/client/informers/informers_generated/externalversions/certificates/v1beta1"
"k8s.io/kubernetes/pkg/controller/certificates"
"k8s.io/kubernetes/pkg/features"
)

type csrRecognizer struct {
Expand All @@ -54,7 +56,7 @@ func NewCSRApprovingController(client clientset.Interface, csrInformer certifica
}

func recognizers() []csrRecognizer {
return []csrRecognizer{
recognizers := []csrRecognizer{
{
recognize: isSelfNodeClientCert,
permission: authorization.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Verb: "create", Subresource: "selfnodeclient"},
Expand All @@ -65,12 +67,15 @@ func recognizers() []csrRecognizer {
permission: authorization.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Verb: "create", Subresource: "nodeclient"},
successMessage: "Auto approving kubelet client certificate after SubjectAccessReview.",
},
{
}
if utilfeature.DefaultFeatureGate.Enabled(features.RotateKubeletServerCertificate) {
recognizers = append(recognizers, csrRecognizer{
recognize: isSelfNodeServerCert,
permission: authorization.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Verb: "create", Subresource: "selfnodeserver"},
successMessage: "Auto approving self kubelet server certificate after SubjectAccessReview.",
},
})
}
return recognizers
}

func (a *sarApprover) handle(csr *capi.CertificateSigningRequest) error {
Expand Down Expand Up @@ -192,9 +197,20 @@ var kubeletServerUsages = []capi.KeyUsage{
}

func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
if !reflect.DeepEqual([]string{"system:nodes"}, x509cr.Subject.Organization) {
return false
}
if len(x509cr.DNSNames) == 0 || len(x509cr.IPAddresses) == 0 {
return false
}
if !hasExactUsages(csr, kubeletServerUsages) {
return false
}
//TODO(jcbsmpsn): implement the rest of this
return false
if !strings.HasPrefix(x509cr.Subject.CommonName, "system:node:") {
return false
}
if csr.Spec.Username != x509cr.Subject.CommonName {
return false
}
return true
}
71 changes: 71 additions & 0 deletions pkg/controller/certificates/approver/sarapprove_test.go
Expand Up @@ -184,6 +184,77 @@ func TestHandle(t *testing.T) {
}
}

func TestSelfNodeServerCertRecognizer(t *testing.T) {
defaultCSR := csrBuilder{
cn: "system:node:foo",
orgs: []string{"system:nodes"},
requestor: "system:node:foo",
usages: []capi.KeyUsage{
capi.UsageKeyEncipherment,
capi.UsageDigitalSignature,
capi.UsageServerAuth,
},
dns: []string{"node"},
ips: []net.IP{net.ParseIP("192.168.0.1")},
}

testCases := []struct {
description string
csrBuilder csrBuilder
expectedOutcome bool
}{
{
description: "Success - all requirements met",
csrBuilder: defaultCSR,
expectedOutcome: true,
},
{
description: "No organization",
csrBuilder: func(b csrBuilder) csrBuilder {
b.orgs = []string{}
return b
}(defaultCSR),
expectedOutcome: false,
},
{
description: "Wrong organization",
csrBuilder: func(b csrBuilder) csrBuilder {
b.orgs = append(b.orgs, "new-org")
return b
}(defaultCSR),
expectedOutcome: false,
},
{
description: "Wrong usages",
csrBuilder: func(b csrBuilder) csrBuilder {
b.usages = []capi.KeyUsage{}
return b
}(defaultCSR),
expectedOutcome: false,
},
{
description: "Wrong common name",
csrBuilder: func(b csrBuilder) csrBuilder {
b.cn = "wrong-common-name"
return b
}(defaultCSR),
expectedOutcome: false,
},
}
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
csr := makeFancyTestCsr(tc.csrBuilder)
x509cr, err := capi.ParseCSR(csr)
if err != nil {
t.Errorf("unexpected err: %v", err)
}
if isSelfNodeServerCert(csr, x509cr) != tc.expectedOutcome {
t.Errorf("expected recognized to be %v", tc.expectedOutcome)
}
})
}
}

func TestRecognizers(t *testing.T) {
goodCases := []func(b *csrBuilder){
func(b *csrBuilder) {
Expand Down
13 changes: 11 additions & 2 deletions pkg/kubelet/kubelet.go
Expand Up @@ -1134,16 +1134,25 @@ func initializeServerCertificateManager(kubeClient clientset.Interface, kubeCfg
CertificateSigningRequestClient: certSigningRequestClient,
Template: &x509.CertificateRequest{
Subject: pkix.Name{
CommonName: string(nodeName),
CommonName: fmt.Sprintf("system:node:%s", nodeName),
Copy link
Member

@liggitt liggitt Jun 11, 2017

Choose a reason for hiding this comment

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

I'd really like to see this change make 1.7. I'd prefer not to have a mix of CSR formats to recognize unnecessarily

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on it.

Organization: []string{"system:nodes"},
},
DNSNames: hostnames,
IPAddresses: ips,
},
Usages: []certificates.KeyUsage{
// https://tools.ietf.org/html/rfc5280#section-4.2.1.3
//
// Digital signature allows the certificate to be used to verify
// digital signatures used during TLS negotiation.
certificates.UsageDigitalSignature,
// KeyEncipherment allows the cert/key pair to be used to encrypt
// keys, including the symetric keys negotiated during TLS setup
// and used for data transfer.
certificates.UsageKeyEncipherment,
// ServerAuth allows the cert to be used by a TLS server to
// authenticate itself to a TLS client.
certificates.UsageServerAuth,
certificates.UsageSigning,
},
CertificateStore: certificateStore,
})
Expand Down