From 1487f36f2137da1a66d233c1080d2bccfa692ce8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Mon, 26 Aug 2024 16:17:12 +0200 Subject: [PATCH 1/8] disable the `config.server` field when using --venafi-connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For context, EU customers need to change the URL of the VCP API to point to https://api.venafi.eu. To do that, and imagining that they are using the VenafiConnection authentication, they may try to use the `spec.vcp.url` field on their VenafiConnection resource and find that this field doesn’t do anything because the Helm chart's `config.server` is set to https://api.venafi.cloud by default. Another possible scenario is that EU customers may end up with a VenafiConnection configured with the `spec.vcp.url` field set to `https://api.venafi.eu`. This VenafiConnection will have been already working well with venafi-enhanced-issuer and approver-policy-enterprise. Once they try to switch the Agent to the VenafiConnection auth method, they will see that it doesn’t work because the Agent picks up the default value in the Agent’s helm chart, i.e., ``` config: server: https://api.venafi.cloud. ``` --- deploy/charts/venafi-kubernetes-agent/README.md | 4 ++-- deploy/charts/venafi-kubernetes-agent/values.yaml | 11 ++++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/deploy/charts/venafi-kubernetes-agent/README.md b/deploy/charts/venafi-kubernetes-agent/README.md index 2071a435..e15cfd2d 100644 --- a/deploy/charts/venafi-kubernetes-agent/README.md +++ b/deploy/charts/venafi-kubernetes-agent/README.md @@ -26,13 +26,13 @@ If you do not have one, you can sign up for a free trial now at: | authentication.venafiConnection.namespace | string | `"venafi"` | The namespace of a VenafiConnection resource which contains the configuration for authenticating to Venafi. | | command | list | `[]` | Specify the command to run overriding default binary. | | config | object | `{"clientId":"","clusterDescription":"","clusterName":"","configmap":{"key":null,"name":null},"ignoredSecretTypes":["kubernetes.io/service-account-token","kubernetes.io/dockercfg","kubernetes.io/dockerconfigjson","kubernetes.io/basic-auth","kubernetes.io/ssh-auth","bootstrap.kubernetes.io/token","helm.sh/release.v1"],"period":"0h1m0s","server":"https://api.venafi.cloud/"}` | Configuration section for the Venafi Kubernetes Agent itself | -| config.clientId | string | `""` | The client-id returned from the Venafi Control Plane | +| config.clientId | string | `""` | The client-id to be used for authenticating with the Venafi Control Plane. Only useful when using a Key Pair Service Account in the Venafi Control Plane. You can obtain the cliend ID by creating a Key Pair Service Account in the Venafi Control Plane. | | config.clusterDescription | string | `""` | Description for the cluster resource if it needs to be created in Venafi Control Plane | | config.clusterName | string | `""` | Name for the cluster resource if it needs to be created in Venafi Control Plane | | config.configmap | object | `{"key":null,"name":null}` | Specify ConfigMap details to load config from an existing resource. This should be blank by default unless you have you own config. | | config.ignoredSecretTypes | list | `["kubernetes.io/service-account-token","kubernetes.io/dockercfg","kubernetes.io/dockerconfigjson","kubernetes.io/basic-auth","kubernetes.io/ssh-auth","bootstrap.kubernetes.io/token","helm.sh/release.v1"]` | Reduce the memory usage of the agent and reduce the load on the Kubernetes API server by omitting various common Secret types when listing Secrets. These Secret types will be added to a "type!=" field selector in the agent config. * https://docs.venafi.cloud/vaas/k8s-components/t-cfg-tlspk-agent/#configuration * https://kubernetes.io/docs/concepts/configuration/secret/#secret-types * https://kubernetes.io/docs/concepts/overview/working-with-objects/field-selectors/#list-of-supported-fields | | config.period | string | `"0h1m0s"` | Send data back to the platform every minute unless changed | -| config.server | string | `"https://api.venafi.cloud/"` | Overrides the server if using a proxy in your environment For the EU variant use: https://api.venafi.eu/ | +| config.server | string | `"https://api.venafi.cloud/"` | API URL of the Venafi Control Plane API. For EU tenants, set this value to https://api.venafi.eu/. If you are using the VenafiConnection authentication method, you must set the API URL using the field `spec.vcp.url` on the VenafiConnection resource instead. | | crds.forceRemoveValidationAnnotations | bool | `false` | The 'x-kubernetes-validations' annotation is not supported in Kubernetes 1.22 and below. This annotation is used by CEL, which is a feature introduced in Kubernetes 1.25 that improves how validation is performed. This option allows to force the 'x-kubernetes-validations' annotation to be excluded, even on Kubernetes 1.25+ clusters. | | crds.venafiConnection | object | `{"include":false}` | Optionally include the VenafiConnection CRDs | | crds.venafiConnection.include | bool | `false` | When set to false, the rendered output does not contain the VenafiConnection CRDs and RBAC. This is useful for when the Venafi Connection resources are already installed separately. | diff --git a/deploy/charts/venafi-kubernetes-agent/values.yaml b/deploy/charts/venafi-kubernetes-agent/values.yaml index 35bef6e5..d1342741 100644 --- a/deploy/charts/venafi-kubernetes-agent/values.yaml +++ b/deploy/charts/venafi-kubernetes-agent/values.yaml @@ -191,10 +191,15 @@ authentication: # -- Configuration section for the Venafi Kubernetes Agent itself config: - # -- Overrides the server if using a proxy in your environment - # For the EU variant use: https://api.venafi.eu/ + # -- API URL of the Venafi Control Plane API. For EU tenants, set this value to + # https://api.venafi.eu/. If you are using the VenafiConnection authentication + # method, you must set the API URL using the field `spec.vcp.url` on the + # VenafiConnection resource instead. server: "https://api.venafi.cloud/" - # -- The client-id returned from the Venafi Control Plane + # -- The client-id to be used for authenticating with the Venafi Control + # Plane. Only useful when using a Key Pair Service Account in the Venafi + # Control Plane. You can obtain the cliend ID by creating a Key Pair Service + # Account in the Venafi Control Plane. clientId: "" # -- Send data back to the platform every minute unless changed period: "0h1m0s" From f326423d0a5f8b3858a41d8dfde88353fef3cfcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Mon, 26 Aug 2024 20:07:52 +0200 Subject: [PATCH 2/8] venconn: bug: the err from NewVenConnClient was shadowed by a local copy I caught this thanks to golangci-lint's ineffassign which showed the warning "ineffectual assignment to err". --- pkg/agent/config.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/agent/config.go b/pkg/agent/config.go index 8eb7b6fb..9b107d12 100644 --- a/pkg/agent/config.go +++ b/pkg/agent/config.go @@ -354,7 +354,8 @@ func getConfiguration(log *log.Logger, cfg Config, flags AgentCmdFlags) (Config, log.Printf(`ignoring venafi-cloud.uploader_id. In Venafi Connection mode, this field is not needed.`) } - restCfg, err := kubeconfig.LoadRESTConfig("") + var restCfg *rest.Config + restCfg, err = kubeconfig.LoadRESTConfig("") if err != nil { return Config{}, nil, fmt.Errorf("failed to load kubeconfig: %w", err) } From 726bb7e874196e0f9ff27afb72c976af6dcb43b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Mon, 26 Aug 2024 20:37:03 +0200 Subject: [PATCH 3/8] keypair_auth: remove ineffective `err != nil` golangci-lint had been showing this warning for a while... The actual warning is "impossible condition: nil != nil" from gopls' nilness linter. --- pkg/client/client_venafi_cloud.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/client/client_venafi_cloud.go b/pkg/client/client_venafi_cloud.go index d0068141..69e8c1ca 100644 --- a/pkg/client/client_venafi_cloud.go +++ b/pkg/client/client_venafi_cloud.go @@ -288,9 +288,6 @@ func (c *VenafiCloudClient) updateAccessToken() error { values.Set("assertion", jwtToken) tokenURL := fullURL(c.baseURL, accessTokenEndpoint) - if err != nil { - return err - } encoded := values.Encode() request, err := http.NewRequest(http.MethodPost, tokenURL, strings.NewReader(encoded)) From 14ec08e8d5df8e8185a2c04770f964963dc863e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Thu, 29 Aug 2024 20:14:37 +0200 Subject: [PATCH 4/8] undent: last line's tabs weren't stripped --- pkg/testutil/testutil.go | 328 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 328 insertions(+) create mode 100644 pkg/testutil/testutil.go diff --git a/pkg/testutil/testutil.go b/pkg/testutil/testutil.go new file mode 100644 index 00000000..cd27ca72 --- /dev/null +++ b/pkg/testutil/testutil.go @@ -0,0 +1,328 @@ +package testutil + +import ( + "crypto/x509" + "io" + "net/http" + "net/http/httptest" + "os" + "strings" + "sync" + "testing" + + "github.com/jetstack/venafi-connection-lib/api/v1alpha1" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/yaml" + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/clientcmd" + clientcmdapi "k8s.io/client-go/tools/clientcmd/api" + ctrlruntime "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" +) + +// To see the API server logs, set: +// +// export KUBEBUILDER_ATTACH_CONTROL_PLANE_OUTPUT=true +func WithEnvtest(t testing.TB) (_ *envtest.Environment, _ *rest.Config, kclient ctrlruntime.WithWatch) { + t.Helper() + + // If KUBEBUILDER_ASSETS isn't set, show a warning to the user. + if os.Getenv("KUBEBUILDER_ASSETS") == "" { + t.Fatalf("KUBEBUILDER_ASSETS isn't set. You can run this test using `make test`.\n" + + "But if you prefer not to use `make`, run these two commands first:\n" + + " make _bin/tools/{kube-apiserver,etcd}\n" + + " export KUBEBUILDER_ASSETS=$PWD/_bin/tools") + } + envtest := &envtest.Environment{ + ErrorIfCRDPathMissing: true, + CRDDirectoryPaths: []string{"../../deploy/charts/venafi-kubernetes-agent/crd_bases/jetstack.io_venaficonnections.yaml"}, + } + + restconf, err := envtest.Start() + t.Cleanup(func() { + t.Log("Waiting for envtest to exit") + e := envtest.Stop() + require.NoError(t, e) + }) + require.NoError(t, err) + + sch := runtime.NewScheme() + _ = v1alpha1.AddToScheme(sch) + _ = corev1.AddToScheme(sch) + _ = rbacv1.AddToScheme(sch) + + kclient, err = ctrlruntime.NewWithWatch(restconf, ctrlruntime.Options{Scheme: sch}) + require.NoError(t, err) + + return envtest, restconf, kclient +} + +// Copied from https://github.com/kubernetes/client-go/issues/711#issuecomment-1666075787. +func WithKubeconfig(t testing.TB, restCfg *rest.Config) string { + t.Helper() + + clusters := make(map[string]*clientcmdapi.Cluster) + clusters["default-cluster"] = &clientcmdapi.Cluster{ + Server: restCfg.Host, + CertificateAuthorityData: restCfg.CAData, + } + contexts := make(map[string]*clientcmdapi.Context) + contexts["default-context"] = &clientcmdapi.Context{ + Cluster: "default-cluster", + AuthInfo: "default-user", + } + authinfos := make(map[string]*clientcmdapi.AuthInfo) + authinfos["default-user"] = &clientcmdapi.AuthInfo{ + ClientCertificateData: restCfg.CertData, + ClientKeyData: restCfg.KeyData, + } + clientConfig := clientcmdapi.Config{ + Kind: "Config", + APIVersion: "v1", + Clusters: clusters, + Contexts: contexts, + CurrentContext: "default-context", + AuthInfos: authinfos, + } + + d := t.TempDir() + kubeconfig, _ := os.CreateTemp(d, "kubeconfig") + defer kubeconfig.Close() + + err := clientcmd.WriteToFile(clientConfig, kubeconfig.Name()) + require.NoError(t, err) + + return kubeconfig.Name() +} + +// Undent removes leading indentation/white-space from given string and returns +// it as a string. Useful for inlining YAML manifests in Go code. Inline YAML +// manifests in the Go test files makes it easier to read the test case as +// opposed to reading verbose-y Go structs. +// +// This was copied from https://github.com/jimeh/Undent/blob/main/Undent.go, all +// credit goes to the author, Jim Myhrberg. +func Undent(s string) string { + const ( + tab = 9 + lf = 10 + spc = 32 + ) + + if len(s) == 0 { + return "" + } + + // find smallest indent relative to each line-feed + min := 99999999999 + count := 0 + + lfs := make([]int, 0, strings.Count(s, "\n")) + if s[0] != lf { + lfs = append(lfs, -1) + } + + indent := 0 + for i := 0; i < len(s); i++ { + if s[i] == lf { + lfs = append(lfs, i) + indent = 0 + } else if indent < min { + switch s[i] { + case spc, tab: + indent++ + default: + if indent > 0 { + count++ + } + if indent < min { + min = indent + } + } + } + } + + // extract each line without indentation + out := make([]byte, 0, len(s)-(min*count)) + + for i := 0; i < len(lfs); i++ { + offset := lfs[i] + 1 + end := len(s) + if i+1 < len(lfs) { + end = lfs[i+1] + 1 + } + + if offset+min <= end { + out = append(out, s[offset+min:end]...) + } else if offset < end { + out = append(out, s[offset:end]...) + } + } + + return string(out) +} + +// Parses the YAML manifest. Useful for inlining YAML manifests in Go test +// files, to be used in conjunction with `undent`. +func Parse(yamlmanifest string) []ctrlruntime.Object { + dec := yaml.NewYAMLOrJSONDecoder(strings.NewReader(yamlmanifest), 4096) + var objs []ctrlruntime.Object + for { + obj := &unstructured.Unstructured{} + err := dec.Decode(obj) + if err == io.EOF { + break + } + if err != nil { + panic(err) + } + + objs = append(objs, obj) + } + return objs +} + +type AssertRequest func(t testing.TB, r *http.Request) + +func FakeVenafiCloud(t *testing.T) (_ *httptest.Server, _ *x509.Certificate, setAssert func(AssertRequest)) { + t.Helper() + + var assertFn AssertRequest + assertFnMu := sync.Mutex{} + setAssert = func(setAssert AssertRequest) { + assertFnMu.Lock() + defer assertFnMu.Unlock() + assertFn = setAssert + } + + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Logf("fake api.venafi.cloud received request: %s %s", r.Method, r.URL.Path) + + assertFnMu.Lock() + defer assertFnMu.Unlock() + assertFn(t, r) + + accessToken := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") + apiKey := r.Header.Get("tppl-api-key") + if accessToken != "VALID_ACCESS_TOKEN" && apiKey != "VALID_API_KEY" { + w.WriteHeader(http.StatusUnauthorized) + return + } + if r.URL.Path == "/v1/tlspk/upload/clusterdata/no" { + if r.URL.Query().Get("name") != "test cluster name" { + w.WriteHeader(http.StatusBadRequest) + return + } + _, _ = w.Write([]byte(`{"status":"ok","organization":"756db001-280e-11ee-84fb-991f3177e2d0"}`)) + } else if r.URL.Path == "/v1/useraccounts" { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"user": {"username": "user","id": "76a126f0-280e-11ee-84fb-991f3177e2d0"}}`)) + + } else if r.URL.Path == "/v1/oauth2/v2.0/756db001-280e-11ee-84fb-991f3177e2d0/token" { + _, _ = w.Write([]byte(`{"access_token":"VALID_ACCESS_TOKEN","expires_in":900,"token_type":"bearer"}`)) + } else { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":"unexpected path in the test server","path":"` + r.URL.Path + `"}`)) + } + })) + t.Cleanup(server.Close) + + cert, err := x509.ParseCertificate(server.TLS.Certificates[0].Certificate[0]) + require.NoError(t, err) + + return server, cert, setAssert +} + +func FakeTPP(t testing.TB) (*httptest.Server, *x509.Certificate) { + t.Helper() + + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Logf("fake tpp.example.com received request: %s %s", r.Method, r.URL.Path) + + accessToken := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") + + if r.URL.Path == "/vedsdk/Identity/Self" { + if accessToken != "VALID_ACCESS_TOKEN" { + w.WriteHeader(http.StatusUnauthorized) + return + } + _, _ = w.Write([]byte(`{"Identities":[{"Name":"TEST"}]}`)) + } else if r.URL.Path == "/vedsdk/certificates/checkpolicy" { + _, _ = w.Write([]byte(`{"Policy":{"Subject":{"Organization":{"Value": "test-org"}}}}`)) + } else { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":"unexpected path in the test server","path":"` + r.URL.Path + `"}`)) + } + })) + t.Cleanup(server.Close) + + cert, err := x509.ParseCertificate(server.TLS.Certificates[0].Certificate[0]) + require.NoError(t, err) + + return server, cert +} + +// Generated using: +// +// helm template ./deploy/charts/venafi-kubernetes-agent -n venafi --set venafiConnection.include=true --show-only templates/venafi-connection-VenConnRBAC.yaml | grep -ivE '(helm|\/version)' +const VenConnRBAC = ` +apiVersion: v1 +kind: Namespace +metadata: + name: venafi +--- +# Source: venafi-kubernetes-agent/templates/venafi-connection-rbac.yaml +# The 'venafi-connection' service account is used by multiple +# controllers. When configuring which resources a VenafiConnection +# can access, the RBAC rules you create manually must point to this SA. +apiVersion: v1 +kind: ServiceAccount +metadata: + name: venafi-connection + namespace: "venafi" + labels: + app.kubernetes.io/name: "venafi-connection" + app.kubernetes.io/instance: release-name +--- +# Source: venafi-kubernetes-agent/templates/venafi-connection-rbac.yaml +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: venafi-connection-role + labels: + app.kubernetes.io/name: "venafi-connection" + app.kubernetes.io/instance: release-name +rules: +- apiGroups: [ "" ] + resources: [ "namespaces" ] + verbs: [ "get", "list", "watch" ] + +- apiGroups: [ "jetstack.io" ] + resources: [ "venaficonnections" ] + verbs: [ "get", "list", "watch" ] + +- apiGroups: [ "jetstack.io" ] + resources: [ "venaficonnections/status" ] + verbs: [ "get", "patch" ] +--- +# Source: venafi-kubernetes-agent/templates/venafi-connection-rbac.yaml +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: venafi-connection-rolebinding + labels: + app.kubernetes.io/name: "venafi-connection" + app.kubernetes.io/instance: release-name +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: venafi-connection-role +subjects: +- kind: ServiceAccount + name: venafi-connection + namespace: "venafi" +` From 57492b64c69d1bc402daffd7c6b51625368a6dbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 30 Aug 2024 08:12:14 +0200 Subject: [PATCH 5/8] config: rest.Config's rest package wasn't being imported --- pkg/agent/config.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/agent/config.go b/pkg/agent/config.go index 9b107d12..ac79c69e 100644 --- a/pkg/agent/config.go +++ b/pkg/agent/config.go @@ -19,6 +19,7 @@ import ( "github.com/pkg/errors" "github.com/spf13/cobra" "gopkg.in/yaml.v3" + "k8s.io/client-go/rest" ) const ( From fc82e00eb7d2b00a52e0b7d92f5a58254222ec17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 30 Aug 2024 08:40:53 +0200 Subject: [PATCH 6/8] venconn_tests: forgot to use testutil --- pkg/client/client_venconn_test.go | 213 ++---------------------------- 1 file changed, 10 insertions(+), 203 deletions(-) diff --git a/pkg/client/client_venconn_test.go b/pkg/client/client_venconn_test.go index 67062a65..d644880b 100644 --- a/pkg/client/client_venconn_test.go +++ b/pkg/client/client_venconn_test.go @@ -3,28 +3,18 @@ package client_test import ( "context" "crypto/x509" - "io" - "net/http" - "net/http/httptest" - "os" "strings" "testing" "github.com/jetstack/preflight/api" "github.com/jetstack/preflight/pkg/client" + "github.com/jetstack/preflight/pkg/testutil" "github.com/jetstack/venafi-connection-lib/api/v1alpha1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" - rbacv1 "k8s.io/api/rbac/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/yaml" - "k8s.io/client-go/rest" ctrlruntime "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/envtest" ) // These are using envtest (slow) rather than a fake clientset (fast) because @@ -44,7 +34,7 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { t.Parallel() t.Run("valid accessToken", run(testcase{ - given: undent(` + given: testutil.Undent(` apiVersion: jetstack.io/v1alpha1 kind: VenafiConnection metadata: @@ -63,7 +53,7 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { // Why isn't it possible to use the 'apiKey' field? Although the // Kubernetes Discovery endpoint works with an API key, we have decided // to not support it because it isn't recommended. - given: undent(` + given: testutil.Undent(` apiVersion: jetstack.io/v1alpha1 kind: VenafiConnection metadata: @@ -84,7 +74,7 @@ func TestVenConnClient_PostDataReadingsWithOptions(t *testing.T) { // debugging and making the venafi connection work, and then find out // that it doesn't work. The reason is because as of now, we don't first // check if the user has used the 'tpp' field before running Get. - given: undent(` + given: testutil.Undent(` apiVersion: jetstack.io/v1alpha1 kind: VenafiConnection metadata: @@ -171,9 +161,9 @@ type testcase struct { func run(test testcase) func(t *testing.T) { return func(t *testing.T) { - fakeVenafiCloud, certCloud := fakeVenafiCloud(t) - fakeTPP, certTPP := fakeTPP(t) - _, restconf, kclient := startEnvtest(t) + fakeVenafiCloud, certCloud, _ := testutil.FakeVenafiCloud(t) + fakeTPP, certTPP := testutil.FakeTPP(t) + _, restconf, kclient := testutil.WithEnvtest(t) certPool := x509.NewCertPool() certPool.AddCert(certCloud) @@ -210,8 +200,8 @@ func run(test testcase) func(t *testing.T) { test.given = strings.ReplaceAll(test.given, "FAKE_TPP_URL", fakeTPP.URL) var given []ctrlruntime.Object - given = append(given, parse(rbac)...) - given = append(given, parse(undent(` + given = append(given, testutil.Parse(rbac)...) + given = append(given, testutil.Parse(testutil.Undent(` apiVersion: v1 kind: Secret metadata: @@ -252,7 +242,7 @@ func run(test testcase) func(t *testing.T) { - kind: ServiceAccount name: venafi-connection namespace: venafi`))...) - given = append(given, parse(test.given)...) + given = append(given, testutil.Parse(test.given)...) for _, obj := range given { require.NoError(t, kclient.Create(context.Background(), obj)) } @@ -269,186 +259,3 @@ func run(test testcase) func(t *testing.T) { assert.Equal(t, test.expectReadyCondMsg, got.Status.Conditions[0].Message) } } - -func fakeVenafiCloud(t *testing.T) (*httptest.Server, *x509.Certificate) { - server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - t.Logf("fake api.venafi.cloud received request: %s %s", r.Method, r.URL.Path) - accessToken := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") - apiKey := r.Header.Get("tppl-api-key") - if accessToken != "VALID_ACCESS_TOKEN" && apiKey != "VALID_API_KEY" { - w.WriteHeader(http.StatusUnauthorized) - return - } - if r.URL.Path == "/v1/tlspk/upload/clusterdata/no" { - if r.URL.Query().Get("name") != "test cluster name" { - w.WriteHeader(http.StatusBadRequest) - return - } - _, _ = w.Write([]byte(`{"status":"ok","organization":"756db001-280e-11ee-84fb-991f3177e2d0"}`)) - } else if r.URL.Path == "/v1/useraccounts" { - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`{"user": {"username": "user","id": "76a126f0-280e-11ee-84fb-991f3177e2d0"}}`)) - - } else if r.URL.Path == "/v1/oauth2/v2.0/756db001-280e-11ee-84fb-991f3177e2d0/token" { - _, _ = w.Write([]byte(`{"access_token":"VALID_ACCESS_TOKEN","expires_in":900,"token_type":"bearer"}`)) - } else { - w.WriteHeader(http.StatusInternalServerError) - _, _ = w.Write([]byte(`{"error":"unexpected path in the test server","path":"` + r.URL.Path + `"}`)) - } - })) - t.Cleanup(server.Close) - - cert, err := x509.ParseCertificate(server.TLS.Certificates[0].Certificate[0]) - require.NoError(t, err) - - return server, cert -} - -func fakeTPP(t testing.TB) (*httptest.Server, *x509.Certificate) { - server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - t.Logf("fake tpp.example.com received request: %s %s", r.Method, r.URL.Path) - - accessToken := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") - - if r.URL.Path == "/vedsdk/Identity/Self" { - if accessToken != "VALID_ACCESS_TOKEN" { - w.WriteHeader(http.StatusUnauthorized) - return - } - _, _ = w.Write([]byte(`{"Identities":[{"Name":"TEST"}]}`)) - } else if r.URL.Path == "/vedsdk/certificates/checkpolicy" { - _, _ = w.Write([]byte(`{"Policy":{"Subject":{"Organization":{"Value": "test-org"}}}}`)) - } else { - w.WriteHeader(http.StatusInternalServerError) - _, _ = w.Write([]byte(`{"error":"unexpected path in the test server","path":"` + r.URL.Path + `"}`)) - } - })) - t.Cleanup(server.Close) - - cert, err := x509.ParseCertificate(server.TLS.Certificates[0].Certificate[0]) - require.NoError(t, err) - - return server, cert -} - -// To see the API server logs, set: -// -// export KUBEBUILDER_ATTACH_CONTROL_PLANE_OUTPUT=true -func startEnvtest(t testing.TB) (_ *envtest.Environment, _ *rest.Config, kclient ctrlruntime.WithWatch) { - // If KUBEBUILDER_ASSETS isn't set, show a warning to the user. - if os.Getenv("KUBEBUILDER_ASSETS") == "" { - t.Fatalf("KUBEBUILDER_ASSETS isn't set. You can run this test using `make test`.\n" + - "But if you prefer not to use `make`, run these two commands first:\n" + - " make _bin/tools/{kube-apiserver,etcd}\n" + - " export KUBEBUILDER_ASSETS=$PWD/_bin/tools") - } - envtest := &envtest.Environment{ - ErrorIfCRDPathMissing: true, - CRDDirectoryPaths: []string{"../../deploy/charts/venafi-kubernetes-agent/crd_bases/jetstack.io_venaficonnections.yaml"}, - } - - restconf, err := envtest.Start() - t.Cleanup(func() { - t.Log("Waiting for envtest to exit") - e := envtest.Stop() - require.NoError(t, e) - }) - require.NoError(t, err) - - sch := runtime.NewScheme() - _ = v1alpha1.AddToScheme(sch) - _ = corev1.AddToScheme(sch) - _ = rbacv1.AddToScheme(sch) - - kclient, err = ctrlruntime.NewWithWatch(restconf, ctrlruntime.Options{Scheme: sch}) - require.NoError(t, err) - - return envtest, restconf, kclient -} - -// Undent removes leading indentation/white-space from given string and returns -// it as a string. Useful for inlining YAML manifests in Go code. Inline YAML -// manifests in the Go test files makes it easier to read the test case as -// opposed to reading verbose-y Go structs. -// -// This was copied from https://github.com/jimeh/undent/blob/main/undent.go, all -// credit goes to the author, Jim Myhrberg. -func undent(s string) string { - const ( - tab = 9 - lf = 10 - spc = 32 - ) - - if len(s) == 0 { - return "" - } - - // find smallest indent relative to each line-feed - min := 99999999999 - count := 0 - - lfs := make([]int, 0, strings.Count(s, "\n")) - if s[0] != lf { - lfs = append(lfs, -1) - } - - indent := 0 - for i := 0; i < len(s); i++ { - if s[i] == lf { - lfs = append(lfs, i) - indent = 0 - } else if indent < min { - switch s[i] { - case spc, tab: - indent++ - default: - if indent > 0 { - count++ - } - if indent < min { - min = indent - } - } - } - } - - // extract each line without indentation - out := make([]byte, 0, len(s)-(min*count)) - - for i := 0; i < len(lfs); i++ { - offset := lfs[i] + 1 - end := len(s) - if i+1 < len(lfs) { - end = lfs[i+1] + 1 - } - - if offset+min < end { - out = append(out, s[offset+min:end]...) - } else if offset < end { - out = append(out, s[offset:end]...) - } - } - - return string(out) -} - -// Parses the YAML manifest. Useful for inlining YAML manifests in Go test -// files, to be used in conjunction with `undent`. -func parse(yamlmanifest string) []ctrlruntime.Object { - dec := yaml.NewYAMLOrJSONDecoder(strings.NewReader(yamlmanifest), 4096) - var objs []ctrlruntime.Object - for { - obj := &unstructured.Unstructured{} - err := dec.Decode(obj) - if err == io.EOF { - break - } - if err != nil { - panic(err) - } - - objs = append(objs, obj) - } - return objs -} From 8a34fcf90d97f67958169eee5c9843fb66e64601 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 30 Aug 2024 09:00:31 +0200 Subject: [PATCH 7/8] venconn: test that venconn's url is used, not server field --- pkg/agent/config_test.go | 132 ++++++++++++++++++++++++++++++++++- pkg/client/client_venconn.go | 17 +++-- pkg/testutil/testutil.go | 1 + 3 files changed, 142 insertions(+), 8 deletions(-) diff --git a/pkg/agent/config_test.go b/pkg/agent/config_test.go index 1f8887f9..99f5d01b 100644 --- a/pkg/agent/config_test.go +++ b/pkg/agent/config_test.go @@ -2,17 +2,24 @@ package agent import ( "bytes" + "context" + "crypto/x509" "fmt" "io" "log" + "net/http" "os" "strings" "testing" "time" "github.com/jetstack/preflight/pkg/client" + "github.com/jetstack/preflight/pkg/testutil" "github.com/kylelemons/godebug/diff" + "github.com/spf13/cobra" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" ) func TestGetConfiguration(t *testing.T) { @@ -188,6 +195,100 @@ func TestGetConfiguration(t *testing.T) { }) } +// Slower test cases due to envtest. That's why they are separated from the +// other tests. +func Test_getConfiguration_urlWhenVenafiConnection(t *testing.T) { + t.Run("the server field is ignored when VenafiConnection is used", func(t *testing.T) { + _, restCfg, kcl := testutil.WithEnvtest(t) + os.Setenv("KUBECONFIG", testutil.WithKubeconfig(t, restCfg)) + srv, fakeCrt, setVenafiCloudAssert := testutil.FakeVenafiCloud(t) + for _, obj := range testutil.Parse( + testutil.VenConnRBAC + testutil.Undent(fmt.Sprintf(` + --- + apiVersion: jetstack.io/v1alpha1 + kind: VenafiConnection + metadata: + name: venafi-components + namespace: venafi + spec: + vcp: + url: "%s" + accessToken: + - secret: + name: accesstoken + fields: [accesstoken] + --- + apiVersion: v1 + kind: Secret + metadata: + name: accesstoken + namespace: venafi + stringData: + accesstoken: VALID_ACCESS_TOKEN + --- + apiVersion: rbac.authorization.k8s.io/v1 + kind: Role + metadata: + name: venafi-connection-accesstoken-reader + namespace: venafi + rules: + - apiGroups: [""] + resources: ["secrets"] + verbs: ["get"] + resourceNames: ["accesstoken"] + --- + apiVersion: rbac.authorization.k8s.io/v1 + kind: RoleBinding + metadata: + name: venafi-connection-accesstoken-reader + namespace: venafi + roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: venafi-connection-accesstoken-reader + subjects: + - kind: ServiceAccount + name: venafi-connection + namespace: venafi`, srv.URL))) { + require.NoError(t, kcl.Create(context.Background(), obj)) + } + + // The URL received by the fake Venafi Cloud server should be the one + // coming from the VenafiConnection, not the one from the config. + setVenafiCloudAssert(func(t testing.TB, r *http.Request) { + assert.Equal(t, srv.URL, "https://"+r.Host) + }) + + cfg, err := ParseConfig([]byte(testutil.Undent(` + server: "http://should-be-ignored" + period: 1h + `)), true) + assert.NoError(t, err) + + _, cl, err := getConfiguration(discardLogs(t), + cfg, + withCmdLineFlags("--venafi-connection", "venafi-components", "--install-namespace", "venafi"), + ) + assert.NoError(t, err) + + // `Start(ctx)` needs to be stopped before the apiserver is stopped. + // https://github.com/jetstack/venafi-connection-lib/pull/158#issuecomment-1949002322 + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + go func() { + require.NoError(t, cl.(*client.VenConnClient).Start(ctx)) + }() + certPool := x509.NewCertPool() + certPool.AddCert(fakeCrt) + tr := http.DefaultTransport.(*http.Transport).Clone() + tr.TLSClientConfig.RootCAs = certPool + cl.(*client.VenConnClient).Client.Transport = tr + + err = cl.PostDataReadingsWithOptions(nil, client.Options{ClusterName: "test cluster name"}) + assert.NoError(t, err) + }) +} + // Fills in the `server` and `period` as they appear in each and every test // case. func fillRequired(c Config) Config { @@ -457,15 +558,42 @@ func withFile(t testing.TB, content string) string { return f.Name() } -func withLogs(t testing.TB) (*log.Logger, *bytes.Buffer) { +func withLogs(_ testing.TB) (*log.Logger, *bytes.Buffer) { b := bytes.Buffer{} return log.New(&b, "", 0), &b } -func discardLogs(t testing.TB) *log.Logger { +func discardLogs(_ testing.TB) *log.Logger { return log.New(io.Discard, "", 0) } +// Shortcut for ParseConfig. +func withConfig(s string) Config { + var cfg Config + + err := yaml.Unmarshal([]byte(s), &cfg) + if err != nil { + panic(err) + } + return cfg +} + +func withCmdLineFlags(flags ...string) AgentCmdFlags { + parsed := withoutCmdLineFlags() + agentCmd := &cobra.Command{} + InitAgentCmdFlags(agentCmd, &parsed) + err := agentCmd.ParseFlags(flags) + if err != nil { + panic(err) + } + + return parsed +} + +func withoutCmdLineFlags() AgentCmdFlags { + return AgentCmdFlags{} +} + const fakeKubeconfig = ` apiVersion: v1 clusters: diff --git a/pkg/client/client_venconn.go b/pkg/client/client_venconn.go index eb403cd7..df92ef99 100644 --- a/pkg/client/client_venconn.go +++ b/pkg/client/client_venconn.go @@ -29,10 +29,15 @@ import ( type VenConnClient struct { agentMetadata *api.AgentMetadata connHandler venafi_client.ConnectionHandler - installNS string // Namespace in which the agent is running in. - venConnName string // Name of the VenafiConnection resource to use. - venConnNS string // Namespace of the VenafiConnection resource to use. - client *http.Client // Used to make HTTP requests to Venafi Cloud. + installNS string // Namespace in which the agent is running in. + venConnName string // Name of the VenafiConnection resource to use. + venConnNS string // Namespace of the VenafiConnection resource to use. + + // Used to make HTTP requests to Venafi Cloud. This field is public for + // testing purposes so that we can configure trusted CAs; there should be a + // way to do that without messing with the client directly (e.g., a flag to + // pass a custom CA?), but it's not there yet. + Client *http.Client } // NewVenConnClient lets you make requests to the Venafi Cloud backend using the @@ -111,7 +116,7 @@ func NewVenConnClient(restcfg *rest.Config, agentMetadata *api.AgentMetadata, in installNS: installNS, venConnName: venConnName, venConnNS: venConnNS, - client: vcpClient, + Client: vcpClient, }, nil } @@ -180,7 +185,7 @@ func (c *VenConnClient) PostDataReadingsWithOptions(readings []*api.DataReading, } req.URL.RawQuery = q.Encode() - res, err := c.client.Do(req) + res, err := c.Client.Do(req) if err != nil { return err } diff --git a/pkg/testutil/testutil.go b/pkg/testutil/testutil.go index cd27ca72..444141b4 100644 --- a/pkg/testutil/testutil.go +++ b/pkg/testutil/testutil.go @@ -215,6 +215,7 @@ func FakeVenafiCloud(t *testing.T) (_ *httptest.Server, _ *x509.Certificate, set if r.URL.Path == "/v1/tlspk/upload/clusterdata/no" { if r.URL.Query().Get("name") != "test cluster name" { w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":"unexpected name query param in the test server: ` + r.URL.Query().Get("name") + `"}`)) return } _, _ = w.Write([]byte(`{"status":"ok","organization":"756db001-280e-11ee-84fb-991f3177e2d0"}`)) From d03c70587ab7ff585958b96a75eac3135bef968e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 30 Aug 2024 09:23:43 +0200 Subject: [PATCH 8/8] venconn: the server field in config can now be omitted --- pkg/agent/config.go | 23 ++++++++++++++++------- pkg/agent/config_test.go | 16 ++++++++++++++-- pkg/testutil/testutil.go | 2 +- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/pkg/agent/config.go b/pkg/agent/config.go index ac79c69e..05125cdc 100644 --- a/pkg/agent/config.go +++ b/pkg/agent/config.go @@ -267,13 +267,22 @@ func getConfiguration(log *log.Logger, cfg Config, flags AgentCmdFlags) (Config, flags.VenafiCloudMode = true } - baseURL := cfg.Server - if baseURL == "" { - log.Printf("Using deprecated Endpoint configuration. User Server instead.") - baseURL = fmt.Sprintf("%s://%s", cfg.Endpoint.Protocol, cfg.Endpoint.Host) - _, err := url.Parse(baseURL) - if err != nil { - return Config{}, nil, fmt.Errorf("failed to parse server URL: %w", err) + // In VenafiConnection mode, we don't need the server field. For the other + // modes, we do need to validate the server field. + var baseURL string + if flags.VenConnName != "" { + if cfg.Server != "" { + log.Printf("ignoring the server field specified in the config file. In Venafi Connection mode, this field is not needed. Use the VenafiConnection's spec.vcp.url field instead.") + } + } else { + baseURL = cfg.Server + if baseURL == "" { + log.Printf("Using deprecated Endpoint configuration. User Server instead.") + baseURL = fmt.Sprintf("%s://%s", cfg.Endpoint.Protocol, cfg.Endpoint.Host) + _, err := url.Parse(baseURL) + if err != nil { + return Config{}, nil, fmt.Errorf("failed to parse server URL: %w", err) + } } } diff --git a/pkg/agent/config_test.go b/pkg/agent/config_test.go index 99f5d01b..f274f6a4 100644 --- a/pkg/agent/config_test.go +++ b/pkg/agent/config_test.go @@ -178,7 +178,7 @@ func TestGetConfiguration(t *testing.T) { assert.Equal(t, Config{}, got) }) - t.Run("warning about venafi-cloud.uploader_id and venafi-cloud.upload_path being skipped", func(t *testing.T) { + t.Run("warning about server, venafi-cloud.uploader_id, and venafi-cloud.upload_path being skipped", func(t *testing.T) { log, out := withLogs(t) cfg := fillRequired(Config{VenafiCloud: &VenafiCloudConfig{ UploaderID: "test-agent", @@ -191,6 +191,17 @@ func TestGetConfiguration(t *testing.T) { assert.Equal(t, cfg, got) assert.Contains(t, out.String(), "ignoring venafi-cloud.uploader_id") assert.Contains(t, out.String(), "ignoring venafi-cloud.upload_path") + assert.Contains(t, out.String(), "ignoring the server field") + }) + + t.Run("server field can be left empty in venconn mode", func(t *testing.T) { + _, _, err := getConfiguration(discardLogs(t), + withConfig(testutil.Undent(` + server: "" + period: 1h`, + )), + withCmdLineFlags("--venafi-connection", "venafi-components", "--install-namespace", "venafi")) + assert.NoError(t, err) }) }) } @@ -567,7 +578,8 @@ func discardLogs(_ testing.TB) *log.Logger { return log.New(io.Discard, "", 0) } -// Shortcut for ParseConfig. +// ParseConfig does some validation but we don't want this extra validation, so +// this is just using yaml.Unmarshal. func withConfig(s string) Config { var cfg Config diff --git a/pkg/testutil/testutil.go b/pkg/testutil/testutil.go index 444141b4..4f99417a 100644 --- a/pkg/testutil/testutil.go +++ b/pkg/testutil/testutil.go @@ -191,7 +191,7 @@ type AssertRequest func(t testing.TB, r *http.Request) func FakeVenafiCloud(t *testing.T) (_ *httptest.Server, _ *x509.Certificate, setAssert func(AssertRequest)) { t.Helper() - var assertFn AssertRequest + assertFn := func(_ testing.TB, _ *http.Request) {} assertFnMu := sync.Mutex{} setAssert = func(setAssert AssertRequest) { assertFnMu.Lock()