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

2023-07-05 18:57:06  来源:博客园

目录逛社区领任务复现 Bug用 GPT-4 来准备 Bug 复现材料对着 GPT-4 给的命令复现 Bug定位 Bug修复思路编写代码测试提 PR逛社区

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


(资料图片仅供参考)

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

盘它!

领任务

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

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

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

复现 Bug

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

用 GPT-4 来准备 Bug 复现材料我的需求

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

我发现 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 命令,看结果是不是存在敏感数据泄露。请告诉我相关命令和操作等。
将需求告诉 DevChatDevChat 给出的完整回答

首先,我会概括你的需求:

你想要复现一个问题,该问题是在使用 Helm 进行 upgrade 操作时,如果出错,会导致 Secret 中的数据被明文打印出来,存在敏感数据泄露风险。你已经设想了一些步骤,包括创建一个包含 Secret 的 Helm Chart,然后进行一些操作来触发这个问题。下面是我为你准备的相关命令和操作:创建一个包含 Secret 的 Helm Chart。首先,你需要创建一个新的 Helm Chart,可以使用以下命令:
helm create mychart

然后,你需要在 mychart/templates 目录下创建一个新的文件 secret.yaml,并添加以下内容:

apiVersion: v1kind: Secretmetadata:  name: mysecrettype: Opaquedata:  mykey: {{ .Values.mysecret.mykey }}

在 mychart/values.yaml 文件中,添加以下内容:

mysecret:  mykey: "SGVsbG8gd29ybGQ="  # "Hello world" 的 base64 编码
执行 helm install 命令。在你的 Helm Chart 目录(即 mychart)下,执行以下命令:
helm install myrelease .

此时,应该没有任何问题或异常。

修改 values.yaml 文件中的 base64 字符串,让其非法。你可以将 values.yaml 文件中的 mykey 值修改为一个非法的 base64 字符串,例如删除一个结尾的“=”:
mysecret:  mykey: "SGVsbG8gd29ybGQ"  # 非法的 base64 字符串
执行 helm upgrade 命令。在你的 Helm Chart 目录下,执行以下命令:
helm upgrade myrelease .

此时,你应该能看到错误信息,包括你的 Secret 数据。

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

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

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

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

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 这里),那就长刀直入吧:

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 里包含了我们所寻找的错误日志。

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

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.ecase result := <-ctxChan:return result.r, result.e}
继续看 releasingUpgrade()

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

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()函数:

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()函数
func updateResource(c *Client, target *resource.Info, currentObj runtime.Object, force bool) error {// ……if force {// ……} else {// ……// send patch to serverc.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 的“管辖范围”。

修复思路

很明显,敏感数据来自于这几行代码:

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之前加入一个日志过滤函数,在这个函数内实现日志脱敏。

编写代码增加一个函数来完成脱敏逻辑:
// 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 stringstart := strings.Index(errLog, "{\\\"apiVersion")end := strings.LastIndex(errLog, ",\\\"kind\\\":\\\"Secret\\\"")// Extract the JSON string and unescape itjsonStr := strings.ReplaceAll(errLog[start:end], "\\\"", "\"")+ "}"// Unmarshal the JSON string into a Secret structvar secret Secretjson.Unmarshal([]byte(jsonStr), &secret)// Desensitize the data in the Secret structfor key := range secret.Data {secret.Data[key] = "***"}// Marshal the Secret struct back into a JSON stringdesensitizedJson, _ := json.Marshal(secret)// Escape the JSON string and replace it in the original logfixedDesensitizedJson := strings.ReplaceAll(string(desensitizedJson), "\"", "\\\"")desensitizedLog := errLog[:start] + fixedDesensitizedJson[:len(fixedDesensitizedJson)-1] + errLog[end:]return desensitizedLog}
把函数调用加到老代码里:
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 吧:
func TestDesensitizeLog(t *testing.T) {// Define a test error logerrLog := `cannot patch "test-release-secret" with kind Secret:  "" is invalid: patch: Invalid value: "{\"apiVersion\":\"v1\",\"data\":{\"secretKey\":\"hello\", \"anotherKey\":\"world\"},\"kind\":\"Secret\",`// Call the functionresult := desensitizeLog(errLog)// Check if the "data" field has been correctly sanitizedif !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,不啰嗦了。可能你已经发现,结尾有点仓促,没错,我写到一半的时候,到饭点了,赶紧收个尾,填肚子去咯~