diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 656a7c6cc..0668c6f46 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -20,6 +20,7 @@ * [Code structure and practices](#code-structure-and-practices) * [What is the general code layout?](#what-is-the-general-code-layout) * [Logging](#logging) + * [Tracing sensitive data](#tracing-sensitive-data) * [Breaking Changes](#breaking-changes) * [Infrastructure](#infrastructure) * [CDN Setup](#cdn-setup) @@ -587,7 +588,7 @@ fmt.Fprintln(p.Err, "DEBUG: loading plans from r2d2...") ``` Most of the structs in Porter have an embedded -`get.porter.sh/porter/pkg/context.Context` struct. This has both `Out` and +`get.porter.sh/porter/pkg/portercontext.Context` struct. This has both `Out` and `Err` which represent stdout and stderr respectively. You should log to those instead of directly to stdout/stderr because that is how we capture output in our unit tests. That means use `fmt.Fprint*` instead of `fmt.Print*` so that you @@ -601,6 +602,32 @@ we send regular command output to `Out` and debug information to `Err`. It allows us to then run the command and see the debug output separately, like so `porter schema --debug 2> err.log`. +### Tracing Sensitive Data + +Sometimes when debugging your code you may need to print out variables that can contain sensitive data, for example printing out the resolved values for parameters and credentials. +In this case, do not use fmt.Println and instead use the open telemetry trace logger to include the data in attributes. + +```go +func myFunc(ctx context.Context) { + ctx, span := tracing.StartSpan(ctx) + defer span.EndSpan() + + // This contains resolved sensitive values, so only trace it in special dev builds (nothing is traced for release builds) + span.SetSensitiveAttributes(attribute.String("sensitive-stuff", mysensitiveVar)) +} +``` + +In normal builds of Porter, created with `go build`, `mage Build` or `mage XBuildAll`, no sensitive data is ever traced because `SetSensitiveAttributes` is compiled to do nothing by default. +Calls to `SetSensitiveAttributes` are only implemented when Porter is built with the `traceSensitiveAttributes` build tag. + +Each time you call `SetSensitiveAttributes` include in your comment why the data is sensitive, and that it is only traced in special dev builds since people may not think to read the function documentation. + +In order to debug your code and see the sensitive data, you need to: + +1. Compile a build of porter with sensitive tracing turned on, `go build -tags traceSensitiveAttributes -o bin/porter ./cmd/porter`. +2. [Enable tracing with Jaeger](#view-a-trace-of-a-porter-command) +3. Run a porter command and then view the sensitive attributes in the trace data sent to Jaeger. + ## Breaking Changes Some changes in Porter break our compatibility with previous versions of Porter. diff --git a/magefile.go b/magefile.go index 8761285e0..79968dbb2 100644 --- a/magefile.go +++ b/magefile.go @@ -6,6 +6,7 @@ package main import ( + "bytes" "fmt" "go/build" "log" @@ -229,6 +230,8 @@ func TestUnit() { // Verify integration tests compile since we don't run them automatically on pull requests must.Run("go", "test", "-run=non", "-tags=integration", "./...") + + TestInitWarnings() } // Run smoke tests to quickly check if Porter is broken @@ -467,6 +470,21 @@ func TestIntegration() { must.Command("go", "test", verbose, "-timeout=30m", run, "-tags=integration", "./...").CollapseArgs().RunV() } +func TestInitWarnings() { + // This is hard to test in a normal unit test because we need to build porter with custom build tags, + // so I'm testing it in the magefile directly in a way that doesn't leave around an unsafe Porter binary. + + // Verify that running Porter with traceSensitiveAttributes set that a warning is printed + fmt.Println("Validating traceSensitiveAttributes warning") + output := &bytes.Buffer{} + must.Command("go", "run", "-tags=traceSensitiveAttributes", "./cmd/porter", "schema"). + Stderr(output).Stdout(output).Exec() + if !strings.Contains(output.String(), "WARNING! This is a custom developer build of Porter with the traceSensitiveAttributes build flag set") { + fmt.Printf("Got output: %s\n", output.String()) + panic("Expected a build of Porter with traceSensitiveAttributes build tag set to warn at startup but it didn't") + } +} + // TryRegisterLocalHostAlias edits /etc/hosts to use porter-test-registry hostname alias // This is not safe to call more than once and is intended for use on the CI server only func TryRegisterLocalHostAlias() { diff --git a/pkg/cnab/provider/action.go b/pkg/cnab/provider/action.go index a7cf7c8f5..13cbe6ae7 100644 --- a/pkg/cnab/provider/action.go +++ b/pkg/cnab/provider/action.go @@ -179,7 +179,13 @@ func (r *Runtime) Execute(ctx context.Context, args ActionArguments) error { } } - opResult, result, err := a.Run(currentRun.ToCNAB(), creds.ToCNAB(), r.ApplyConfig(ctx, args)...) + cnabClaim := currentRun.ToCNAB() + cnabCreds := creds.ToCNAB() + // The claim and credentials contain sensitive values. Only trace it in special dev builds (nothing is traced for release builds) + log.SetSensitiveAttributes( + tracing.ObjectAttribute("cnab-claim", cnabClaim), + tracing.ObjectAttribute("cnab-credentials", cnabCreds)) + opResult, result, err := a.Run(cnabClaim, cnabCreds, r.ApplyConfig(ctx, args)...) if currentRun.ShouldRecord() { if err != nil { diff --git a/pkg/porter/porter.go b/pkg/porter/porter.go index 621479d99..93662877f 100644 --- a/pkg/porter/porter.go +++ b/pkg/porter/porter.go @@ -3,7 +3,9 @@ package porter import ( "context" "errors" + "fmt" "strings" + "sync" "get.porter.sh/porter/pkg/build" "get.porter.sh/porter/pkg/build/buildkit" @@ -81,9 +83,23 @@ func NewFor(c *config.Config, store storage.Store, secretStorage secrets.Store) } } +// Used to warn just a single time when Porter starts up. +// Connect is called more than once, and this helps us validate certain things, like build flags, a single time only. +var initWarnings sync.Once + // Connect initializes Porter for use and must be called before other Porter methods. // It is the responsibility of the caller to also call Close when done with Porter. func (p *Porter) Connect(ctx context.Context) error { + initWarnings.Do(func() { + // Check if this is a special dev build that will trace sensitive data and strongly warn people + if tracing.IsTraceSensitiveAttributesEnabled() { + fmt.Fprintln(p.Err, "🚨 WARNING! This is a custom developer build of Porter with the traceSensitiveAttributes build flag set. "+ + "Porter will include sensitive data, such as parameters and credentials, in the telemetry trace data. "+ + "This build flag should only be used for local development only. "+ + "If you didn't intend to use a custom build of Porter with this flag enabled, reinstall Porter using the official builds from https://getporter.org/install.") + } + }) + // Load the config file and replace any referenced secrets return p.Config.Load(ctx, func(innerCtx context.Context, secret string) (string, error) { value, err := p.Secrets.Resolve(innerCtx, "secret", secret) diff --git a/pkg/tracing/attributes.go b/pkg/tracing/attributes.go new file mode 100644 index 000000000..66910f9e2 --- /dev/null +++ b/pkg/tracing/attributes.go @@ -0,0 +1,17 @@ +package tracing + +import ( + "encoding/json" + + "go.opentelemetry.io/otel/attribute" +) + +// ObjectAttribute writes the specified value as a json encoded value. +func ObjectAttribute(key string, value interface{}) attribute.KeyValue { + data, err := json.Marshal(value) + if err != nil { + data = []byte("unable to marshal object to an otel attribute") + } + + return attribute.Key(key).String(string(data)) +} diff --git a/pkg/tracing/tag_trace_sensitive_attributes.go b/pkg/tracing/tag_trace_sensitive_attributes.go new file mode 100644 index 000000000..e72ce2b89 --- /dev/null +++ b/pkg/tracing/tag_trace_sensitive_attributes.go @@ -0,0 +1,9 @@ +//go:build traceSensitiveAttributes + +package tracing + +func init() { + // This file should only be included in the build when we run `go build -tags traceSensitiveAttributes` + // and it helps a dev create a local build of Porter that will included sensitive data in traced attributes + traceSensitiveAttributes = true +} diff --git a/pkg/tracing/traceLogger.go b/pkg/tracing/traceLogger.go index fdff2b8af..331adb9bc 100644 --- a/pkg/tracing/traceLogger.go +++ b/pkg/tracing/traceLogger.go @@ -15,6 +15,18 @@ import ( "go.uber.org/zap/zapcore" ) +// traceSensitiveAttributes is a build flag that is off for release builds of Porter +// A Porter developer may decide that they need sensitive data included in traces, and can create a custom build of Porter with this flag enabled. +// When it is off, calls to SetSensitiveAttributes does nothing. +// You can enable it with `go build +var traceSensitiveAttributes = false + +// IsTraceSensitiveAttributesEnabled returns if sensitive data traced with TraceLogger.SetSensitiveAttributes +// will trace the data. Defaults to false, which means that function is a noop and does nothing. +func IsTraceSensitiveAttributesEnabled() bool { + return traceSensitiveAttributes +} + // TraceLogger how porter emits traces and logs to any configured listeners. type TraceLogger interface { // StartSpan retrieves a logger from the current context and starts a new span @@ -28,6 +40,9 @@ type TraceLogger interface { // SetAttributes applies additional key/value pairs to the current trace span. SetAttributes(attrs ...attribute.KeyValue) + // SetSensitiveAttributes applies attributes that contain SENSITIVE DATA. It is only enabled on debug builds of Porter with the traceSensitiveAttributes build tag set. + SetSensitiveAttributes(attrs ...attribute.KeyValue) + // EndSpan finishes the span and submits it to the otel endpoint. EndSpan(opts ...trace.SpanEndOption) @@ -152,6 +167,14 @@ func (l traceLogger) SetAttributes(attrs ...attribute.KeyValue) { l.span.SetAttributes(attrs...) } +// SetSensitiveAttributes applies attributes that contain SENSITIVE DATA. +// It is only enabled on debug builds of Porter with the traceSensitiveAttributes build flag set. +func (l traceLogger) SetSensitiveAttributes(attrs ...attribute.KeyValue) { + if traceSensitiveAttributes { + l.SetAttributes(attrs...) + } +} + // Debug logs a message at the debug level. func (l traceLogger) Debug(msg string, attrs ...attribute.KeyValue) { l.logger.Debug(msg, convertAttributesToFields(attrs)...) diff --git a/pkg/tracing/traceLogger_test.go b/pkg/tracing/traceLogger_test.go index b626f469f..43facb0ef 100644 --- a/pkg/tracing/traceLogger_test.go +++ b/pkg/tracing/traceLogger_test.go @@ -20,3 +20,7 @@ func TestTraceLogger_ShouldLog(t *testing.T) { assert.False(t, l.ShouldLog(zap.InfoLevel)) assert.False(t, l.ShouldLog(zap.DebugLevel)) } + +func TestTraceSensitiveAttributesBuildFlag(t *testing.T) { + assert.False(t, traceSensitiveAttributes, "traceSensitiveAttributes should be disabled by default and require a custom build to enable") +}