skip write /proc/self/attr/keycreate when selinux label is null

[ningmingxiao]

https://github.com/opencontainers/runc/pull/4354/files

[ningmingxiao]

@kolyshkin

[thaJeztah]

Looks like your commit is missing a DCO sign-off in the commit message; can you add one? When doing so, please use your real name for the sign-off (not your github handle/username); you can find some instructions in the output from the DCO check here; https://github.com/opencontainers/selinux/pull/214/checks?check_run_id=28026773649

[ningmingxiao]

Looks like your commit is missing a DCO sign-off in the commit message; can you add one? When doing so, please use your real name for the sign-off (not your github handle/username); you can find some instructions in the output from the DCO check here; https://github.com/opencontainers/selinux/pull/214/checks?check_run_id=28026773649

done

[thaJeztah]

Thanks! Oh minor nit; label is a string, but the commit message says when selinux label is null

Perhaps;

SetKeyLabel: ignore empty selinux labels

Looking at the change; it looks like we don't have a test-case for setting an empty label; would probably be good to add one;

selinux/go-selinux/label/label_linux_test.go

Lines 197 to 211 in bb1ec25

	func TestKeyLabel(t *testing.T) {
	needSELinux(t)
	label := "system_u:object_r:container_t:s0:c1,c2"
	if err := selinux.SetKeyLabel(label); err != nil {
	t.Fatal(err)
	}
	nlabel, err := selinux.KeyLabel()
	if err != nil {
	t.Fatal(err)
	}
	if label != nlabel {
	t.Errorf("KeyLabel %s != %s", nlabel, label)
	}
	}

[thaJeztah]

In general; I see that previously we would try to set an empty label, but then ignore the error if it was a os.ErrPermission.

Slightly wondering, but maybe out of scope for this PR (and I could use input from @kolyshkin and @rhatdan on this);

• Should (ideally?) empty labels be handled by the caller (i.e. don't call SetKeyLabel if there's no label)
• And instead SetKeyLabel returning an error (invalid label)?

Somewhat related to the above; could there be valid use-cases of trying to unset this? (so could an empty label have the intent of "unset" what's there?). I see you linked to a runc PR;

• skip read /proc/filesystems if process_label is null runc#4354

Looking at that PR;

• I think it might be more correct for the caller (runc) to not call SetKeyLabel if there's no value to set?
• Also looking at the code, I see various instances of defer selinux.SetKeyLabel("") - do we know what the intent there was? Was the intent to reset values that were previously set?

Because if that's the case, then possibly this PR is actually hiding a bug / incorrect use in runc, and it would be better for SetKeyLabel to return an error on empty values.

[ningmingxiao]

we should let runc do it