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

Use relevant SSHProxyAddr after Login #2657

Merged
merged 1 commit into from
Apr 24, 2019
Merged

Use relevant SSHProxyAddr after Login #2657

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
46 changes: 30 additions & 16 deletions lib/client/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright 2016 Gravitational, Inc.
Copyright 2016-2019 Gravitational, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -70,7 +70,7 @@ var log = logrus.WithFields(logrus.Fields{
})

const (
// Directory location where tsh profiles (and session keys) are stored
// ProfileDir is a directory location where tsh profiles (and session keys) are stored
ProfileDir = ".tsh"
)

Expand Down Expand Up @@ -446,6 +446,9 @@ func Status(profileDir string, proxyHost string) (*ProfileStatus, []*ProfileStat
if file.IsDir() {
continue
}
if file.Mode()&os.ModeSymlink != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment explaining why symlinks are skipped because it's not clear from reading this function that you are skipping over aliases.

// Skip over symlinks (aliases) to prevent double printing of profiles.

continue
}
if !strings.HasSuffix(file.Name(), ".yaml") {
continue
}
Expand Down Expand Up @@ -525,7 +528,7 @@ func (c *Config) LoadProfile(profileDir string, proxyName string) error {

// SaveProfile updates the given profiles directory with the current configuration
// If profileDir is an empty string, the default ~/.tsh is used
func (c *Config) SaveProfile(profileDir string, profileOptions ...ProfileOptions) error {
func (c *Config) SaveProfile(profileAliasHost, profileDir string, profileOptions ...ProfileOptions) error {
if c.WebProxyAddr == "" {
return nil
}
Expand All @@ -535,6 +538,11 @@ func (c *Config) SaveProfile(profileDir string, profileOptions ...ProfileOptions
profileDir = FullProfilePath(profileDir)
profilePath := path.Join(profileDir, webProxyHost) + ".yaml"

profileAliasPath := ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Proxy alias logic is split in two different places in SaveProfile and SaveTo. However, SaveTo appears to mainly be working with files on disk where as SaveProfile is updating configuration.

Moving that configuration logic from SaveTo into SaveProfile would simply things. Something like the following.

// Set alias path if it differs from the address advertised by Teleport.
aliasPath = ""
if profilePath != profileAliasPath {
    aliasPath = profileAliasPath
}

err := SaveTo(&profile{
    path: profilePath,
    alias: aliasPath,
    options: ...,
})

Then SaveTo once again only works with files on disk:

// If a alias has been provided, update the symlink to to point to the profile
// that Teleport advertises.
if p.alias != "" {
    if err := os.Remove(profileAliasPath); err != nil {
        log.Warningf("Failed to remove symlink alias: %v", err)
    }

    err := os.Symlink(filepath.Base(filePath), profileAliasPath)
    if err != nil {
        log.Warningf("Failed to create profile alias: %v", err)
    }
}

if profileAliasHost != "" {
profileAliasPath = path.Join(profileDir, profileAliasHost) + ".yaml"
}

var cp ClientProfile
cp.Username = c.Username
cp.WebProxyAddr = c.WebProxyAddr
Expand All @@ -553,7 +561,7 @@ func (c *Config) SaveProfile(profileDir string, profileOptions ...ProfileOptions
opts |= flag
}
}
if err := cp.SaveTo(profilePath, opts); err != nil {
if err := cp.SaveTo(profileAliasPath, profilePath, opts); err != nil {
return trace.Wrap(err)
}
return nil
Expand All @@ -562,7 +570,7 @@ func (c *Config) SaveProfile(profileDir string, profileOptions ...ProfileOptions
// ParseProxyHost parses the proxyHost string and updates the config.
//
// Format of proxyHost string:
// proxy_web_addr:<proxy_web_port>,<proxy_ssh_portt>
// proxy_web_addr:<proxy_web_port>,<proxy_ssh_port>
func (c *Config) ParseProxyHost(proxyHost string) error {
host, port, err := net.SplitHostPort(proxyHost)
if err != nil {
Expand Down Expand Up @@ -1408,7 +1416,6 @@ func (tc *TeleportClient) connectToProxy(ctx context.Context) (*ProxyClient, err
var err error

proxyPrincipal := tc.getProxySSHPrincipal()
proxyAddr := tc.Config.SSHProxyAddr
sshConfig := &ssh.ClientConfig{
User: proxyPrincipal,
HostKeyCallback: tc.HostKeyCallback,
Expand All @@ -1419,7 +1426,7 @@ func (tc *TeleportClient) connectToProxy(ctx context.Context) (*ProxyClient, err
return &ProxyClient{
teleportClient: tc,
Client: sshClient,
proxyAddress: proxyAddr,
proxyAddress: tc.Config.SSHProxyAddr,
proxyPrincipal: proxyPrincipal,
hostKeyCallback: sshConfig.HostKeyCallback,
authMethod: m,
Expand All @@ -1428,14 +1435,14 @@ func (tc *TeleportClient) connectToProxy(ctx context.Context) (*ProxyClient, err
clientAddr: tc.ClientAddr,
}
}
successMsg := fmt.Sprintf("Successful auth with proxy %v", proxyAddr)
successMsg := fmt.Sprintf("Successful auth with proxy %v", tc.Config.SSHProxyAddr)
// try to authenticate using every non interactive auth method we have:
for i, m := range tc.authMethods() {
log.Infof("Connecting proxy=%v login='%v' method=%d", proxyAddr, sshConfig.User, i)
log.Infof("Connecting proxy=%v login='%v' method=%d", tc.Config.SSHProxyAddr, sshConfig.User, i)
var sshClient *ssh.Client

sshConfig.Auth = []ssh.AuthMethod{m}
sshClient, err = ssh.Dial("tcp", proxyAddr, sshConfig)
sshClient, err = ssh.Dial("tcp", tc.Config.SSHProxyAddr, sshConfig)
if err != nil {
if utils.IsHandshakeFailedError(err) {
log.Warn(err)
Expand All @@ -1450,35 +1457,38 @@ func (tc *TeleportClient) connectToProxy(ctx context.Context) (*ProxyClient, err
// is disabled in configuration, or the user refused connecting to untrusted hosts
if tc.Config.SkipLocalAuth || tc.localAgent.UserRefusedHosts() {
if err == nil {
err = trace.BadParameter("failed to authenticate with proxy %v", proxyAddr)
err = trace.BadParameter("failed to authenticate with proxy %v", tc.Config.SSHProxyAddr)
}
return nil, trace.Wrap(err)
}
// if we get here, it means we failed to authenticate using stored keys
// and we need to ask for the login information
// Login reaches out to ping endpoint of the proxy
// and has a side effect of updating tc.Config.SSHProxyAddr
// and other parameters, that's why they are referenced directly below
key, err := tc.Login(ctx, true)
if err != nil {
// we need to communicate directly to user here,
// otherwise user will see endless loop with no explanation
if trace.IsTrustError(err) {
fmt.Printf("Refusing to connect to untrusted proxy %v without --insecure flag\n", proxyAddr)
fmt.Printf("Refusing to connect to untrusted proxy %v without --insecure flag\n", tc.Config.SSHProxyAddr)
}
return nil, trace.Wrap(err)
}
// Save profile to record proxy credentials
if err := tc.SaveProfile("", ProfileCreateNew); err != nil {
if err := tc.SaveProfile(key.ProxyHost, "", ProfileCreateNew|ProfileMakeCurrent); err != nil {
log.Warningf("Failed to save profile: %v", err)
}
authMethod, err := key.AsAuthMethod()
if err != nil {
return nil, trace.Wrap(err)
}

// After successful login we have local agent updated with latest
// and greatest auth information, try it now
sshConfig.Auth = []ssh.AuthMethod{authMethod}
sshConfig.User = proxyPrincipal
sshClient, err := ssh.Dial("tcp", proxyAddr, sshConfig)
sshConfig.User = tc.getProxySSHPrincipal()
log.Infof("Connecting proxy=%v login='%v' using local agent.", tc.Config.SSHProxyAddr, sshConfig.User)
sshClient, err := ssh.Dial("tcp", tc.Config.SSHProxyAddr, sshConfig)
if err != nil {
return nil, trace.Wrap(err)
}
Expand Down Expand Up @@ -1535,6 +1545,9 @@ func (tc *TeleportClient) Login(ctx context.Context, activateKey bool) (*Key, er
}
}

// preserve original web proxy host that could have
webProxyHost, _ := tc.WebProxyHostPort()

if err := tc.applyProxySettings(pr.Proxy); err != nil {
return nil, trace.Wrap(err)
}
Expand Down Expand Up @@ -1592,6 +1605,7 @@ func (tc *TeleportClient) Login(ctx context.Context, activateKey bool) (*Key, er
// extract the new certificate out of the response
key.Cert = response.Cert
key.TLSCert = response.TLSCert
key.ProxyHost = webProxyHost

if len(response.HostSigners) <= 0 {
return nil, trace.BadParameter("bad response from the server: expected at least one certificate, got 0")
Expand Down
15 changes: 13 additions & 2 deletions lib/client/profile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,18 +131,29 @@ func ProfileFromFile(filePath string) (*ClientProfile, error) {
}

// SaveTo saves the profile into a given filename, optionally overwriting it.
func (cp *ClientProfile) SaveTo(filePath string, opts ProfileOptions) error {
func (cp *ClientProfile) SaveTo(profileAliasPath, filePath string, opts ProfileOptions) error {
bytes, err := yaml.Marshal(&cp)
if err != nil {
return trace.Wrap(err)
}
if err = ioutil.WriteFile(filePath, bytes, 0660); err != nil {
return trace.Wrap(err)
}
if profileAliasPath != "" && filepath.Base(profileAliasPath) != filepath.Base(filePath) {
if err := os.Remove(profileAliasPath); err != nil {
log.Warningf("Failed to remove symlink alias: %v", err)
}
err := os.Symlink(filepath.Base(filePath), profileAliasPath)
if err != nil {
log.Warningf("Failed to create profile alias: %v", err)
}
}
// set 'current' symlink:
if opts&ProfileMakeCurrent != 0 {
symlink := filepath.Join(filepath.Dir(filePath), CurrentProfileSymlink)
os.Remove(symlink)
if err := os.Remove(symlink); err != nil {
log.Warningf("Failed to remove symlink: %v", err)
}
err = os.Symlink(filepath.Base(filePath), symlink)
}
return trace.Wrap(err)
Expand Down
18 changes: 14 additions & 4 deletions lib/client/profile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright 2016 Gravitational, Inc.
Copyright 2016-2019 Gravitational, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -42,11 +42,11 @@ func (s *ProfileTestSuite) TestEverything(c *check.C) {
pfile := path.Join(home, "test.yaml")

// save to a file:
err := p.SaveTo(pfile, 0)
err := p.SaveTo("", pfile, 0)
c.Assert(err, check.IsNil)

// try to save to non-existent dir, should get an error
err = p.SaveTo("/bad/directory/profile.yaml", 0)
err = p.SaveTo("", "/bad/directory/profile.yaml", 0)
c.Assert(err, check.NotNil)

// make sure there is no symlink:
Expand All @@ -55,7 +55,7 @@ func (s *ProfileTestSuite) TestEverything(c *check.C) {
c.Assert(os.IsNotExist(err), check.Equals, true)

// save again, this time with a symlink:
p.SaveTo(pfile, ProfileMakeCurrent)
p.SaveTo("", pfile, ProfileMakeCurrent)
stat, err := os.Stat(symlink)
c.Assert(err, check.IsNil)
c.Assert(stat.Size() > 10, check.Equals, true)
Expand All @@ -69,4 +69,14 @@ func (s *ProfileTestSuite) TestEverything(c *check.C) {
clone, err = ProfileFromDir(home, "test")
c.Assert(err, check.IsNil)
c.Assert(*clone, check.DeepEquals, *p)

// Save with alias
aliasPath := path.Join(home, "alias.yaml")
err = p.SaveTo(aliasPath, pfile, ProfileMakeCurrent)
c.Assert(err, check.IsNil)

// Load from alias works
clone, err = ProfileFromDir(home, "alias")
c.Assert(err, check.IsNil)
c.Assert(*clone, check.DeepEquals, *p)
}
16 changes: 10 additions & 6 deletions tool/tsh/tsh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright 2016 Gravitational, Inc.
Copyright 2016-2019 Gravitational, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -398,7 +398,7 @@ func onLogin(cf *CLIConf) {
// but cluster is specified, treat this as selecting a new cluster
// for the same proxy
case (cf.Proxy == "" || host(cf.Proxy) == host(profile.ProxyURL.Host)) && cf.SiteName != "":
tc.SaveProfile("")
tc.SaveProfile("", "")
Copy link
Contributor

@russjones russjones Apr 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

SaveProfile should take a struct of params, SaveProfile("", "") is not clear what's being saved.

This would also simply things when no alias is needed (see other comment), because you could do things like:

tc.SaveProfile(&profile{
   path: "/path/to/profile",
})

if err := kubeclient.UpdateKubeconfig(tc); err != nil {
utils.FatalError(err)
}
Expand Down Expand Up @@ -442,7 +442,7 @@ func onLogin(cf *CLIConf) {
}

// Regular login without -i flag.
tc.SaveProfile("")
tc.SaveProfile(key.ProxyHost, "")

// Print status to show information of the logged in user. Update the
// command line flag (used to print status) for the proxy to make sure any
Expand Down Expand Up @@ -495,7 +495,10 @@ func onLogout(cf *CLIConf) {
utils.FatalError(err)
return
}
profiles := append(available, active)
profiles := append([]*client.ProfileStatus{}, available...)
if active != nil {
profiles = append(profiles, active)
}

// Extract the proxy name.
proxyHost, _, err := net.SplitHostPort(cf.Proxy)
Expand Down Expand Up @@ -847,15 +850,16 @@ func makeClient(cf *CLIConf, useProfileLogin bool) (tc *client.TeleportClient, e
fmt.Printf("WARNING: Failed to load tsh profile for %q: %v\n", cf.Proxy, err)
}
}

// 3: override with the CLI flags
if cf.Namespace != "" {
c.Namespace = cf.Namespace
}
if cf.Username != "" {
c.Username = cf.Username
}
if cf.Proxy != "" {
// if proxy is set, and proxy is not equal to profile's
// loaded addresses, override the values
if cf.Proxy != "" && c.WebProxyAddr == "" {
err = c.ParseProxyHost(cf.Proxy)
if err != nil {
return nil, trace.Wrap(err)
Expand Down