i/prompting/requestprompts: add package to manage outstanding request prompts

[olivercalder]

This is part of ongoing work to support AppArmor Prompting. When a request is received from the listener, a prompt is created with information which is provided to the user so that they may decide whether to allow or deny the requested action. This package manages the prompt database and individual request prompts.

This task is tracked internally by SNAPDENG-8329

[MiguelPires]

Why does AvailablePermissions not matter here?

[olivercalder]

AvailablePermissions are directly populated based on the interface of the prompt with which the constraints are associated. If the interface for two prompts is the same, the AvailablePermissions in the constraints will be the same, and two constraints should never be checked for equality unless they are associated with the same interface. So I don't really think it's necessary or beneficial to check their equality.

[olivercalder]

Should add note to the comment clarifying that replies are not sent to the listener when the prompts are cleaned up -- the listener should be closed separately by the caller of CleanUp and it is in charge of sending denies for outstanding prompts.

[pedronis]

did a first pass

[pedronis]

is the goal to start from zero after each reboot? something under /run/snapd might be more appropriate for that nowadays

[bboozzoo]

Something like /run/snapd/prompts/<stuff-here> looks appropriate. We don't explicitly set RuntimeDirectory in systemd, but rahter create one during startup.

[olivercalder]

I'll put the max ID in /run/snapd/request-prompt-max-id. For rules (rather than prompts) we'll want to save some state more permanently across reboots (but still not in state.json), so I don't think it makes sense to have run/snapd/prompting/max-{prompt,rule}-id or something of the sort here.

[pedronis]

AddOrMerge and this could share a struct for some of the arguments here

[olivercalder]

User, snap, and interface are shared across almost all prompting-related things, I'll create a prompting.Metadata struct with these fields.

The remaining ones left out of the Metadata or Constraints structs would then be outcome, lifespan, duration. I think that's fine, we can revisit later if need be.

[pedronis]

why does this notify first and reply after while Reply does the reverse? if that wasn't the case maybe the two could also share some code ?

[olivercalder]

I think there were two reasons for the differences:

1. Here, we continue if some but not all of the permissions of the prompt are satisfied by the new rule, in which case we want to tell the clients to re-check the prompt details via the prompting API. We need to record the notice before the continue, so it can't be any closer to the sendReply call.
2. In Reply, we're sending a user-given reply to the listener. In the off-chance that this results in an error (could be possible in the future if we pass more than a bool as the reply), we don't actually want to resolve the prompt, since the listener request itself has not yet been correctly resolved. So we return an error before removing the prompt of recording a notice. But we shouldn't encounter errors when replying based on a new rule, since we're theoretically in control of the reply format more. But really we should be fully vetting user replies before sending anything to the listener, so this is potentially a moot point.

Given these considerations, I think the best we could do is move the notifyPrompt call to just before the listener req reply loop in Reply, and assume there will be no errors. Or leave the errors return in place in the off-chance that it occurs -- there's no harm in re-recording a notice, telling the client to check again (in this case nothing will have changed, though maybe the client will send a better reply next time).

I'm a bit torn, what do you think about this?

Note to self: in the error case in (2), the listener request can't actually be replied to again, even though the first reply failed, so need to fix that in the listener. Possibilities are either sending an auto-deny on error or using a mutex instead of atomic.CompareAndSwap.

[olivercalder]

Fundamentally, it doesn't matter when within the function the notice is recorded, since the lock is held any future API requests to get more info will block until we're done with this function anyway.

[bboozzoo]

are those requested permissions? if so could the field be named eg RequestedPermissions

[olivercalder]

They are the requested permissions. Iirc, these started out as "requested-permissions", but we moved to "permissions" instead for some reason. I'd be happy to go back to "requested-permissions" if @pedronis agrees.

[pedronis]

you could call the field RequestedPermission why keeping the agreed external JSON name just permissions, otoh maybe the fact that the overall thing is Constrains is clear enough?

[olivercalder]

Yes, the idea was to make PromptConstraints as similar as possible to Constraints, with the exception that we must use path instead of path-pattern. I think using the Permissions field throughout for both types is clearer, especially since the Permissions might get pruned over time as new rules satisfy some of the permissions, so Permissions is no longer matches the originally requested permissions. Also, AvailablePermissions is never used internally, it's just populated once as information for the client.

Furthermore, I'll soon be working on the ability to send back a bitmask of permissions corresponding to those provided in the user's reply, rather than just exactly what was asked for, and I think we'll need to record the original list of abstract permissions (not exported or included in the json) so that prompts which are partially satisfied by several new rules still send back a reply with all the requested permissions. So RequestedPermissions would be a bit ambiguous here whether it's referring to the originally requested permissions or the current remaining permissions.

[bboozzoo]

Something like /run/snapd/prompts/<stuff-here> looks appropriate. We don't explicitly set RuntimeDirectory in systemd, but rahter create one during startup.

[bboozzoo]

sorry if it's a naiive question, but if one user (A) tries to access files of another user (B), is the user here equal to A or B? I suppose it's B, but is the process uid (A) then part of the listener request?

[olivercalder]

The user is the "subject" making the request to access some object (a file, probably). So the user is A here. We don't care about filesystem permissions/ownership here, just apparmor permissions, which can vary depending on the owner of the file, but we let the apparmor profile make those distinctions, and if the request reaches snapd, we no longer care who owns the file.

[bboozzoo]

since we're keeping those in memory, it'd be nice to have some sort of an upper limit on how many requests can be kept

[olivercalder]

Good point. A few considerations:

• What should the limit be (1000?), and should it be global or per-prompt?
  • Should the limit be the responsibility of the listener or the prompt DB?
• Listener requests sit waiting in their own goroutine for a reply, so it's rather important that we don't keep too many open.
• Do we want to auto-deny new listener requests rather than adding them to this list, or replace and deny old ones (I think I favour the latter here)?
• Do we want to apply the same to prompts themselves, not just listener requests associated with a given prompt?
  • Do we want to preserve the oldest prompts and auto-deny new ones, or auto-deny old ones and preserve new ones (I think I favour the former here)?

What do you think of these @bboozzoo and @pedronis ?

[bboozzoo]

is there some sort of limiting on the kernel side or are we at the mercy of the sandboxed process? AFAIU in a stable scenario there should be only a handful of prompts and requests, shouldn't there? Unless the sandboxed process is multi threaded or uses async IO it should be blocked inside a syscall awaiting a reply to a prompt, so I think I'd start with some modest limits and then see how it works in practice. Say, 10 requests and 100 prompts?

[bboozzoo]

Just for the record, perhaps we can address this in a followup PR?

[olivercalder]

There is no limit on the kernel side, a process could theoretically trigger as many syscalls as it can and each could result in an apparmor request.

If the current approach of starting a new goroutine to wait for a reply for each request is too heavy, we could instead actually store the received message in memory and use it later to construct and send a reply. This would also be of great benefit if we want to persist requests (and prompts) across snapd restarts. However, persisting these across snapd restarts comes with a load of complexities on both the kernel and snapd side, discussed a bit below. So if the overhead is not much different, a waiting goroutine seems much safer and simpler.

---

When the listener is created, it opens a file descriptor on the notify.socket and registers it to receive notifications in userspace with some filter (currently empty). Associated with that fd, the kernel creates a notification queue with the given filter. When a new event triggers a notification, the kernel goes through the queues and places the notification into the first queue whose filter matches the notification.

When a listener is closed, it closes the associated fd, and the kernel redistributes any un-read notifications from that queue to the first other queue which matches the filter for the given notification.

Once a message is read from the queue by its associated listener, the kernel will not redistribute it even if that listener closes its fd before it has replied. So it would require lots of work on the kernel side to distribute notifications to multiple queues (which introduces complexity and reply races) or reclaim un-replied-to notifications which have been read by a listener which has since closed its fd without replying (not sure if this is even possible).

[olivercalder]

I think the questions of limits to the number of outstanding prompts (and/or listeners) is a UX question as well, so I'll wait to address it until we've discussed with Desktop and Design people about this point. Persisting across restarts is another related topic we'll discuss.

[zyga]

I thing given that we do a linear scan we should cap at something that is unreasonable to keep scanning this way. Lower thousands perhaps?

[olivercalder]

I'll set the outstanding prompts per user limit to 1000, and not set a limit on number of listener requests per prompt. I think we want to deal with that in the listener itself, since optimizations there could make storing listener requests extremely lightweight. And users are unlikely to be able to answer >1000 prompts, but snapd can easily responds to a million listener requests automatically when their associated prompt is satisfied.

[bboozzoo]

this looks a bit awkward now. Anyhow, the check could be done before taking any lock

[olivercalder]

I agree, it seems like it would return an error if it's not "allow" (e.g. if it's "deny"). This used to be called AsBool, but we renamed it when reviewing #13849 (#13849 (comment)). I'm happy to rename it if you and @pedronis agree.

For now, I'll at least move it up to before the lock, nice catch!

[olivercalder]

I've re-renamed to AsBool for now since I think IsAllow suggests it must be outcome == "allow" else error.

[MiguelPires]

lgtm once the other reviews are addressed

[bboozzoo]

is there some sort of limiting on the kernel side or are we at the mercy of the sandboxed process? AFAIU in a stable scenario there should be only a handful of prompts and requests, shouldn't there? Unless the sandboxed process is multi threaded or uses async IO it should be blocked inside a syscall awaiting a reply to a prompt, so I think I'd start with some modest limits and then see how it works in practice. Say, 10 requests and 100 prompts?

[bboozzoo]

wold there be reasons to start with a randomized value?

[olivercalder]

I think there's not a good reason.

• We can't use notification ID (from the kernel) since this is an opaque value which we can't assume to be unique
• There might be many prompts, so if we're using a monotonically-increasing ID we don't want to start at too high a number where we risk overflowing
• Always starting at 0 makes for easier integration testing, so we can very programmatically do some actions which we believe should trigger some known number of notices with particular IDs in a particular order

[zyga]

Quick pass through non-test code.

Should we avoid the perils of state and use an RWLock up front grabbing shared read lock or exclusive write lock to improve performance of what is otherwise entirely sequential code now?

The scheme of allocating IDs is super slow and I wonder if we could make it faster by holding the ID in memory that is mapped. The kernel will guarantee it is written back to storage even if we die.

[zyga]

How come we read full 16 bytes when the number is in text and not binary? Will this not err when we have less to read?

[olivercalder]

The number is ASCII hex padded by 0s, so "0000000000000000" to "FFFFFFFFFFFFFFFF". But I prefer your suggestion of directly writing an int to a memory-mapped file.

[zyga]

Should we ever reallocate and garbage collect the map so that it does not hold a lot of memory from the past?

[olivercalder]

Can try to make this better. If we store sub-maps by snap and interface, we could provide more opportunities for setting maps to nil (allowing GC). But if loads of prompts are coming from the snap using the same interface, it doesn't help much.

That suggestion would help with speed of looking for matching prompts, though.

[olivercalder]

I explored letting permissions be bitflags which are only converted to []string when marshalled, as this would eliminate problems of ordering and make checking permission equality, containment, subtraction, etc. much faster/simpler. But then marshalling the bitflags into permissions requires knowing the interface, and we'd need a different custom marshaller for PromptConstraints vs Constraints, and both of these still require knowing the interface, which is, again, not known by constraints.

Even in top-level structs containing constraints, the interface isn't always known: https://docs.google.com/document/d/1tBnefdukP69EUJOlH8bgD2hrvZCYoE8-1ZlqRRYlOqc/edit?pli=1#heading=h.ls8izcpzf5fb

From the spec, the request body for posting to /v2/interfaces/requests/prompts/{id}:

{
	"action": "allow" | "deny",
	"lifespan": "single" | "session" | "forever" | "timespan",
	"duration": <duration for which decision applies, if lifespan == "timespan">,
	"constraints": {
		"path-pattern": <path glob for which decision applies>,
		"permissions": [<perm>, <perm>, ...]
	}
}

So at the moment I do not see how this would be viable. It would be nice, though.

[olivercalder]

This branch contains the exploration of using bitflags for permissions: https://github.com/olivercalder/snapd/tree/attempt-permissions-bitflags

[bboozzoo]

I think the only remaining issue I have now is that the notify code is called with the lock held and we can't foresee if the callee won't block for any reason. Since we already have the user ID and prompt ID, this call could be done after we release the db lock?

[olivercalder]

I would really prefer to record the notice while the requestprompts lock is held, since this guarantees the order in which the notices are issued. This lets us rely nicely on the fact that the notice when a prompt is resolved will always be the final notice for that prompt, and thus the notice data indicating the resolved reason will be preserved. We don't need to guarantee this, but it would be much nicer for clients.

Discussed more here: https://warthogs.atlassian.net/browse/SNAPDENG-15165

[pedronis]

not sure it should block landing this but we have the issues that emitting notices atm always require the state lock so we will hold this and then need to get the state lock to emit the notice. Maybe there's a way out once we undetstand more what should happens on snapd restarts. We already know that it doesn't make sense to keep prompt notices across reboots

[olivercalder]

Right, I'm not sure what we can do to improve this at the moment. Perhaps in an ideal world notices would be decoupled from the state lock entirely, so they can relay information while the lock is held. And so that recording notices doesn't lock up the state all the time.

Perhaps something can be optimized so that the notifyPrompt closure kicks off the notice recording, guaranteeing order, but doesn't wait for the state lock to be acquired and released.

In any case, I think we want to call notifyPrompt with the prompt DB lock held, and it's up to the closure implementation to guarantee that it doesn't block.

[olivercalder]

Rebased to fix conflicts with usage of new PathPattern type in constraints.

[pedronis]

code looks good, some considerations

[pedronis]

you could call the field RequestedPermission why keeping the agreed external JSON name just permissions, otoh maybe the fact that the overall thing is Constrains is clear enough?

[pedronis]

not sure it should block landing this but we have the issues that emitting notices atm always require the state lock so we will hold this and then need to get the state lock to emit the notice. Maybe there's a way out once we undetstand more what should happens on snapd restarts. We already know that it doesn't make sense to keep prompt notices across reboots

[zyga]

I think this looks very good.

My main points of feedback are:

• Use ints rather than strings internally and only get to strings when we are closer to external APIs. This will speed things up, use less memory and puts smaller pressure on the garbage collector
• Use a few types to make the semantics of a few integers more clear
• Close should Unmap even though we may eventually GC, this is still a file-like resource on top of the extra memory that is probably unsafe to just wait for
• It'd be nice if we could stop holding the big lock when we transition to notice APIs but I have no insight as to how to do that yet.

[zyga]

Nitpick on the type, let's decide if it's an int or an uint32 and add a type definition for it. The code will read cleaner and we will have fewer places to fix in case we got the guess wrong.

[olivercalder]

Thanks for re-reviews! I've rebased to pull in test fixes from master.