Browsing the Community

While scanning through Helm's issue tracker, I came across an issue that immediately caught my attention. The key terms were: helm upgrade, sensitive data, error. Combined, they indicate a potential security vulnerability where failed upgrade operations might expose sensitive data contained in Secrets.

Claiming the Task

Someone had already left a comment claiming this issue, but it seemed they weren't planning to start coding soon and wanted to discuss the approach first. I offered to take it over, and the maintainers assigned it to me promptly.

Admittedly, I wasn't sure how to fix this bug, wasn't familiar with Helm's codebase, and had barely written any Go in the past six months. Nevertheless, I decided to give it a try.

Reproducing the Bug

The first step in fixing any bug is confirming whether it can be reliably reproduced. The issue description was unclear about the exact reproduction steps, so reproducing the problem became the priority.

Using GPT-4 to Prepare Reproduction Materials

I needed to formulate a plan and asked GPT for guidance. Here's essentially what I said:

I'm investigating a security issue where Helm exposes Secret data in plaintext when an upgrade command fails. My intended reproduction steps are:

1. Create a Helm Chart with a Secret that reads data from values.yaml using base64-encoded strings direct rather than using b64enc in secret.yaml
2. Run helm install - this should succeed without issues
3. Modify the base64 string in values.yaml to make it invalid (for example, remove a trailing "=")
4. Run helm upgrade and verify whether sensitive data gets exposed Please provide the relevant commands and operations needed.

The response provided a complete step-by-step guide including:

helm create mychart

Create mychart/templates/secret.yaml:

apiVersion: v1
kind: Secret
metadata:
  name: mysecret
type: Opaque
data:
  mykey: {{ .Values.mysecret.mykey }}

Add to mychart/values.yaml:

mysecret:
  mykey: "SGVsbG8gd29ybGQ="  # base64 of "Hello world"

Then execute:

helm install myrelease .
helm upgrade myrelease .

Following the Reproduction Steps

After following the provided steps, I successfully reproduced the issue. The error output contained something like this:

Error: UPGRADE FAILED: cannot patch "mysecret" with kind Secret:  "" is invalid: patch: Invalid value: "{\"apiVersion\":\"v1\",\"data\":{\"mykey\":\"SGVsbG8gd29ybGQ\"}……

While SGVsbG8gd29ybGQ might not look particularly sensitive, the real danger becomes apparent when you consider a scenario with 10 passwords in a Secret. If you mistype one during an update, all 10 correct values get printed indiscriminately. This log contains a complete, unredacted Secret resource definition.

Locating the Bug

The JSON portion in the log likely comes from Kubernetes-related libraries, with Helm simply concatenating and printing it. The approach is to find the closest point to this "Kubernetes library call" and trace where this log string originates.

Step 1: Search for "UPGRADE FAILED"

A search yielded only one result in upgrade.go:

rel, err := client.RunWithContext(ctx, args[0], ch, vals)
if err != nil {
    return errors.Wrap(err, "UPGRADE FAILED")
}

Step 2: Trace the RunWithContext() Method

Inside RunWithContext(), the error originates from performUpgrade(). The error message contains "cannot patch mysecret with kind Secret", which isn't Helm's own behavior—it comes from a Kubernetes-related library. This confirms the error happens during execution:

func (u *Upgrade) RunWithContext(ctx context.Context, name string, chart *chart.Chart, vals map[string]interface{}) (*release.Release, error) {
    res, err := u.performUpgrade(ctx, currentRelease, upgradedRelease)
    if err != nil {
        return res, err
    }
    return res, nil
}

Step 3: Examine performUpgrade()

This method uses a Channel where releasingUpgrade writes results. The error we're loooking for is in result.e:

func (u *Upgrade) performUpgrade(ctx context.Context, originalRelease, upgradedRelease *release.Release) (*release.Release, error) {
    go u.releasingUpgrade(rChan, upgradedRelease, current, target, originalRelease)
    go u.handleContext(ctx, doneChan, ctxChan, upgradedRelease)
    select {
    case result := <-rChan:
        return result.r, result.e
    case result := <-ctxChan:
        return result.r, result.e
    }
}

Step 4: Inspect releasingUpgrade()

This is where KubeClient.Update() gets called:

func (u *Upgrade) releasingUpgrade(c chan<- resultMessage, upgradedRelease *release.Release, current kube.ResourceList, target kube.ResourceList, originalRelease *release.Release) {
    results, err := u.cfg.KubeClient.Update(current, target, u.Force)
    if err != nil {
        u.cfg.recordRelease(originalRelease)
        u.reportToPerformUpgrade(c, upgradedRelease, results.Created, err)
        return
    }
}

"KubeClient" clearly indicates we're at the boundary between Helm and Kubernetes libraries. Let's examine the Update() method:

Step 5: Follow the Update() Interface

The implementation is in client.go. Inside the Update() method, there's a critical updateResource() call:

func (c *Client) Update(original, target ResourceList, force bool) (*Result, error) {
    err := target.Visit(func(info *resource.Info, err error) error {
        if err := updateResource(c, info, originalInfo.Object, force); err != nil {
            c.Log("error updating the resource %q:\n\t %v", info.Name, err)
            updateErrors = append(updateErrors, err.Error())
        }
        return nil
    })
}

Step 6: Examine updateResource()

This function wraps the actual Kubernetes call:

func updateResource(c *Client, target *resource.Info, currentObj runtime.Object, force bool) error {
    if force {
        // ...
    } else {
        c.Log("Patch %s %q in namespace %s", kind, target.Name, target.Namespace)
        obj, err = helper.Patch(target.Namespace, target.Name, patchType, patch, nil)
        if err != nil {
            return errors.Wrapf(err, "cannot patch %q with kind %s", target.Name, kind)
        }
    }
    return nil
}

The culprit is helper.Patch(target.Namespace, target.Name, patchType, patch, nil). This call returns an error containing the sensitive Secret data. Further investigation shows this comes from k8s.io/cli-runtime, outside Helm's control.

Fix Approach

The sensitive data originates from this code:

obj, err = helper.Patch(target.Namespace, target.Name, patchType, patch, nil)
if err != nil {
    return errors.Wrapf(err, "cannot patch %q with kind %s", target.Name, kind)
}

To sanitize this data, we need to process it within this function before returning. We can't leave the cleanup to calling functions. The solution is to add a sanitization function that runs between the error check and the return statement.

Writing the Code

Adding a Sanitization Function

// redactSecretData replaces sensitive values in a Secret with "***".
// For example: "data": {"username": "admin", "password": "secret"} 
// becomes "data": {"username": "***", "password": "***"}
func redactSecretData(logMsg string) string {
    // Locate the JSON boundaries in the log message
    startIdx := strings.Index(logMsg, "{\"apiVersion")
    endIdx := strings.LastIndex(logMsg, ",\"kind\":\"Secret\"")

    if startIdx == -1 || endIdx == -1 {
        return logMsg
    }

    // Extract the JSON portion and unescape it
    jsonPortion := strings.ReplaceAll(logMsg[startIdx:endIdx], "\\\"", "\"") + "}"

    // Parse the JSON into a Secret struct
    var secret corev1.Secret
    if err := json.Unmarshal([]byte(jsonPortion), &secret); err != nil {
        return logMsg
    }

    // Replace all data values with masked text
    for key := range secret.Data {
        secret.Data[key] = []byte("***")
    }

    // Convert back to JSON and reconstruct the log message
    sanitizedJson, _ := json.Marshal(secret)
    escapedJson := strings.ReplaceAll(string(sanitizedJson), "\"", "\\\"")
    
    return logMsg[:startIdx] + escapedJson[:len(escapedJson)-1] + logMsg[endIdx:]
}

Integrating the Function

c.Log("Patch %s %q in namespace %s", kind, target.Name, target.Namespace)
obj, err = helper.Patch(target.Namespace, target.Name, patchType, patch, nil)
if err != nil {
    sanitizedMsg := err.Error()
    if kind == "Secret" {
        sanitizedMsg = redactSecretData(err.Error())
    }
    return errors.Wrap(errors.New(sanitizedMsg), "cannot patch %q with kind %s", target.Name, kind)
}

When the resource kind is "Secret", the error message goes through the sanitization process.

Adding Unit Tests

func TestRedactSecretData(t *testing.T) {
    // Sample error log with Secret data
    sampleLog := `cannot patch "test-release-secret" with kind Secret:  "" is invalid: 
patch: Invalid value: "{\"apiVersion\":\"v1\",\"data\":{\"secretKey\":\"hello\", \"anotherKey\":\"world\"},\"kind\":\"Secret\",`

    // Execute the function
    result := redactSecretData(sampleLog)


    // Verify that data values have been replaced
    if !strings.Contains(result, "\\\"secretKey\\\":\\\"***\\\"") ||
        !strings.Contains(result, "\\\"anotherKey\\\":\\\"***\\\"") {
        t.Errorf("Secret data was not properly redacted in error log")
    }
}

Testing

Unit tests must pass before proceeding. After UT verification, manual testing confirmed the fix works as expected—the error output now displays *** instead of actual Secret values.

Submitting the PR

With the fix complete and tested, the final step is submitting a pull request to the Helm repository:

• https://github.com/helm/helm/pull/12183