mirror of
https://github.com/token2/snapd.git
synced 2026-03-13 11:15:47 -07:00
patch-1
264 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Zygmunt Bazyli Krynicki
|
0b52b0eae6 |
many: update apparmor to 4.0.1 (#14150)
* build-aux: update vendored apparmor to 4.0.1 release
Signed-off-by: Alex Murray <alex.murray@canonical.com>
* build-aux: add autoconf-archive to apparmor/build-packages
Unlike the Launchpad tarball, the one from apparmor gitlab tarball
requires this to be present as it is just a snapshot of the git tree,
not a release tarball like those provided by Launchpad.
Signed-off-by: Alex Murray <alex.murray@canonical.com>
* build-aux: remove apparmor parser performance patch
This was already included upstream as part of the 3.1.0 release and
hence is included in the 4.0.1 release which we are now vendoring.
Signed-off-by: Alex Murray <alex.murray@canonical.com>
* build-aux: remove remote patch application logic
They are already included in apparmor 4.x release.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
* build-aux: remove local patch application logic
All local patches are now merged in the 4.x release.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
* cmd/configure.ac: expect apparmor 4.0.1 when building as a snap
Signed-off-by: Alex Murray <alex.murray@canonical.com>
* sandbox/apparmor: use apparmor 4.0 abi with vendored parser
Signed-off-by: Alex Murray <alex.murray@canonical.com>
* sandbox/apparmor: add debug logging when probing parser features
Signed-off-by: Alex Murray <alex.murray@canonical.com>
* sandbox/apparmor: log apparmor_parser version when probing features
This is helpful when trying to debug why certain features may not be supported.
Signed-off-by: Alex Murray <alex.murray@canonical.com>
* tests/main: update for new vendored apparmor 4.0
Signed-off-by: Alex Murray <alex.murray@canonical.com>
* Reapply "i/builtin: allow docker-support to use mqueue (#13738)" (#13765)
This reverts commit
|
||
Jorge Sancho Larraz
|
265b7c44d1 |
sandbox/apparmor: aare exclusion rule generation (#13488)
* sandbox/apparmor: add GenerateAAREExclusionPatterns
This function is generic (and complex) enough to be able to handle all of the
overlapping and wildcard behavior we need in docker-support, and it could also
serve to replace numerous other places in the codebase where we need this sort
of complex behavior. It is a generalization of the existing
aareExclusionPatterns helper, though it's actually unclear if this exact
implementation will currently be able to serve the use case from that helper
directly or if more options/adjustments are needed to enable that use case as
well.
To keep the diff smaller, this patch does not actually change any of the
profiles/interfaces, just TODO's are left for where to use it.
Note that the generated rules are slightly more condensed in terms of number of
rules but significantly more verbose in terms of alternations, not sharing more
of repeated substrings between alternations inside the patterns. This was done
explicitly to keep the generating code simpler and easier to understand, but it
may prove to have performance effects, either detrimental or benevolent but
that should be measured before deciding to make the generation code even more
complex than it already is.
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
* interfaces/docker-support: generate AARE exclusion patterns with helper func
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
* sandbox/apparmor: unexport helper functions
These were not meant to be exported, only the fully generic one is meant to be
exported.
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
* sandbox/apparmor: fix bug mis-sorting capitalized letters in AARE exclude patt
Thanks to Alberto for spotting this :-)
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
* sandbox/apparmor: fix format issues introduced during rebase
* sandbox/apparmor: simplify generateAAREExclusionPatternsGenericImpl
* sandbox/apparmor: add checks for unsupported cases and improve documentation
* sandbox/apparmor: update tests to compare the apparmor binary instead of source
* interfaces/builtin/docker_support: check if userns is supported before adding it to the profile
* interfaces/builtin/docker_support: fix dependencies
* sandbox/apparmor: use placeholders
* i/b/docker_support_test: update TestGenerateAAREExclusionPatterns to use SnapAppSet
* testutil/apparmor: use go crypto/sha1 module instead of system sha1sum command
* {sandbox,testutil}/apparmor: minor format fixes
* move helper to find common prefix to strutil
* add copyright info
* use string builder
* i/b/docker_support_test.go: update accordingly to
|
||
|
Zygmunt Krynicki
|
47bd237b1e |
sandbox/apparmor: refactor parser feature test
The test was particularly expensive as it was testing all the combination of known parser features, 2**N, with an entirely mocked parser written in shell. Replace the test with a per-feature probe detector, a checker for all the recognized features and separate smaller tests for not having any parser and for using the internal parser. Jira: https://warthogs.atlassian.net/browse/SNAPDENG-21937 Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> |
||
|
Zygmunt Krynicki
|
6cf2cfcad7 |
sandbox/apparmor: remove references to ubuntu-core-launcher
Only test code is affected. Jira: SNAPDENG-23247 Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> |
||
Oliver Calder
|
0ff642e82e |
many: include prompt prefix in apparmor rules (#13822)
* features,i/{apparmor,builtin}: include prompt prefix in home interface
If prompting is supported and enabled, include the prompt prefix in
AppArmor rules for the home interface, which will cause AppArmor to send
a prompt when accessing any file in $HOME.
In the future, if other interfaces include the ###PROMPT### prefix in
their rule snippets, this will also be handled accordingly.
At the moment, the status of prompting support is checked whenever the
AppArmor backend prepares profiles. This is okay, since AppArmor support
for prompting depends on kernel and parser features, which are only
probed once after snapd starts. However, to ensure that the same
supported value is used even if that were not the case, and in case we
wish to only use the prompt prefix for some snaps or interfaces, we may
wish to embed whether to use the prompt prefix in the AppArmor
Specification instead.
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* features: adjust unsupported messages when checking apparmor features errors
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* interfaces: add prompting status to system key
Include whether prompting is supported and enabled as a single field in
the system key. This way, if `(supported && enabled)` changes, security
profiles will be regenerated when snapd starts up.
Currently, prompting support only changes when the AppArmor kernel or
parser features change, and profile regeneration is the only other place
where it is checked whether AppArmor prompting is supported and enabled.
Thus, including whether prompting is supported and enabled in the system
key ensures that security profiles are regenerated when necessary during
snapd startup, and only when necessary (e.g. not if support changed but
prompting flag remained disabled nor if flag changed but prompting
remained unsupported).
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* o/c/configcore: restart snapd when prompting value changes
When the prompting flag changes and the result entails that security
profiles should be regenerated, restart snapd to do so.
This is required iff prompting is supported and the experimental
apparmor-prompting flag changes -- if prompting is not supported,
prompting can't be used, so no need to regenerate profiles. Importantly,
prompting support is based entirely on the available AppArmor kernel and
parser features, and these are only probed once during snapd startup, so
prompting support cannot change (under the current implementation)
except when snapd restarts.
Since `(supported && enabled)` is part of the system key, and a restart
is only triggered if prompting is supported and the flag value changes
(which is equivalent to `(supported && enabled)`, since the supported
value cannot change while snapd is running), restarting after the flag
has changed causes the system key to be different, and thus to trigger a
security profile regeneration, as desired.
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* interfaces,o/ifacestate: set prompting in apparmor spec
Rather than checking whether AppArmor prompting is supported and enabled
whenever the AppArmor backend is processing a snippet, instead include
that precomputed value in the Specification itself, and place it there
via `buildConfinementOptions`. This way, any spec created with the same
`confinementOptions` will make the same decision as to whether to
include prompt prefixes on relevant rules.
Currently, `buildConfinementOptions` simply checks whether prompting is
supported and enabled via the methods on `features.AppArmorPrompting`,
but ideally, this value would be looked up from either the system key
or by checking whether the prompting listener is running. It remains to
be seen how the value computed as part of the system key can be
guaranteed to be the same as that used elsewhere, either in
`buildConfinementOptions` or when deciding whether to start the
listener.
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* o/c/configcore: request snapd restart whenever prompting flag changes
Previously, a snapd restart was only requested when the status of the
"apparmor-prompting" experimental feature flag changed and prompting was
supported. However, since prompting support is dependent on AppArmor
kernel and parser features which are probed only once during startup,
and systems which do not use vendored AppArmor may have had an update to
the system AppArmor package which newly supports AppArmor prompting, it
is safer to request a restart of snapd to re-check for prompting
support.
This way, if one is enabling prompting for the first time on a system
without prompting support, they can have snapd installed, update their
kernel or apparmor installation to support prompting, and then set the
prompting flag to enable prompting without needing to manually restart
snapd.
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* interfaces: support optional trailing space after ###PROMPT###
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
i/apparmor: move promptReplacer definition to before its use
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* tests: add test of restart behavior when setting experimental.apparmor-prompting
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* tests: refactor prompting test to reset failed count and safely check for restarts
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
tests: add shellcheck exception for apparmor prompting flag restart test
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* tests: check that snapd PID != 0 and use snap changes to wait for feature change to complete
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* tests: check for start-limit-hit before calling reset-failed
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* tests: add ubuntu core to apparmor prompting flag restart test
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* tests: check apparmor-prompting value after setting it unchanged
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* Revert "tests: check for start-limit-hit before calling reset-failed"
This reverts commit bea68516c3287fa44d6718f0794484746ae99ac5.
* tests: check systemd start-limit-hit when apparmor-prompting flag changed
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* o/configstate/configcore: add unit tests for doExperimentalApparmorPromptingDaemonRestart
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* i/builtin: add missing prompt prefix and adjust test
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* i/apparmor: add test for prompt prefix substitution
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* many: pass prompting value into system key functions
As such, we now precompute whether prompting is supported and enabled in
`InterfaceManager.StartUp()`, store it in the `InterfaceManager`
instance, and pass it into the call to `WriteSystemKey()`.
Additionally, we make `buildConfinementOptions` a method of
`InterfaceManager`, thus eliminating the need to check within the system
key functions whether prompting is supported and enabled.
However, there remains a problem that `snap run` calls
`SystemKeyMismatch()`, which previously invoked
`apparmor.ParserFeatures()` via `AppArmorPrompting.IsSupported()`, and
now calls `AppArmorPrompting.IsSupported()` directly and passes the
result into `SystemKeyMismatch()`. In either case, we really want this
to be avoided if at all possible, since `snap run` does not have access
to the cached value from the `InterfaceManager`, and thus must invoke
the `apparmor_parser` binary to check parser features whenever we want
to run any snap.
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* many: don't probe parser features when checking system key mismatch
Since `snap run` calls `SystemKeyMismatch()`, we want to avoid needing
to call `AppArmorPrompting.IsSupported()` if at all possible, since this
calls `apparmor.ParserFeatures()`, which executes the `apparmor_parser`
binary. We can and should call `AppArmorPrompting.IsSupported()` when
writing the system key, but not when checking for a mismatch.
The system key written to disk should correctly hold the list of kernel
and parser features, the parser mtime, and whether or not prompting was
previously supported and enabled. We can check whether apparmor parser
features have changed by checking the parser mtime, without needing to
probe parser features -- this optimization is already used by
`SystemKeyMismatch()`. If we knew whether prompting was previously
supported (regardless of whether it was enabled), then so long as the
parser and kernel features are unchanged, we know that prompting support
is also unchanged.
Thus, if we add a second prompting-related field to the system key, this
one storing whether prompting is supported (ignoring enabled), we can
check if prompting support is unchanged without needing to call
`AppArmorPrompting.IsSupported()`.
Furthermore, `SystemKeyMismatch()` is the function in question, and if
there is any mismatch detected, it should return such as soon as
possible, regardless of what the mismatch is. Therefore, if we know that
either kernel or parser features have changed, then we can immediately
return that there is a mismatch, and we don't need to check whether
those feature changes affect prompting support.
Therefore, the new cases which we must worry about when checking for a
system key mismatch are the following, when all other system key fields
are unchanged (note that prompting must be supported in order to be
supported&&enabled):
1. supported: F, supported&&enabled: F, newFlag: F, mismatch: F
2. supported: F, supported&&enabled: F, newFlag: T, mismatch: F
3. supported: T, supported&&enabled: F, newFlag: F, mismatch: F
4. supported: T, supported&&enabled: F, newFlag: T, mismatch: T
5. supported: T, supported&&enabled: T, newFlag: F, mismatch: T
6. supported: T, supported&&enabled: T, newFlag: T, mismatch: F
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* interfaces: fix test string formatting
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* o/configstate/configcore: adjust prompting-related comments
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* tests: increase prompting check_snapd_restarted timeout and add systemd show
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* tests: reset start limit when checking if snapd restarted after prompting change
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* many: add system key extra data to hold prompting enabled status
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* many: only store one apparmor prompting value in system key
When checking for a system key mismatch, use the stored AppArmor
parser features from the system key on disk (along with the kernel
features from the newly-generated key) to check whether prompting is
supported, and AND that with the `AppArmorPrompting` value passed in
with the `SystemKeyExtraData`. If the kernel or parser features have
changed, the system key will be a mismatch anyway, so it is perfectly
safe to use the existing parser features to check for prompting support.
As such, the functions to check for prompting support have been moved
from `features` to `sandbox/apparmor`, and the support check has been
separated from the call to get `ParserFeatures()` and
`KernelFeatures()`, so that the values from the system key can be passed
in instead of invoking those functions.
Using the system key's stored parser and kernel features, there is no
need to save whether prompting is supported as part of the system key,
simplifying the key and the logic used to set the prompting value.
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* tests: explicitly install jq in apparmor-prompting-flag-restart test
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* many: consolidate checks for apparmor prompting support
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* interfaces,s/apparmor: use features struct when checking prompting support
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* tests: improve logging in apparmor-prompting-flag-restart test
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* tests: fix prompting flag restart test on core18
For some reason, when snapd fails due to start-limit-hit on core18, the
snapd.failure.service starts and acquires the state lock, thus
preventing snapd from successfully becoming "active" again and leaving
it retrying at "activating". It is unclear why this happens on core18
and not elsewhere.
As a fix, when resetting the start limit, stop snapd.failure.service
manually to ensure that snapd can successfully start.
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
---------
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
|
||
Alfonso Sánchez-Beato
|
56f64edaf7 | many: add options to the logger to be able to enable internally debug traces | ||
|
Oliver Calder
|
bf746388b1 |
s/apparmor: expose entries in policy/permstable32 as kernel features
Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor: handle whitespace in permstable32 when parsing features Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor: sort kernel features explicitly Now that we are probing for features in policy/permstable32 after getting features from the directory structure, the features list must be explicitly sorted. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> |
||
Andrew Phelps
|
c679f43577 |
s/cgroup, systemd: escape systemd unit names in CreateTransientScopeForTracking (#13763)
* systemd: add function that implements "systemd-escape" in addition to already existing "systemd-escape --path" * s/cgroup: escape created unit name in CreateTransientScopeForTracking With the addition of component hooks, we'll have unit names that include a '+', like 'snap.snapname+comp.hook.install'. This causes systemd to complain that the unit isn't properly escaped. On the command line, systemd-run will properly escape this for you (with a warning), but the dbus API doesn't do that. * s/naming: teach ParseSecurityTag to handle tags from component hooks * Revert "systemd: add function that implements "systemd-escape" in addition to already existing "systemd-escape --path"" This reverts commit 0521600ec8fa785b69d2b7a85fa8da9be4938a5a. * systemd: add functions for escaping security tags to valid systemd unit names We must at least partially escape unit names that are created from security tags, since they may potentially contain '+' characters from snap components. Since we already use unit names with '-' in them, we cannot simply use a reimplementation of systemd-escape. This is because '-' is escaped by systemd-escape. Note that '-' is a valid character is a unit name, since it is used as the replacement for the '/' character by systemd-escapes. Thus, we have our own functions for converting a security tag to a unit name, and the inverse. These functions only escape the '+' character that appears in security tags. * s/cgroup: use new conversions from security tags to unit names, and the inverse * systemd: update doc comment on UnitNameFromSecurityTag Co-authored-by: Maciej Borzecki <maciek.borzecki@gmail.com> * s/naming: add ComponentName method to HookSecurityTag interface * systemd: split tests for UnitNameFromSecurityTag and SecurityTagFromUnitName * s/naming: add test for invalid snap instance that is a part of a component * s/naming: refactor ParseSecurityTag to clarify that components cannot have apps yet * systemd, s/cgroup: rename security tag and unit name conversion functions for clarity --------- Co-authored-by: Maciej Borzecki <maciek.borzecki@gmail.com> |
||
|
Zygmunt Bazyli Krynicki
|
d78bbedcb1 |
many: generalize wording of NFS workaround (#13758)
Ahead of introduction of CIFS workaround, generalize the names so that we use more general language rather instead of focusing on NFS. As a special exception the externally visible wording related to NFS is kept intact in two places: - The apparmor "nfs-support" file name - The system key "nfs-home" key. From points of view this is all an elaborate internal rename that should nto be observable outside of snapd, apart from log messages that may no longer speak of NFS but of remote file systems. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> |
||
Miguel Pires
|
79c5ac14b2 |
many: remove usages of deprecated io/ioutil package (#13768)
* many: remove usages of deprecated io/ioutil package Signed-off-by: Miguel Pires <miguel.pires@canonical.com> * .golangci.yml: remove errcheck ignore rule for io/ioutil Signed-off-by: Miguel Pires <miguel.pires@canonical.com> * run-checks: prevent new usages of io/ioutil Signed-off-by: Miguel Pires <miguel.pires@canonical.com> --------- Signed-off-by: Miguel Pires <miguel.pires@canonical.com> |
||
|
Zygmunt Bazyli Krynicki
|
8a6ec07fa2 |
sandbox/apparmor: detect but ignore apparmor 4 (#13740)
Due to issues with incorrect behavior to mediate:
stat /dev/mqueue
For applications governed by the profile that allows it via
mqueue,
We cannot yet use apparmor 4, even if one is supported on the host. This does
impact userns mediation but it is better to have the old mediation and not
break snaps, than to have some new mediation in some cases and some unexpected
mediation in other cases.
Once the mqueue, issue is identified and we have updated bundled apparmor to a
stable release of apparmor 4, this patch can be reverted.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
|
||
|
Zygmunt Krynicki
|
c588146fb9 |
sandbox/apparmor: prefer apprmor 4.0 ABI if available
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> |
||
|
Zygmunt Krynicki
|
b5da22b4bb |
sandbox/apparmor: use host abi 3.0 if present
When using apparmor_parser from the host, pin the supported policy to ABI 3.0 if one is present on the host. This way even on distributions with Apparmor 4.0, we get the behavior of 3.0 and the meaning of our apparmor profiles stays the same. At the same time apparmor 2.0 distributions remain unchanged. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> |
||
Andrew Phelps
|
0c04d6f39a | s/a/n/listener: correct size of Class and Op in msgNotificationFile to match fields in msgNotificationOpKernel | ||
Maciej Borzecki
|
7b09964711 |
sandbox/cgroup: refactor and cleanup, fix path tracking and watch removal (#13508)
This is an bunch of fixes and refactors for the cgroup monitoring code. The main issue addressed is with tracked path removal. We have previously added references to the leaf paths internally, but the actual watch was established using the parent path, for which the reference was never updated. As a result if all leaf paths were gone, we did not clean up the watch on the parent path. While at it, change the structure internally to use a hash map in the group to watch to simplify bookkeeping. Next, added support for the context and a way to cleanup and close the watcher in a reasonable manner. A bunch of improvements to the tests, so that we are no longer guessing and sleeping hoping to observe the right events. Instead provide an observer which the tests can hook to and inspect the state as needed. * sandbox/cgroup: fix handling of watched paths, ensure proper cleanup, refactor When setting up watch for a given path, we have kept a reference to the leaf node, but the watch was really configured for the parent path. However, the references to the parent path were not being properly tracked. This resulted in the watch of the parent path never getting cleaned up. While at it, to keep the bookkeeping simple, since paths are reference counted, we can keep them in a hash map instead of a list for easier manipulation and lookup. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com> * sandbox/cgroup: better handling of watcher errors Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com> * sandbox/cgroup: support context in the watcher Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com> * sandbox/cgroup: add internal observer to facilitate testing Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com> * sandbox/cgroup: update tests Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com> * sandbox/cgroup: debug logs Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com> * sandbox/cgroup: document error handling quirks Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com> * sandbox/group: drop a pointless test Drop a unit test which does not make sense since we cannot confirm that recovery handling inside the goroutine was successful. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com> * sandbox/cgroup: improve tests Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com> * sandbox/cgroup: address review comments Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com> --------- Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com> |
||
Zeyad Gouda
|
2735392add |
sandbox/cgroup: add tracking helper to confirm transient scopes
This is preparation for future work related to refresh-app-awareness UX improvements. Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com> |
||
|
Oliver Calder
|
e639922822 |
s/apparmor/notify/listener: fix concurrency test on slow single-core machines
Signed-off-by: Oliver Calder <oliver.calder@canonical.com> |
||
|
Oliver Calder
|
baa5a285ff |
s/apparmor/notify/listener: do not send auto-denies when listener closed
The kernel should be responsible for sending back deny responses to applications if the listener is closed before it sends back a response to the kernel. Better yet, if a new listener is registered, the kernel can resend previous unanswered requests to the new listener, so there is no signal loss. We still want `Close()` to wait until all tracked goroutines terminate before closing the notify file, since the listener may have just received a reply that we might as well finish up passing along to the kernel. And, that way those tracked goroutines won't be hit with additional errors writing to a closed file, and `Run()` and `Close()` won't return until every thread from the listener is closed down. This greatly simplifies testing and to a lesser extent, the listener itself. It will yield much greater flexibility in the future, however. As part of this change, we also revert to requests being sent over `Reqs()` synchronously, rather than in a goroutine. This was done for a few reasons: 1. We don't need to read the next request from the kernel if there's not a listener ready to receive it --- nothing wrong with just letting the next request sit after the kernel writes it. 2. Sending requests synchronously guarantees their order. 3. We don't need to send an auto-deny on listener close (kernel does this for us) so no need to always call `waitAndRespondAaClassFile` even if the listener is closing before the request is received, and we can instead return early. 4. If we read in a request from the kernel, nobody reads it from `Reqs()` before it times out in the kernel (which sends auto-deny), and then something starts reading from `Reqs()`, that request will still be sent if that send happens in a tracked goroutine, even though the operation has already been denied by the kernel --- this is unavoidable for the first request received from the kernel while there's no reader on `Reqs()`, but we can at least avoid it for any subsequent requests that the kernel might try to send while there is still no reader on `Reqs()`. 5. This reduces epoll syscalls to get data that we may not use, in the case that there's no reader on `Reqs()`: furthermore, the way `epoll.Wait()` is implemented internally, it sends repeated epoll requests with a (rather long) timeout so it can check if the epoll instance has been closed and stop retrying. 6. If a new listener is registered, the kernel will try to send requests to that listener instead, and in the future the kernel may re-issue past requests from listeners which were closed without responding to the new listener --- at the moment, though, this is not enabled, so we may as well read as few requests as is reasonable from the kernel so that they can be issued to future listeners rather than be responded to with an auto-deny from the kernel after the original listener disconnects. The one drawback I could see is that if something finally starts reading from `Reqs()`, the listener has to do all the work to read subsequent requests from the kernel and create waiting goroutines starting at that point, when it could have instead been pre-doing the work before the reads to `Reqs()` begins. But that also would create a bunch of waiting goroutines running concurrently, which may never receive a reply and will only close when the listener closes. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: make concurrency test race-free Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: fix concurrency test mocked function restore Signed-off-by: Oliver Calder <oliver.calder@canonical.com> |
||
|
Oliver Calder
|
37ee1094ce |
s/apparmor/notify/listener: stop epoll waiting when error occurs
Previously, unless the listener was explicitly closed, an internal error would not trigger the listener's epoll instance to be closed, and thus would not unblock the epoll wait call. This commit fixes this behavior by having `Run()` wait for any error to occur, rather than for all goroutines to return, before calling `Close()`. That's really all it took. Additionally, fixes listener `Close()` method so in all cases it does not return until all tracked goroutines spawned by the listener return. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: improve tests, comments, and mocking Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: simplify mocking of `os.Open` Since no tests now use the sockets created by the existing `MockOsOpenWithSockets`, don't bother returning them. Furthermore, don't bother creating a socket pair, just make a single socket, wrap it in a file, and return it to the caller of `os.Open()`. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: add test for epoll mechanism Signed-off-by: Oliver Calder <oliver.calder@canonical.com> |
||
|
Oliver Calder
|
53db19fad3 |
s/apparmor/notify/listener: properly handle if Reqs() never read
Note: there is one side effect of this change: the order of requests sent over `Reqs()` is no longer guaranteed to match the order in which they were received from the kernel. This guarantee should not have been relied on in the past, but now it cannot be relied upon. This should be fine in practice, but it makes testing a bit more difficult. Previously, the listener's main run loop blocked on sending new requests over the `Reqs()` channel, and thus would not receive new requests from the kernel until the listener's caller read from `Reqs()`. This presented two problems: 1. If the listener was closed without a thread reading from `Reqs()`, then it would be stuck in that send and never return, and thus both `Run()` and `Closed()` would hang. 2. In that case, the "wait and respond" function is never called, so a denial is not sent back to the kernel when the listener is closed. 3. If the kernel writes another request to the notify socket while the listener is trying to send a previous request, that request goes unread and unanswered. This commit solves both these problems: 1. Writing the new request to `Reqs()` now occurs in its own (tracked) goroutine, and selects on the listener's tomb dying, so the main loop is not blocked waiting on a reader for `Reqs()`. 2. The new request is now passed into a "wait and respond" function immediately (even if never received over `Reqs()`), so the default send denial on close/error works as expected. 3. The listener continues to read requests from the kernel, even if there is no reader listening on `Reqs()`, and thus default denials are sent after close/error for any new request created since the caller stopped reading from `Reqs()` as well. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> |
||
|
Oliver Calder
|
1b9343a322 |
s/apparmor/notify{,/listener}: add support for multiple requests at once
The kernel may wish to send multiple apparmor messages in a single ioctl buffer. This commit adds handling for this via new methods in the notify message code. Additionally, this exposes a method to get the length of the first message in a data slice. Since the existing concurrency tests use write syscalls on a file descriptor, this new handling allows the concurrency tests to pass now as well, even if multiple requests are written to the notify file descriptor before they are read by the listener again. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> |
||
|
Oliver Calder
|
5fdf29552b |
sandbox/apparmor/notify/listener: gracefully handle listener terminating
Whether an internal error occurs or the listener is explicitly closed, it is important that all tracked goroutines terminate gracefully. This is a squashed commit of many incremental changes which work towards that common goal. Additionally, there are a few usage changes, most notably the use of an explicit `Reply()` method in place of simply writing to the reply channel in the request. Below is the full commit message history of the changes. sandbox/apparmor/notify/listener: stop waiting when tomb is dying Rather than block indefinitely while waiting for a reply over the `yesNo` channel, select on the tomb `Dying()` method as well, and automatically send a deny message back to the kernel if the tomb is dying. Thus, the waiting goroutine can quickly return. Also includes some small naming cleanup and test improvements. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> sandbox/apparmor/notify/listener: export read-only channels for requests and errors Signed-off-by: Oliver Calder <oliver.calder@canonical.com> sandbox/apparmor/notify/listener: reorganize code for clarity Signed-off-by: Oliver Calder <oliver.calder@canonical.com> sandbox/apparmor/notify/listener: improve thread safety The following are intermediate commits which generally improve thread safety, first be modifying `osutil/epoll`, but later reverting that approach, though preserving some of the other improvements. See the commit messages below for details. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> osutil/epoll: added `Wait{,Timeout}WithContext` functions Epoll's `Wait()` function makes an epoll syscall, which cannot be cancelled except by passing in a timeout. `WaitTimeout()` provides this functionality, but it may be the case that the syscall should be cancelled automatically when a context terminates. This commit provides the `WaitWithContext()` and `WaitTimeoutWithContext()` functions, which accomplish this. Note: the `WaitTimeout{,WithContext}` functions do not use a context internally to implement the timeout; instead, they continue to use the timeout functionality of the epoll syscall. This is to differentiate an epoll timeout (no error, treated as a sort of "try epoll") from a context timeout (returns a `context.DeadlineExceeded` error). Signed-off-by: Oliver Calder <oliver.calder@canonical.com> sandbox/apparmor/notify/listener: make listener thread safe Notable changes: - Both `Run` and `Close` block until all spawned goroutines terminate - `Run` should only be called once, and an error is thrown otherwise - `Close` may be called before or after `Run` - If `Close` is called before `Run`, `Run` will immediately terminate - Atomics used to keep track of the listener status and ensure `Run` is only called once. Internally, the status of the listener must be one of: - Ready - Running - Closed Atomics are used to ensure that only the following status changes are allowed, and each must only occur at most once: - Ready -> Running (via the `Run` function) - Ready -> Closed (if `Close` called before `Run`) - Running -> Closed (if `Close` called after `Run`) The listener can now be run, closed, or encounter an error in any order and on any number of concurrent threads without risk of panic. Errors are now safely thrown to enforce the above status rules. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> sandbox/apparmor/notify/listener: close epoll instead of using context Passing a context into a function which invokes an epoll wait syscall is difficult without signal loss, since the syscall returning some events after the context has been cancelled results in those events being lost, unless they are otherwise stored and returned on the next wait call. Thankfully, this is not necessary. The only time we would want a listener to use a context to cancel an epoll wait syscall is when the listener is dying or being closed. In this case, we can simply close the epoll instance to cause all `Wait()` calls to return immediately. Any signal loss is irrelevant since the epoll instance has itself been closed, and we must open a new instance in order to wait again. The listener `Close()` function can then, after killing the tomb and closing the epoll instance (which ends the `Run()` loop), safely wait on the listener tomb until the remaining request waiters send deny responses back over the notify socket and then terminate. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> Revert "osutil/epoll: added `Wait{,Timeout}WithContext` functions" This reverts commit a7a3f1ce0bec771c9bf05a2c89d8731d500197d3. As discussed in the commit message for the previous commit: * c3681a5aa233c714c030cc063575e744263c58cc it is dangerous to mix contexts with the epoll wait syscall. There is another simpler solution which does not require using contexts with epoll, and that solution is preferred. Substantial work would need to be done to the epoll package to ensure that signals are not lost when an epoll wait syscall returns events after the context associated with its invocation has been terminated, as these events will not necessarily be returned again by the kernel on subsequent epoll wait syscalls, and thus must be stored internal and returned on future calls to the epoll `Wait()` functions. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> sandbox/apparmor/notify/listener: small tweaks following reviews Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: simplified error reporting and request replies Replace `Errs()` with `Err()`, which returns an error channel from which reads block until the internal tomb is dead, after which time the error with which the tomb was killed is sent, and the channel is closed. This simplifies the listener and eliminated the need for a dedicated `errs` channel. Additionally, replaces the `YesNo chan bool` with a `Reply()` method which is responsible for writing any response (`interface{}`) back to the waiter goroutine for that request. This allows the waiter to cast the response according to the apparmor mediation class associated with the request, and makes adding or adjusting the handling of different mediation classes in the future much easier. Also, it ensures that the reply channel may only be read internally, and that each request may receive at most one reply. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: log activity with Debugf instead of Noticef Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: remove `Err()` and status errors This is a squash commit of two commits related to `Err()`. See below for details. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: make `Err()` return an error, not a channel Previously, `Reqs()` and `Err()` both returned channels, with the expectation that the caller would `select` on them both. However, there was a race between the channel backing `Reqs()` closing and the error being sent over the channel returned by `Err()`. Now, the caller is expected to read requests from `<-Reqs()` until it closes (returns `nil`), after which point `Err()` returns the reason that the listener stopped. This is either `ErrClosed`, indicating that the listener was explicitly closed via the `Close()` method, or an internal error message. In general, `Err()` can now be used much like `tomb.Err()`, in that it returns a status (`ErrReady`, `ErrRunning`, `ErrClosed`) if an internal error has not occurred, or the first internal error which occurred and caused the listener to stop. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: remove `Err()` and status errors Don't expose a dedicated `Err()` function which returns a status if the listener has not yet been closed or experienced an error, and otherwise returns the reason the listener stopped running. The latter is already returned by `Run()`, and since the requests channel is closed when an error occurs or the listener is explicitly closed, there's no need to have another method which the caller can use to check if they should stop waiting on `Reqs()`. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: expose class and check type of reply Include the apparmor mediation class associated with the request in the request struct itself, so clients can decide how to parse the requested permissions and reply in the correct format. Then, check the type of the reply received over the request's reply channel to make sure that the client sent the correct type. If not, send back a deny response to the kernel. Adds tests for this new behavior, and streamlines existing tests a bit to reduce duplicate struct declaration code. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: validate response type in `Reply` Previously, if `Reply(response interface{})` was called with a response type which was not compatible with the mediation class of the request, a deny response was sent to the kernel. Now that requests contain information about their mediation class, it is possible to check that the response type is correct, and if not, return an error to the caller of `Reply()` indicating as much. Hopefully, the client will later send back another response, this time of the correct type. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: improve test robustness Importantly, `Close()` now sets the return value of `Run()` to be `ErrClosed` if no other error occurs. Additionally, `TestRunConcurrency` is flakey since the listener cannot handle multiple requests in the same ioctl buffer. At the moment, the kernel only ever sends one request at a time, so this should not occur. However, it is worth adding safe handling for this in a future commit. Reading requests and writing responses to the same file descriptor which is being being tracked by epoll doesn't make much sense, as writing back a response should trigger another epoll event, which is not desirable. Thus, this commit replaces all uses of `MockNotifyIoctlWithReadWrite` with `MockNotifyIoctlWithRecvReadSendChan`, which receives some fixes as well (was broken due to incorrectly truncating the buffer to the minimum template size). In tests where the notify file descriptor should not be read/written to, `MockNotifyIoctl` has been used with an explicitly minimal function which performs additional checks and indicates that there is no assumption of the behavior of reading/writing data in those tests. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: remove redundant parentheses in test Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: improve tests robustness some more This commit includes the generic test improvements from an old commit which has been split in two during a rebase, formerly titled: s/apparmor/notify{,/listener}: add support for multiple requests at once Since the existing concurrency tests use write syscalls on a file descriptor, this new handling allows the concurrency tests to pass now as well, even if multiple requests are written to the notify file descriptor before they are read by the listener again. Additionally, tests are generally cleaned up a bit, and new ones added for the new functionality. Since the read/write to socket-based tests were a bit flakey with sockets being opened and closed repeatedly in some tests, a new more robust ioctl (and epoll) mocking function was added which uses channels for all communication, ensuring distinctness of messages and synchronization between the listener and the fake sender. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: improve test reliability No more sockets and syscalls. These are too flakey in concurrent tests. Instead, mock these and use channels internally. Additionally, fix TestRegisterOverridePath to restore the path after the test, thus allowing repeated runs. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> |
||
|
Oliver Calder
|
78a1709db5 |
s/apparmor/notify/listener: remove pointer to parent listener from request struct
Signed-off-by: Oliver Calder <oliver.calder@canonical.com> |
||
|
Oliver Calder
|
ca402b8b0c |
s/apparmor/notify/listener: allow originally allowed perms even when request denied
Signed-off-by: Oliver Calder <oliver.calder@canonical.com> |
||
|
Oliver Calder
|
a7f8f90058 |
s/apparmor/notify/listener: add listener package
Add the apparmor prompting listener, formerly named notifier. Initial commit for apparmor prompting notifier Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> Signed-off-by: Oliver Calder <oliver.calder@canonical.com> Do not wait for .notify writablility events We are only interested in the reading side, which corresponds to incoming requests. This surprisingly fixes the multi-request issue that I was experiencing before, allowing us to process requests asynchronously. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> Signed-off-by: Oliver Calder <oliver.calder@canonical.com> Process requests asynchronously This allows us to wait for multiple responses from userspace and still process incoming kernel message. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> Signed-off-by: Oliver Calder <oliver.calder@canonical.com> Move the Notifier to a dedicated package The Notifier is a high-level abstraction that does not leak any apparmor implementation details directly. As such it should be in a different package. Some methods were adjusted to better accommodate the new package name. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> Signed-off-by: Oliver Calder <oliver.calder@canonical.com> Don't log request/responses Those are very low level and they don't add anything at the current stage of development. Instead provide a quieter but more readable interface. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com> Signed-off-by: Oliver Calder <oliver.calder@canonical.com> Merge pull request #14 from snapcore/feature/decode-file-perms Add opaque Permission attribute to notification requests Signed-off-by: Oliver Calder <oliver.calder@canonical.com> notifier: improve error messages Signed-off-by: Oliver Calder <oliver.calder@canonical.com> Merge pull request #13 from snapcore/feature/file-perm Model file permissions described by notification messages Signed-off-by: Oliver Calder <oliver.calder@canonical.com> many: rework for new RegisterAgent based API Signed-off-by: Oliver Calder <oliver.calder@canonical.com> prompting: set all prompt replies to "no-cache" for now Signed-off-by: Oliver Calder <oliver.calder@canonical.com> prompting/notifier: added `tomb` parameter to `Run()` so it can be safely terminated Signed-off-by: Oliver Calder <oliver.calder@canonical.com> sandbox/apparmor/notify: renamed `notifier` to `listener` as sub-package of `notify` Signed-off-by: Oliver Calder <oliver.calder@canonical.com> sandbox/apparmor/notify/listener: improved unit tests Signed-off-by: Oliver Calder <oliver.calder@canonical.com> sandbox/apparmor/notify/listener: improved unit tests for error handling Signed-off-by: Oliver Calder <oliver.calder@canonical.com> s/apparmor/notify/listener: reformatted and renamed listener code Signed-off-by: Oliver Calder <oliver.calder@canonical.com> |