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.

Sign up for GitHub

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

Jump to bottom

sarapprover: remove self node cert #62471

Merged
merged 1 commit into from Apr 13, 2018
Merged

sarapprover: remove self node cert #62471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 0 additions & 2 deletions pkg/controller/certificates/approver/BUILD
View file
Open in desktop
Expand Up @@ -28,10 +28,8 @@ go_library(
deps = [
"//pkg/apis/certificates/v1beta1:go_default_library",
"//pkg/controller/certificates:go_default_library",
"//pkg/features:go_default_library",
"//vendor/k8s.io/api/authorization/v1beta1:go_default_library",
"//vendor/k8s.io/api/certificates/v1beta1:go_default_library",
"//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//vendor/k8s.io/client-go/informers/certificates/v1beta1:go_default_library",
"//vendor/k8s.io/client-go/kubernetes:go_default_library",
],
Expand Down
34 changes: 0 additions & 34 deletions pkg/controller/certificates/approver/sarapprove.go
View file
Open in desktop
Expand Up @@ -25,12 +25,10 @@ import (

authorization "k8s.io/api/authorization/v1beta1"
capi "k8s.io/api/certificates/v1beta1"
utilfeature "k8s.io/apiserver/pkg/util/feature"
certificatesinformers "k8s.io/client-go/informers/certificates/v1beta1"
clientset "k8s.io/client-go/kubernetes"
k8s_certificates_v1beta1 "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
"k8s.io/kubernetes/pkg/controller/certificates"
"k8s.io/kubernetes/pkg/features"
)

type csrRecognizer struct {
Expand Down Expand Up @@ -69,13 +67,6 @@ func recognizers() []csrRecognizer {
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
}

Expand Down Expand Up @@ -201,28 +192,3 @@ func isSelfNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.Cert
}
return true
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this change but what do you think about adding debug logging in the recognizer's about the exact condition which led to CSR not being approved?

Copy link
Member Author

Choose a reason for hiding this comment

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

Events are better for exposing reasons to end users. Would this be for certs that are recognized but the subject access review is denied?

Copy link
Contributor

@rtripat rtripat Apr 12, 2018 •
edited

Choose a reason for hiding this comment

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

Not so much for the SAR since I think the result of that request will show up in API server logs at appropriate verbosity.

If we can have greater visibility via logs or events into which condition in the recognizer failed to approve the CSR that will be great.

This is more useful when someone generates their own bootstrap token and wants to debug why their CSR isn't being approved. Otherwise, the controller leaves the CSR in pending without any trace in the logs. Example: username of token should match the CommonName in CSR like system:node:$NodeName

Copy link
Member Author

Choose a reason for hiding this comment

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

Debug logging might be ok but not all CSRs are expected to be approved by this controller. Unrecognized CSRs are expected under normal circumstances.

var kubeletServerUsages = []capi.KeyUsage{
capi.UsageKeyEncipherment,
capi.UsageDigitalSignature,
capi.UsageServerAuth,
}

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
}
if !strings.HasPrefix(x509cr.Subject.CommonName, "system:node:") {
return false
}
if csr.Spec.Username != x509cr.Subject.CommonName {
return false
}
return true
}
71 changes: 0 additions & 71 deletions pkg/controller/certificates/approver/sarapprove_test.go
View file
Open in desktop
Expand Up @@ -187,77 +187,6 @@ 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 := k8s_certificates_v1beta1.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
10 changes: 0 additions & 10 deletions plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go
View file
Open in desktop
Expand Up @@ -501,16 +501,6 @@ func ClusterRoles() []rbac.ClusterRole {
},
}

if utilfeature.DefaultFeatureGate.Enabled(features.RotateKubeletServerCertificate) {
roles = append(roles, rbac.ClusterRole{
// a role making the csrapprover controller approve a node server CSR requested by the node itself
ObjectMeta: metav1.ObjectMeta{Name: "system:certificates.k8s.io:certificatesigningrequests:selfnodeserver"},
Rules: []rbac.PolicyRule{
rbac.NewRule("create").Groups(certificatesGroup).Resources("certificatesigningrequests/selfnodeserver").RuleOrDie(),
},
})
}

if utilfeature.DefaultFeatureGate.Enabled(features.VolumeScheduling) {
roles = append(roles, rbac.ClusterRole{
ObjectMeta: metav1.ObjectMeta{Name: "system:volume-scheduler"},
Expand Down