Fix setting setuid/gid bits with uid/gid and add a test

[travier]

Support for setuid/setgid/sticky bit got added in 1 but we did not add tests and it's unclear when support for it broke.

See: #2042

[prestist]

Test makes sense, looking at the error I am not sure; I am trying to read up on special mode bits to make sure I understand them. Would the out bits be different the in ? ( I assume yes based on your test) but I am not able to find documentation that can explain it to me.

[prestist]

@travier I think I found it, its because our order of setting ownership and perms were backwards. It passes locally 🤞

[travier]

Nice catch!

[travier]

So, this should fix the issue, but the problem is the same that the one we had in #1325, in that it's kind of a security issue to change this behavior for existing spec releases, as binaries that were previously not SetUID/SetGID would start becoming it after an Ignition update on new nodes.

So we might need to wait for a spec bump and update the docs as well.

[travier]

Hum, the test is also incomplete as it does not test for the case where we both set the mode and the ownership.

[prestist]

So, this should fix the issue, but the problem is the same that the one we had in #1325, in that it's kind of a security issue to change this behavior for existing spec releases, as binaries that were previously not SetUID/SetGID would start becoming it after an Ignition update on new nodes.

So we might need to wait for a spec bump and update the docs as well.

Yeah; that makes sense to me, I will take a look at docs and see what can be done to make it exp only :/

[prestist]

Hum, the test is also incomplete as it does not test for the case where we both set the mode and the ownership.

Still have not done this

[prestist]

Hum, the test is also incomplete as it does not test for the case where we both set the mode and the ownership.

Regarding this what would you have in mind @travier ? would I need to add a user to ignition config and then do this? for the blackbox tests?

[travier]

Hum, the test is also incomplete as it does not test for the case where we both set the mode and the ownership.

Regarding this what would you have in mind @travier ? would I need to add a user to ignition config and then do this? for the blackbox tests?

Yes, we need to test root + setuid/gid and newuser + setuid/gid

[prestist]

Alright I added the test cases and updated version target 🤞 runs fine locally; lmk if we need anything else; otherwise I will re-base squash the fixup.

[travier]

Maybe let's add a test that specifically verify that the bits are ignored in 3.4.0/3.5.0 as well.

[prestist]

Maybe let's add a test that specifically verify that the bits are ignored in 3.4.0/3.5.0 as well.

Sure, I can do that :)
(after some time...)

Slightly more annoying to test then I initially thought.

So blackbox tests generate from "min version" and populate for each next version of ignition. This makes sense for most things, and makes testing that 3.6+ will have sticky bit support; however testing that older version dont have it is a bit more tricky. While locally if I do this it will pass for 3.4, 3.5, but then fail for 3.6 +. Its is because there is no "maxVersion" I.e verifying a bug continues to exist in older versions is hard to do via blackbox tests (currently)

The test object is currently structured as

type Test struct {
	Name              string
	In                []Disk // Disk state before running Ignition
	Out               []Disk // Expected disk state after running Ignition
	MntDevices        []MntDevice
	SystemDirFiles    []File
	Env               []string // Environment variables for Ignition
	Config            string
	ConfigMinVersion  string
	ConfigVersion     string
	ConfigShouldBeBad bool // Set to true to skip config validation step
	SkipCriticalCheck bool // Set to true to skip critical logging check
}

And since we have two expected outputs depending on the version of ignition, ither 0755 before 3.6 or special bits + 0755 after.

The way to fix this is likely to add a config max version, i.e defaults to "" and if max is set we simply dont create a test for that verstion or higher.

Though in implementing it I cannot seem to make that work.

[travier]

Two minor comments but I think were getting closer now. I'll do a final round tomorrow.

[travier]

We'll also need release notes.

[travier]

Thanks, LGTM. I have some minore whitespace fixes that I'll push as a follow up PR later.

[travier]

Hum, this is weird, I don't have the approve power anymore here.

[travier]

Anyway, this is LGTM from my side. Someone else will have to figure out the approuve part :)

[prestist]

Thanks, LGTM. I have some minore whitespace fixes that I'll push as a follow up PR later.

Yay! thank you!

Hum, this is weird, I don't have the approve power anymore here.

Ah weird, maybe because you opened the PR

Anyway, this is LGTM from my side. Someone else will have to figure out the approuve part :)

Alrighty; It looks like I can approve but that feels a bit odd, posting it to the channel.