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

Default cache directory should be user-specific #35

Open
alexmyczko opened this issue Jun 13, 2023 · 9 comments
Open

Default cache directory should be user-specific #35

alexmyczko opened this issue Jun 13, 2023 · 9 comments

Comments

@alexmyczko
Copy link

alexmyczko commented Jun 13, 2023

so i really like bkt alot, however on multiuser system. or when root is using bkt -- somecommand, and after wards user tries bkt -- somesamecommandasother user ending up with above msg :'(

$ bkt -- ....
bkt: Cache lookup failed: Failed to access cache file: Permission denied (os error 13)

strace revelead to me:

openat(AT_FDCWD, "/tmp/bkt-0.5-cache/keys/D6A5253D88DA21DC", O_RDONLY|O_CLOEXEC) = -1 EACCES (Permission denied)

not sure if this is a build or runtime option? probably i'm using it wrong and there's options. but would prefer to have it just work without options (thinking)

@dimo414
Copy link
Owner

dimo414 commented Jun 13, 2023

Thanks for the report! Yes permissions in /tmp are tricky with multi-user usage on a single system. To my knowledge there really isn't a good or canonical solution, but there's some thoughts in #32 about this issue. Feel free to weigh in there as well.

The advice I give currently is that users should configure TMPDIR to a directory they alone have access to. It's unfortunate that this isn't a more standard practice, since it seems like a security risk in general that applications with different permissions all typically write to the same temp directory.

The generic issue aside, I agree this specific failure behavior isn't great. I have a TODO to isolate the cache directory per-user, which would make this experience a bit better. We can also improve this error message to at least be clearer.

One thought is that we perhaps should only lock down the cache directory if not specified by the user (i.e. via --cache-dir / BKT_CACHE_DIR), and expect the user to properly configure the permissions of a directory they manually select. However I dislike that approach because it would mean offloading this security concern to users in a really subtle and easy to forget way.

Another approach would be to add a flag to configure the permissions the directory is created with. That's much more explicit, which I like, but it's still cumbersome.

Also, just to point out, these permissions are (currently?) only set if the directory doesn't exist, so a workaround is to simply use your own directory that you've already created with your desired permissions. I'm not convinced ignoring the permission issue in this case is "correct" behavior (it just seems wasteful to check the directory permissions every time), so don't assume this workaround will continue to work indefinitely.

@dimo414 dimo414 changed the title bkt: Cache lookup failed: Failed to access cache file: Permission denied (os error 13) Cache directory permission failures are confusing Jun 13, 2023
@dimo414
Copy link
Owner

dimo414 commented Jun 13, 2023

Another thing to note - sharing a cache directory across multiple users can be a problem even if you intended to do so (i.e. you're not concerned about security). Because the cache doesn't key off the process' invoking user you can get cached output polluted with state from the other user. As a simple example, caching your user ID:

$ sudo bkt -- id -u
0

$ bkt -- id -u
0

So I'd strongly recommend using separate directories per-user even if you don't think it's necessary from a security standpoint - correctness could also be impacted. I will likely resolve that TODO in a future release so that this happens by default.

@alexmyczko
Copy link
Author

believe me there's many places where the output for any user and root are the same, here's an example:

i'm using bkt to cache client/server side outputs:
https://github.com/alexmyczko/ruptime/tree/debian (the systemd service file has it, but a lot of systems don't have bkt yet)

it could as well be used in cgi web pages that take lot of time to build page, as well as fnt preview

@dimo414
Copy link
Owner

dimo414 commented Jun 15, 2023

Oh totally, I appreciate that it's fine in many cases. But in the general case it's both a security risk and a correctness risk - and the correctness risks can be subtle - so I'm pretty hesitant to intentionally support cross-user caching (at least not without requiring something explicit like both callers specifying the same --cach-dir).

dimo414 added a commit that referenced this issue Jan 20, 2024
@dimo414 dimo414 changed the title Cache directory permission failures are confusing Default cache directory should be user-specific Jan 20, 2024
@inkarkat
Copy link

Wouldn't $HOME/.cache/bkt (XDG_CACHE_HOME) be a better default than the temp dir?

@dimo414
Copy link
Owner

dimo414 commented May 23, 2024

Yes possibly, but it would have a performance impact on systems that use a tmpfs for /tmp, so I've been hesitant to change the default. See also #32.

@inkarkat
Copy link

For an actual performance impact, there would have to be lots of accesses, of which most likely would be reads. So the default file-system cache (assuming there's enough available RAM for that) of the operating system likely will speed this up also without an underlying tmpfs.
Of course, with such a generic caching tool, you'll never know how it's used, but for the use cases I can imagine, the benefit of having the canonical cache dir used by default far outweighs potential performance impacts (and special use cases can always override the cache dir, or even mount a tmpfs under ~/.cache[/bkt]).

@dimo414
Copy link
Owner

dimo414 commented May 23, 2024

Agreed, I'm just sharing the original motivation for /tmp.

FWIW it's pretty easy to observe a meaningful performance difference between caching to a tmpfs vs. local disk. I am also aware of users that rely on /tmp being a tmpfs, and it would be burdensome for them to be asked to reconfigure their system to mount a new tmpfs under ~/.cache. Maybe we just decide "tough", but it's not simply academic.

In the meantime, anyone wanting to use ~/.cache can do so easily by setting BKT_TMPDIR=${XDG_CACHE_HOME} (or BKT_TMPDIR=${HOME}/.cache) in their environment.

@inkarkat
Copy link

Thanks for sharing this background; I know that these tradeoffs are hard, and it's good that you care about backwards compatibility!
On Ubuntu, /tmp is on a regular file system (by default, but most Ubuntu users probably don't reconfigure this). Even worse, it gets wiped on each reboot, which may be unexpected for a cache (or not, again highly dependent on the use case). But tmpfs isn't persisted, neither (which for me is another argument for changing the default, but using the environment variable to reconfigure this is a valid workaround, so I'll shut up now ;-)

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

No branches or pull requests

3 participants