[builder] Add sanitization for cfg.Distribution.Name/BuildTags

[fatsheep9146]

trafficstars

Track issues for #5659

Previously, the nosec line was right before the exec.Command, and it's not anymore. Move it here and it will skip the linter.

That said, I'd be more comfortable if we were to sanitize both the argument that was originally causing G204 (cfg.Distribution.Name) and the new one (cfg.Distribution.BuildTags).

Originally posted by @jpkrohling in https://github.com/open-telemetry/opentelemetry-collector/pull/5659#discussion_r918158813

[fatsheep9146]

Ping @jpkrohling

[Chinwendu20]

I would like on this issue, but is there really a way? as I have seen this comment https://github.com/securego/gosec/issues/831#issuecomment-1196292035

[jpkrohling]

I don't know yet if there's a way. Perhaps there is? It's worth spending some time thinking about it, or documenting why the solution is indeed to skip the check. For completeness, here's the line that causes the linter to skip the check: https://github.com/open-telemetry/opentelemetry-collector/blob/c157b128e9ff32ed06e2913e2afa83c9abbed9ba/cmd/builder/internal/builder/main.go#L93

[Chinwendu20]

Thanks a lot for your response. I was wondering if we would also carry out sanitation on cfg.distribution.go argument as well. As this line does not have the cfg.Distribution.Name and cfg.Distribution.BuildTags arguments and yet has this line // #nosec G204 on it

https://github.com/open-telemetry/opentelemetry-collector/blob/c157b128e9ff32ed06e2913e2afa83c9abbed9ba/cmd/builder/internal/builder/main.go#L112

[Chinwendu20]

I plan on sanitizing the arguments by ensuring they match a regex pattern containing letters and numbers only. How is that? Is there a doc with laid out specification that I can use to come up with a more acceptable pattern?

[jpkrohling]

Given the alternatives, I think it's OK to skip the check here if you add a comment on the reasoning. To me, blocking a potentially good input due to a bug in the validation (regex) is more problematic than a possible bad input causing a bad result.

This lack of validation is a problem only if the machine running this command does not belong to the person requesting this command to be executed. This is a problem if we expose this API to remote users, but in that case, we should provide a static list of options instead of a free text field.

What I would do then is:

1. add a comment to the Distribution.Go property, stating that we trust the caller to have set this to a safe path
2. skip the verification, with a comment that we trust the caller did the necessary input validation

[Chinwendu20]

Cool! thanks from what I understand we would still have the "#nosec" line but with additional comments stating the reason behind it right?

[jpkrohling]

That's correct.