fix: More consistent protected env vars

[DrJosh9000]

Description

Make the whole situation with env var protection clearer.

• Apply normalisation when checking protection
• Allow some env vars that are mutable from hooks and plugins to be modifiable from the Job API
• Prevent mutating some env vars via hook wrapper

Context

https://linear.app/buildkite/issue/A-991, and others

Testing

• Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
• Code is formatted (with go tool gofumpt -extra -w .)

Disclosures / Credits

The wet noodles of brain tissue curling around inside my skull

[zhming0]

It looks mostly good, I can see the rationale. I mainly have questions to prevent me misunderstanding this

[zhming0]

is this change a feature? it feels like a big expansion to plugin capabilities.

[DrJosh9000]

Plugins are already able to set these, e.g. https://github.com/buildkite-plugins/git-clean-buildkite-plugin/blob/master/hooks/pre-checkout

[zhming0]

ah then is this change re envs that are created as part of Pipeline yaml??

[DrJosh9000]

All these env vars are ignored from the pipeline YAML or build creation dialog - that was already the case. The change here is to make it clearer that some of them can be set from plugins, hooks, or the command.

[zhming0]

All these env vars are ignored from the pipeline YAML or build creation dialog

TIL! I thought I got it working via Pipeline yaml before.

I have doubt but all good for now 👍🏿

[zhming0]

Just for my understanding, why would we disallow an env to be mutated by job env var but instead allow it to be mutated by hook and plugin?

[zhming0]

Might it be something worth writing down as well?

[DrJosh9000]

Good question. The main question I ask is: if someone has rights to start a build and provide env vars, could an env var they set be able to bypass other restrictions like no-command-eval? And would the env var be a reasonable way for a plugin to set config. Git flags are an example. There are some shell injections possible via Git flag, so they should be rejected from the build creation dialog, but plugins should be able to adjust Git flags prior to checkout. (Plugins can also implement the checkout hook, bypassing the default checkout process entirely.)

[DrJosh9000]

I've written some of this into the comment.

[zhming0]

Interesting, I thought someone who has rights to start a build are often the one that has code commit permission, so this permission mechanism seems quite narrow.

But thanks for clarifying it!

[zhming0]

Is this change "Prevent mutating some env vars via hook wrapper"?

If I am not mistaken, we didn't add any new protected env var in this PR?

[DrJosh9000]

The lines where you've commented, the change is to allow more env vars to be changed via the Job API. Previously, the git flags vars would have been rejected here, even though they can be set via the hook wrapper.

The hook wrapper and Job API should have consistent logic, since they're both doing the same thing (allowing a script or command to change env vars), so the same check is now added to the hook wrapper.

[zhming0]

👍🏿 yeah, my question was mainly about where is the code change for this item in PR description?

Prevent mutating some env vars via hook wrapper

[DrJosh9000]

The change in executor.go (

agent/internal/job/executor.go

Lines 631 to 644 in 0aef51d

	// Prevent any changes to vars that are protected even within the job.
	// This is a courtesy to hook/plugin authors that write a hook that mutates
	// an env var only to find out that the change has no effect, or perhaps
	// breaks the rest of the job.
	var protected []string
	for k := range changes.Diff.Keys {
	if env.IsProtectedFromWithinJob(k) {
	protected = append(protected, k)
	changes.Diff.Remove(k)
	}
	}
	if len(protected) > 0 {
	e.shell.Commentf("Changes to some env vars were blocked; the affected vars are: %v", protected)
	}

)

[zhming0]

LGTM though I think it might be worth to re-clarify some names.

[zhming0]

Without reading more context, the allowWriteFromJob seems to be saying allow Job environment to modify these env vars.

But the comments above say:

But we don't want the job environment to be able to set them, because git is riddled with shell injections

[zhming0]

All these env vars are ignored from the pipeline YAML or build creation dialog

TIL! I thought I got it working via Pipeline yaml before.

I have doubt but all good for now 👍🏿

[zhming0]

👍🏿 yeah, my question was mainly about where is the code change for this item in PR description?

Prevent mutating some env vars via hook wrapper

[DrJosh9000]

bazel run :gazelle generated this