From 4189b0a32bfa5d182079115150b8c330ea6cd849 Mon Sep 17 00:00:00 2001 From: Steven Borrelli Date: Wed, 5 Aug 2026 12:07:54 +0100 Subject: [PATCH 1/5] fix reported issues Signed-off-by: Steven Borrelli --- .github/workflows/ci.yml | 4 +-- .golangci.yml | 1 - env.go | 2 +- example/datadog-dashboard-ids/functions.yaml | 2 +- fn.go | 6 +++- fn_test.go | 36 +++++++++++++++++-- input/v1alpha1/parameters.go | 4 ++- .../template.fn.crossplane.io_parameters.yaml | 6 ++++ 8 files changed, 51 insertions(+), 10 deletions(-) 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/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..4e13b25 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 diff --git a/fn_test.go b/fn_test.go index 12c6e51..5922a58 100644 --- a/fn_test.go +++ b/fn_test.go @@ -333,7 +333,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 +377,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 +618,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 +722,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 From 58336676dd89b33c94e24579549b77cf8748f37b Mon Sep 17 00:00:00 2001 From: Steven Borrelli Date: Wed, 5 Aug 2026 12:29:14 +0100 Subject: [PATCH 2/5] docs: add security considerations for log sensitivity Document that shell commands and their output are logged, which could expose sensitive data. Provide recommendations for safer secret handling and log management practices. Signed-off-by: Steven Borrelli Co-Authored-By: Claude Opus 4.5 --- README.md | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) 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. From 64f0e43a22c577d9ef4509f588a24932d8a40002 Mon Sep 17 00:00:00 2001 From: Steven Borrelli Date: Wed, 5 Aug 2026 12:34:51 +0100 Subject: [PATCH 3/5] fix: log successful output at debug level, errors at info - Successful command output (no error, no stderr) logs at Debug level - Commands with errors or stderr output log at Info level for visibility - Reduces exposure of sensitive data in production logs while maintaining visibility for errors Signed-off-by: Steven Borrelli Co-Authored-By: Claude Opus 4.5 --- fn.go | 7 ++++++- fn_test.go | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/fn.go b/fn.go index 4e13b25..7365200 100644 --- a/fn.go +++ b/fn.go @@ -154,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 { diff --git a/fn_test.go b/fn_test.go index 5922a58..e63890b 100644 --- a/fn_test.go +++ b/fn_test.go @@ -93,7 +93,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}, @@ -109,7 +109,7 @@ func TestRunFunctionLogs(t *testing.T) { logs: []wantLog{ {level: "info", msg: "Running function", kvs: map[string]any{"tag": 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}}, }, }, }, From c4eb3422e0c439c9a72cc70eae2f1561af81dc9b Mon Sep 17 00:00:00 2001 From: Steven Borrelli Date: Wed, 5 Aug 2026 12:48:42 +0100 Subject: [PATCH 4/5] fix: add completion log for successful function runs Log "Function completed successfully" at Info level when the shell command completes without error. This provides visibility into successful runs without exposing sensitive output data. Signed-off-by: Steven Borrelli Co-Authored-By: Claude Opus 4.5 --- fn.go | 2 ++ fn_test.go | 1 + 2 files changed, 3 insertions(+) diff --git a/fn.go b/fn.go index 7365200..68ac3d9 100644 --- a/fn.go +++ b/fn.go @@ -181,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 e63890b..ba3349d 100644 --- a/fn_test.go +++ b/fn_test.go @@ -110,6 +110,7 @@ func TestRunFunctionLogs(t *testing.T) { {level: "info", msg: "Running function", kvs: map[string]any{"tag": testTagFoo}}, {level: "debug", msg: "Executing shell command", kvs: map[string]any{"cmd": "echo hello"}}, {level: "debug", msg: "Function output", kvs: map[string]any{"stdout": "hello", "stderr": "", "tag": testTagFoo}}, + {level: "info", msg: "Function completed successfully", kvs: map[string]any{"tag": testTagFoo}}, }, }, }, From 922a869d62e984c1614c31db0d7ab8014699e8a0 Mon Sep 17 00:00:00 2001 From: Steven Borrelli Date: Wed, 5 Aug 2026 12:54:03 +0100 Subject: [PATCH 5/5] fix: add testKeyTag constant to fix goconst lint error Signed-off-by: Steven Borrelli Co-Authored-By: Claude Opus 4.5 --- fn_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/fn_test.go b/fn_test.go index ba3349d..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 { @@ -107,10 +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: "debug", msg: "Function output", kvs: map[string]any{"stdout": "hello", "stderr": "", "tag": testTagFoo}}, - {level: "info", msg: "Function completed successfully", kvs: map[string]any{"tag": testTagFoo}}, + {level: "info", msg: "Function completed successfully", kvs: map[string]any{testKeyTag: testTagFoo}}, }, }, }, @@ -130,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}}, },