Return actionable error message when enrolling #6144
Conversation
|
This pull request does not have a backport label. Could you fix it @kaanyalti? 🙏
|
|
|
mergify
bot
added
the
backport-8.x
pierrehilbert
added
the
Team:Elastic-Agent-Control-Plane
kaanyalti
changed the title
kaanyalti
force-pushed
the
enhancement/4889_enroll_match_file_owner
branch
from
6834e0b
to
38be49e
Compare
|
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
kaanyalti
changed the title
…e executing user and binary owner don't match, added tests
…indows, updated integration tests
kaanyalti
force-pushed
the
enhancement/4889_enroll_match_file_owner
branch
from
5718b15
to
1d02094
Compare
| return curUser == fileOwner, nil | ||
| } | ||
|
|
||
| func isEnroll() (bool, error) { |
There was a problem hiding this comment.
i don't think isEnroll is a proper name for this. not sure it captures properly what this func is doing
There was a problem hiding this comment.
I can also split this function into its constituent functions. It would be more readable. All the steps would be clearly outlined. I couldn't come up with a reasonably short function name that describes exactly what's happening.
If I split it, these functions will be called in order in enroll.go
- getFileOwner
- getCurrentUser
- isFileOwner
There was a problem hiding this comment.
I agree with @michalpristas here, this is not the proper name for this function. It is just verifying that the executing users is the same as the owner of the executable.
To me the better name would be isOwnerExec?
| return fmt.Errorf("ran into an error while figuring out if user is allowed to execute the enroll command") | ||
| } | ||
| if !isEnroll { | ||
| return UserOwnerMismatchError |
There was a problem hiding this comment.
On Windows you cannot actually open a shell as the elastic-agent-user so how would they be able to run enroll command on Windows as the elastic-agent-user.
There was a problem hiding this comment.
I can return an error message stating the fact that user cannot execute the command as admin for windows. Linux/mac error message will can still mention running the command as the elastic-agent-user
| return curUser == fileOwner, nil | ||
| } | ||
|
|
||
| func isEnroll() (bool, error) { |
There was a problem hiding this comment.
I agree with @michalpristas here, this is not the proper name for this function. It is just verifying that the executing users is the same as the owner of the executable.
To me the better name would be isOwnerExec?
|
What does this PR do?
This PR adds checks to the enroll command to respond with an error message in case the user executing the command and the user that's the owner of the elastic program files don't match. Replaces #6038 based on the following comment
Why is it important?
Currently there are no checks in place to prevent or correct the situation where a user who is not the owner of the program files executes enroll. This leads to a broken state.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files[ ] I have added tests that prove my fix is effective or that my feature works./changelog/fragmentsusing the changelog toolHow to test this PR locally
The testing steps are only for linux and mac
Related issues
enrollan unprivileged Agent as a privileged user #4889