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

adjust WSL detection #209

Merged
merged 3 commits into from
Jul 27, 2022
Merged

adjust WSL detection #209

merged 3 commits into from
Jul 27, 2022

Conversation

xaevman
Copy link
Contributor

@xaevman xaevman commented Jul 22, 2022

Overview

Working within containers as an interactive developer environment (devcontainer) is becoming more prevalent, but improper WSL detection when using OIDC based authentication methods with Vault and Boundary makes using these command line tools problematic in such an environment.

The current method for detecting WSL is to examine /proc/version, looking for the presence of the string "microsoft". However, since Microsoft also uses their linux kernels as the basis of their published devcontainer images/layers, this causes the isWSL() function to return true within that environment as well. When openURL() is subsequently called in order to launch a browser and complete authentication, vault and boundary will opt to try and launch cmd.exe, thinking the environment is WSL, which then fails.

This PR is a proposal for one way to disambiguate this situation, resulting in the auth processes properly calling xdg-open instead of cmd.exe when run in a linux container.

Design of Change

When detecting WSL, compare both the output of /proc/version as well as the root of the hierarchies in /proc/1/cgroup to determine if we are running within Docker or LCX. Assert that we are WSL only if we detect a microsoft linux kernel and are also not running from within a container.

I am not an expert in runtime detection of containerization. It's possible there may be better ways to detect that scenario.

Related Issues/Pull Requests

I did not file an issue separately since I was proposing a PR fix. Is one needed?

Contributor Checklist

[x ] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet.
Not a new feature and did not find existing documentation RE: how the command line tools launch external browsers.
[ x] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
Unsure how to document output from the launch of an external application but I did compile both vault and boundary (which uses the cap library and a slightly different function signature) and test within a devcontainer with success.
[x ] Backwards compatible

imporoper WSL detection causes problems when developing inside of
devcontainers

- running a microsoft linux distro instead of a container is not WSL
@hashicorp-cla
Copy link

hashicorp-cla commented Jul 22, 2022

CLA assistant check
All committers have signed the CLA.

@austingebauer
Copy link
Contributor

Thanks for submitting this, @xaevman. Any chance you'd be willing to switch this plugin over to using the code from hashicorp/cap#51 directly? It would be a similar to #166. No worries if you'd rather not take on that additional work.

@xaevman
Copy link
Contributor Author

xaevman commented Jul 27, 2022

I went ahead and updated to call the cap functions directly and deleted the duplicate functions within this file. Is this minimal change what you were looking for? In general, I'm not yet familiar enough with everything in cap to confidently go hunting for other duplicate implementations to replace :)

@austingebauer
Copy link
Contributor

Thanks for doing that! That's the change that I was looking for (no need to replace anything else with cap code). I'm going to give this a quick test and will approve/merge if things look good.

cli.go Show resolved Hide resolved
Copy link
Contributor

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks again for this @xaevman.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants