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

Apparmor prompting: store decisions by ID #29

Conversation

olivercalder
Copy link

Store decisions by ID as well as in the previous tree organized by UID, label (now snap, app), permission, and path. This allows constant-time decision lookup, modification, and deletion by ID. Changes to decisions stored by-ID are propagated to the tree, and vice versa, guaranteeing consistency at all times.

More details discussed here: https://warthogs.atlassian.net/browse/SNAPDENG-7613

Decisions-by-ID are written to disk to allow fast startup times, but there are a few things to consider:

  1. If decisions effectively stored in both ways, need to verify consistency between the two at startup (else assume consistency, which is risky)
  2. If decisions only stored in tree, rebuilding the by-id map involves scanning the tree and then consolidating the various leaves into their corresponding decisions (many leaves may correspond to a single decision)
  3. If decisions only stored by-ID, then the tree must be rebuilt at startup, which is O(n), but this maybe isn't bad, since parsing all the json is O(n) anyway... But it is much harder to read/parse the written data on disk (such as for debugging), which maybe isn't a problem either, since only snapd should ever write to it

So perhaps I favor only storing decisions on-disk by-id.

…ptsDB`

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Added `Snap` and `App` information to requests so that prompt clients
do not have to parse apparmor labels, and because that is used in the
`StoredDecision` struct.  Also added `ResourceType` to the request so
that this information can be displayed in the prompt to the user.  The
`newRequest()` function has been modified to parse the label from
apparmor into `Snap` and `App`, and to assign a `ResourceType` of
`"file"` if `msg.Class == apparmor.MediationClassFile` (which is the
only handled case in `newRequest()` at the moment).

Also, a `newStoredDecision()` function has been added which constructs a
new `StoredDecision` struct based on the information from a given
`notifier.Request` and other information which is produced early on
during the `Set()` function.  The purpose of `newStoredDecision()` is to
be a helper function for `Set()` which create decisions with unique IDs
and sets up most of the boilerplate for the decisions.  The
`Permissions` field is left as an empty list to which `Set()` will
append each permission which is actually added to the DB, since any
which are implied by previous decisions are not added.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
…fied

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Previously, only the IDs of modified and deleted decisions were returned
by functions which caused those modifications (`Set()` being the primary
exported one).  These modifications and deletions can occur as the
result of pruning when a new decision overrides a previous.  However,
since stored decisions are modified immediately during pruning when a
corresponding leaf is removed from the allow tree, and the `ById` map is
immediately updated to reflect that change, before the end of the
overarching function call which results in the changes, the original
states of the modified decisions have been list, and any deleted
decisions have been removed from the storage entirely.

This commit changes that.  Now, whenever a stored decision is modified
for the first time as a result of pruning, its original state is deep
copied and stored so that it can be later returned to the caller.

As this is more complicated than simply returning the IDs of changed
decisions, some refactoring was done in an attempt to reduce duplicate
code and increase modularity.  Some helper functions were also exported
for testing.

Still TODO: now that the original state of changes decisions are stored,
use that to roll back changes to other decisions if/when an error
occurs.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
… decision storage -- WIP

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
@olivercalder
Copy link
Author

Regarding the initial comment: I believe the best approach is to store the decisions on disk by ID, and then when first reading from disk, rebuild the tree. Rebuilding the tree should be O(n), since there should be no need for pruning, though the pruning may still occur within particular allow maps (per permission). Effectively fast enough, however.

However, the written decisions first need to be sorted by timestamp so that older decisions do not overwrite newer ones.

The only way I can see to avoid both these problems (pruning slow, sorting slow) might be to either manually disable pruning on load (which is dangerous: what if there is fact a conflict which was not correctly pruned before?) or ordering the entries in each final allow map in some way so that full pruning is not necessary (this is likely slow by itself, and complicated).

If we assume the state of the disk is okay with respect to timestamps and pruning, we can simply disable pruning on first load (likely use a different dedicated code path that is not Set() -> insertAndPrune()). There are not checks for timestamp when doing overwrites, so there is not danger of error here, I believe it is primarily information for the user.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
@olivercalder
Copy link
Author

This PR has been replaced by work in https://github.com/olivercalder/snapd/tree/prompting

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