From 1025878c765ac6302ea0152877006a9ec07eb441 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 19 Oct 2023 14:21:28 +0200 Subject: [PATCH] Let the user pass a secret for on git API resolver We were previously only allowing using a secret from the global configmap, which mean every users on the cluster would have to use the same git value (which could be a security issue). We now let the user use their own token on their own namespace with the optional "token" and (even more optional) "tokenKey" params. The fallback will still be the global configmap. Added a bit logic to make sure we are caching secrets only when user use the token from global configmap. Since we can't ensure for local secrets on namespace may be rotated often. Signed-off-by: Chmouel Boudjnah --- docs/git-resolver.md | 10 +-- pkg/resolution/resolver/git/params.go | 6 ++ pkg/resolution/resolver/git/resolver.go | 74 ++++++++++++++------ pkg/resolution/resolver/git/resolver_test.go | 51 ++++++++++++-- 4 files changed, 107 insertions(+), 34 deletions(-) diff --git a/docs/git-resolver.md b/docs/git-resolver.md index 6d0a165cbec..b81f362009d 100644 --- a/docs/git-resolver.md +++ b/docs/git-resolver.md @@ -18,8 +18,10 @@ This Resolver responds to type `git`. | `url` | URL of the repo to fetch and clone anonymously. Either `url`, or `repo` (with `org`) must be specified, but not both. | `https://github.com/tektoncd/catalog.git` | | `repo` | The repository to find the resource in. Either `url`, or `repo` (with `org`) must be specified, but not both. | `pipeline`, `test-infra` | | `org` | The organization to find the repository in. Default can be set in [configuration](#configuration). | `tektoncd`, `kubernetes` | +| `token` | An optional secret name in the `PipelineRun` namespace to fetch the token from. Defaults to empty, meaning it will try to use the configuration from the global configmap. | `secret-name`, (empty) | +| `tokenKey` | An optional key in the token secret name in the `PipelineRun` namespace to fetch the token from. Defaults to `token`. | `token` | | `revision` | Git revision to checkout a file from. This can be commit SHA, branch or tag. | `aeb957601cf41c012be462827053a21a420befca` `main` `v0.38.2` | -| `pathInRepo` | Where to find the file in the repo. | `task/golang-build/0.3/golang-build.yaml` | +| `pathInRepo` | Where to find the file in the repo. | `task/golang-build/0.3/golang-build.yaml` | ## Requirements @@ -158,8 +160,8 @@ spec: ## `ResolutionRequest` Status `ResolutionRequest.Status.RefSource` field captures the source where the remote resource came from. It includes the 3 subfields: `url`, `digest` and `entrypoint`. - `url` - - If users choose to use anonymous cloning, the url is just user-provided value for the `url` param in the [SPDX download format](https://spdx.github.io/spdx-spec/package-information/#77-package-download-location-field). - - If scm api is used, it would be the clone URL of the repo fetched from scm repository service in the [SPDX download format](https://spdx.github.io/spdx-spec/package-information/#77-package-download-location-field). + - If users choose to use anonymous cloning, the url is just user-provided value for the `url` param in the [SPDX download format](https://spdx.github.io/spdx-spec/package-information/#77-package-download-location-field). + - If scm api is used, it would be the clone URL of the repo fetched from scm repository service in the [SPDX download format](https://spdx.github.io/spdx-spec/package-information/#77-package-download-location-field). - `digest` - The algorithm name is fixed "sha1", but subject to be changed to "sha256" once Git eventually uses SHA256 at some point later. See https://git-scm.com/docs/hash-function-transition for more details. - The value is the actual commit sha at the moment of resolving the resource even if a user provides a tag/branch name for the param `revision`. @@ -189,7 +191,7 @@ spec: apiVersion: resolution.tekton.dev/v1alpha1 kind: ResolutionRequest metadata: - ... + ... spec: params: pathInRepo: pipeline.yaml diff --git a/pkg/resolution/resolver/git/params.go b/pkg/resolution/resolver/git/params.go index e167a00b63f..252ee81875b 100644 --- a/pkg/resolution/resolver/git/params.go +++ b/pkg/resolution/resolver/git/params.go @@ -27,4 +27,10 @@ const ( pathParam string = "pathInRepo" // revisionParam is the git revision that a file should be fetched from. This is used with both approaches. revisionParam string = "revision" + // tokenParam is an optional reference to a secret name for SCM API authentication + tokenParam string = "token" + // tokenKeyParam is an optional reference to a key in the tokenParam secret for SCM API authentication + tokenKeyParam string = "tokenKey" + // defaultTokenKeyParam is the default key in the tokenParam secret for SCM API authentication + defaultTokenKeyParam string = "token" ) diff --git a/pkg/resolution/resolver/git/resolver.go b/pkg/resolution/resolver/git/resolver.go index d32acf76cc5..5c4436d9e69 100644 --- a/pkg/resolution/resolver/git/resolver.go +++ b/pkg/resolution/resolver/git/resolver.go @@ -35,6 +35,7 @@ import ( "github.com/jenkins-x/go-scm/scm/factory" resolverconfig "github.com/tektoncd/pipeline/pkg/apis/config/resolver" pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" + "github.com/tektoncd/pipeline/pkg/resolution/common" resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common" "github.com/tektoncd/pipeline/pkg/resolution/resolver/framework" "go.uber.org/zap" @@ -147,7 +148,19 @@ func (r *Resolver) resolveAPIGit(ctx context.Context, params map[string]string) if err != nil { return nil, err } - apiToken, err := r.getAPIToken(ctx) + secretRef := &secretCacheKey{ + name: params[tokenParam], + key: params[tokenKeyParam], + } + if secretRef.name != "" { + if secretRef.key == "" { + secretRef.key = defaultTokenKeyParam + } + secretRef.ns = common.RequestNamespace(ctx) + } else { + secretRef = nil + } + apiToken, err := r.getAPIToken(ctx, secretRef) if err != nil { return nil, err } @@ -366,51 +379,66 @@ func (r *Resolver) getSCMTypeAndServerURL(ctx context.Context) (string, string, return scmType, serverURL, nil } -func (r *Resolver) getAPIToken(ctx context.Context) ([]byte, error) { +func (r *Resolver) getAPIToken(ctx context.Context, apiSecret *secretCacheKey) ([]byte, error) { conf := framework.GetResolverConfigFromContext(ctx) - cacheKey := secretCacheKey{} - ok := false - if cacheKey.name, ok = conf[APISecretNameKey]; !ok || cacheKey.name == "" { - err := fmt.Errorf("cannot get API token, required when specifying '%s' param, '%s' not specified in config", repoParam, APISecretNameKey) - r.logger.Info(err) - return nil, err + // NOTE(chmouel): only cache secrets when user hasn't passed params in their resolver configuration + cacheSecret := false + if apiSecret == nil { + cacheSecret = true + apiSecret = &secretCacheKey{} } - if cacheKey.key, ok = conf[APISecretKeyKey]; !ok || cacheKey.key == "" { - err := fmt.Errorf("cannot get API token, required when specifying '%s' param, '%s' not specified in config", repoParam, APISecretKeyKey) - r.logger.Info(err) - return nil, err + + if apiSecret.name == "" { + if apiSecret.name, ok = conf[APISecretNameKey]; !ok || apiSecret.name == "" { + err := fmt.Errorf("cannot get API token, required when specifying '%s' param, '%s' not specified in config", repoParam, APISecretNameKey) + r.logger.Info(err) + return nil, err + } } - if cacheKey.ns, ok = conf[APISecretNamespaceKey]; !ok { - cacheKey.ns = os.Getenv("SYSTEM_NAMESPACE") + if apiSecret.key == "" { + if apiSecret.key, ok = conf[APISecretKeyKey]; !ok || apiSecret.key == "" { + err := fmt.Errorf("cannot get API token, required when specifying '%s' param, '%s' not specified in config", repoParam, APISecretKeyKey) + r.logger.Info(err) + return nil, err + } + } + if apiSecret.ns == "" { + if apiSecret.ns, ok = conf[APISecretNamespaceKey]; !ok { + apiSecret.ns = os.Getenv("SYSTEM_NAMESPACE") + } } - val, ok := r.cache.Get(cacheKey) - if ok { - return val.([]byte), nil + if cacheSecret { + val, ok := r.cache.Get(apiSecret) + if ok { + return val.([]byte), nil + } } - secret, err := r.kubeClient.CoreV1().Secrets(cacheKey.ns).Get(ctx, cacheKey.name, metav1.GetOptions{}) + secret, err := r.kubeClient.CoreV1().Secrets(apiSecret.ns).Get(ctx, apiSecret.name, metav1.GetOptions{}) if err != nil { if apierrors.IsNotFound(err) { - notFoundErr := fmt.Errorf("cannot get API token, secret %s not found in namespace %s", cacheKey.name, cacheKey.ns) + notFoundErr := fmt.Errorf("cannot get API token, secret %s not found in namespace %s", apiSecret.name, apiSecret.ns) r.logger.Info(notFoundErr) return nil, notFoundErr } - wrappedErr := fmt.Errorf("error reading API token from secret %s in namespace %s: %w", cacheKey.name, cacheKey.ns, err) + wrappedErr := fmt.Errorf("error reading API token from secret %s in namespace %s: %w", apiSecret.name, apiSecret.ns, err) r.logger.Info(wrappedErr) return nil, wrappedErr } - secretVal, ok := secret.Data[cacheKey.key] + secretVal, ok := secret.Data[apiSecret.key] if !ok { - err := fmt.Errorf("cannot get API token, key %s not found in secret %s in namespace %s", cacheKey.key, cacheKey.name, cacheKey.ns) + err := fmt.Errorf("cannot get API token, key %s not found in secret %s in namespace %s", apiSecret.key, apiSecret.name, apiSecret.ns) r.logger.Info(err) return nil, err } - r.cache.Add(cacheKey, secretVal, r.ttl) + if cacheSecret { + r.cache.Add(apiSecret, secretVal, r.ttl) + } return secretVal, nil } diff --git a/pkg/resolution/resolver/git/resolver_test.go b/pkg/resolution/resolver/git/resolver_test.go index 6d24c697e03..9d4384326b5 100644 --- a/pkg/resolution/resolver/git/resolver_test.go +++ b/pkg/resolution/resolver/git/resolver_test.go @@ -192,10 +192,13 @@ type params struct { pathInRepo string org string repo string + token string + tokenKey string + namespace string } func TestResolve(t *testing.T) { - // lcoal repo set up for anonymous cloning + // local repo set up for anonymous cloning // ---- commits := []commitForRepo{{ Dir: "foo/", @@ -327,6 +330,24 @@ func TestResolve(t *testing.T) { url: anonFakeRepoURL, }, expectedErr: createError("revision error: reference not found"), + }, { + name: "api: successful task from params api information", + args: ¶ms{ + revision: "main", + pathInRepo: "tasks/example-task.yaml", + org: testOrg, + repo: testRepo, + token: "token-secret", + tokenKey: "token", + namespace: "foo", + }, + config: map[string]string{ + ServerURLKey: "fake", + SCMTypeKey: "fake", + }, + apiToken: "some-token", + expectedCommitSHA: commitSHAsInSCMRepo[0], + expectedStatus: internal.CreateResolutionRequestStatusWithData(mainTaskYAML), }, { name: "api: successful task", args: ¶ms{ @@ -539,21 +560,27 @@ func TestResolve(t *testing.T) { } frtesting.RunResolverReconcileTest(ctx, t, d, resolver, request, expectedStatus, tc.expectedErr, func(resolver framework.Resolver, testAssets test.Assets) { - if tc.config[APISecretNameKey] == "" || tc.config[APISecretNamespaceKey] == "" || tc.config[APISecretKeyKey] == "" || tc.apiToken == "" { + var secretName, secretNameKey, secretNamespace string + if tc.config[APISecretNameKey] != "" && tc.config[APISecretNamespaceKey] != "" && tc.config[APISecretKeyKey] != "" && tc.apiToken != "" { + secretName, secretNameKey, secretNamespace = tc.config[APISecretNameKey], tc.config[APISecretKeyKey], tc.config[APISecretNamespaceKey] + } + if tc.args.token != "" && tc.args.namespace != "" && tc.args.tokenKey != "" { + secretName, secretNameKey, secretNamespace = tc.args.token, tc.args.tokenKey, tc.args.namespace + } + if secretName == "" || secretNameKey == "" || secretNamespace == "" { return } tokenSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: tc.config[APISecretNameKey], - Namespace: tc.config[APISecretNamespaceKey], + Name: secretName, + Namespace: secretNamespace, }, Data: map[string][]byte{ - tc.config[APISecretKeyKey]: []byte(base64.StdEncoding.Strict().EncodeToString([]byte(tc.apiToken))), + secretNameKey: []byte(base64.StdEncoding.Strict().EncodeToString([]byte(tc.apiToken))), }, Type: corev1.SecretTypeOpaque, } - - if _, err := testAssets.Clients.Kube.CoreV1().Secrets(tc.config[APISecretNamespaceKey]).Create(ctx, tokenSecret, metav1.CreateOptions{}); err != nil { + if _, err := testAssets.Clients.Kube.CoreV1().Secrets(secretNamespace).Create(ctx, tokenSecret, metav1.CreateOptions{}); err != nil { t.Fatalf("failed to create test token secret: %v", err) } }) @@ -734,6 +761,16 @@ func createRequest(args *params) *v1beta1.ResolutionRequest { Name: orgParam, Value: *pipelinev1.NewStructuredValues(args.org), }) + if args.token != "" { + rr.Spec.Params = append(rr.Spec.Params, pipelinev1.Param{ + Name: tokenParam, + Value: *pipelinev1.NewStructuredValues(args.token), + }) + rr.Spec.Params = append(rr.Spec.Params, pipelinev1.Param{ + Name: tokenKeyParam, + Value: *pipelinev1.NewStructuredValues(args.tokenKey), + }) + } } return rr