给 Helm 提一个 PR，重温开源项目参与过程

2023-07-05 约 3155 字预计阅读 7 分钟

逛社区

注：这一篇文章发出来后，又用 GPT-4 重写了一次代码，详情见《给 Helm 修复一个 Bug - 每天5分钟玩转 GPT 编程系列(2)》。为了独立成文，两篇文章都加了一些开头结尾，所以部分重复。本文更加侧翻阅源码重定位 Bug 的过程，隔壁那篇侧重用 GPT-4 写代码的过程。

酒足饭饱，闲来无事，打开 Helm 项目的 issues 列表，咦，这个 issue 看起来眉清目秀呀：

其实一点都不“眉清目秀”，这段 issue 描述太冗长了，没有任何排版，看起来很累。咱就看关键词吧：helm upgrade，sensitive data，error，好的，这就够了，加在一起就是说当 helm upgrade 命令执行失败的时候，可能回导致敏感数据泄露。

盘它！

领任务

尽管已经有人留言认领这个任务，不过看描述似乎他并不会马上开始编码，而是希望进一步沟通方案。好办好办，转给我吧：

倒是挺干脆，分分钟就 assign 给我了。

不过，其实，这会我并不知道怎么修复这个 bug，我也不熟悉 Helm 的源码，外加已经小半年没咋写过 Golang 了，其实心里挺慌。试试看吧，究竟是宝刀未老还是老骥伏枥。

复现 Bug

要修复 Bug，第一步自然是先看能不能稳定复现。外加 issue 描述里其实我看不出来如何复现，也不是太看清楚问题现象是什么，总之，复现问题是当务之急。

用 GPT-4 来准备 Bug 复现材料

我的需求

下面就是想想自己需要啥材料，然后抛给 GPT。或许我可以这样说：

1
2
3
4
5
6


我发现 Helm 在执行 upgrade 的时候，如果出错，会导致 Secret 里的数据被明文打印出来，存在敏感数据泄露风险。现在我想复现这个问题，我设想的步骤是这样的：
1. 构造一个包含 Secret 的正常的 Helm Chart，Secret 里的数据是从 values.yaml 里读取的，values.yaml 中直接使用 base64 编码字符串，而不是在 secret.yaml 中使用 b64enc；
2. 然后执行 helm install，此刻应该没有问题异常；
3. 修改 values.yaml 中的 base64 字符串，让其非法（比如删除一个结尾的“=”）；
4. 执行 helm upgrade 命令，看结果是不是存在敏感数据泄露。
请告诉我相关命令和操作等。

将需求告诉 DevChat

DevChat 给出的完整回答

首先，我会概括你的需求：

你想要复现一个问题，该问题是在使用 Helm 进行 upgrade 操作时，如果出错，会导致 Secret 中的数据被明文打印出来，存在敏感数据泄露风险。

你已经设想了一些步骤，包括创建一个包含 Secret 的 Helm Chart，然后进行一些操作来触发这个问题。下面是我为你准备的相关命令和操作：

创建一个包含 Secret 的 Helm Chart。首先，你需要创建一个新的 Helm Chart，可以使用以下命令：
1
helm create mychart
然后，你需要在 mychart/templates 目录下创建一个新的文件 secret.yaml，并添加以下内容：
1
2
3
4
5
6
7
apiVersion: v1
kind: Secret
metadata:
  name: mysecret
type: Opaque
data:
  mykey: {{ .Values.mysecret.mykey }}
在 mychart/values.yaml 文件中，添加以下内容：
1
2
mysecret:
  mykey: "SGVsbG8gd29ybGQ="  # "Hello world" 的 base64 编码
执行 helm install 命令。在你的 Helm Chart 目录（即 mychart）下，执行以下命令：
1
helm install myrelease .
此时，应该没有任何问题或异常。

修改 values.yaml 文件中的 base64 字符串，让其非法。你可以将 values.yaml 文件中的 mykey 值修改为一个非法的 base64 字符串，例如删除一个结尾的“=”：
1
2
mysecret:
  mykey: "SGVsbG8gd29ybGQ"  # 非法的 base64 字符串
执行 helm upgrade 命令。在你的 Helm Chart 目录下，执行以下命令：
1
helm upgrade myrelease .
此时，你应该能看到错误信息，包括你的 Secret 数据。

以上就是复现你所说问题的步骤。如果你在执行这些步骤时遇到任何问题，或者需要更多的帮助，欢迎随时向我提问。

对着 GPT-4 给的命令复现 Bug

DevChat 诚不我欺啊，一顿复制粘贴敲键盘，最后我看到了这个：

注意这个错误日志里的这部分：

1

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

这里的 mykey: SGVsbG8gd29ybGQ 看起来似乎不太敏感，不过它其实意味着假如你存了10个密码在 Secret 里，当更新的时候写错了一个，其他正确的9个会被无差别打印出来。这个日志里包含的是一个未脱敏的完整的 Secret 资源实例内容。

定位 Bug

从日志里来看，JSON 部分大概率是 K8s 相关的库返回的内容，Helm 里直接拼接到自己日志里打印出来了。我们需要找到最接近这个“K8s 相关调用”的地方，然后找到这串日志怎么来的，再加一层“脱敏”。

搜索关键字“UPGRADE FAILED”

太幸运了，只有一个结果（明显是 upgrade.go 这里），那就长刀直入吧：

1
2
3
4


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

继续跟 RunWithContext() 方法里哪里返回了错误日志

进去 RunWithContext() 方法，基本从名字上就能判断是“执行升级过程”的时候出的错，因为一开始的错误日志里有一句“cannot patch “mysecret” with kind Secret”，这显然也不是 Helm 本身的行为，而是某个 K8s 相关的库返回的，因此肯定是开始执行的时候出的错，也就是 performUpgrade() 这个方法返回的 err 里包含了我们所寻找的错误日志。

 1
 2
 3
 4
 5
 6
 7
 8
 9
10


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
}

继续跟 performUpgrade()

这个方法有点长，仔细看看，大概率是在结尾的时候 result.e 里包含了我们寻找的 err，而这个 result 是一个 Channel，在 releasingUpgrade 这个协程里完成了数据写入。行，下一步，继续看 releasingUpgrade()

 1
 2
 3
 4
 5
 6
 7
 8
 9
10


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
	}

继续看 releasingUpgrade()

到这里就差不多了，这里有关键的一行代码：u.cfg.KubeClient.Update(current, target, u.Force)：

 1
 2
 3
 4
 5
 6
 7
 8
 9
10


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
	}
    //……
}

到这里就能猜到很快就应该接近 Helm 和 K8s 相关库的交界点了，“KubeClient”这个名字看起来就不像是 Helm 自身逻辑里的代码。我们继续来看 Update 就行：

继续跟 Update() 接口

这里比较直观，我们要找到的实现肯定是 client.go 里面那个 Update。

看 Update() 接口的实现

这个方法看起来也挺复杂，里面藏了一个关键的 updateResource() 函数：

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12


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())
		}
		// ……
	})
    // ……
}

继续看 updateResource() 函数

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15


func updateResource(c *Client, target *resource.Info, currentObj runtime.Object, force bool) error {
	// ……
	if force {
		// ……
	} else {
		// ……
		// send patch to server
		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)
		}
	}
	// ……
}

到这里就比较明确了，是 helper.Patch(target.Namespace, target.Name, patchType, patch, nil) 这个方法调用返回了一个包含敏感数据的 err。如果继续看一眼 Patch 方法的定义，就能找到：

这是 k8s.io/cli-runtime 里的代码，已经离开了 Helm 的“管辖范围”。

修复思路

很明显，敏感数据来自于这几行代码：

1
2
3
4


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)
}

我们希望尽脱敏这些数据，自然是在当前函数/方法内完成这个过程，不能把锅留给调用方（上层函数）。所以下一步就是在 if err != nil { 之后，return 之前加入一个日志过滤函数，在这个函数内实现日志脱敏。

编写代码

增加一个函数来完成脱敏逻辑：

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28


// desensitizeLog replaces the data in a Secret with {"key": "***"}.
// e.g. "data": {"username": "admin", "password": "password"} becomes "data": {"username": "***", "password": "***"}
func desensitizeLog(errLog string) string {
	// Find the start and end index of the JSON string
	start := strings.Index(errLog, "{\\\"apiVersion")
	end := strings.LastIndex(errLog, ",\\\"kind\\\":\\\"Secret\\\"")

	// Extract the JSON string and unescape it
	jsonStr := strings.ReplaceAll(errLog[start:end], "\\\"", "\"")+ "}"

	// Unmarshal the JSON string into a Secret struct
	var secret Secret
	json.Unmarshal([]byte(jsonStr), &secret)

	// Desensitize the data in the Secret struct
	for key := range secret.Data {
		secret.Data[key] = "***"
	}

	// Marshal the Secret struct back into a JSON string
	desensitizedJson, _ := json.Marshal(secret)

	// Escape the JSON string and replace it in the original log
	fixedDesensitizedJson := strings.ReplaceAll(string(desensitizedJson), "\"", "\\\"")
	desensitizedLog := errLog[:start] + fixedDesensitizedJson[:len(fixedDesensitizedJson)-1] + errLog[end:]

	return desensitizedLog
}

把函数调用加到老代码里：

1
2
3
4
5
6
7
8
9


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 {
	sanitizeLog:=err.Error()
	if kind == "Secret" {
		sanitizeLog = desensitizeLog(err.Error())
	}
	return errors.Wrapf(errors.New(sanitizeLog), "cannot patch %q with kind %s", target.Name, kind)
}

当 kind 为 Secret 的时候，就走一遍脱敏过程。

再加一段 UT 吧：

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14


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

	// Call the function
	result := desensitizeLog(errLog)

	// Check if the "data" field has been correctly sanitized
	if !strings.Contains(result, "\\\"secretKey\\\":\\\"***\\\"") ||
		!strings.Contains(result, "\\\"anotherKey\\\":\\\"***\\\"") {
		t.Errorf("The function did not correctly sanitize the error log")
	}
}

测试

UT 需要能通过，这就不用赘述了。UT 过了之后，手动测试下：

酷，看起来挺和谐了。

提 PR

最后一步，提个 PR：

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

关于如何参与开源项目，如何在 GitHub 上提 PR，我这有一篇非常详细的文章：

如何参与开源项目 - 细说 GitHub 上的 PR 全过程

ok，不啰嗦了。可能你已经发现，结尾有点仓促，没错，我写到一半的时候，到饭点了，赶紧收个尾，填肚子去咯~