Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rewrite check for admin #23970

Merged
merged 6 commits into from
Feb 25, 2021
Merged

Rewrite check for admin #23970

merged 6 commits into from
Feb 25, 2021

Conversation

narph
Copy link
Contributor

@narph narph commented Feb 10, 2021

What does this PR do?

Re writes the HasRoot function.
The token.IsMember(sid) returns TRUE if the caller's process is a member of the Administrators local group. Caller is NOT
expected to be impersonating anyone and is expected to be able to open its own process and process token. In order for the process to "Run as administrator" the user must be in the BUILTIN\Administrators Group.

Why is it important?

In order to install the elastic agent we are checking for Admin by accessing "\.\PHYSICALDRIVE0".
This will not work on all setups since the physical drive might be missing, ex testing with windows containers, kubelet startup.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • 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
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label Team:Ingest Management labels Feb 10, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ingest-management (Team:Ingest Management)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Feb 10, 2021
@narph narph self-assigned this Feb 10, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Feb 10, 2021 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #23970 updated

  • Start Time: 2021-02-23T16:45:43.588+0000

  • Duration: 63 min 31 sec

  • Commit: 87e5a50

Test stats 🧪

Test Results
Failed 0
Passed 6548
Skipped 16
Total 6564

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 6548
Skipped 16
Total 6564

@andresrc andresrc added Team:Elastic-Agent Label for the Agent team and removed Team:Ingest Management labels Feb 10, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/agent (Team:Agent)

blakerouse
blakerouse previously requested changes Feb 10, 2021
View reviewed changes
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change. I think we originally had something similar, but had to remove it because it didn't work as expected. Hopefully this does not have the same issue (which I can't even remember).

Just have an issue with the test, I think that might need to be fixed.

x-pack/elastic-agent/pkg/agent/install/root_windows_test.go Outdated Show resolved Hide resolved
@michalpristas
Copy link
Contributor

related issue is here: https://github.com/elastic/beats/pull/21884/files
we wll need to recheck this manually on various systems to see if token is not missing

@michalpristas
Copy link
Contributor

/package

3 similar comments
@narph
Copy link
Contributor Author

narph commented Feb 16, 2021

/package

@narph
Copy link
Contributor Author

narph commented Feb 19, 2021

/package

@narph
Copy link
Contributor Author

narph commented Feb 19, 2021

/package

@narph narph requested a review from michalpristas February 22, 2021 09:51
michalpristas
michalpristas approved these changes Feb 23, 2021
View reviewed changes
Copy link
Contributor

@michalpristas michalpristas left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks correct, please run mage fmt to make sure imports are ordered
tested on win10

x-pack/elastic-agent/pkg/agent/cmd/install.go Outdated Show resolved Hide resolved
@narph narph requested a review from blakerouse February 25, 2021 08:50
@narph narph dismissed blakerouse’s stale review February 25, 2021 08:51

Requested changed has been implemented

@narph narph merged commit 29942ff into elastic:master Feb 25, 2021
@narph narph deleted the has-root branch February 25, 2021 08:51
narph added a commit to narph/beats that referenced this pull request Feb 25, 2021
@narph
Rewrite check for admin (elastic#23970)
f2b6d7e 
* rewrite hasroot

* rename

* updates

* fix linux

* update changelog

(cherry picked from commit 29942ff)
@narph narph mentioned this pull request Feb 25, 2021
Merged
6 tasks
@narph narph added the v7.12.0 label Feb 25, 2021
narph added a commit to narph/beats that referenced this pull request Feb 25, 2021
@narph
Rewrite check for admin (elastic#23970)
8188a8a 
* rewrite hasroot

* rename

* updates

* fix linux

* update changelog

(cherry picked from commit 29942ff)
@narph narph mentioned this pull request Feb 25, 2021
Merged
6 tasks
@narph narph added the v7.13.0 label Feb 25, 2021
narph added a commit that referenced this pull request Mar 2, 2021
@narph
Rewrite check for admin (#23970) (#24226)
1184c49 
* rewrite hasroot

* rename

* updates

* fix linux

* update changelog

(cherry picked from commit 29942ff)
narph added a commit that referenced this pull request Mar 2, 2021
@narph
Rewrite check for admin (#23970) (#24227)
92b5a7a 
* rewrite hasroot

* rename

* updates

* fix linux

* update changelog

(cherry picked from commit 29942ff)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalpristas michalpristas michalpristas approved these changes

@blakerouse blakerouse Awaiting requested review from blakerouse

Assignees

@narph narph

Labels
Team:Elastic-Agent Label for the Agent team v7.12.0 v7.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@narph @elasticmachine @michalpristas @blakerouse @andresrc