BUG: AddRule: fix to handle EACCES

[kolyshkin]

In case a rule with the action that equals to the default one was added,
libseccomp used to return EPERM, and libseccomp-golang converted it into
a more user-friendly "requested action matches default action of
filter" error.

From various bug reports I have noticed this is no longer a case.

The cause is seccomp/libseccomp@83989be02 (appeared in v2.5.0), which
changes EPERM to EACCES.

Since we still support libseccomp < 2.5.0, check for either EPERM or
EACCES, and add a TODO item to remove the former.

Add a test case, which fails like this before the fix:

seccomp_test.go:580: expected error to contain "matches default action", got "permission denied"

[kolyshkin]

Alternatively, we can introduce

var errRuleActionMatchesDefault = errors.New("requested action matches default action of filter")

in the code, and compare against it here in the test. This way, if the wording is ever changed, the test would not need fixing (but will still check the error returned is as expected).

[kolyshkin]

Implemented.

[pcmoore]

Hmmm, I think @drakenclimber and I may need to sort an aspect of this out in the main libseccomp library first as the seccomp_rule_add(3) function returning -EACCESS or -EPERM is not documented as part of our stable API promise.

Either we need to update the docs (the manpage) or we need to shift to using one of the approved error codes, -EEXIST seems the most appropriate since the rule is conflicting with the default action/rule.

[pcmoore]

As I look at this I continue to be a bit torn, -EACCESS does make sense in the main libseccomp API for seccomp_rule_add(3) based on what we use it for in other funcs in the libseccomp API. Curious to hear what you think @drakenclimber.

[drakenclimber]

Interesting. For good or for bad, libseccomp proper is consistent with error codes (-EACCES) around the default action. I haven't thought deeply about it, but -EPERM feels more intuitive to me. I could see someone misinterpreting -EACCES as insufficient privileges.

Paul is correct in that we need to fix something (code and/or manpages) in libseccomp. Late on a Friday of a long week, I would vote for returning -EPERM and updating the manpage accordingly. But I don't feel strongly about it... and it's getting late on Friday :)

[kolyshkin]

the seccomp_rule_add(3) function returning -EACCESS or -EPERM is not documented as part of our stable API promise

... yet the commit seccomp/libseccomp@83989be02 I refer to in description says "[t]his is part of our error code cleanup and API promise".

I haven't thought deeply about it, but -EPERM feels more intuitive to me. I could see someone misinterpreting -EACCES as insufficient privileges.

My take on that is, since either EACCES or EPERM can be misinterpreted, let's stick with the one we already have (since 2.5.0), i.e. EACCES. Changing it back to EPERM, it seems, will do more harm than good.

Paul is correct in that we need to fix something (code and/or manpages) in libseccomp.

I agree. Filed seccomp/libseccomp#339 so we can continue discussion in there.

Moving this PR to draft for now.

[pcmoore]

As a quick FYI, seccomp/libseccomp#339 has now been merged upstream.

[kolyshkin]

No longer a draft.

[drakenclimber]

This looks good to me. Thanks, @kolyshkin.

One question regarding your comment in the first commit about switching from fmt.Errorf to errors.New. Should we someday consider going even further and defining a seccomp Error Type (or perhaps multiple Error Types) and propagate that to the user? Then the user could get out of the business of parsing seccomp error strings. Thoughts?

Acked-by: Tom Hromatka <tom.hromatka@oracle.com>

[kolyshkin]

One question regarding your comment in the first commit about switching from fmt.Errorf to errors.New. Should we someday consider going even further and defining a seccomp Error Type (or perhaps multiple Error Types) and propagate that to the user? Then the user could get out of the business of parsing seccomp error strings. Thoughts?

The same outcome can be achieved easily by merely exporting the errors that are now internal.

Say, if we have

var (
	ErrBadFilter = errors.New("filter is invalid or uninitialized")
	ErrDefAction = errors.New("requested action matches default action of filter")
)

a user can merely compare the returned error against these values either directly (no longer recommended since Go 1.13 error wrapping was introduced) or using errors.Is. Something like

if errors.Is(err, libseccomp.ErrDefAction) {
...

For more complicated cases where error message is not fixed (i.e. contains some more context) there are also ways to make it works with errors.Is(), albeit it's a little bit more complicated.

That's a separate issue though (and I will look into it later).

[kolyshkin]

@pcmoore PTAL

[pcmoore]

@pcmoore PTAL

A quick note: please refrain from using "PTAL", I personally find it very annoying. I get notified of all the activity in the repo and I'm well aware there have been updates; if I haven't responded it is because I haven't had the time.

With that out of the way, it has been a busy week with other things, I'll try to get back to this early next week.

[pcmoore]

Thanks for your patience, I just merged this at HEAD 77bddc2.