secprofnodestatus: Link the node profile status with profile using a custom label

[jhrozek]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

We were using the profile name as the value of the spo.x-k8s.io/profile-name
label to be able to list node profile statuses for a profile. This is
user-friendly but would fail in case the profile name was longer than 64
characters as that's the label size limit.

Instead, let's rename the label to spo.x-k8s.io/profile-id. In case the
profile name is shorter than the limit, let's still use the name because
it's user-friendly, but in case the name is longer, let's instead use a
sha1 hash of the name with the profile Kind as a prefix.

Using sha1 is OK in this case, because we're not using it for any crypto
purposes, just to shorten a name.

Which issue(s) this PR fixes:

Fixes #408

Does this PR have test?

Yes

Special notes for your reviewer:

N/A

Does this PR introduce a user-facing change?

The securityprofilenodestatus CR now links with the security profile its status
it represents using label spo.x-k8s.io/profile-id. If the profile name is less
than 64 characters long, then the label value is the profile name, otherwise it's
kind-sha256hashofthename, trimmed to fit into 64 characters

This change supports profile names whose names are over 64 characters.

[codecov-commenter]

Codecov Report

Merging #685 (0d0f5a5) into master (54872f9) will decrease coverage by 0.34%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #685      +/-   ##
==========================================
- Coverage   51.57%   51.22%   -0.35%     
==========================================
  Files          31       31              
  Lines        2065     2079      +14     
==========================================
  Hits         1065     1065              
- Misses        947      961      +14     
  Partials       53       53              

[jhrozek]

hmm, apart from the verify failures the test also seem to be failing on vanilla kube..let me investigate..
/hold

[JAORMX]

I think it could be the case that we have a SELinux and a Seccomp policy with the same name... Seems to me like that would cause issues. Why not identify profiles with a combinarion of <kind> + <name> instead?

[jhrozek]

I think it could be the case that we have a SELinux and a Seccomp policy with the same name... Seems to me like that would cause issues. Why not identify profiles with a combinarion of <kind> + <name> instead?

Yes, good idea. The labels now look like this:

spo.x-k8s.io/profile-id: SeccompProfile-log-enricher-trace

for short names and like this:

spo.x-k8s.io/profile-id: SeccompProfile-8b3e816a82b57243d0a7c6500d49a6424f69221a

for long names.

The tests should pass now, I was an idiot and thought the nodes array contains all nodes, while in fact it contains worker nodes only.

[jhrozek]

ugh I suck. I'll fix this tomorrow.

[jhrozek]

@saschagrunert @pjbgf @JAORMX I'm having trouble running the make verify target locally to get the same error or understand how to fix it.

Locally I'm running:

jhrozek@kyubey:~/devel/security-profiles-operator|label_len ⇒  ./build/golangci-lint run --fix
jhrozek@kyubey:~/devel/security-profiles-operator|label_len ⇒  gci -w -local internal/pkg/util/names_test.go
jhrozek@kyubey:~/devel/security-profiles-operator|label_len ⇒  git st
On branch label_len
Your branch is ahead of 'origin/master' by 2 commits.
  (use "git push" to publish your local commits)

nothing to commit, working tree clean

I have the same golangci version and I'm running go 1.17. How do I fix the linter issues?

[jhrozek]

OK, "solved". I spun up a local toolbox container and installed the bpf libraries using the hack-XXX scripts. For some reason that did help.

[jhrozek]

/hold cancel

[jhrozek]

FWIW, I think the fedora test failure is unrelated. That test had passed in the previous iteration of this PR.

[pjbgf]

nice one @jhrozek, we may need a small change on the sha1. apart from that it looks good.

[pjbgf]

Completely understand the rational here. I would probably use sha256 though, given the project's focus in security we should refrain from taking dependencies on algorithmns or libraries that are known to be weak/vulnerable.

This may also be a problem for users in high compliance sectors, as this is not NIST compliant.

[jhrozek]

Thank you for the review. I understand where you're coming from, but the problem is the 64 characters length that labels are restricted to in k8s. Consider this (it's python but I hope it's readable nonetheless):

In [5]: hashlib.sha1('foo'.encode()).hexdigest()
Out[5]: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33'

In [6]: hashlib.sha256('foo'.encode()).hexdigest()
Out[6]: '2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae'

In [7]: len(hashlib.sha1('foo'.encode()).hexdigest())
Out[7]: 40

In [8]: len(hashlib.sha256('foo'.encode()).hexdigest())
Out[8]: 64

so sha256 doesn't already fit on its own:

kubectl label sp log-enricher-trace hash=2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae
error: invalid label value: "hash=2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae": must be no more than 63 characters

One thing we could do (but..it's kindof a hack..) is to trim the length a bit, e.g. use an equivalent of git's abbrev and fall back to using the full hash without prefix if there is a collision, which frankly is quite unlikely. And even if there was a collision, it wouldn't be that much of a deal as the hash is not used for any security critical functionality, just a link between two objects.

[pjbgf]

If you are cool with using sha256, trimming the tail end of it is a common approach which slighty increases chance of collision but that isn't a problem on that case - and less likely to occur than sha1 anyway.

Otherwise, would you be opposed to simply restricting the size of profile name and using that instead?

In both the approaches above, any concerns around name collision through profiles with the same name deployed in different namespaces?

Now if this can be non-deterministic, we could also just append the hex of some random values at the tail end using rand.Read(buf).

[jhrozek]

If you are cool with using sha256, trimming the tail end of it is a common approach which slighty increases chance of collision but that isn't a problem on that case - and less likely to occur than sha1 anyway.

How much is reasonable to trim? As little as possible to fill the 63 characters?

Otherwise, would you be opposed to simply restricting the size of profile name and using that instead?

I would not be opposed to that. The only catch is that we use sometimes derive security profile resources from other resources (e.g. we derive from recording name), so we need to be careful and restrict them all or we might be unable to generate the profiles later.

In both the approaches above, any concerns around name collision through profiles with the same name deployed in different namespaces?

Both objects are namespaced and live in the same namespace, so I think using the namehash is no different than using the plain name. Or did you have some other issue in mind?

Now if this can be non-deterministic, we could also just append the hex of some random values at the tail end using rand.Read(buf).

I honestly prefer deterministic at least to make testing easier.

[jhrozek]

ok, let me start by trimming the hash and in parallel look at limiting the name. We can compare the two approaches then (and see test results!)

[pjbgf]

How much is reasonable to trim? As little as possible to fill the 63 characters?

Yep, as little as possible would be ideal.

I would not be opposed to that. The only catch is that we use sometimes derive security profile resources from other resources (e.g. we derive from recording name), so we need to be careful and restrict them all or we might be unable to generate the profiles later.

Very good point. That's definitely something we want to get right and aligned sooner rather than later.

Both objects are namespaced and live in the same namespace, so I think using the namehash is no different than using the plain name. Or did you have some other issue in mind?

That's right - same issue in both cases. I was thinking this could be a problem when we deploy SPO to watch multiple namespaces for Security Profiles, which I think we still support - but not completely sure.

[jhrozek]

OK, for now I've changed the hash to use sha256. Still todo is to look at name length validation and check the multi-namespace labels.

[jhrozek]

/retest

[jhrozek]

OK, for limiting the name we have basically two options:

• let the object be created with a long name and either mark it with an error and refuse to process it further or just issue a warning (because the name is only used in labels for the secprofnodestatus object which is not super critical)
• add a validating webhook. This is probably the more correct way, but honestly I dislike adding another webhook just for this case..

Thoughts @pjbgf ?

[jhrozek]

/test pull-security-profiles-operator-test-e2e
/test pull-security-profiles-operator-test-verify

[pjbgf]

/test pull-security-profiles-operator-test-verify

[k8s-ci-robot]

@pjbgf: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

• /test pull-security-profiles-operator-build
• /test pull-security-profiles-operator-build-image
• /test pull-security-profiles-operator-test-e2e
• /test pull-security-profiles-operator-test-unit
• /test pull-security-profiles-operator-verify

Use /test all to run all jobs.

In response to this:

/test pull-security-profiles-operator-test-verify

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[pjbgf]

/test pull-security-profiles-operator-verify

[pjbgf]

fedora-e2e failed after running for over 50 minutes - retriggering it now.

[pjbgf]

let the object be created with a long name and either mark it with an error and refuse to process it further or just issue a warning (because the name is only used in labels for the secprofnodestatus object which is not super critical)

happy to take this approach for now.

[pjbgf]

/lgtm

the validation can be done on a follow-up PR.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhrozek, pjbgf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [jhrozek,pjbgf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jhrozek]

/test pull-security-profiles-operator-test-e2e

[jhrozek]

/test pull-security-profiles-operator-verify

[jhrozek]

/test pull-security-profiles-operator-verify

[saschagrunert]

/override pull-security-profiles-operator-verify

[k8s-ci-robot]

@saschagrunert: Overrode contexts on behalf of saschagrunert: pull-security-profiles-operator-verify

In response to this:

/override pull-security-profiles-operator-verify

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.