leafac/caxa

Permission denied at /tmp/caxa - Multi user problem in linux

SrZorro opened this issue · 17 comments

Hi,
I just got this error when my CI tried to execute my binary:
2022/04/13 09:12:57 caxa stub: Failed to create the lock directory: mkdir /tmp/caxa/locks/myprogram/7qsvoyerli: permission denied

When I was trying my binary without CI, I used myuser, so caxa created a /tmp/caxa folder with drwxr-xr-x 4 myuser myuser permissions.

But when the CI user wanted to create the lock directory, it failed because the permissions of the caixa directory are not open enough for ciuser.

As a workaround I just chmod 777 -R /tmp/caxa/, but not sure if that could work for all caxa users, maybe some users want to scope the programs per user? Or a shared (with 777) for multi user programs?

Hi @SrZorro,

Thanks for reaching out and for the investigation.

This topic is actually a thorny one: In multiuser environments, how can we prevent other users from tampering with the programs in the temporary directory?

I’m glad you found a workaround for your use case, and now I think we should address the more general problem I posed above. Perhaps we should have something like /tmp/caxa/<USER>/, in which /tmp/caxa/ is accessible by any user, but /tmp/caxa/<USER>/ is accessible only by the given <USER>?

What do you think?

For my problem, having /tmp/caxa/<USER>/ would fix it, and for my usecase (only 2 users, myself and my CI agent) the duplication of the uncomperessed program in each directory wouldn't be an issue

Maybe to some caxa users the duplication of the program in the tmp directory could be an issue (really big programs with a lot of users?)

I would go with /tmp/caxa/<USER>/ and if the duplication its a problem for someone down the road they can open an issue to find a solution for that edge case

The current proposal sounds good to me 👍

Okay. Do you think you could draft a pull request with the proposed changes to get things started?

I can try, I never used Go but the proposed changes should be easy to do.

Whenever I have time this week I will draft the pull request 👍

Hi @leafac,
I started working on this today, I got it working for linux, now I was working on make it work for windows but I found a problem with how go and node gets the username, more about this problem at the end of the comment

  • I changed all references from /tmp/caxa to /tmp/caxa/<username>
  • Tests also updated to use path with username (/tmp/caxa/<username>/[tests|examples])
  • /tmp/caxa directory is set with 777 permissions
  • /tmp/caxa/<username> is set with 700 permissions
  • All current tests pass ✅ in multi user testing with a little bit of manual help
    • native-modules test does npm ci. examples/native-modules/node_modules has to be deleted before testing with another user (or it fails with permission error)
      • ✍️ Should be easy to fix deleting node_modules when this test finishes: fixed ✅
    • In my dev setup I used docker to build the stubs and manually run the tests with 2 users, im not sure how we can test it correctly with index.test.ts as we need to switch users during the test, what are your thoughts on this matter? With a bit of docker we can test multi user in linux, Windows containers could be an option to test windows, but for Mac I don't have a good option yet. Also you can only run windows containers in windows, and not at the same time as linux containers (you have to switch engines with DockerCli.exe -SwitchDaemon)

Now, the windows username problem

In my investigation, node:require("os").userInfo().username and go:import("os/user"); user.Current().Username returns the same string in linux, but in windows go returns DOMAIN\username and node username

This causes two problems,

  • go and node doesn't return the same so %tmp%/caxa/<username> its not the same in both runtimes
  • DOMAIN\username has a backslash that its converted to a path route when working with the filesystem from go

From what I found, node doesn't have an option to get the domain in a node way, so we need to fix the username parity and the backslash path collision

We can fix the Go side with

import (
  // Imports ...
  "os/user"
  "strings"
)

// Main etc...
user, err := user.Current()
if err != nil {
  log.Fatalf("caxa stub: Failed to get OS username, error: %v", err)
}
username := strings.Replace(user.Username, "\\", "_", 1)

And the node side with

import os from "os";

const username = process.platform === "win32" ?
        `${process.env.USERDOMAIN}_${process.env.USERNAME}`
        : os.userInfo().username;

This way, in linux both will be username and in windows DOMAIN_username, this fixes the username parity and also prevents filesystem operations from making a subdirectory inside the domain

For today this is what I could do, the next week I will finish the windows fixes to make it work there if you are ok with the proposed changes

I just found out that in windows os.tmpdir() returns a user scoped tmp directory C:\Users\<username>\AppData\Local\Temp

I fixed for windows the username but seeing this its probably not needed as every user will be the only writing to his tmp path

@leafac draft request crated 🚀

Its still missing tests but first I want you to check it out so we can decide how it should be tested

Excellent work! 😁

I think automating the tests for all aspects of multi-user scenarios may be too cumbersome. The tests may become too brittle (for example, I’d like to avoid involving Docker into the situation), and I think that the benefit may not be that great. So how about we perform some manual testing and call it a day? 🤷

There’s even a precedent.

Perfect, we can do that

When I have time this week I will set up documentation and helper scripts for manual testing

Soo... at the end I tried to automatize the tests and atleast for linux was posible, I just had to create and then delete the test users

Because useradd and userdel are elevated commands, to run the multi user linux tests first its needed to sudo su, login one time to remove the login prompt for a couple of minutes, exit to return to your normal non root user

sudo su
# write password
# logged as root
exit
# back to your user, but now sudo will not ask the password for a couple of minutes
# Setting the environment variable MULTI_USER, it will enable the tests for multi user
# and when execa needs to do sudo useradd or userdel it will work because sudo password its catched
MULTI_USER=true npm run test

Tell me what do you think about this method for testing the multi user, I tried to see how could be documented to be manually executed but there where too many steps involving the need of switching between two accounts

Tell me what do you think about this method for testing the multi user […]

I think it’s nice that you found this solution to the puzzle, but it’s perhaps too brittle for the little benefit it brings. But you’re more in this headspace than I am, so I’ll let you be the judge.

I had been thinking quite a bit about this, and I think the best option would be to remove the tests and leave this feature as "There be dragons"

Currently, just the tests for Ubuntu 20.04 are 126 lines long in a test file of 594 lines (21% of the tests!), and adding the Windows version + the needed modifications to the Linux test to work on Mac + all the non-tested Linux distros that could have edge cases in the current test suite, at the end it would get at minimum the 50% of the test file just to test this with all OS coverage, where 90% of it would be user management for each OS

When I was doing some more tests in windows, I found out that this issue wasn't an issue there, at least in my Windows version (Windows 10 21H2) the node and go env variable for the %tmp% directory points to a user scoped directory at C:\Users\<user>\AppData\Local\Temp\caxa

Instead, in windows there is the issue when randomly files are getting deleted from %tmp% that causes #37, so at the end of the day the fix I did with #56 would be more like a patch until caxa moves to another storage location for the installed apps


In summary, I will remove the tests and add an entry in the README (or where you prefer) explaining the situation with the testing and linking this issue for further information about it

I could add a manual test like the MacOS one you mentioned, but I think it's not a trivial thing to test on the fly, as it would require swapping between multiple users to really test if it's working and sounds more like a chore than a test, that's why I think the best way forward will be to leave a note with there be dragons for this feature

I approve of the “here be dragons” approach. Simpler is better.

Thanks again for the thoughtful pull request! 😁

Hi @leafac

Sorry for taking so long, I removed the tests and added the dragons warning to the README

From my side I think we are ready to close this issue, let me know if we need something more

@SrZorro Thank you very much for your work. I’ll get to this soon…

@leafac I’ve been running into this issue as well and am trying to find a fix before swapping tools, is this going to be merged soon? Seems like it’s getting stale

leafac commented

Hi y’all,

Thanks for using caxa and for the conversation here.

I’ve been thinking about the broad strategy employed by caxa and concluded that there is a better way to solve the problem. It doesn’t include extracting the application into a temporary directory, so it should sidestep this issue entirely. Can you please give it a try and report back?

It’s a different enough approach that I think it deserves a new name, and it’s part of a bigger toolset that I’m building, which I call Radically Straightforward · Package.

I’m deprecating caxa and archiving this repository. I invite you to continue the conversation in Radically Straightforward’s issues.

Best.