Add a .tsh/config file, add support for configuring custom http headers from the config file #10336

lxea · 2022-02-14T12:41:26Z

This adds a tsh config file to ~/.tsh/config/config.yaml. It includes support for configuring custom http headers as specified in #9838. Support for the aliases configuration option will be created in a seprate PR.

Current supported config file:

add_headers:
  - proxy: "*.example.com"
    headers:
      foo: bar
  - proxy: us-west-1.example.com
    headers:
      baz: qux

Also includes a change to stop tsh logout from blowing all of ~/.tsh away so config files can be retained.

ravicious · 2022-02-14T15:46:33Z

How can I test this myself? I wanted to see how tsh behaves when I provide invalid headers, but I wasn't able to send valid headers first.

I created a request bin on requestbin.com, I compiled tsh from your branch, then added ~/.tsh/config/config.yaml with this content:

add_headers:
  - proxy: *
    headers:
      foo: bar
  - proxy: enj0qqqty1z2.x.pipedream.net
    headers:
      baz: quux

When I do ./build/tsh login --proxy=enj0qqqty1z2.x.pipedream.net, I can see that the request to /webapi/ping doesn't include my headers. Maybe that's just an issue with this specific request.

ravicious

I just want to say that I'm new to both Teleport and Go, so I looked at the changes included in this PR just as GitHub shows them to me, but it'd be hard for me to say if there's a place we forgot to add the extra headers to.

Looks good in general, I just have a small question about hardcoding the path.

Also, when do we document such changes? Do other people write docs for them?

tool/tsh/tsh.go

lxea · 2022-02-14T16:01:13Z

How can I test this myself? I wanted to see how tsh behaves when I provide invalid headers, but I wasn't able to send valid headers first.

I created a request bin on requestbin.com, I compiled tsh from your branch, then added ~/.tsh/config/config.yaml with this content:
add_headers:
  - proxy: *
    headers:
      foo: bar
  - proxy: enj0qqqty1z2.x.pipedream.net
    headers:
      baz: quux
When I do ./build/tsh login --proxy=enj0qqqty1z2.x.pipedream.net, I can see that the request to /webapi/ping doesn't include my headers. Maybe that's just an issue with this specific request.

Is there a log at the beginning of the tsh output complaining about the .yaml file format? I think '*' needs to be in quotes.

Currently it just continues, but maybe it should fail if a config file is invalid, I'm not sure which would be preferred.

ravicious · 2022-02-15T14:37:52Z

I scrolled up to see what was the output after running tsh and there was no log related to the error in config.

However, I did git pull and it updated your branch from ce1acad to a1c87ee and now everything works as expected: the extra headers are sent to /webapi/ping. If the config is invalid, tsh prints information about it, although I'm not sure if it should use stdout rather than stderr.

But yeah, I'm also not sure if it should fail if the config file has invalid YAML. I'd ask people more familiar with tsh than me.

In case the header itself is invalid, tsh fails only when making the actual request, but I think that's okay.

ravicious · 2022-02-15T14:45:09Z

tool/tsh/tsh.go

+	confOptions, err := loadConfig()
+	if err != nil {
+		fmt.Println("Failed to load tsh config: ", err)
+	}


If I remove the config file completely, tsh logs this message:

Failed to load tsh config: open /Users/rav/.tsh/config/config.yaml: no such file or directory

I'm pretty sure we would like to avoid logging this, right? It seems like having no config file should let tsh continue as if confOptions was nil.

We could take inspiration from kubeconfig.Load:

teleport/lib/kube/kubeconfig/kubeconfig.go

Lines 218 to 220 in 84b64fe

// Load tries to read a kubeconfig file and if it can't, returns an error.

// One exception, missing files result in empty configs, not an error.

func Load(path string) (*clientcmdapi.Config, error) {

A design decision I don't agree with, but at least the behavior would be consistent across different config files. ;)

Ah yes - sorry didn't see this comment. I guess it's consistent but it's a poor user experience when replacing a binary on a patch change.

ravicious

I'm not sure about the arg change that I pointed out, but the rest of the code looks fine to me, minus those failing tests.

Leaving an approve so that I don't obstruct your progress there after someone more familiar with the project reviews the PR.

lib/reversetunnel/transport.go

r0mant

Also might be useful to share a dev build with a customer to make sure it addresses their use-case and we didn't miss any requests headers should be injected into.

api/client/webclient/webclient.go

tool/tsh/tsh.go

tool/tsh/tsh_test.go

lib/client/keystore.go

lib/reversetunnel/transport.go

lxea · 2022-02-18T13:22:56Z

Also might be useful to share a dev build with a customer to make sure it addresses their use-case and we didn't miss any requests headers should be injected into.

@r0mant This makes sense, I did try to be thorough while trying to find places headers could be inserted from tsh commands.

lib/web/apiserver.go

api/client/webclient/webclient.go

r0mant · 2022-03-03T00:29:05Z

tool/tsh/tshconfig.go

+// .tsh config must go in a subdir as all .yaml files in .tsh get
+// parsed automatically by the profile loader and results in yaml
+// unmarshal errors.


Can the profile loader be updated to ignore config.yaml file instead?

It could be but I think this would mean clusters couldn't be named config which doesn't seem ideal

This makes sense. Let me check with the product team on this. I think another option could be to just do something like ~/.tshrc which seems consistent with where e.g. shells keep their preferences and it would have another benefit that we won't have to touch ~/.tsh directory. But let's check with product first.

@lxea We have decided that keeping it under ~/.tsh/config/config.yaml is ok for now. Let's merge this as-is.

tool/tsh/tshconfig.go

tool/tsh/tsh.go

api/client/webclient/webclient.go

r0mant · 2022-03-15T21:46:43Z

tool/tsh/tshconfig.go

+// .tsh config must go in a subdir as all .yaml files in .tsh get
+// parsed automatically by the profile loader and results in yaml
+// unmarshal errors.


@lxea We have decided that keeping it under ~/.tsh/config/config.yaml is ok for now. Let's merge this as-is.

…aders

zmb3 · 2022-03-23T14:45:02Z

@lxea please don't forget to open a docs PR documenting this new config file

endzyme · 2022-03-30T14:27:56Z

When this file is missing the tsh version command returns a error. I would have expected maybe a warning vs an outright failure.

lxea · 2022-03-31T10:18:38Z

@endzyme This is fixed here: #11495

github-actions bot requested review from ravicious and timothyb89 February 14, 2022 12:41

github-actions bot added the tsh tsh - Teleport's command line tool for logging into nodes running Teleport. label Feb 14, 2022

lxea force-pushed the lxea/tsh-config branch from 7269027 to a1c87ee Compare February 14, 2022 13:48

ravicious reviewed Feb 14, 2022

View reviewed changes

tool/tsh/tsh.go Outdated Show resolved Hide resolved

tool/tsh/tsh.go Outdated Show resolved Hide resolved

ravicious reviewed Feb 15, 2022

View reviewed changes

lxea force-pushed the lxea/tsh-config branch from a1c87ee to 7322f67 Compare February 16, 2022 16:07

russjones linked an issue Feb 16, 2022 that may be closed by this pull request

Add tsh configuration file #9838

Closed

russjones requested a review from r0mant February 17, 2022 01:41

ravicious approved these changes Feb 17, 2022

View reviewed changes

lib/reversetunnel/transport.go Outdated Show resolved Hide resolved

r0mant reviewed Feb 18, 2022

View reviewed changes

lxea force-pushed the lxea/tsh-config branch from 7322f67 to cbeb23f Compare February 18, 2022 13:21

lxea force-pushed the lxea/tsh-config branch 3 times, most recently from 37e8151 to 66cb586 Compare February 18, 2022 15:09

timothyb89 approved these changes Feb 25, 2022

View reviewed changes

lib/web/apiserver.go Outdated Show resolved Hide resolved

lxea force-pushed the lxea/tsh-config branch 3 times, most recently from 5210182 to c7a29d2 Compare February 28, 2022 09:49

r0mant mentioned this pull request Mar 1, 2022

Respect HTTP_PROXY and HTTPS_PROXY environment variables #9376

Closed

r0mant reviewed Mar 3, 2022

View reviewed changes

lxea force-pushed the lxea/tsh-config branch from c7a29d2 to c51e76a Compare March 8, 2022 10:49

r0mant approved these changes Mar 15, 2022

View reviewed changes

lxea force-pushed the lxea/tsh-config branch from c51e76a to f962118 Compare March 23, 2022 12:41

lxea enabled auto-merge (rebase) March 23, 2022 12:43

lxea force-pushed the lxea/tsh-config branch from f962118 to bbec705 Compare March 23, 2022 13:09

lxea force-pushed the lxea/tsh-config branch from bbec705 to d6051b2 Compare March 23, 2022 13:14

Add a .tsh/config file and add support for configuring custom http he…

5bd2797

…aders

lxea force-pushed the lxea/tsh-config branch from d6051b2 to 5bd2797 Compare March 23, 2022 13:55

lxea merged commit 3d35263 into master Mar 23, 2022

lxea deleted the lxea/tsh-config branch March 23, 2022 14:19

This was referenced Mar 23, 2022

[v9] Backport #10336: Add a tsh config file and add support for configuring custom http headers #11378

Merged

[v8] backport #10336: Add a .tsh/config file and add support for configuring custom http headers #11379

Merged

lxea mentioned this pull request Mar 29, 2022

Add documentation for the tsh config file #11531

Merged

This was referenced Dec 30, 2022

Allow custom HTTP headers in API requests provided through the environment #9375

Closed

RFD 47 and 48 #9475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a .tsh/config file, add support for configuring custom http headers from the config file #10336

Add a .tsh/config file, add support for configuring custom http headers from the config file #10336

lxea commented Feb 14, 2022 •

edited

Loading

ravicious commented Feb 14, 2022

ravicious left a comment

lxea commented Feb 14, 2022 •

edited

Loading

ravicious commented Feb 15, 2022

ravicious Feb 15, 2022

endzyme Mar 30, 2022

ravicious left a comment

r0mant left a comment

lxea commented Feb 18, 2022 •

edited

Loading

r0mant Mar 3, 2022

lxea Mar 8, 2022

r0mant Mar 11, 2022

r0mant Mar 15, 2022

r0mant Mar 15, 2022

zmb3 commented Mar 23, 2022

endzyme commented Mar 30, 2022

lxea commented Mar 31, 2022

	// Load tries to read a kubeconfig file and if it can't, returns an error.
	// One exception, missing files result in empty configs, not an error.
	func Load(path string) (*clientcmdapi.Config, error) {

Add a .tsh/config file, add support for configuring custom http headers from the config file #10336

Add a .tsh/config file, add support for configuring custom http headers from the config file #10336

Conversation

lxea commented Feb 14, 2022 • edited Loading

ravicious commented Feb 14, 2022

ravicious left a comment

Choose a reason for hiding this comment

lxea commented Feb 14, 2022 • edited Loading

ravicious commented Feb 15, 2022

ravicious Feb 15, 2022

Choose a reason for hiding this comment

endzyme Mar 30, 2022

Choose a reason for hiding this comment

ravicious left a comment

Choose a reason for hiding this comment

r0mant left a comment

Choose a reason for hiding this comment

lxea commented Feb 18, 2022 • edited Loading

r0mant Mar 3, 2022

Choose a reason for hiding this comment

lxea Mar 8, 2022

Choose a reason for hiding this comment

r0mant Mar 11, 2022

Choose a reason for hiding this comment

r0mant Mar 15, 2022

Choose a reason for hiding this comment

r0mant Mar 15, 2022

Choose a reason for hiding this comment

zmb3 commented Mar 23, 2022

endzyme commented Mar 30, 2022

lxea commented Mar 31, 2022

lxea commented Feb 14, 2022 •

edited

Loading

lxea commented Feb 14, 2022 •

edited

Loading

lxea commented Feb 18, 2022 •

edited

Loading