diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 55e21f0..3ed0a85 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -68,7 +68,8 @@ jobs: - name: Check Generated Code run: | go generate ./... - git diff --exit-code || (echo "Generated code is out of date. Run 'go generate ./...' and commit the changes." && exit 1) + test -z "$(git status --porcelain --untracked-files=all)" || \ + (git status --short && echo "Generated code is out of date. Run 'go generate ./...' and commit the changes." && exit 1) - name: Run Unit Tests run: go test -v -cover ./... @@ -96,7 +97,6 @@ jobs: with: buildkitd-flags: --debug version: ${{ env.DOCKER_BUILDX_VERSION }} - install: true - name: Checkout uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 diff --git a/.golangci.yml b/.golangci.yml index 9efda5b..d1921b8 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -165,7 +165,6 @@ linters: - gochecknoinits - gocognit - gosec - - scopelint - unparam - embeddedstructfieldcheck path: _test(ing)?\.go diff --git a/README.md b/README.md index 34cee42..75dc3fb 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,7 @@ returns the output to specified fields. - [Parameters](#parameters) - [Error Handling and Output Capture](#error-handling-and-output-capture) - [Caching Function Outputs](#caching-function-outputs) +- [Security Considerations](#security-considerations) - [Running as Arbitrary User IDs](#running-as-arbitrary-user-ids) - [Examples](#examples) - [Development and Test](#development-and-test) @@ -154,6 +155,38 @@ set a new cache duration for the output. See the echo [composition.yaml](example/echo/composition.yaml) for an example. +## Security Considerations + +### Log Sensitivity + +When running with `--debug` enabled, function-shell logs shell commands and their output. This can expose sensitive data in logs if: + +- **Shell commands contain secrets**: Environment variables with sensitive values that are interpolated into commands will appear in debug logs +- **Command output contains secrets**: stdout/stderr from commands may contain API responses, credentials, or other sensitive data + +**Recommendations:** + +1. **Avoid interpolating secrets directly in commands** - Use environment variables instead of embedding secrets in `shellCommand` +2. **Review log retention policies** - Ensure Kubernetes log retention and access controls are appropriate for your security requirements +3. **Use debug mode sparingly in production** - Only enable `--debug` when actively troubleshooting +4. **Consider log aggregation security** - If forwarding logs to external systems, ensure they have appropriate access controls + +Example of safer secret handling: + +```yaml +# Less safe: secret visible in debug logs +shellCommand: 'curl -H "Authorization: Bearer my-secret-token" https://api.example.com' + +# Safer: secret passed via environment variable +shellEnvVars: + - key: API_TOKEN + fieldRef: + path: spec.credentials.token +shellCommand: 'curl -H "Authorization: Bearer ${API_TOKEN}" https://api.example.com' +``` + +Note: Even with environment variables, the command output (stdout/stderr) is logged at Info level for observability. Ensure your commands don't echo sensitive data unnecessarily. + ## Running as Arbitrary User IDs Some Kubernetes environments (OpenShift, restricted Pod Security Standards) force containers to run as arbitrary user IDs that don't exist in `/etc/passwd`. This can cause tools like AWS CLI, git, and ssh to fail because they can't look up the current user. diff --git a/env.go b/env.go index 36f22cc..c78d530 100644 --- a/env.go +++ b/env.go @@ -34,7 +34,7 @@ func fromFieldRef(req *fnv1.RunFunctionRequest, fieldRef v1alpha1.FieldRef) (str return "", errors.New("path must be set") } // Check for context key presence and capture context key and path - contextRegex := regexp.MustCompile(`^context\[(.+?)].(.+)$`) + contextRegex := regexp.MustCompile(`^context\[(.+?)]\.(.+)$`) if match := contextRegex.FindStringSubmatch(fieldRef.Path); match != nil { if v, ok := request.GetContextKey(req, match[1]); ok { context := &unstructured.Unstructured{} diff --git a/example/datadog-dashboard-ids/functions.yaml b/example/datadog-dashboard-ids/functions.yaml index f728ed9..86d438e 100644 --- a/example/datadog-dashboard-ids/functions.yaml +++ b/example/datadog-dashboard-ids/functions.yaml @@ -4,7 +4,7 @@ kind: Function metadata: name: function-shell annotations: - # This tells crossplane beta render to connect to the functi on locally. + # This tells crossplane beta render to connect to the function locally. render.crossplane.io/runtime: Development spec: # This is ignored when using the Development runtime. diff --git a/fn.go b/fn.go index f87e2ed..68ac3d9 100644 --- a/fn.go +++ b/fn.go @@ -113,9 +113,13 @@ func (f *Function) RunFunction(_ context.Context, req *fnv1.RunFunctionRequest) } shellEnvVars[envVar.Key] = envValue case v1alpha1.ShellEnvVarTypeFieldRef: + if envVar.FieldRef == nil { + response.Fatal(rsp, errors.Errorf("shellEnvVars: fieldRef must be set for key %s", envVar.Key)) + return rsp, nil + } envValue, err := fromFieldRef(req, *envVar.FieldRef) if err != nil { - response.Fatal(rsp, errors.Wrapf(err, "cannot process contents of fieldRef %s", envVar.ValueRef)) + response.Fatal(rsp, errors.Wrapf(err, "cannot process contents of fieldRef for key %s", envVar.Key)) return rsp, nil } shellEnvVars[envVar.Key] = envValue @@ -150,7 +154,12 @@ func (f *Function) RunFunction(_ context.Context, req *fnv1.RunFunctionRequest) sout := strings.TrimSpace(stdout.String()) serr := strings.TrimSpace(stderr.String()) - log.Info("Function output", "tag", req.GetMeta().GetTag(), "stdout", sout, "stderr", serr) + // Log output at appropriate level: errors at Info for visibility, success at Debug + if cmderr != nil || serr != "" { + log.Info("Function output", "tag", req.GetMeta().GetTag(), "stdout", sout, "stderr", serr) + } else { + log.Debug("Function output", "tag", req.GetMeta().GetTag(), "stdout", sout, "stderr", serr) + } err = dxr.Resource.SetValue(stdoutField, sout) if err != nil { @@ -172,7 +181,9 @@ func (f *Function) RunFunction(_ context.Context, req *fnv1.RunFunctionRequest) msg := fmt.Sprintf("shellCmd %q for %q failed with %s", shellCmd, oxr.Resource.GetKind(), exiterr.Stderr) response.Fatal(rsp, errors.Wrap(cmderr, msg)) } + return rsp, nil } + log.Info("Function completed successfully", "tag", req.GetMeta().GetTag()) return rsp, nil } diff --git a/fn_test.go b/fn_test.go index 12c6e51..4e74f72 100644 --- a/fn_test.go +++ b/fn_test.go @@ -20,6 +20,7 @@ import ( const ( testTagHello = "hello" testTagFoo = "foo" + testKeyTag = "tag" ) type logEntry struct { @@ -93,7 +94,7 @@ func TestRunFunctionLogs(t *testing.T) { want want }{ "LogsStdoutOnSuccess": { - reason: "Function output log should contain stdout key and empty stderr", + reason: "Function output log should be at debug level on success with no stderr", args: args{ req: &fnv1.RunFunctionRequest{ Meta: &fnv1.RequestMeta{Tag: testTagFoo}, @@ -107,9 +108,10 @@ func TestRunFunctionLogs(t *testing.T) { }, want: want{ logs: []wantLog{ - {level: "info", msg: "Running function", kvs: map[string]any{"tag": testTagFoo}}, + {level: "info", msg: "Running function", kvs: map[string]any{testKeyTag: testTagFoo}}, {level: "debug", msg: "Executing shell command", kvs: map[string]any{"cmd": "echo hello"}}, - {level: "info", msg: "Function output", kvs: map[string]any{"stdout": "hello", "stderr": "", "tag": testTagFoo}}, + {level: "debug", msg: "Function output", kvs: map[string]any{"stdout": "hello", "stderr": "", "tag": testTagFoo}}, + {level: "info", msg: "Function completed successfully", kvs: map[string]any{testKeyTag: testTagFoo}}, }, }, }, @@ -129,7 +131,7 @@ func TestRunFunctionLogs(t *testing.T) { }, want: want{ logs: []wantLog{ - {level: "info", msg: "Running function", kvs: map[string]any{"tag": testTagFoo}}, + {level: "info", msg: "Running function", kvs: map[string]any{testKeyTag: testTagFoo}}, {level: "debug", msg: "Executing shell command", kvs: map[string]any{"cmd": "echo hello> /dev/stderr; exit 1"}}, {level: "info", msg: "Function output", kvs: map[string]any{"stdout": "", "stderr": "hello", "tag": testTagFoo}}, }, @@ -333,7 +335,7 @@ func TestRunFunction(t *testing.T) { Results: []*fnv1.Result{ { Severity: fnv1.Severity_SEVERITY_FATAL, - Message: "shellCmd \"set -euo pìpefail\" for \"\" failed with : exit status 2", + Message: "shellCmd.*pìpefail.*failed.*exit status.*", Target: fnv1.Target_TARGET_COMPOSITE.Enum(), }, }, @@ -377,7 +379,7 @@ func TestRunFunction(t *testing.T) { Results: []*fnv1.Result{ { Severity: fnv1.Severity_SEVERITY_FATAL, - Message: "shellCmd unknown-shell-command for failed: exit status 127", + Message: "shellCmd.*unknown-shell-command.*failed.*exit status 127", Target: fnv1.Target_TARGET_COMPOSITE.Enum(), }, }, @@ -618,6 +620,33 @@ func TestRunFunction(t *testing.T) { }, }, }, + "ResponseIsErrorWhenFieldRefTypeWithNilFieldRef": { + reason: "The Function should return an error when type is FieldRef but fieldRef is nil", + args: args{ + req: &fnv1.RunFunctionRequest{ + Meta: &fnv1.RequestMeta{Tag: testTagHello}, + Input: resource.MustStructJSON(`{ + "apiVersion": "template.fn.crossplane.io/v1alpha1", + "kind": "Parameters", + "shellEnvVars": [{"key": "TEST_ENV_VAR", "type": "FieldRef"}], + "shellCommand": "echo ${TEST_ENV_VAR}", + "stdoutField": "spec.atFunction.shell.stdout" + }`), + }, + }, + want: want{ + rsp: &fnv1.RunFunctionResponse{ + Meta: &fnv1.ResponseMeta{Tag: testTagHello, Ttl: durationpb.New(response.DefaultTTL)}, + Results: []*fnv1.Result{ + { + Severity: fnv1.Severity_SEVERITY_FATAL, + Message: "shellEnvVars: fieldRef must be set for key TEST_ENV_VAR", + Target: fnv1.Target_TARGET_COMPOSITE.Enum(), + }, + }, + }, + }, + }, "ResponseWithCustomCacheTTL": { reason: "The Function should set custom TTL when cacheTTL is specified", args: args{ @@ -695,7 +724,10 @@ func TestRunFunction(t *testing.T) { rsp, err := f.RunFunction(tc.args.ctx, tc.args.req) var cmpOpts []cmp.Option - cmpOpts = append(cmpOpts, protocmp.Transform(), protocmp.IgnoreFields(&fnv1.Result{}, "message")) + cmpOpts = append(cmpOpts, protocmp.Transform()) + if !tc.args.useRegex { + cmpOpts = append(cmpOpts, protocmp.IgnoreFields(&fnv1.Result{}, "message")) + } if tc.args.useRegex { cmpOpts = append(cmpOpts, cmp.Comparer(func(expected, actual string) bool { diff --git a/input/v1alpha1/parameters.go b/input/v1alpha1/parameters.go index 5a06bf4..d7f5265 100644 --- a/input/v1alpha1/parameters.go +++ b/input/v1alpha1/parameters.go @@ -76,6 +76,7 @@ type ShellEnvVar struct { // FieldRef is a reference to a field in the Composition. FieldRef *FieldRef `json:"fieldRef,omitempty"` // Type is the type of ShellEnVar: Value, ValueRef, FieldRef. + // +kubebuilder:validation:Enum=Value;ValueRef;FieldRef Type ShellEnvVarType `json:"type,omitempty"` } @@ -119,6 +120,7 @@ const FieldRefDefault = "" // FieldRef refers to a composite field like spec.region. type FieldRef struct { // Path is the field path of the field being referenced, i.e. spec.myfield, status.output + // +kubebuilder:validation:MinLength=1 Path string `json:"path"` // Policy when the field is not available. If set to "Required" will return // an error if a field is missing. If set to "Optional" will return DefaultValue. @@ -128,6 +130,6 @@ type FieldRef struct { Policy FieldRefPolicy `json:"policy,omitempty"` // DefaultValue when Policy is Optional and field is not available defaults to "" // +optional - // +kbuebuilder:default:="" + // +kubebuilder:default:="" DefaultValue string `json:"defaultValue,omitempty"` } diff --git a/package/input/template.fn.crossplane.io_parameters.yaml b/package/input/template.fn.crossplane.io_parameters.yaml index cc5eba5..2b5e923 100644 --- a/package/input/template.fn.crossplane.io_parameters.yaml +++ b/package/input/template.fn.crossplane.io_parameters.yaml @@ -61,12 +61,14 @@ spec: description: FieldRef is a reference to a field in the Composition. properties: defaultValue: + default: "" description: DefaultValue when Policy is Optional and field is not available defaults to "" type: string path: description: Path is the field path of the field being referenced, i.e. spec.myfield, status.output + minLength: 1 type: string policy: default: Required @@ -85,6 +87,10 @@ spec: type: string type: description: 'Type is the type of ShellEnVar: Value, ValueRef, FieldRef.' + enum: + - Value + - ValueRef + - FieldRef type: string value: description: Value is a fixed value, like http://api.example.com