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

RFD 47 and 48 #9475

Closed
wants to merge 1 commit into from
Closed

RFD 47 and 48 #9475

wants to merge 1 commit into from

Conversation

stouset
Copy link

@stouset stouset commented Dec 17, 2021

Closes #6831 and #9375.

Implemented by #9376 and #9377.

@@ -0,0 +1,34 @@
---
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

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.

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.

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!

@klizhentas
Copy link
Contributor

We have discussed this as a product team, our suggestion is to use the following format:

$ 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 ...

Removing needs-product-decision

@r0mant
Copy link
Collaborator

r0mant commented Dec 30, 2022

This was implemented in #10336.

@r0mant r0mant closed this Dec 30, 2022
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.

add TLS client cert and cookie injection to tsh
4 participants