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

RFD 47 and 48 #9475

Closed
wants to merge 1 commit into from
+78 −0
Closed

RFD 47 and 48 #9475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions rfd/0047-environment-proxies.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
---
authors: Stephen Touset ([email protected])
state: draft
---

# RFD 47 - Environment-Configured Proxies
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me. 👍 Thank you!


## What

This RFD proposes that all HTTP clients should be configurable to perform
requests through intermediate proxies, using the standard `HTTP_PROXY`,
`HTTPS_PROXY`, and `NOPROXY` enviroment variables.

## Why

Client environments sometimes need to issue requests through proxy servers,
either to get out of restricted corporate networks or to get into
[BeyondCorp][https://cloud.google.com/beyondcorp] production environments.

## Scope

The changes should be relatively limited in scope, requiring only that HTTP
clients are initialized with a transport that respects these environment
variables. The golang `http` library ships a feature that transparently enables
this behavior.

## UX

Teleport gains the power to be invoked with environment variables specifying the
locations of HTTP proxies.

```bash
env HTTPS_PROXY=http://proxy.example.com:80 tsh login --proxy teleport-proxy.example.com
```
44 changes: 44 additions & 0 deletions rfd/0048-environment-headers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the contents of this RFD are incorrect, looks like a copy of rfd/0047-environment-proxies.md. Do you mind updating the contents?

However, based on #9375 and #9377 I understand what you are trying to do.

Instead of using an environment variable to read in headers to inject, let's add a ~/.tsh/headers file. If tsh finds this file, it will read it in and use it to set headers for HTTP requests to the proxy. tsh itself won't ever write to this file, so it will be preserved across multiple logout/login.

The format of this file should be YAML key/value pairs. For now let's only support strings with no evaluation to prevent log4j-ing ourselves.

So something like, this:

$ cat ~/.tsh/headers
foo: bar
baz: qux

Copy link
Author

Choose a reason for hiding this comment

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

Fixed!

Can do. Would you like me to update the implementation or will you?

Copy link
Author

@stouset stouset Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, on further consideration, we have a few issues with this approach.

First is that the secret is dynamic. It rotates frequently and on an unpredictable basis, so in practice this file will need to be updated immediately before every tsh invocation.

Second—and more important—is that the secret is proxy-specific. It must not be sent to the wrong server, and should only be used when connecting to a specific proxy. We could add the secret immediately before tsh and remove it immediately after, but that leaves a window where another tsh invocation to a different location sends along a critical secret. And of course, cleanup may never even run for a variety of reasons.

Both of these combined lead me to the conclusion that a configuration file is not the best means of specifying this behavior. I do still believe that an environment variable is the most appropriate choice here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a wrapper that sets TELEPORT_WEBCLIENT_HEADERS based off the proxy you are trying to access? Because otherwise you'd still have the same problem because TELEPORT_WEBCLIENT_HEADERS will be applied to all tsh invocations?

Copy link
Author

Choose a reason for hiding this comment

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

That is correct. We have a wrapper that sets this for the tsh invocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

@klizhentas @xinding33 Based off the requirements above (needing to define headers per-cluster), what do you think about the following?

$ cat ~/.tsh/headers
us-west-1.example.com:
  foo: bar
  baz: qux
us-west-2.example.com:
  foo: bar
  baz: qux

Copy link
Author

Choose a reason for hiding this comment

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

That's a small improvement, but we will need to update this file with every tsh invocation anyway.

What's the underlying draw here for this to be configurable with a file on-disk and not through environment variables? I'm trying to understand the advantages you're seeing in this approach so we can come up with something that works best.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stouset We've been having some internal discussion about this. We like using a configuration file because we're thinking about extending this functionality further than just headers in the future.

After another round of discussion internally, we've come up with the following format for a configuration file that will include a add_headers block that can be used to inject headers into requests.

$ cat ~/.tsh/config.yaml
add_headers:
  - proxy: *.example.com
    headers:
      foo: bar
  - proxy: us-west-1.example.com
    headers:
      baz: qux
aliases:
   ls: tsh ls | awk ...

Since this now clearly goes outside of the scope of what you need (as we will be supporting aliases, etc), we can continue the RFD from here.

That being said, I do want to double check with you that a configuration file like the above would solve your problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

#9838

Copy link
Author

Choose a reason for hiding this comment

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

We'd prefer not having to update a YAML file every tsh invocation, but if that's the approach you prefer to use then we can make it work.

authors: Stephen Touset ([email protected])
state: draft
---

# RFD 48 - Environment-Configured Custom HTTP Headers

## What

This RFD proposes that all HTTP clients should be configurable to add custom,
opaque HTTP headers to their requests using an environment variable.

## Why

At Square, we connect from corporate laptops to services inside of our cloud and
datacenter enviornments through a [BeyondCorp][https://cloud.google.com/beyondcorp]
proxy. Clients authenticate to the proxy by providing a specially-crafted HTTP
header that contains an opaque authentication token.

As such, we need a way to inject this header into requests made by the
webclient.

## Scope

These changes only affect the webclient (and roundtrip library), causing them to
look for user-provided headers in an environment variable and inserting those
headers into HTTP requests.

## UX

Teleport gains the power to be invoked with environment variables specifying
custom HTTP headers. Multiple headers may be separated with

```bash
env TELEPORT_WEBCLIENT_HEADERS="Authorization: Basic xxxx\nCookie: some_cookie=yyy" \
tsh login --proxy teleport-proxy.example.com
```

## Security

If an attacker can control a user's environment when invoking the `tsh` command,
the user has already lost (e.g., `env LD_PRELOAD=/path/to/evil.so tsh`). So I do
not believe we need to whitelist or blacklist headers that we allow to be
provided with this mechanism.