libelektra icon indicating copy to clipboard operation
libelektra copied to clipboard

let root access other user's configs

Open atmaxinger opened this issue 3 years ago • 18 comments

Currently, the resolver plugin implements the following lookup mechanismns:

  • reading from passwd (using the user id)
  • environment variable XDG_CONFIG_HOME
  • environment variable HOME
  • environment variable USER
  • buildin (?)

When kdb is run using sudo

  • user id is 0 (root),
  • HOME is /root,
  • USER is root.

In the record tooling (#4399 #4371), we want to store recording information and settings in the user:/ namespace. However, some cases like modifying keys in the system:/ namespace may require running kdb via sudo. In this case we wouldn't be able to access the user:/ namespace of the correct user.

The sudo command defines the environment variable SUDO_USER, which contains the name of the user who started the command. If present, we could use it instead of the USER variable in the USER resolver and instead of the user id in the passwd resolver. We would still need to find another way if the HOME or XDG_CONFIG_HOME resolver is active.

atmaxinger avatar Jul 05 '22 15:07 atmaxinger

For the passwd resolution, we should use SUDO_UID.

For the HOME, XDG_CONFIG_HOME and built-in fallback I don't see a good solution when using sudo.

Also when you do

sudo kdb set user:/foo 123

and this creates a new file, the file will be owned by root, unless we also add special code for this.

Also, what if somebody actually wants to use /root when doing sudo kdb get user:/foo? I guess they could use su -c 'kdb get user:/foo', but it's not quite the same.


All in all, I don't think we should implement the SUDO_USER/SUDO_UID resolution in general. But it seems the best solution for kdb record. I'd say lets add the code to the resolver, but only enable it for the user:/elektra/record keys, or the plugin for kdb record could call the special resolver mode some other way.

kodebach avatar Jul 05 '22 19:07 kodebach

I'd say lets add the code to the resolver, but only enable it for the user:/elektra/record keys, or the plugin for kdb record could call the special resolver mode some other way.

So in summary create a new build configuration for the resolver, i.e. resolver_fm_su_b where s stands for SUDO_USER, and mount user:/elektra/record (either hardcoded or with a one time setup) with this custom resolver. Sounds easy enough. Where's the catch? 😆

atmaxinger avatar Jul 06 '22 10:07 atmaxinger

For the passwd resolution, we should use SUDO_UID.

Yes, good point. Unfortunately doas only has DOAS_USER but not DOAS_UID. So we probably need to implement user -> uid mapping for BSD anyway.

For the HOME, XDG_CONFIG_HOME and built-in fallback I don't see a good solution when using sudo.

There isn't. That is why we do not recommend to use HOME, XDG or any other environment variables except those as specified in spec:.

So we should make sure that a resolver with sudo support doesn't use any non-preserved environment variables.

and this creates a new file, the file will be owned by root, unless we also add special code for this.

We already have chown/chmod code. To fully solve this issue, this should be handled properly, i.e., user: config is written so that the user can access its config.

Also, what if somebody actually wants to use /root when doing sudo kdb get user:/foo?

It is the same like for any other user: we didn't implement that yet. Ideally we find a solution that gives the admin a possibility to change any user's config.

One way would be if sudo -u root kdb get user:/key/from/root works but I do not know how to distinguish these cases. At least the environment is identical.

All in all, I don't think we should implement the SUDO_USER/SUDO_UID resolution in general.

I have to agree that HOME=/root (set by sudo and doas) is somewhat contradictory to the goal of this issue.

So in summary create a new build configuration for the resolver ... and mount user:/elektra/record

Yes, exactly.

Where's the catch?

I made the scope of this issue a bit wider :wink:

markus2330 avatar Jul 06 '22 11:07 markus2330

So we probably need to implement user -> uid mapping for BSD anyway.

We don't need to. Instead of using getpwuid_r we could use getpwnam_r which does this mapping internally.

atmaxinger avatar Jul 06 '22 11:07 atmaxinger

Good news :wink:

So the question to answer is: how can we enable tooling to access the desired user: namespace. We also need to answer the question what the desired namespace is¹. Ideally we do not need to reimplement sudo/doas but use these tools.

Non-goal: any changes for applications, they should be limited to access the one user: namespace of the user running the application.

¹ To start answering the question: for me it makes sense that sudo myscript like echo myscript | at now always has the same config (including user: config) as "./myscript".

markus2330 avatar Jul 06 '22 12:07 markus2330

I'm not sure whether we can solve this in general for all types of resolvers. Also I think the base issue here is not "let root access other user's configs", because root can just do that by using the su command.

The issue we want to solve is accessing the user:/ namespace for a user that invoked kdb using sudo. While techically yes, it's now root that wants to access the user:/ namespace - for me this is semantically different. When I invoke a command with sudo I don't do it because I want to execute something as root. I do it because it requires elevated priviliges. The fact that this command then gets executed as root is an implementation detail.

That said, what we could do is defining an environment variable, e.g. ELEKTRA_USER that contains the user whoms user:/ we want to access. In the resolver, we have to check whether this variable exists in every user:/ resolving strategy, and then act accordingly. The kdb tooling can also accept a --user parameter and then set this environment variable itself.

This will however fail for HOME and the XDG variables - we have no way of evaluating those for another user without being in a session of that user. Best we can do is a fallback to passwd in those cases.

This also won't be of any help in the case of sudo, as we still would have to manually specify the --user parameter. Unless we handle the SUDO_USER the same way as ELEKTRA_USER ... which we don't want to.

atmaxinger avatar Jul 06 '22 14:07 atmaxinger

For a quick-win I have implemented a passwd-based strategy as a draft in #4402. This solution would suffice for the use-case of the kdb record tooling.

atmaxinger avatar Jul 06 '22 15:07 atmaxinger

I think we are seriously over-complicating things here.

AFAIK root can currently already access user:/ keys of other users. This works by simply using sudo -u:

sudo -u otheruser kdb get user:/foo

The whole issue with sudo kdb record goes away, if kdb record stores its data outside the KDB. We can still use getpwnam_r and SUDO_USER/DOAS_USER/LOGNAME to find the home directory and put the data there. But we don't need to involve the resolver or any other part of the KDB machinery. I say this because AFAICT, there are only two things that kdb record needs to store:

  1. The active session: We could just use a file $HOME/.elektra_active_session (where $HOME is not an env-var but resolved by custom code) which contains the path to the active session.
  2. Session recording logs: One way to solve the whole problem would be, if a session recording is stored as a directory instead of a single file. Then we can use unique filenames for the log entries of the session, thereby removing the need for the atomicity checks from the resolver. For example, we can use $timestamp.old and $timestamp.new to store the old and new data. Both of these files could contain standard KeySets and could be written by any standard storage plugin. Since timestamps could still collide, we could use sequence numbers with open(..., O_EXCL | O_CREAT).

kodebach avatar Jul 06 '22 16:07 kodebach

getlogin(3) (used by logname(1)) seems to do what we want, both

sudo logname
doas logname

prints for me the original user (not root).

As this solution is completely without environment variables, I prefer this than to the solution in https://github.com/ElektraInitiative/libelektra/pull/4402 (which would be tied to sudo and maybe doas).

@atmaxinger do you already have a solution how to write to different user:/ in Ansible? If this is also possible, we can fully resolve this issue.

markus2330 avatar Jul 07 '22 14:07 markus2330

do you already have a solution how to write to different user:/ in Ansible?

I'm not quite sure what you mean by that. Do you mean that we can specify in the Ansible Galaxy Elektra Plugin which user(s) should be affected by keys in the user:/ namespace?

atmaxinger avatar Jul 07 '22 14:07 atmaxinger

getlogin(3) (used by logname(1)) seems to do what we want,

Nice find! Unfortunately, this does not seem to work under WSL. Curiously, the environment variable LOGNAME is defined, but running the command logname yields the error "logname: no login name".

atmaxinger avatar Jul 07 '22 14:07 atmaxinger

Unfortunately, this does not seem to work under WSL. Curiously, the environment variable LOGNAME is defined, but running the command logname yields the error "logname: no login name".

Seems to be a longstanding known issue https://github.com/microsoft/WSL/issues/888.

getlogin(3) (used by logname(1)) seems to do what we want

Using this would make it impossible to write to another user's config, other than the one I'm logged in as, i.e. the exact opposite of what the current issue title says. It works for the sudo kdb record case, but you "made the scope of this issue a bit wider".

do you already have a solution how to write to different user:/ in Ansible?

Why would that be needed? If we really need it, there is https://docs.ansible.com/ansible/latest/user_guide/become.html

kodebach avatar Jul 07 '22 14:07 kodebach

Do you mean that we can specify in the Ansible Galaxy Elektra Plugin which user(s) should be affected by keys in the user:/ namespace?

Exactly.

Using this would make it impossible to write to another user's config

This is the purpose of this kind of resolving. It works like needed for recording (and probably other use cases, too).

made the scope of this issue a bit wider

The issue is only discussion, so with "widening the scope" I meant we should think about if recording will also properly work if root is becoming a user (and not a user becoming root we talked before). Ideally also nesting of several sudo/doas commands should work. I think only getlogin_r can do this correctly.

In general environment variables are, in the best case, a workaround. They are not suitable to be used to influence Elektra, as environment variables' goal is that processes can have different environments. Elektra's goal is that applications always get the same configuration, except if specified otherwise (e.g. spec:/ says that an environment variable should influence a specific variable). To give an example: we want to avoid that applications executed from different shells, started by different desktop environments etc. have different configurations.

The current result is:

  • it is possible to lock user:/ keys to the user that logged in, as needed by recording
  • locking in such a way is not suitable as new default, as it would disable the "sudo/doas/become functionality" we need so that admins can write to user's configs

markus2330 avatar Jul 08 '22 16:07 markus2330

In general environment variables are [...] we want to avoid that applications executed from different shells, started by different desktop environments etc. have different configurations.

I fully agree. But kdb record is not a regular application using Elektra. In fact, I would say kdb record perfectly fits the intended use case of an environment variable. I want to start a record session and within the environment where I started the session, I want to record configuration changes. I don't want other processes running in the background, started by another user, etc. to pollute my recording.

That said, I agree that using getlogin_r for kdb record is a very good idea. I just still doubt that storing all the kdb record state (1) within the KDB is a good idea. That's why I proposed a different solution with getlogin_r.


(1) yes it is state and not config, and that's the problem

kodebach avatar Jul 08 '22 18:07 kodebach

I don't exclude the possibility that the log should be outside of KDB. First we need to determine, what exactly the log contains.

Currently, I see two vastly different use cases, that probably need different implementations:

  1. collect base, ours and assertions per session: then KDB is well suited and the solution described here is perfect for logs of a user
  2. fully record everything that happens: then probably using existing loggers like journald make more sense

But lets give @atmaxinger more time to better dive into the use cases (Ansible etc.).

markus2330 avatar Jul 09 '22 11:07 markus2330

I don't exclude the possibility that the log should be outside of KDB.

Not responding at all to my suggestion -- which I think was a valid suggestion, since it makes this whole issue irrelevant as my solution doesn't use the resolver -- certainly made it seem like it...

Currently, I see two vastly different use cases, that probably need different implementations

AFAICT @atmaxinger and I agree that the diff use case could be implemented by recording a full log and then collapsing this log into a single diff. So it's not certain that different implementations are needed.

collect base, ours and assertions per session

I've stated multiple times that this would require duplicating potentially very large chunks of the KDB, even if in the end the diff would be very small. The feature for resuming sessions makes this even worse, since you'll need to keep the initial duplicate around as long as resuming the session is possible. Without resuming, you could at least get rid of the duplicate data once you stop the session and just keep the diff. The alternative would be to use extra code to merge a diff from A to B and one from B to C, at which point you might was well record full logs and always merge the individual steps.

then KDB is well suited and the solution described here is perfect for logs of a user

I still disagree. The general KeySet format and the various storage plugins to serialize it are a good fit (since we are storing data from a KeySet after all). However, the KDB as a system-wide structure for configuration data is not. The data produced by kdb record (whatever implementation) is clearly no longer configuration data. IMHO it is even a small hack that a recording session is associated with a user. The hack is needed, just to make things fit into the KDB. In reality there is no reason why a recording session couldn't be system-wide (i.e. multi-user), or limited to a single application (i.e. multiple active sessions per user), other than the fact that we couldn't properly store the currently active session in the KDB.

In the solution I proposed above, the KeySet structure and the storage plugins would still be reused. But we wouldn't use the KDB. Instead a recording would just be any directory. By default it could be in the home directory of the logged-in user. But there is no reason why it couldn't be stored elsewhere. Similarly, by default a file in the logged-in users home directory determines the active session. But there is again no reason why it couldn't be override by (or combined with) an env-var (for single-process recording) or fall back to a system file e.g. in /etc (for system-wide recording).

existing loggers like journald make more sense

I'm not sure journald is the right fit either. AFAIK it is mostly intended to be written to, but not read back from by programs. AFAIK journald also isn't cross-platform. But I agree, there may be existing audit solutions that could help. At the same time, working around the limitations of these tools (because of differences in intended use-case) may be more work in the long run than creating something from scratch (possibly based on the techniques of existing tools).

kodebach avatar Jul 09 '22 12:07 kodebach

Please keep yourself shorter. Help is very much appreciated but the lead in the discussions/decisions must be @atmaxinger. If he doesn't ask for solutions, we should not try to give them. So please give the next reply to @atmaxinger.

Not responding at all to my suggestion

What you linked to was a suggestion that we could use files/directories instead of KDB, which, imho does not make sense for neither use case (1) nor (2). And it does not matter at the moment anyway because first #4399 must be clarified.

I've stated multiple times that this

Stating something many times does not make something more right. On contrary, it gives unfair bias to a potentially wrong solution. This is why I constantly urge that we write decisions where all pros and cons can be listed, instead of repeating them many times scattered over many threads.

would require duplicating potentially very large chunks of the KDB

We would only store keys that changed/are relevant. So every change makes at most two keys: in base and in ours (or in assert).

I'm not sure journald is the right fit either

journald was not my suggestion, it was an example because it could be unclear what I mean with "loggers". If we want to go this way @atmaxinger would need to check what is available etc. Then we would also replace the logging plugins we already have with this new style of logging. (What they do at the moment is inconsistent, some log individual changes, some only state the number of changes.)

markus2330 avatar Jul 09 '22 14:07 markus2330

@atmaxinger: I fully agree that you need to decide what we do in regards to kdb record. There are now enough suggestions that you should be able to find a good solution. If you need more input, please just ask.

Also I think this issue and #4402 should be postponed until #4399 is done. It seems @markus2330 agrees that it is not yet clear that the KDB should be used as data storage for kdb record. Therefore, it is also not clear that we need the resolver at all. Which means we should not waste time discussing this issue further until, we now we actually need this feature.


@markus2330: See https://github.com/ElektraInitiative/libelektra/pull/4399#discussion_r917286486

kodebach avatar Jul 09 '22 16:07 kodebach

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

github-actions[bot] avatar Aug 02 '23 01:08 github-actions[bot]

I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

github-actions[bot] avatar Aug 17 '23 01:08 github-actions[bot]