NFD

Why command line instead of configuration file?

Oh.. I will update the spec. Managed to forget that we have the configuration file :)

Just in case. This should not be the default settings (it will be default for packaged binaries).

What are default settings/behavior for building from source?

Also, is it correct for ps -eaf | grep nfd to show two nfd processes after the seteuid + setegid have been executed? (On OS X 10.9.2, started as sudo ./build/nfd)

  0 83900 81597   0 12:09PM ttys002    0:00.02 sudo ./build/nfd
  501 83901 83900   0 12:09PM ttys002    0:00.05 ./build/nfd

seteuid stuff is new to me, so this seems strange.

Unless somebody uncomments group and/or user in the config file, the privilege drop should not happen. The default is basically to give a commented out group and user, as in the task description.

I have no experience with seteuid/setegid, so somebody else should clarify that. I suppose that it is fine.

Steve DiBenedetto wrote:

Also, is it correct for ps -eaf | grep nfd to show two nfd processes after the seteuid + setegid have been executed? (On OS X 10.9.2, started as sudo ./build/nfd)

  0 83900 81597   0 12:09PM ttys002    0:00.02 sudo ./build/nfd
  501 83901 83900   0 12:09PM ttys002    0:00.05 ./build/nfd

I don't see two nfd processes... I see one sudo and one nfd, which looks fine since you said you launched sudo ./build/nfd.

Sounds like I was misinterpreting the output. Thanks for the clarification.

The current WIP behavior is:

1. no general section (or has general section, no user and no group) -> continue with current privileges
2. user and group are specified -> seteuid and setegid
3. user is specified, no group -> seteuid to user, setegid to group with same name as user
4. no user, group is specified -> exception (do we want to allow this?)

Item 3 will fail on OS X because users don't belong to a group of the same name (looks like it's staff instead). Is this the desired behavior? Also, the usage of seteuid/setegid is based on Junxiao's last comment on #1332. Is this still what we want (as opposed to setuid/setgid)?

For 3 and 4 I would suggest different way. If not specified, just don't set it. We shouldn't infer anything about the group name if it is not specified. Also, if only group specified, why this should be an error?

Just being overzealous. I'll go with your way.

As for setuid vs seteuid: it seems like the difference is whether we intended to permanently (setuid) or temporarily drop privileges (seteuid). Is there any reason for us to only drop privileges temporarily?

Yes, there is a reason. Some operations may require root privileges in the future (e.g., rescanning interfaces and starting yo work with new ethernet device when it becomes available). If we drop privilege permanently, we will require restart whenever such action is required, which is very undesirable.

Suggested API:

// daemon/core/privilege-helper.hpp
class PrivilegeHelper
{
public:
  void
  setConfig(...)

  void
  drop();

  void
  runElevated(boost::function<void()> f);
};

runElevated should have try-finally to ensure privilege is dropped even if there's an exception.

Looks good. The class (or runElevated at least) could be made static so that it's easier to use throughout the codebase. The user/group -> uid/gid resolution should be done only once, while reading the config file.

Currently, user and group are specified in the general section of the configuration file. Should they be moved to their own section (runtime_privileges?) or should the privilege helper handle the general section (and renamed appropriately)?

It doesn't look nice to have too many sections. Add mgmt/general-config-section.hpp (which contains a function, not a class) to process the general section, and calls a method on PrivilegeHelper static class to pass the user and group as string.

Junxiao Shi wrote:

Add mgmt/general-config-section.hpp (which contains a function, not a class) to process the general section

Such a function could be put directly in mgmt/config-file.{cpp,hpp} in my opinion.