expose improved kernel logging features through libseccomp-golang #29
Conversation
| #ifndef SCMP_ACT_LOG | ||
| #define SCMP_ACT_LOG 0x7ffc0000U | ||
| #endif |
There was a problem hiding this comment.
Is this the right thing to do here? This is to allow a new libseccomp-golang to build with an older libseccomp which doesn't have SCMP_ACT_LOG support. Is it acceptable to fail the build if the installed libseccomp is not new enough or should I leave this #define here?
There was a problem hiding this comment.
This matches what we're been doing elsewhere - we need to support building against older libseccomp, so defining if it's undefined is necessary. So yes, this is the correct thing to do, though it's not very convenient or good practice in general.
| #ifndef SCMP_FLTATR_CTL_LOG | ||
| #define SCMP_FLTATR_CTL_LOG _SCMP_FLTATR_MIN | ||
| #endif |
There was a problem hiding this comment.
Similar question here regarding what to do if the installed libseccomp is not new enough to know about SCMP_FLTATR_CTL_LOG.
There was a problem hiding this comment.
This should also be correct.
There was a problem hiding this comment.
This ended up not working since SCMP_FLTATR_CTL_LOG is a member of an enum and the C preprocessor isn't able to handle enum members in ifndef conditionals. In the update to this PR, I conditionalized based on the (upcoming) libseccomp version of 2.3.3.
| // GetLogBit returns the current state the Log bit will be set to on the filter | ||
| // being loaded, or an error if an issue was encountered retrieving the value. | ||
| // The Log bit tells the kernel that all actions taken by the filter, with the | ||
| // exception of ActAllow, should be logged. |
There was a problem hiding this comment.
Can you add something in the comment regarding a minimum required kernel version for this working?
There was a problem hiding this comment.
Yes, this is a good idea. There's, unfortunately, now a little uncertainty around which kernel release it'll be present in due to an unrelated issue. I'll update the comment as soon as there's some clarity.
There was a problem hiding this comment.
In the update to this PR, I've added a comment about the minimum API level required (3).
| } | ||
|
|
||
| // SetLogBit sets the state of the Log bit, which will be applied on filter | ||
| // load, or an error if an issue was encountered setting the value. |
There was a problem hiding this comment.
Same as above, please add something about minimum kernel version where this will take effect. And/or minimum libseccomp version.
There was a problem hiding this comment.
In the update to this PR, I've added a comment about the minimum API level required (3).
| } | ||
| } | ||
|
|
||
| func TestLogAct(t *testing.T) { |
There was a problem hiding this comment.
Can we skip this if the libseccomp version we build against is too low to have support? We have a function to grab library version we built against, it should be possible to skip the test if it's detected as too low.
There was a problem hiding this comment.
Yes! Good idea. Unfortunately, it doesn't solve the case where libseccomp is "new" but the kernel is "old" but at least it improves the situation.
There was a problem hiding this comment.
The new API level support in libseccomp allows us to handle the case that I mentioned above. The updated PR now handles all combinations of new/old libseccomp-golang, new/old libseccomp, and new/old kernel.
|
Have a few minor concerns mostly related to tests/comments, but overall looks fine. |
|
@pcmoore Roger that, was going to ask about that. I'll mark this as WIP until we get support merged into the main library. |
mheon
changed the title
tyhicks
force-pushed
the
improved-logging
branch
2 times, most recently
from
e4dd320 to
acabcf2
Compare
|
@mheon @pcmoore Hello! I've updated this PR with API level bindings (in support of seccomp/libseccomp@e89d182) and I've updated the SCMP_FLTATR_CTL_LOG and SCMP_ACT_LOG related patches accordingly for API level support and to match what is nearly merged in seccomp/libseccomp#92. |
| // ActLog permits the syscall to continue execution after logging it. | ||
| // This action is only usable when libseccomp API level 3 or higher is | ||
| // supported. | ||
| ActLog ScmpAction = iota |
There was a problem hiding this comment.
Can you put this after ActAllow? By placing it between ActTrace and ActAllow, ActAllow's constant is renumbered versus current versions of the library
| return verMajor, verMinor, verMicro | ||
| } | ||
|
|
||
| // GetApi returns the API level supported by the system. |
There was a problem hiding this comment.
Can you give a brief description of what API levels there are in the comments for this and SetAPI?
There was a problem hiding this comment.
I sure can. My reasoning for not including API level descriptions in libseccomp-golang was that they may become outdated because developers could easily forget to keep the descriptions updated when new API levels are added. The seccomp_api_get man page is probably going to be the most authoritative location for API level descriptions but I don't know how you feel about pointing to that man page from the function documentation.
There was a problem hiding this comment.
I'd be fine with directing people to a manpage. A link to the libseccomp github (if it's documented anywhere on there) would also be fine. I'd just like to give people a pointer as to what valid API levels look like.
|
Can you add API level and/or library version checks in |
| // Returns an error if the API level could not be set. An error is always | ||
| // returned if the library is older then v2.3.3 | ||
| func SetApi(api uint) error { | ||
| return setApi(api) |
There was a problem hiding this comment.
I don't see us calling this internally (and if we do, the public version has no locking). As such, I don't think we need the internal helper - you can just move the logic out into here.
There was a problem hiding this comment.
Sure! That means that I'd have to move the cgo glue code to handle old versions of libseccomp into seccomp.go. The seccomp.go file doesn't have any cgo glue code like that currently which is why I opted to stick it into seccomp_internal.go. I'll move everything to seccomp.go.
There was a problem hiding this comment.
That is an excellent reason to keep it exactly where it is. Didn't look closely enough, sorry! If it depends on CGo in seccomp_internal.go I'm fine with keeping the helpers.
tyhicks
force-pushed
the
improved-logging
branch
from
acabcf2 to
c2a3b57
Compare
|
@mheon I've addressed all of your feedback. Here are the changes that I made:
|
|
Code LGTM though I'd like to run some tests on my end to verify new functionality (and that we haven't broken earlier library versions). We're also still pending on final support landing in the library. Once that lands, ping me and I'll get this tested and merged. |
|
@mheon thanks! I'll describe the testing that I performed. I've tested with libseccomp-golang's
When I say "new", I mean that the project has been updated with the respective logging patches and, in the case of userspace projects, API level patches. By "old", I mean that API level and logging patches are not applied. All tests pass for each scenario. |
|
@mheon just an FYI that the libseccomp changes all are merged. No changes to my libseccomp-golang branch are needed so it is ready for your testing. Thanks! |
|
Alright, I'll try and get this merged sometime tomorrow then. Thanks! |
tyhicks
changed the title
|
I've removed "[WIP]" from the title so that we don't forget that this PR is potentially ready to be merged. |
|
I am keen to use this new logging feature in one of my projects. Anything I can help with to get this merged? |
|
Sorry for the delay here, it's been a busy few months. I'll get this tested and merged tonight |
|
Tests look good, but I noticed something concerning. Libseccomp just released v2.3.3, which is referenced a lot in this PR as the version that will include SCMP_ACT_LOG support (and API levels). Upstream v2.3.3 does not appear to have either of those, however. @pcmoore What release are those likely to come out in? Can we throw v2.3.4 in there and be safe? |
|
@mheon the changes from Tyler are not simple bug fixes, but new functionality, so it would be the next "Y" release (given the X.Y.Z versioning). Considering that the logging capability in the golang bindings needs to be conditional on the main libseccomp version, and we haven't released a libseccomp update with the logging functionality, it seems premature to add this to the golang bindings right now. FWIW, I'm trying very hard to get a new libseccomp Y release ready, but I'm chasing some nasty bugs at the moment in the src/db.c code. I'm getting very close, but not there yet. |
|
ACK. I'll hold off until you cut a new version with the appropriate defines, then. |
The libseccomp-golang upstream had previously requested that ActLog be placed below ActAllow. This change reflects the request from upstream. seccomp#29 (comment) Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
tyhicks
force-pushed
the
improved-logging
branch
from
c2a3b57 to
5711ac2
Compare
|
I found a bug in this PR that was introduced when I moved ActLog after ActAllow in the list of ScmpActions. I forgot to update actionEnd to be ActLog instead of ActAllow. This prevented ActLog from being used as the default action for a filter. I've incorporated that change into the |
The libseccomp-golang upstream had previously requested that ActLog be placed below ActAllow. This change reflects the request from upstream. seccomp#29 (comment) Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
The libseccomp-golang upstream had previously requested that ActLog be placed below ActAllow. This change reflects the request from upstream. seccomp#29 (comment) Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
This reverts commit eee98e2. The libseccomp-golang maintainer requested that ActLog be placed below ActAllow. Revert the outdated version of this patch so that we can apply the new version that reflects the request from upstream. seccomp#29 (comment) Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
|
Is there any reason this isn't able to be merged in now? |
|
We're still waiting on a 2.4.0 release from the upstream library - the relevant features haven't landed in a released library version yet |
|
Looks like you guys have a 2.4.0 release now 😁 @mheon 🙏 |
|
ping? |
|
@mheon are you waiting on anything else from the main libseccomp library? |
|
Nope, I think this one is set, now that 2.4.0 is released. Sorry for the delay here, been busy with other projects. I have one lingering issue with the version check macros, but otherwise this seems set. @tyhicks If possible, can you push a new version with the version check macros set for 2.4.0 instead? If not, I can handle the changes when I merge. |
|
Works fine for me! |
Add bindings for getting and setting the libseccomp API level which was
added in the following libseccomp commit:
commit e89d18205c7dcd7582f41051cd6389c9b12dfccf
Author: Paul Moore <paul@paul-moore.com>
Date: Thu Sep 21 10:27:38 2017 -0400
api: create an API level construct as part of the supported API
The added tests for getting and setting the API level detect if API
level support is available in libseccomp and tests the behavior of
GetApi() and SetApi() accordingly.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Create a new scmpFilterAttr, filterAttrLog, to represent libseccomp's SCMP_FLTATR_CTL_LOG. A new set of getter and setter functions are created to set the log filter attribute. They are named GetLogBit() and SetLogBit(). Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Update TestFilterAttributeGettersAndSetters() to set the log filter flag and then verify its value. The log filter flag is not tested when libseccomp is not new enough to support API level operations. Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Represent libseccomp's SCMP_ACT_LOG action with ActLog. This action logs before allowing the syscall. Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Add a new test that defaults to ActErrno, allows the syscalls needed to carry out a basic test, and sets the getpid() syscall to ActLog. The getpid() syscall is called before the filter is loaded and once again after the filter is loaded. The test is successful if the return values match. The test is skipped when libseccomp is not new enough to support API level operations or the API level is less than 3. Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
tyhicks
force-pushed
the
improved-logging
branch
from
5711ac2 to
a722f44
Compare
|
@mheon Done! Note that I didn't retest any of it since I no longer have the environment set up to do so. These were all simple text changes and the |
|
I'll run the test suite locally to verify and push the commit to master once that's done |
|
Had some local testing hiccups, but it looks like it was purely environment - the tests were all passing, but failing to write results. Took a bit to sort out, but the tests are passing. Pushed to master. Thanks for your patience here! |
5 participants
This is a pull request that complements seccomp/libseccomp#92. It is not yet ready for merging as there's some pending work left to do for that libseccomp PR to allow applications, such as the libseccomp and libseccomp-golang test suites, to request/force a certain API level. See seccomp/libseccomp#92 (comment) for more context.
The SECCOMP_FILTER_FLAG_LOG filter flag and SECCOMP_RET_LOG action allow applications to have more control over what actions should be logged.
The tests added by this PR pass when run under a kernel that has support for the new filter flag and action and a libseccomp that has been built with seccomp/libseccomp#92. They fail otherwise, pending the API level changes that would allow the libseccomp-golang tests to request a certain API level.