Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

s/a/notify: implement protocol v5 with metadata tagging parser #15089

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

olivercalder
Copy link
Member

@olivercalder olivercalder commented Feb 14, 2025

Implement version 5 of the apparmor notification protocol, as defined here: https://docs.google.com/document/d/1_WvEM9Qi2Je2Vwulzv5TzHwLJ8ld0W8gTNESZZd9VsY/edit?tab=t.0#heading=h.q2d8zdyargyy

This work is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-32517

Copy link

codecov bot commented Feb 15, 2025

Codecov Report

Attention: Patch coverage is 51.87970% with 64 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@cccee5f). Learn more about missing BASE report.
Report is 233 commits behind head on master.

Files with missing lines Patch % Lines
sandbox/apparmor/notify/version.go 18.75% 26 Missing ⚠️
sandbox/apparmor/notify/message.go 61.11% 14 Missing and 7 partials ⚠️
sandbox/apparmor/notify/strings.go 63.82% 11 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master   #15089   +/-   ##
=========================================
  Coverage          ?   78.04%           
=========================================
  Files             ?     1180           
  Lines             ?   157898           
  Branches          ?        0           
=========================================
  Hits              ?   123232           
  Misses            ?    27002           
  Partials          ?     7664           
Flag Coverage Δ
unittests 78.04% <51.87%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Sat Feb 15 01:48:21 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13339511492

Failures:

Executing:

  • google:ubuntu-20.04-64:tests/main/snapd-snap:lxd

Comment on lines +74 to +83
// XXX: apparmor.KernelFeatures() already has this information, but
// we can't import apparmor here since that would be circular.
data, err := os.ReadFile(protocolFeaturesPath)
if err != nil {
return false
}
features := strings.Fields(string(data))
if !strutil.ListContains(features, "tags") {
return false
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to integrate tagging support checks into the existing AppArmor feature probing, since that will affect the way we generate profiles (to include tags or not). Then we should use the pre-checked apparmor features here, rather than re-writing this kind of thing here.

But as mentioned, sandbox/apparmor imports this package, so we can't import it here. We may need to move the actual feature probing to a dedicated sub-package of sandbox/apparmor and import it from both here and there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant