enhancement(4889): added check to abort enrolling if user is privileg… #6038
Conversation
|
This pull request does not have a backport label. Could you fix it @kaanyalti? 🙏
|
|
|
mergify
bot
added
the
backport-8.x
kaanyalti
force-pushed
the
enhancement/4889_add_actionable_error_message_enroll_unprivileged
branch
2 times, most recently
from
fc7a200 to
0e77313
Compare
kaanyalti
force-pushed
the
enhancement/4889_add_actionable_error_message_enroll_unprivileged
branch
5 times, most recently
from
7f3bf53 to
9bbb3c0
Compare
pierrehilbert
added
the
Team:Elastic-Agent-Control-Plane
|
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
internal/pkg/agent/cmd/enroll_cmd.go
Outdated
| if hasRoot && as.Info.Unprivileged && !c.options.FromInstall { | ||
| return unprivilegedAgentRootUserError | ||
| } | ||
| } |
There was a problem hiding this comment.
I did not see the original filled issue for this and I would have made a comment there. I know this is coming late, but I believe this is the wrong approach.
If you call enroll as root, you can easily tell if Elastic Agent is installed without communicating with the daemon. You can read the user/group from the executing binary from the filesystem, because you are root.
You can then just ReExec the enroll command directly using the user you retrieved from reading from the disk. You need to read user from the filesystem as @michalpristas is adding the ability to use a custom user, so you cannot rely on the idea that the user is always the same.
This change also will allow a root user to call enroll and have the enroll work correctly as the unprivileged user.
There was a problem hiding this comment.
Would you be ok with implementing your suggestion in another issue? The goal of this implementation is just to provide a descriptive error message back to the user. Would this implementation break anything for our users? I do agree what you're suggesting is a better solution.
There was a problem hiding this comment.
I just think the implementation suggestion I provided would replace this work in the PR, not really making this work required.
There was a problem hiding this comment.
Yes it would, and I am now thinking that the current implementation is tricky to get working with delayed enrolls. I started looking into your suggestion and will be replacing this PR.
kaanyalti
force-pushed
the
enhancement/4889_add_actionable_error_message_enroll_unprivileged
branch
from
9bbb3c0 to
b582d40
Compare
internal/pkg/agent/cmd/enroll_cmd.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| var unprivilegedAgentRootUserError = errors.New("cannot execute this command as root, agent is unprivileged. either execute the command as the elastic-agent-user or add your user to the elastic-agent group") |
There was a problem hiding this comment.
I don't think it works the same even if the user is in that group. Because the file ownership will be incorrect.
Providing specific error message based on the platform would be a better approach.
internal/pkg/agent/cmd/enroll_cmd.go
Outdated
| if hasRoot && as.Info.Unprivileged && !c.options.FromInstall { | ||
| return unprivilegedAgentRootUserError | ||
| } | ||
| } |
There was a problem hiding this comment.
I just think the implementation suggestion I provided would replace this work in the PR, not really making this work required.
kaanyalti
force-pushed
the
enhancement/4889_add_actionable_error_message_enroll_unprivileged
branch
from
70bccd2 to
e32ddc9
Compare
kaanyalti
force-pushed
the
enhancement/4889_add_actionable_error_message_enroll_unprivileged
branch
from
e32ddc9 to
e983650
Compare
|
|
Closed in favor of #6144 |
What does this PR do?
This PR updates the enroll command so that it provides a more descriptive error message to the user in the case a root user tries to enroll an unprivileged agent.
Why is it important?
Provides users with clearer information.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files./changelog/fragmentsusing the changelog tool[ ] I have added an integration test or an E2E testHow to test this PR locally
Related issues
enrollan unprivileged Agent as a privileged user #4889