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.

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

[BUG] Cannot incorporate automated testing for Windows install with current runningAsRoot() requirements #4942

Open
stephen-crawford opened this issue Oct 26, 2022 · 15 comments
Labels
bug Something isn't working

Comments

@stephen-crawford
Copy link
Contributor

stephen-crawford commented Oct 26, 2022

As things stand, it is not possible for any team to run a GitHub Action to test a Windows demo configuration. The original merge #4656 incorporated a change which requires all instances of OpenSearch be run as a non-root user. Currently, we have not found a solution for running a Powershell GitHub Action to install the demo configurations as part of testing.

This has become a blocker for opensearch-project/security#2161 which is an expected part of the 2.4 release for Windows support.

After trying numerous fixes, and finding no solution, this issue is being made with the intention of escalating it to someone who is either able to revert the requirements/make it so you can turn them off or someone who knows more about Windows & GitHub Actions.

To Reproduce
Steps to reproduce the behavior:

Go to: opensearch-project/security#2161
Take the code changes copy them into a cloned branch of Opensearch Security Main.
Look at the actions result and there will be a log message in the Opensearch/logs/opensearch.log that states that Opensearch cannot be run as root.

Plugins

Opensearch Security

Host/Environment (please complete the following information):

  • OS: Windows on GitHub Actions

Additional context
The batch script and the plugin_install steps function properly when manually verified but cannot perform on GitHub Actions specifically.

@stephen-crawford stephen-crawford added bug Something isn't working untriaged labels Oct 26, 2022
@stephen-crawford
Copy link
Contributor Author

stephen-crawford commented Oct 26, 2022

@cwperks @peternied @dbwiddis @davidlago

@peternied
Copy link
Member

peternied commented Oct 26, 2022

We've depended on that plugin-install workflow to help identify security plugin blockers before they get to the OpenSearch distribution build since it impacts so many more teams, and its caught many subtle errors in the past.

If we are still having trouble figuring out how to get unto the correct execution context in GitHub, what about adding a flag to disable the admin check --dangerousRunAsAdmin to work around the issue?

This idea has been discussed in opensearch-project/security#2161 and framed more cleanly - credit to @cwperks

At one point in time there may have been something similar to:

if (Boolean.parseBoolean(System.getProperty("os.insecure.allow.root"))) {
      logger.warn("running as ROOT user. this is a bad idea!");
}

in Bootstrap.java, but there is no such option.
I'm not sure if it helps, but github's documentation says UAC is disabled on the runners. See https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#administrative-privileges
Ideally, the workflow creates a user opensearch and uses this user account to bring up a node.

Originally posted by @cwperks in opensearch-project/security#2161 (comment)

@dblock
Copy link
Member

dblock commented Oct 26, 2022

On Windows "Administrator" and "root" aren't the same thing. For example, you still get elevation prompts. So maybe the feature of disallowing running OpenSearch as root should be implemented differently on Windows and consider, for example, whether OpenSearch can run as localsystem. Related to the option to disable that check, for Windows it should probably not be called "root". Can we remove that limitation on windows for the initial release?

@stephen-crawford
Copy link
Contributor Author

Excellent idea. I am not as familiar with Windows so I am glad someone with more insight could shine some light. Thank you for your guidance with this.

@dbwiddis
Copy link
Member

Sorry for slow replies as I am not on the office today. I suggest reverting my PR 4656 until we have a better understanding of what we are trying to prevent with root/admin/elevated access.

@minalsha
Copy link
Contributor

PR#4656 was reverted with #4949.

@dbwiddis
Copy link
Member

On Windows "Administrator" and "root" aren't the same thing. For example, you still get elevation prompts. So maybe the feature of disallowing running OpenSearch as root should be implemented differently on Windows and consider, for example, whether OpenSearch can run as localsystem. Related to the option to disable that check, for Windows it should probably not be called "root". Can we remove that limitation on windows for the initial release?

Now that we've addressed the near-term blocker, can we continue this discussion?= on whether "running OpenSearch as root should be implemented differently on Windows"?

I've tried to research the reason that we prevent root on other OS's and there seem to be no specific reasons, other than "it's a bad idea and we have enough CVEs that we shouldn't trust it". And certainly we can change the log error from "root" to "elevated". But if we are concerned about nefarious actors using OpenSearch to Do Bad Things™️, elevated permissions are still a big issue.

I do know that having elevated permissions on Windows (which the reverted PR tested for) allows one to enable Debug Privilege which is pretty powerful:

This user privilege provides complete access to sensitive and critical operating system components. By default, this property is enabled for users with Administrator rights. A user with Administrator privileges can enable this property for other user groups.

It allows for inspecting the memory of other processes that you don't own. If you know what the process executable is, you can directly manipulate the internals of another running process. There are many tutorials out there doing exactly this to cheat at Minecraft and other games. And even if you don't do it yourself, you could (much more sneakily) just enable the privilege for any other user of your choice (perhaps one you just created).

I do think we should put the check from my previous PR back in (perhaps with different language) but also enable some sort of override via command-line parameter that permits CI testing or even permits users to choose to take on this additional risk, if They Know What They Are Doing™️.

Thoughts?

@dbwiddis
Copy link
Member

dbwiddis commented Nov 1, 2022

@cwperks @scrawfor99 I've spent about 20 hours (multi-tasking, but still a long time with a total of 56 GHA trial runs) trying everything I possibly could to lower permissions and execute, and at this point I think it's somewhat impossible. What I've learned:

  • Github runs as admin with UAC disabled
  • The disabling of UAC prevents the de-elevation through the normal means of de-elevating (runas /trustlevel, runasuser, etc.)
  • The disabling of UAC prevents the workarounds of running a non-elevated program (e.g. explorer.exe) from launching with lower permissions

I still think there's some possibility of creating another user and executing via psexec, but haven't managed to crack the "get the output" piece of that.

At this point I see two (reasonable) possibilities:

  1. Add code (in Windows) when OpenSearch launches to de-elevate it (stripping tokens and launching another process from within OS). I think this is possible, but a lot of work, and brittle, and I don't want to try to maintain that code.
  2. Add some sort of command-line switch or allow an environment variable present in GHA's to bypass the run-as-root check. I'd prefer the command line as it's a very explicit opt-in "I know what I'm doing" step.

@dblock
Copy link
Member

dblock commented Nov 1, 2022

@dbwiddis I like the idea to bypass the check, but command line arguments is not standard way of doing things in OpenSearch, so probably an opensearch.yml option is what you're looking for. That primarily because configuration gets distributed across clusters and you don't want to introduce new ways of doing it.

@cwperks
Copy link
Member

cwperks commented Nov 1, 2022

@dbwiddis Thank you very much for looking at this as well. I got what I thought was pretty far with the psexec approach and I'm able to capture output on a Windows workspace, but not within the Github runner. Since its fairly easy to try different approaches asynchronously I plan to try a few more tricks with psexec before declaring that route impossible, but I agree with @dblock that an opensearch.yml setting is a suitable solution for this problem.

@stephen-crawford
Copy link
Contributor Author

Hi @dbwiddis, thank you for spending your time on this issue. Apologies for the late reply as well. I believe that like @dblock and @cwperks that the best route forward is likely a "I know what I am doing" setting. I believe you are aware but I created an issue community/community#37148 on the GitHub forum asking about this issue and my suspicion is that this may end being something that GitHub eventually implements with a new versioning of the GHA framework. Please let me know if you need any help adding in the currently planned "I know what I doing" setting.

@dbwiddis
Copy link
Member

dbwiddis commented Nov 3, 2022

@scrawfor99 @cwperks @dblock @minalsha

<insert "boom" emoji here>

I got it working! (At least the hard part.)

As indicated earlier, I was pretty confident (based on run time) that the psexec variant was working, but the output wasn't very informative and I didn't have the insight to use the logging switch of the command I was running (the maven wrapper in my tests) to snag the output.

In the meantime I discovered an alternative to psexec called paexec that is open source and has a very permissive license: https://github.com/poweradminllc/PAExec and a few more command line switches than psexec.

The following works on another project I maintain which uses exactly the same JNA-based code to determine elevation.

In my GHA workflow:

- name: Download PaExec
  run: |
    choco install wget --no-progress 
    wget -q https://www.poweradmin.com/paexec/paexec.exe -P C:\Windows\System32
- name: Create new user
  run: |
    net user foo p@ssw0rd /add
- name: Test with Maven
  run: |
    paexec -u foo -p p@ssw0rd -lo $pwd\output.txt -w $pwd $pwd\mvnw.cmd test -l $pwd\mvntest.txt 
    cat output.txt
    cat mvntest.txt

Output from the program showing executing "as foo" (output.txt)

PAExec v1.29 - Execute Programs Remotely
Copyright (c) 2012-2021 Power Admin LLC
www.poweradmin.com/PAExec

PAExec starting process ["D:\a\oshi\oshi\mvnw.cmd" test -l D:\a\oshi\oshi\mvntest.txt] as foo

Output from the maven execution of tests (truncated to interesting part -- no elevation, and a user named foo):

Running without elevated permissions.
Sessions:
 LOCAL SERVICE, Console, No login, (NT AUTHORITY)
 NETWORK SERVICE, Console, No login, (NT AUTHORITY)
 foo, Console, No login, (fv-az153-646)
 SYSTEM, Console, No login, (NT AUTHORITY)
...
TEMP=C:\Users\foo\AppData\Local\Temp
TMP=C:\Users\foo\AppData\Local\Temp
USERDOMAIN=fv-az153-646
USERDOMAIN_ROAMINGPROFILE=fv-az153-646
USERNAME=foo
USERPROFILE=C:\Users\foo

So this works, and I think has a friendlier license.

I suspect psexec would work as well; no need to use wget, we can install using

choco install -y --no-progress sysinternals

I'm not an expert on gradle but as long as there is a command-line switch to pipe output to a log file, I suspect you can get this working.

One other change needed: I think if the build fails (return value other than 0) it would never get to the "cat" step, so failure exit codes also need to be handled. This should work:

  - name: Test step that may fail
    id: theteststep
    continue-on-error: true
    run: |
      # insert test here
  - name: Output logs
      # log output here 
  - name: Check if 'Test step that may fail' step failed
    if: steps.theteststep.outcome != 'success'
    run: |
      echo "test failed"
      exit 1

@stephen-crawford
Copy link
Contributor Author

Wow @dbwiddis this is great! Thank you so much for spending your time to help with this. It is greatly appreciated and will mean that the previous check for permissions can be reinstated without worry for blocking the workflows. Again thank you very much!

@stephen-crawford
Copy link
Contributor Author

@dbwiddis as a follow-up/to clarify, do you recommend we use psexec or PaExec? It seems they both have their pros and cons but I know that you seemed to like the licensing better on Pa butt the install process better on Ps. I could script either but do not know if in your opinion there is one that is an overall better option.

@dbwiddis
Copy link
Member

@dbwiddis as a follow-up/to clarify, do you recommend we use psexec or PaExec? It seems they both have their pros and cons but I know that you seemed to like the licensing better on Pa butt the install process better on Ps. I could script either but do not know if in your opinion there is one that is an overall better option.

Personally I'd prefer paexec for licensing. It just feels odd having a script accept a EULA and there's a weird thought we'd need to get lawyers involved to even ask if it was OK. Not sure I'd use paexec in "production" but in a GHA it seems totally fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants