From 96e3810d5fdb0a85a88f6faaf06a85aced1467fb Mon Sep 17 00:00:00 2001 From: mprahl Date: Tue, 20 Feb 2024 09:28:37 -0500 Subject: [PATCH] Remove client certificate authentication This will not be used and it didn't work on OpenShift because the CAs included in the Kubeconfig don't include the CA that signed the client certificate. This would have required additional code to pull the correct CA. Signed-off-by: mprahl --- controllers/complianceeventsapi/auth.go | 93 ++--------- controllers/complianceeventsapi/server.go | 56 ++----- go.mod | 2 - go.sum | 4 - main.go | 18 +-- test/e2e/case18_compliance_api_test.go | 2 +- .../case20_compliance_api_controller_test.go | 153 +----------------- 7 files changed, 26 insertions(+), 302 deletions(-) diff --git a/controllers/complianceeventsapi/auth.go b/controllers/complianceeventsapi/auth.go index 0962cd16..7fe00a30 100644 --- a/controllers/complianceeventsapi/auth.go +++ b/controllers/complianceeventsapi/auth.go @@ -2,10 +2,8 @@ package complianceeventsapi import ( - "crypto/x509" "encoding/base64" "encoding/json" - "errors" "net/http" "slices" "strings" @@ -15,8 +13,6 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" - apiserverx509 "k8s.io/apiserver/pkg/authentication/request/x509" - "k8s.io/apiserver/pkg/server/dynamiccertificates" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" ) @@ -69,69 +65,10 @@ func parseToken(req *http.Request) string { return strings.TrimSpace(strings.TrimPrefix(req.Header.Get("Authorization"), "Bearer")) } -// canRecordComplianceEvent will perform certificate or token authentication and perform a subject access review to +// canRecordComplianceEvent will perform token authentication and perform a self subject access review to // ensure the input user has patch access to patch the policy status in the managed cluster namespace. An error is -// returned if the authorization could not be determined. Note that authenticatedClient and authenticator can be nil -// if certificate authentication isn't used. If both certificate and token authentication is present, certificate takes -// precedence. -func canRecordComplianceEvent( - cfg *rest.Config, - authenticatedClient *kubernetes.Clientset, - authenticator *apiserverx509.Authenticator, - clusterName string, - req *http.Request, -) (bool, error) { - postRules := authzv1.ResourceAttributes{ - Group: "policy.open-cluster-management.io", - Version: "v1", - Resource: "policies", - Verb: "patch", - Namespace: clusterName, - Subresource: "status", - } - - // req.TLS.PeerCertificates will be empty if certificate authentication is not enabled (e.g. endpoint is not HTTPS) - if req.TLS != nil && len(req.TLS.PeerCertificates) > 0 { - resp, ok, err := authenticator.AuthenticateRequest(req) - if err != nil { - if errors.As(err, &x509.UnknownAuthorityError{}) || errors.As(err, &x509.CertificateInvalidError{}) { - return false, ErrUnauthorized - } - - return false, err - } - - if !ok { - return false, ErrUnauthorized - } - - review, err := authenticatedClient.AuthorizationV1().SubjectAccessReviews().Create( - req.Context(), - &authzv1.SubjectAccessReview{ - Spec: authzv1.SubjectAccessReviewSpec{ - ResourceAttributes: &postRules, - User: resp.User.GetName(), - Groups: resp.User.GetGroups(), - UID: resp.User.GetUID(), - }, - }, - metav1.CreateOptions{}, - ) - if err != nil { - return false, err - } - - if !review.Status.Allowed { - log.V(0).Info( - "The user is not authorized to record a compliance event", - "cluster", clusterName, - "user", resp.User.GetName(), - ) - } - - return review.Status.Allowed, nil - } - +// returned if the authorization could not be determined. +func canRecordComplianceEvent(cfg *rest.Config, clusterName string, req *http.Request) (bool, error) { userConfig, err := getUserKubeConfig(cfg, req) if err != nil { return false, err @@ -146,7 +83,14 @@ func canRecordComplianceEvent( req.Context(), &authzv1.SelfSubjectAccessReview{ Spec: authzv1.SelfSubjectAccessReviewSpec{ - ResourceAttributes: &postRules, + ResourceAttributes: &authzv1.ResourceAttributes{ + Group: "policy.open-cluster-management.io", + Version: "v1", + Resource: "policies", + Verb: "patch", + Namespace: clusterName, + Subresource: "status", + }, }, }, metav1.CreateOptions{}, @@ -203,21 +147,6 @@ func getTokenUsername(token string) string { return username } -// getCertAuthenticator returns an Authenticator that can validate that an input certificate is signed by the API -// server represented in the input clientAuthCAs and that the certificate can be used for client authentication -// (e.g. key usage). -func getCertAuthenticator(clientAuthCAs []byte) (*apiserverx509.Authenticator, error) { - p, err := dynamiccertificates.NewStaticCAContent("client-ca", clientAuthCAs) - if err != nil { - return nil, err - } - - // This is the same approach taken by kube-rbac-proxy. - authenticator := apiserverx509.NewDynamic(p.VerifyOptions, apiserverx509.CommonNameUserConversion) - - return authenticator, nil -} - func getUserKubeConfig(config *rest.Config, r *http.Request) (*rest.Config, error) { userConfig := &rest.Config{ Host: config.Host, diff --git a/controllers/complianceeventsapi/server.go b/controllers/complianceeventsapi/server.go index d2e1571b..79072fbe 100644 --- a/controllers/complianceeventsapi/server.go +++ b/controllers/complianceeventsapi/server.go @@ -23,10 +23,7 @@ import ( "time" "github.com/lib/pq" - apiserverx509 "k8s.io/apiserver/pkg/authentication/request/x509" - "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" - certutil "k8s.io/client-go/util/cert" ) // init dynamically parses the database columns of each struct type to create a mapping of user provided sort/filter @@ -135,21 +132,17 @@ var ( ) type ComplianceAPIServer struct { - server *http.Server - addr string - clientAuthCAs []byte - cert *tls.Certificate - cfg *rest.Config + server *http.Server + addr string + cert *tls.Certificate + cfg *rest.Config } -func NewComplianceAPIServer( - listenAddress string, cfg *rest.Config, clientAuthCAs []byte, cert *tls.Certificate, -) *ComplianceAPIServer { +func NewComplianceAPIServer(listenAddress string, cfg *rest.Config, cert *tls.Certificate) *ComplianceAPIServer { return &ComplianceAPIServer{ - addr: listenAddress, - clientAuthCAs: clientAuthCAs, - cert: cert, - cfg: cfg, + addr: listenAddress, + cert: cert, + cfg: cfg, } } @@ -194,34 +187,13 @@ func (s *ComplianceAPIServer) Start(ctx context.Context, serverContext *Complian return err } - var authenticator *apiserverx509.Authenticator - var authenticatedClient *kubernetes.Clientset - if s.cert != nil { - clientAuthCAPool, err := certutil.NewPoolFromBytes(s.clientAuthCAs) - if err != nil { - return err - } - s.server.TLSConfig = &tls.Config{ MinVersion: tls.VersionTLS12, Certificates: []tls.Certificate{*s.cert}, - // Let the Kubernetes apiserver package validate it if the certificate is presented - ClientAuth: tls.RequestClientCert, - ClientCAs: clientAuthCAPool, } listener = tls.NewListener(listener, s.server.TLSConfig) - - authenticator, err = getCertAuthenticator(s.clientAuthCAs) - if err != nil { - return err - } - - authenticatedClient, err = kubernetes.NewForConfig(s.cfg) - if err != nil { - return err - } } // register handlers here @@ -250,7 +222,7 @@ func (s *ComplianceAPIServer) Start(ctx context.Context, serverContext *Complian } getComplianceEvents(serverContext.DB, w, r, userConfig) case http.MethodPost: - postComplianceEvent(serverContext.DB, s.cfg, authenticatedClient, authenticator, w, r) + postComplianceEvent(serverContext.DB, s.cfg, w, r) default: writeErrMsgJSON(w, "Method not allowed", http.StatusMethodNotAllowed) } @@ -1003,13 +975,7 @@ LEFT JOIN policies ON compliance_events.policy_id = policies.id` + whereClause / } } -func postComplianceEvent(db *sql.DB, - cfg *rest.Config, - authenticatedClient *kubernetes.Clientset, - authenticator *apiserverx509.Authenticator, - w http.ResponseWriter, - r *http.Request, -) { +func postComplianceEvent(db *sql.DB, cfg *rest.Config, w http.ResponseWriter, r *http.Request) { body, err := io.ReadAll(r.Body) if err != nil { log.Error(err, "error reading request body") @@ -1032,7 +998,7 @@ func postComplianceEvent(db *sql.DB, return } - allowed, err := canRecordComplianceEvent(cfg, authenticatedClient, authenticator, reqEvent.Cluster.Name, r) + allowed, err := canRecordComplianceEvent(cfg, reqEvent.Cluster.Name, r) if err != nil { if errors.Is(err, ErrUnauthorized) { writeErrMsgJSON(w, "Unauthorized", http.StatusUnauthorized) diff --git a/go.mod b/go.mod index 72d69cd1..2a6a77b5 100644 --- a/go.mod +++ b/go.mod @@ -18,7 +18,6 @@ require ( github.com/stolostron/kubernetes-dependency-watches v0.5.2-0.20231212185913-628ab39622b8 k8s.io/api v0.27.7 k8s.io/apimachinery v0.27.7 - k8s.io/apiserver v0.27.7 k8s.io/client-go v0.27.7 k8s.io/klog/v2 v2.100.1 open-cluster-management.io/api v0.12.0 @@ -31,7 +30,6 @@ require ( github.com/Masterminds/semver/v3 v3.2.1 // indirect github.com/Masterminds/sprig/v3 v3.2.3 // indirect github.com/beorn7/perks v1.0.1 // indirect - github.com/blang/semver/v4 v4.0.0 // indirect github.com/cespare/xxhash/v2 v2.2.0 // indirect github.com/davecgh/go-spew v1.1.1 // indirect github.com/emicklei/go-restful/v3 v3.11.0 // indirect diff --git a/go.sum b/go.sum index 2cdbe54c..bacdb68a 100644 --- a/go.sum +++ b/go.sum @@ -17,8 +17,6 @@ github.com/antihax/optional v1.0.0/go.mod h1:uupD/76wgC+ih3iEmQUL+0Ugr19nfwCT1kd github.com/benbjohnson/clock v1.1.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= -github.com/blang/semver/v4 v4.0.0 h1:1PFHFE6yCCTv8C1TeyNNarDzntLi7wMI5i/pzqYIsAM= -github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ= github.com/buger/jsonparser v1.1.1/go.mod h1:6RYKKt7H4d4+iWqouImQ9R2FZql3VbhNgx27UK13J/0= github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU= github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc= @@ -366,8 +364,6 @@ k8s.io/apiextensions-apiserver v0.27.7 h1:YqIOwZAUokzxJIjunmUd4zS1v3JhK34EPXn+pP k8s.io/apiextensions-apiserver v0.27.7/go.mod h1:x0p+b5a955lfPz9gaDeBy43obM12s+N9dNHK6+dUL+g= k8s.io/apimachinery v0.27.7 h1:Gxgtb7Y/Rsu8ymgmUEaiErkxa6RY4oTd8kNUI6SUR58= k8s.io/apimachinery v0.27.7/go.mod h1:jBGQgTjkw99ef6q5hv1YurDd3BqKDk9YRxmX0Ozo0i8= -k8s.io/apiserver v0.27.7 h1:E8sDHwfUug82YC1++qvE73QxihaXDqT4tr8XYBOEtc4= -k8s.io/apiserver v0.27.7/go.mod h1:OrLG9RwCOerutAlo8QJW5EHzUG9Dad7k6rgcDUNSO/w= k8s.io/client-go v0.27.7 h1:+Xgh9OOKv6A3qdD4Dnl/0VOI5EvAv+0s/OseDxVVTwQ= k8s.io/client-go v0.27.7/go.mod h1:dZ2kqcalYp5YZ2EV12XIMc77G6PxHWOJp/kclZr4+5Q= k8s.io/component-base v0.27.7 h1:kngM58HR9W9Nqpv7e4rpdRyWnKl/ABpUhLAZ+HoliMs= diff --git a/main.go b/main.go index e9dfcf66..6b0adc05 100644 --- a/main.go +++ b/main.go @@ -583,24 +583,8 @@ func startComplianceEventsAPI( reconciler.DynamicWatcher = dbSecretDynamicWatcher var cert *tls.Certificate - var clientAuthCAs []byte if complianceAPICert != "" && complianceAPIKey != "" { - if cfg.CAData == nil && cfg.CAFile == "" { - log.Info("The kubeconfig does not contain a CA") - os.Exit(1) - } - - if cfg.CAData == nil { - clientAuthCAs, err = os.ReadFile(cfg.CAFile) - if err != nil { - log.Error(err, "The kubeconfig does not contain a valid CA") - os.Exit(1) - } - } else { - clientAuthCAs = cfg.CAData - } - certTemp, err := tls.LoadX509KeyPair(complianceAPICert, complianceAPIKey) if err != nil { log.Error( @@ -617,7 +601,7 @@ func startComplianceEventsAPI( log.Info("The compliance events history API will listen on HTTP since no certificate was provided") } - complianceAPI := complianceeventsapi.NewComplianceAPIServer(complianceAPIAddr, cfg, clientAuthCAs, cert) + complianceAPI := complianceeventsapi.NewComplianceAPIServer(complianceAPIAddr, cfg, cert) wg.Add(1) diff --git a/test/e2e/case18_compliance_api_test.go b/test/e2e/case18_compliance_api_test.go index 42e8c55f..9badf9a8 100644 --- a/test/e2e/case18_compliance_api_test.go +++ b/test/e2e/case18_compliance_api_test.go @@ -131,7 +131,7 @@ var _ = Describe("Test the compliance events API", Label("compliance-events-api" err = complianceServerCtx.MigrateDB(ctx, k8sClient, "open-cluster-management") Expect(err).ToNot(HaveOccurred()) - complianceAPI := complianceeventsapi.NewComplianceAPIServer("localhost:8385", k8sConfig, nil, nil) + complianceAPI := complianceeventsapi.NewComplianceAPIServer("localhost:8385", k8sConfig, nil) httpCtx, httpCtxCancel := context.WithCancel(context.Background()) diff --git a/test/e2e/case20_compliance_api_controller_test.go b/test/e2e/case20_compliance_api_controller_test.go index 00721342..80f1bc08 100644 --- a/test/e2e/case20_compliance_api_controller_test.go +++ b/test/e2e/case20_compliance_api_controller_test.go @@ -5,14 +5,8 @@ package e2e import ( "bytes" "context" - cryptorand "crypto/rand" - "crypto/rsa" - "crypto/tls" - "crypto/x509" - "crypto/x509/pkix" "database/sql" "encoding/json" - "encoding/pem" "fmt" "io" "math/rand" @@ -22,13 +16,11 @@ import ( "github.com/google/uuid" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - certv1 "k8s.io/api/certificates/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - certutil "k8s.io/client-go/util/cert" "open-cluster-management.io/governance-policy-propagator/controllers/complianceeventsapi" "open-cluster-management.io/governance-policy-propagator/controllers/propagator" @@ -233,7 +225,6 @@ var _ = Describe("Test compliance events API authentication and authorization", eventsEndpoint := fmt.Sprintf("https://localhost:%d/api/v1/compliance-events", complianceAPIPort) const saName string = "compliance-api-user" var token string - var clientCert tls.Certificate getSamplePostRequest := func(clusterName string) *bytes.Buffer { payload := []byte(fmt.Sprintf(`{ @@ -339,63 +330,6 @@ var _ = Describe("Test compliance events API authentication and authorization", token = string(secret.Data["token"]) }, defaultTimeoutSeconds, 1).Should(Succeed()) - - privateKey, err := rsa.GenerateKey(cryptorand.Reader, 2048) - Expect(err).ToNot(HaveOccurred()) - - csr, err := certutil.MakeCSR( - privateKey, - &pkix.Name{CommonName: fmt.Sprintf("system:serviceaccount:%s:%s", testNamespace, saName)}, - nil, - nil, - ) - Expect(err).ToNot(HaveOccurred()) - - csrObj, err := clientHub.CertificatesV1().CertificateSigningRequests().Create( - ctx, - &certv1.CertificateSigningRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: saName, - }, - Spec: certv1.CertificateSigningRequestSpec{ - Request: csr, - SignerName: certv1.KubeAPIServerClientSignerName, - Usages: []certv1.KeyUsage{ - certv1.UsageDigitalSignature, - certv1.UsageKeyEncipherment, - certv1.UsageClientAuth, - }, - }, - }, - metav1.CreateOptions{}, - ) - Expect(err).ToNot(HaveOccurred()) - - csrObj.Status.Conditions = append(csrObj.Status.Conditions, certv1.CertificateSigningRequestCondition{ - Type: certv1.CertificateApproved, - Reason: "test", - Message: "This CSR was approved by the test", - LastUpdateTime: metav1.Now(), - Status: corev1.ConditionTrue, - }) - - _, err = clientHub.CertificatesV1().CertificateSigningRequests().UpdateApproval( - ctx, saName, csrObj, metav1.UpdateOptions{}, - ) - Expect(err).ToNot(HaveOccurred()) - - Eventually(func(g Gomega) { - csrObj, err = clientHub.CertificatesV1().CertificateSigningRequests().Get(ctx, saName, metav1.GetOptions{}) - g.Expect(csrObj.Status.Certificate).ToNot(BeNil()) - }, defaultTimeoutSeconds, 1).Should(Succeed()) - - keyPem := pem.EncodeToMemory(&pem.Block{ - Type: "RSA PRIVATE KEY", - Bytes: x509.MarshalPKCS1PrivateKey(privateKey), - }) - - clientCert, err = tls.X509KeyPair(csrObj.Status.Certificate, keyPem) - Expect(err).ToNot(HaveOccurred()) }) AfterAll(func(ctx context.Context) { @@ -420,11 +354,6 @@ var _ = Describe("Test compliance events API authentication and authorization", Expect(err).ToNot(HaveOccurred()) } - err = clientHub.CertificatesV1().CertificateSigningRequests().Delete(ctx, saName, metav1.DeleteOptions{}) - if !k8serrors.IsNotFound(err) { - Expect(err).ToNot(HaveOccurred()) - } - By("Deleting all database records") connectionURL := "postgresql://grc:grc@localhost:5432/ocm-compliance-history?sslmode=disable" db, err := sql.Open("postgres", connectionURL) @@ -459,7 +388,7 @@ var _ = Describe("Test compliance events API authentication and authorization", Expect(resp.StatusCode).To(Equal(http.StatusUnauthorized)) }) - It("Rejects recording the compliance event for the wrong namespace (token auth)", func(ctx context.Context) { + It("Rejects recording the compliance event for the wrong namespace", func(ctx context.Context) { payload := getSamplePostRequest("cluster") req, err := http.NewRequestWithContext(ctx, http.MethodPost, eventsEndpoint, payload) @@ -477,61 +406,7 @@ var _ = Describe("Test compliance events API authentication and authorization", Expect(resp.StatusCode).To(Equal(http.StatusForbidden)) }) - It("Rejects recording the compliance event for the wrong namespace (cert auth)", func(ctx context.Context) { - payload := getSamplePostRequest("cluster") - - req, err := http.NewRequestWithContext(ctx, http.MethodPost, eventsEndpoint, payload) - Expect(err).ToNot(HaveOccurred()) - req.Header.Set("Content-Type", "application/json") - - httpClient := http.Client{ - Timeout: 30 * time.Second, - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true, Certificates: []tls.Certificate{clientCert}}, - }, - } - - resp, err := httpClient.Do(req) - Expect(err).ToNot(HaveOccurred()) - - if resp != nil { - defer resp.Body.Close() - } - - Expect(resp.StatusCode).To(Equal(http.StatusForbidden)) - }) - - It("Rejects recording the compliance event with cert signed by unknown CA (cert auth)", func(ctx context.Context) { - payload := getSamplePostRequest(testNamespace) - - req, err := http.NewRequestWithContext(ctx, http.MethodPost, eventsEndpoint, payload) - Expect(err).ToNot(HaveOccurred()) - req.Header.Set("Content-Type", "application/json") - - cert, key, err := certutil.GenerateSelfSignedCertKey("test.local", nil, nil) - Expect(err).ToNot(HaveOccurred()) - - badClientCert, err := tls.X509KeyPair(cert, key) - Expect(err).ToNot(HaveOccurred()) - - httpClient := http.Client{ - Timeout: 30 * time.Second, - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true, Certificates: []tls.Certificate{badClientCert}}, - }, - } - - resp, err := httpClient.Do(req) - Expect(err).ToNot(HaveOccurred()) - - if resp != nil { - defer resp.Body.Close() - } - - Expect(resp.StatusCode).To(Equal(http.StatusUnauthorized)) - }) - - It("Allows recording the compliance event (token auth)", func(ctx context.Context) { + It("Allows recording the compliance event", func(ctx context.Context) { payload := getSamplePostRequest(testNamespace) req, err := http.NewRequestWithContext(ctx, http.MethodPost, eventsEndpoint, payload) @@ -548,28 +423,4 @@ var _ = Describe("Test compliance events API authentication and authorization", Expect(resp.StatusCode).To(Equal(http.StatusCreated)) }) - - It("Allows recording the compliance event (cert auth)", func(ctx context.Context) { - payload := getSamplePostRequest(testNamespace) - - req, err := http.NewRequestWithContext(ctx, http.MethodPost, eventsEndpoint, payload) - Expect(err).ToNot(HaveOccurred()) - req.Header.Set("Content-Type", "application/json") - - httpClient := http.Client{ - Timeout: 30 * time.Second, - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true, Certificates: []tls.Certificate{clientCert}}, - }, - } - - resp, err := httpClient.Do(req) - Expect(err).ToNot(HaveOccurred()) - - if resp != nil { - defer resp.Body.Close() - } - - Expect(resp.StatusCode).To(Equal(http.StatusCreated)) - }) })