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

server,*: the go runtime and vendored deps are sensitive to environment variable #66838

Open
knz opened this issue Jun 24, 2021 · 3 comments
Open
Labels
A-observability-inf C-investigation Further steps needed to qualify. C-label will change. T-observability

Comments

@knz
Copy link
Contributor

knz commented Jun 24, 2021

See discussion in #66805: at least the Go runtime http package transparently uses the HTTP_PROXY, HTTPS_PROXY, NO_PROXY and REQUEST_TYPE env vars to issue HTTP requests.

This impacts e.g:

We have at least one customer who is requesting crdb to not be influenced by env vars generally, especially not those that have an impact on networking, unless opted in explicitly for specific features (E.g. include a proxy flag in the BACKUP SQL statement)

What to do about this?

Option 1: unset the env vars explicitly when the process starts up.

This option is to unset all env vars except for a few "allowlist" (including the COCKROACH_ env vars, GOMAXPROCS etc and perhaps TERM).

We cannot use an env var denylist: as detailed in #66805 there are dozens of environment variables that may influence the process implicitly. Even if we had an explicit list of variables to clear before the process starts up, we'd need to manually review that list every time we upgrade a dependency, which could introduce new env vars.

Pros: very conservative. Certainty of no impact from env vars anywhere.

Cons: for certain deployment scenarios it's actually beneficial to let some amount of control on the process "from the outside" to the system administrator. Having a blanket ban on pre-set env vars would completely shut out system administrators from what's customizable in the go source code.

In fact we even had customer demand for the HTTP_PROXY env var before (#34067) and this option would break that use case.

Option 2: inform in logs and docs, and recommend env var hygiene in orchestration

In this option, we keep the current behavior and let the process be influenced by env vars.
We would do three things:

  1. audit the code for areas where we want to opt out of env var impact for a good reason, for example what we did already in rpc: disable use of http proxies by default #55502
  2. produce logs upon startup including all env vars (but maybe not their value) to ensure we have an audit trail in support cases
  3. document that env vars may have an impact in our official docs, and make a recommendation to deploy crdb in such a way that only the specific desired env vars are visible to the server processes.

Pros: moves control in the hands of the user DBA/SRE

Cons: unsure if there is one

Jira issue: CRDB-8253

@knz knz added the C-investigation Further steps needed to qualify. C-label will change. label Jun 24, 2021
@knz
Copy link
Contributor Author

knz commented Jun 24, 2021

@bdarnell suggests:

the usual approach is not "use a proxy for s3 and nothing else", but "use this proxy for everything, and the proxy will disallow anything but s3"

@knz
Copy link
Contributor Author

knz commented Jun 24, 2021

@dt suggests:

i would be fine with, in addition to supporting the usual env vars as we do now, allowing a &proxy_uri=... on an s3 URI that is used only if a) outbound network is allowed and b) a proxy is not configured in the env

craig bot pushed a commit that referenced this issue Jul 30, 2021
66842: envutil,server: report more env vars upon server startup and in telemetry r=cameronnunez a=knz

Supersedes #66805. 
Informs #66838 (option 2).

We want more visibility into the env vars that influence the server's
state, for example HTTP_PROXY or more generally those env vars that
our dependency and the go runtime observe.

However, we need to be careful:

- we can't report *all* env vars because there are many cases where
  even the name of an env var or its presence is confidential
  information.
- even for the env vars where we're confident that we can
  report that it is set, we cannot always report its value
  because the value itself could be confidential (e.g. HTTP_PROXY).

This patch thus adds env var reporting with a nuance between the
following:

- safe name, safe value (e.g. GOMAXPROCS)
- safe name, redactable value (e.g. TERM)
- safe name, no value (e.g HTTP_PROXY)

Example:
![image](https://user-images.githubusercontent.com/642886/127644871-816b4e0d-4de8-4e37-a451-dcdafc8b4bc4.png)


Release note (cli change): CockroachDB server nodes now report
more environment variables in logs upon startup. Only certain
environment variables that may have an influence on the server's
behavior are reported.

Co-authored-by: Raphael 'kena' Poss <[email protected]>
@github-actions
Copy link

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-observability-inf C-investigation Further steps needed to qualify. C-label will change. T-observability
Projects
None yet
Development

No branches or pull requests

1 participant