Skip to content

Commit a1283bf

Browse files
Merge pull request #2560 from pperiyasamy/signer-username-validation
SDN-5342: Signer username validation
2 parents 66f0e13 + c88f5d8 commit a1283bf

File tree

2 files changed

+272
-24
lines changed

2 files changed

+272
-24
lines changed

pkg/controller/signer/signer-controller.go

Lines changed: 34 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@ import (
1212
corev1 "k8s.io/api/core/v1"
1313
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
1414

15-
"k8s.io/client-go/kubernetes"
16-
1715
"k8s.io/apimachinery/pkg/runtime"
1816
"k8s.io/apimachinery/pkg/types"
1917

@@ -30,22 +28,17 @@ import (
3028
const signerName = "network.openshift.io/signer"
3129

3230
// Add controller and start it when the Manager is started.
33-
func Add(mgr manager.Manager, status *statusmanager.StatusManager, _ cnoclient.Client) error {
34-
reconciler, err := newReconciler(mgr, status)
31+
func Add(mgr manager.Manager, status *statusmanager.StatusManager, client cnoclient.Client) error {
32+
reconciler, err := newReconciler(client, mgr, status)
3533
if err != nil {
3634
return err
3735
}
3836
return add(mgr, reconciler)
3937
}
4038

4139
// newReconciler returns a new reconcile.Reconciler
42-
func newReconciler(mgr manager.Manager, status *statusmanager.StatusManager) (reconcile.Reconciler, error) {
43-
// We need a clientset in order to UpdateApproval() of the CertificateSigningRequest
44-
clientset, err := kubernetes.NewForConfig(mgr.GetConfig())
45-
if err != nil {
46-
return nil, err
47-
}
48-
return &ReconcileCSR{client: mgr.GetClient(), scheme: mgr.GetScheme(), status: status, clientset: clientset}, nil
40+
func newReconciler(client cnoclient.Client, mgr manager.Manager, status *statusmanager.StatusManager) (reconcile.Reconciler, error) {
41+
return &ReconcileCSR{client: client, scheme: mgr.GetScheme(), status: status}, nil
4942
}
5043

5144
// add adds a new Controller to mgr with r as the reconcile.Reconciler
@@ -82,23 +75,16 @@ var _ reconcile.Reconciler = &ReconcileCSR{}
8275
type ReconcileCSR struct {
8376
// This client, initialized using mgr.GetClient() above, is a split client
8477
// that reads objects from the cache and writes to the apiserver
85-
client crclient.Client
78+
client cnoclient.Client
8679
scheme *runtime.Scheme
8780
status *statusmanager.StatusManager
88-
89-
// Note: We need a Clientset as the controller-runtime client does not
90-
// support non-CRUD subresources (see
91-
// https://github.com/kubernetes-sigs/controller-runtime/issues/452)
92-
// This may risk invalidating the cache but in our case, this is not a
93-
// problem as we only use this to update the approval status of the csr.
94-
clientset *kubernetes.Clientset
9581
}
9682

9783
// Reconcile CSR
9884
func (r *ReconcileCSR) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) {
9985
defer utilruntime.HandleCrash(r.status.SetDegradedOnPanicAndCrash)
10086
csr := &csrv1.CertificateSigningRequest{}
101-
err := r.client.Get(ctx, request.NamespacedName, csr)
87+
err := r.client.Default().CRClient().Get(ctx, request.NamespacedName, csr)
10288
if err != nil {
10389
if apierrors.IsNotFound(err) {
10490
// Request object not found, could have been deleted after reconcile request.
@@ -118,6 +104,17 @@ func (r *ReconcileCSR) Reconcile(ctx context.Context, request reconcile.Request)
118104
return reconcile.Result{}, nil
119105
}
120106

107+
isValid, err := r.isValidUserName(ctx, csr.Spec.Username)
108+
if err != nil {
109+
return reconcile.Result{}, fmt.Errorf("error occurred while validating CSR %s: %w", csr.Name, err)
110+
}
111+
if !isValid {
112+
// Update CSR status condition with Failed condition.
113+
updateCSRStatusConditions(r, csr, "CSRInvalidUser",
114+
"Certificate Signing Request is set with invalid user name, can't sign it")
115+
return reconcile.Result{}, nil
116+
}
117+
121118
if len(csr.Status.Certificate) != 0 {
122119
// Request already has a certificate. There is nothing
123120
// to do as we will, currently, not re-certify or handle any updates to
@@ -136,7 +133,7 @@ func (r *ReconcileCSR) Reconcile(ctx context.Context, request reconcile.Request)
136133
Reason: "AutoApproved",
137134
Message: "Automatically approved by " + signerName})
138135
// Update status to "Approved"
139-
_, err = r.clientset.CertificatesV1().CertificateSigningRequests().UpdateApproval(ctx, request.Name, csr, metav1.UpdateOptions{})
136+
_, err = r.client.Default().Kubernetes().CertificatesV1().CertificateSigningRequests().UpdateApproval(ctx, request.Name, csr, metav1.UpdateOptions{})
140137
if err != nil {
141138
log.Printf("Unable to approve certificate for %v and signer %v: %v", request.Name, signerName, err)
142139
return reconcile.Result{}, err
@@ -151,7 +148,7 @@ func (r *ReconcileCSR) Reconcile(ctx context.Context, request reconcile.Request)
151148

152149
// Get our CA that was created by the operatorpki.
153150
caSecret := &corev1.Secret{}
154-
err = r.client.Get(ctx, types.NamespacedName{Namespace: "openshift-ovn-kubernetes", Name: "signer-ca"}, caSecret)
151+
err = r.client.Default().CRClient().Get(ctx, types.NamespacedName{Namespace: "openshift-ovn-kubernetes", Name: "signer-ca"}, caSecret)
155152
if err != nil {
156153
signerFailure(r, csr, "CAFailure",
157154
fmt.Sprintf("Could not get CA certificate and key: %v", err))
@@ -201,7 +198,7 @@ func (r *ReconcileCSR) Reconcile(ctx context.Context, request reconcile.Request)
201198
return reconcile.Result{}, nil
202199
}
203200

204-
err = r.client.Status().Update(ctx, csr)
201+
err = r.client.Default().CRClient().Status().Update(ctx, csr)
205202
if err != nil {
206203
log.Printf("Unable to update signed certificate for %v and signer %v: %v", request.Name, signerName, err)
207204
return reconcile.Result{}, err
@@ -212,6 +209,19 @@ func (r *ReconcileCSR) Reconcile(ctx context.Context, request reconcile.Request)
212209
return reconcile.Result{}, nil
213210
}
214211

212+
func (r *ReconcileCSR) isValidUserName(ctx context.Context, csrUserName string) (bool, error) {
213+
nodeList, err := r.client.Default().Kubernetes().CoreV1().Nodes().List(ctx, metav1.ListOptions{})
214+
if err != nil {
215+
return false, fmt.Errorf("failed to list nodes: %v", err)
216+
}
217+
for _, node := range nodeList.Items {
218+
if fmt.Sprintf("system:ovn-node:%s", node.Name) == csrUserName {
219+
return true, nil
220+
}
221+
}
222+
return false, nil
223+
}
224+
215225
// isCertificateRequestApproved returns true if a certificate request has the
216226
// "Approved" condition and no "Denied" conditions; false otherwise.
217227
func isCertificateRequestApproved(csr *csrv1.CertificateSigningRequest) bool {
@@ -247,7 +257,7 @@ func updateCSRStatusConditions(r *ReconcileCSR, csr *csrv1.CertificateSigningReq
247257
Reason: reason,
248258
Message: message})
249259

250-
err := r.client.Status().Update(context.TODO(), csr)
260+
err := r.client.Default().CRClient().Status().Update(context.TODO(), csr)
251261
if err != nil {
252262
log.Printf("Could not update CSR status: %v", err)
253263
r.status.SetDegraded(statusmanager.CertificateSigner, "UpdateFailure",

pkg/controller/signer/signer_test.go

Lines changed: 238 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,238 @@
1+
package signer
2+
3+
import (
4+
"bytes"
5+
"context"
6+
"crypto/rand"
7+
"crypto/rsa"
8+
"crypto/x509"
9+
"encoding/pem"
10+
"fmt"
11+
"testing"
12+
"time"
13+
14+
. "github.com/onsi/gomega"
15+
configv1 "github.com/openshift/api/config/v1"
16+
operv1 "github.com/openshift/api/operator/v1"
17+
cnoclient "github.com/openshift/cluster-network-operator/pkg/client"
18+
"github.com/openshift/cluster-network-operator/pkg/client/fake"
19+
"github.com/openshift/cluster-network-operator/pkg/controller/statusmanager"
20+
"github.com/openshift/cluster-network-operator/pkg/names"
21+
"github.com/openshift/library-go/pkg/crypto"
22+
certificatev1 "k8s.io/api/certificates/v1"
23+
corev1 "k8s.io/api/core/v1"
24+
apierrors "k8s.io/apimachinery/pkg/api/errors"
25+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
26+
"k8s.io/apimachinery/pkg/types"
27+
"k8s.io/client-go/kubernetes/scheme"
28+
"sigs.k8s.io/controller-runtime/pkg/reconcile"
29+
)
30+
31+
var (
32+
csrName = "ipsec-csr"
33+
nodeName = "testnode"
34+
coName = "testing"
35+
)
36+
37+
//nolint:errcheck
38+
func init() {
39+
certificatev1.AddToScheme(scheme.Scheme)
40+
corev1.AddToScheme(scheme.Scheme)
41+
}
42+
43+
func TestSigner_reconciler(t *testing.T) {
44+
g := NewGomegaWithT(t)
45+
client := fake.NewFakeClient()
46+
status := statusmanager.New(client, coName, names.StandAloneClusterName)
47+
signer := ReconcileCSR{client: client, status: status}
48+
49+
co := &configv1.ClusterOperator{ObjectMeta: metav1.ObjectMeta{Name: coName}}
50+
setCO(t, client, co)
51+
no := &operv1.Network{ObjectMeta: metav1.ObjectMeta{Name: names.OPERATOR_CONFIG}}
52+
setOC(t, client, no)
53+
54+
csr, err := generateCSR()
55+
g.Expect(err).NotTo(HaveOccurred())
56+
csrObj := &certificatev1.CertificateSigningRequest{}
57+
csrObj.Name = csrName
58+
csrObj.Spec.Request = []byte(csr)
59+
csrObj.Spec.SignerName = signerName
60+
csrObj.Spec.Usages = []certificatev1.KeyUsage{"ipsec tunnel"}
61+
csrObj.Spec.Username = fmt.Sprintf("system:ovn-node:%s", nodeName)
62+
csrObj.Status.Conditions = append(csrObj.Status.Conditions, certificatev1.CertificateSigningRequestCondition{
63+
Type: certificatev1.CertificateApproved,
64+
Status: "True",
65+
Reason: "AutoApproved",
66+
Message: "Automatically approved by " + signerName})
67+
68+
err = client.Default().CRClient().Create(context.TODO(), csrObj)
69+
g.Expect(err).NotTo(HaveOccurred())
70+
_, err = client.Default().Kubernetes().CertificatesV1().CertificateSigningRequests().Create(context.TODO(), csrObj, metav1.CreateOptions{})
71+
g.Expect(err).NotTo(HaveOccurred())
72+
73+
node := &corev1.Node{}
74+
node.Name = nodeName
75+
_, err = client.Default().Kubernetes().CoreV1().Nodes().Create(context.TODO(), node, metav1.CreateOptions{})
76+
g.Expect(err).NotTo(HaveOccurred())
77+
78+
ca, err := crypto.MakeSelfSignedCAConfigForDuration(signerName, 10*time.Minute)
79+
g.Expect(err).NotTo(HaveOccurred())
80+
certBytes := &bytes.Buffer{}
81+
keyBytes := &bytes.Buffer{}
82+
err = ca.WriteCertConfig(certBytes, keyBytes)
83+
g.Expect(err).NotTo(HaveOccurred())
84+
caSecret := &corev1.Secret{}
85+
caSecret.Name = "signer-ca"
86+
caSecret.Namespace = "openshift-ovn-kubernetes"
87+
caSecret.Data = make(map[string][]byte)
88+
caSecret.Data["tls.crt"] = certBytes.Bytes()
89+
caSecret.Data["tls.key"] = keyBytes.Bytes()
90+
err = client.Default().CRClient().Create(context.TODO(), caSecret)
91+
g.Expect(err).NotTo(HaveOccurred())
92+
93+
_, err = signer.Reconcile(context.TODO(),
94+
reconcile.Request{NamespacedName: types.NamespacedName{Name: csrName}})
95+
g.Expect(err).NotTo(HaveOccurred())
96+
97+
err = client.Default().CRClient().Get(context.TODO(), types.NamespacedName{Name: csrName}, csrObj)
98+
g.Expect(err).NotTo(HaveOccurred())
99+
g.Expect(csrObj.Status.Certificate).ShouldNot(BeEmpty())
100+
101+
co, _, err = getStatuses(client, "testing")
102+
if err != nil {
103+
t.Fatalf("error getting network.operator: %v", err)
104+
}
105+
g.Expect(err).NotTo(HaveOccurred())
106+
g.Expect(conditionsInclude(co.Status.Conditions, []configv1.ClusterOperatorStatusCondition{
107+
{
108+
Type: configv1.OperatorDegraded,
109+
Status: configv1.ConditionFalse,
110+
},
111+
})).To(BeTrue())
112+
g.Expect(conditionsInclude(co.Status.Conditions, []configv1.ClusterOperatorStatusCondition{
113+
{
114+
Type: configv1.OperatorUpgradeable,
115+
Status: configv1.ConditionTrue,
116+
},
117+
})).To(BeTrue())
118+
}
119+
120+
func TestSigner_reconciler_withInvalidUserName(t *testing.T) {
121+
g := NewGomegaWithT(t)
122+
client := fake.NewFakeClient()
123+
status := statusmanager.New(client, coName, names.StandAloneClusterName)
124+
signer := ReconcileCSR{client: client, status: status}
125+
126+
co := &configv1.ClusterOperator{ObjectMeta: metav1.ObjectMeta{Name: coName}}
127+
setCO(t, client, co)
128+
no := &operv1.Network{ObjectMeta: metav1.ObjectMeta{Name: names.OPERATOR_CONFIG}}
129+
setOC(t, client, no)
130+
131+
csr, err := generateCSR()
132+
g.Expect(err).NotTo(HaveOccurred())
133+
csrObj := &certificatev1.CertificateSigningRequest{}
134+
csrObj.Name = csrName
135+
csrObj.Spec.Request = []byte(csr)
136+
csrObj.Spec.SignerName = signerName
137+
csrObj.Spec.Usages = []certificatev1.KeyUsage{"ipsec tunnel"}
138+
csrObj.Spec.Username = fmt.Sprintf("system:ovn-node:%s", "suspicious-node")
139+
140+
err = client.Default().CRClient().Create(context.TODO(), csrObj)
141+
g.Expect(err).NotTo(HaveOccurred())
142+
_, err = client.Default().Kubernetes().CertificatesV1().CertificateSigningRequests().Create(context.TODO(), csrObj, metav1.CreateOptions{})
143+
g.Expect(err).NotTo(HaveOccurred())
144+
145+
node := &corev1.Node{}
146+
node.Name = nodeName
147+
_, err = client.Default().Kubernetes().CoreV1().Nodes().Create(context.TODO(), node, metav1.CreateOptions{})
148+
g.Expect(err).NotTo(HaveOccurred())
149+
150+
_, err = signer.Reconcile(context.TODO(),
151+
reconcile.Request{NamespacedName: types.NamespacedName{Name: csrName}})
152+
g.Expect(err).NotTo(HaveOccurred())
153+
154+
err = client.Default().CRClient().Get(context.TODO(), types.NamespacedName{Name: csrName}, csrObj)
155+
g.Expect(err).NotTo(HaveOccurred())
156+
g.Expect(csrObj.Status.Certificate).Should(BeEmpty())
157+
csrConditions := csrObj.Status.Conditions
158+
g.Expect(len(csrConditions)).To(Equal(1))
159+
g.Expect(csrConditions[0].Reason).To(Equal("CSRInvalidUser"))
160+
g.Expect(csrConditions[0].Type).To(Equal(certificatev1.CertificateFailed))
161+
}
162+
163+
func generateCSR() (string, error) {
164+
// Create private key.
165+
csrKey, err := rsa.GenerateKey(rand.Reader, 2048)
166+
if err != nil {
167+
return "", fmt.Errorf("failed to generate private key: %v", err)
168+
}
169+
// Create CSR with private key.
170+
csrBytes, err := x509.CreateCertificateRequest(rand.Reader, &x509.CertificateRequest{}, csrKey)
171+
if err != nil {
172+
return "", err
173+
}
174+
// Encode CSR in PEM format.
175+
csrPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrBytes})
176+
if csrPEM == nil {
177+
return "", fmt.Errorf("failed to encode CSR in PEM format")
178+
}
179+
return string(csrPEM), nil
180+
}
181+
182+
func setOC(t *testing.T, client cnoclient.Client, oc *operv1.Network) {
183+
t.Helper()
184+
g := NewGomegaWithT(t)
185+
_, err := client.Default().OpenshiftOperatorClient().OperatorV1().Networks().Update(context.TODO(), oc, metav1.UpdateOptions{})
186+
if apierrors.IsNotFound(err) {
187+
_, err = client.Default().OpenshiftOperatorClient().OperatorV1().Networks().Create(context.TODO(), oc, metav1.CreateOptions{})
188+
}
189+
g.Expect(err).NotTo(HaveOccurred())
190+
}
191+
192+
func setCO(t *testing.T, client cnoclient.Client, co *configv1.ClusterOperator) {
193+
t.Helper()
194+
g := NewGomegaWithT(t)
195+
err := client.Default().CRClient().Update(context.TODO(), co)
196+
if apierrors.IsNotFound(err) {
197+
err = client.Default().CRClient().Create(context.TODO(), co)
198+
}
199+
g.Expect(err).NotTo(HaveOccurred())
200+
}
201+
202+
func getStatuses(client cnoclient.Client, name string) (*configv1.ClusterOperator, *operv1.Network, error) {
203+
co := &configv1.ClusterOperator{ObjectMeta: metav1.ObjectMeta{Name: name}}
204+
err := client.ClientFor("").CRClient().Get(context.TODO(), types.NamespacedName{Name: name}, co)
205+
if err != nil {
206+
return nil, nil, err
207+
}
208+
oc, err := client.Default().OpenshiftOperatorClient().OperatorV1().Networks().Get(context.TODO(), names.OPERATOR_CONFIG, metav1.GetOptions{})
209+
return co, oc, err
210+
}
211+
212+
// Tests that the parts of newConditions that are set match what's in oldConditions (but
213+
// doesn't look at anything else in oldConditions)
214+
func conditionsInclude(oldConditions, newConditions []configv1.ClusterOperatorStatusCondition) bool {
215+
for _, newCondition := range newConditions {
216+
foundMatchingCondition := false
217+
218+
for _, oldCondition := range oldConditions {
219+
if newCondition.Type != oldCondition.Type || newCondition.Status != oldCondition.Status {
220+
continue
221+
}
222+
if newCondition.Reason != "" && newCondition.Reason != oldCondition.Reason {
223+
return false
224+
}
225+
if newCondition.Message != "" && newCondition.Message != oldCondition.Message {
226+
return false
227+
}
228+
foundMatchingCondition = true
229+
break
230+
}
231+
232+
if !foundMatchingCondition {
233+
return false
234+
}
235+
}
236+
237+
return true
238+
}

0 commit comments

Comments
 (0)