Skip to content

Trace sensitive attributes in custom dev builds #2478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
carolynvs merged 3 commits into from
Dec 6, 2022
Merged

Trace sensitive attributes in custom dev builds #2478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9211900
Trace sensitive attributes in custom dev builds
carolynvs Dec 1, 2022
f1f146c
Add tracing.ObjectAttribute
carolynvs Dec 2, 2022
7c4416b
Use span.SetSensitiveAttributes when running a bundle
carolynvs Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down
18 changes: 18 additions & 0 deletions magefile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package main

import (
"bytes"
"fmt"
"go/build"
"log"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand Down
8 changes: 7 additions & 1 deletion pkg/cnab/provider/action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
16 changes: 16 additions & 0 deletions pkg/porter/porter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down
17 changes: 17 additions & 0 deletions pkg/tracing/attributes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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))
}
9 changes: 9 additions & 0 deletions pkg/tracing/tag_trace_sensitive_attributes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
}
23 changes: 23 additions & 0 deletions pkg/tracing/traceLogger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)

Expand Down Expand Up @@ -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)...)
Expand Down
4 changes: 4 additions & 0 deletions pkg/tracing/traceLogger_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}