-i flag for `tsh ssh` implementation. #1063

kontsevoy · 2017-06-10T04:40:20Z

This PR implements #1033

This commit does not change tctl behavior

Fixes #857

kontsevoy · 2017-06-10T04:55:00Z

retest this please

First part of addressing #1033 is ability to load credentials from the credentials file(s). This commit adds -i flag processing, i.e. a certificate can be fed via a cert.file and used to login.

Hooked up -i auth into the proper place

kontsevoy · 2017-06-12T02:32:15Z

retest this please

russjones · 2017-06-12T04:44:06Z

tool/tsh/common/tsh.go

+		caCert []byte
+	)
+	// read the identity file line by line:
+	scanner := bufio.NewScanner(f)


Do we control the format of the identity file? I see that we use tctl auth sign to generate it. If that's the case, what do you think about being more strict about the format, something like this:

Line 1: User public in authorized_keys format. Line 2: Host public key in known_hosts format. Line 3-n: User private key in PEM format.

A couple of advantages of this:

This making parsing this file much easier. You pull off the first line and process it. Pull off the second line and process it. Then take the rest of the input and process it as a PEM file.

OpenSSH interoperability because OpenSSH stores user and host public keys in different formats.

russjones · 2017-06-12T04:52:49Z

tool/tsh/common/tsh.go

+		}
+		// found CA cert in the indentity file? construct the host key checking function
+		// and return it:
+		hostAuthFunc = func(host string, a net.Addr, hostKey ssh.PublicKey) error {


Why not add the certificate to ~/.tsh/known_hosts here? If the user is explicitly passing in a identity with the -i flag, they're effectively bypassing the interactive parts of the Teleport login. A successful interactive login ends up with the user having a signed certificate plus the certificate of the CA being added to ~/.tsh/known_hosts so shouldn't -i do the same thing?

russjones · 2017-06-12T05:17:48Z

lib/client/keyagent.go

 			return nil
 		}
 	}
-	err = trace.AccessDenied("untrusted host %v", hostId)
-	log.Error(err)
+	// final step: lets ask user:


Can you update the comment here to something like: "final step: we have not seen this host certificate before, prompt the user, then add to known hosts".

On my first pass I was a little confused why we were always asking the user in the final step when they had a certificate. I missed the return nil a few lines up because typically you return an error in the body and return nil (success) at the end.

Actually it appears we now do always prompt users if they trust the host?

added the comments, fixed the 3rd party agent-stored keys issue

russjones · 2017-06-12T05:19:35Z

lib/client/keyagent.go

+		if !a.noHosts[host] {
+			fmt.Printf("The authenticity of host '%s' can't be established. "+
+				"Its public key is:\n%s\nAre you sure you want to continue (yes/no)? ",
+				host, ssh.MarshalAuthorizedKey(key))


I'm curious why you went with authorized_keys format here over known_hosts format? Also why not a key fingerprint like OpenSSH does?

russjones · 2017-06-12T05:31:59Z

lib/client/api.go

-	if tc.Config.SkipLocalAuth {
-		return nil, trace.BadParameter("failed to authenticate with proxy %v", proxyAddr)
+	// is disabled in configuration, or the user refused connecting to untrusted hosts
+	if tc.Config.SkipLocalAuth || tc.localAgent.UserRefusedHosts() {


It might be useful to have these in separate blocks then return different errors. Knowing if authentication failed because because all auth methods were exhausted or because of an invalid host certificate would be useful when debugging issues.

russjones · 2017-06-12T05:33:14Z

lib/client/keyagent.go

+
+	// map of "no hosts". these are hosts that user manually (via keyboard
+	// input) refused connecting to.
+	noHosts map[string]bool


When would noHosts be used? It doesn't appear to be persisted to disk and isn't LocalKeyAgent recreated everytime you run tsh?

russjones · 2017-06-12T05:42:47Z

lib/client/keyagent.go

@@ -258,7 +292,7 @@ func (a *LocalKeyAgent) AddKey(host string, username string, key *Key) (*CertAut
 	// save it to disk (usually into ~/.tsh)
 	err := a.keyStore.AddKey(host, username, key)
 	if err != nil {
-		return nil, trace.Wrap(err)
+


I think you might have removed this line accidentally?

russjones · 2017-06-12T16:57:47Z

tool/tctl/common/tctl.go

+			defer f.Close()
+		}
+		// write key:
+		if _, err = output.Write(privateKey); err != nil {


Not sure how I feel about this, in general I think it's better to generate the private key on the client and then send the public key to be signed by the CA.

russjones · 2017-06-12T17:03:58Z

tool/tctl/common/tctl.go

+	//
+	// dump user identity into a single file:
+	//
+	case IdentityFormatFile:


I'm not seeing the host certificate being exported and as far as I can tell the --type flag does not exist? Because the following from the issue does not work: tctl auth sign --type=host.

Also, we should probably add some documentation for how to use this feature to the Admin Guide.

tctl now dumps the identity to stdout by default but you can set -o to something else

russjones · 2017-06-12T17:05:06Z

tool/tctl/common/tctl.go

+			a.output = fmt.Sprintf("%s.pem", a.genUser)
+		}
+		var (
+			output  io.Writer


This isn't really consistent with how tctl has worked up to this point, tctl auth export and tctl saml export output to stdout and we redirect to a file.

key-agent changes: tsh now ignores public keys loaded from the SSH key agent unless they are certificates. this fixes the problem of 'host auth' callback being called for 3rd party public keys in the SSH-agent (and tsh had no choice but to prompt user if he wanted to add thoses hosts to known_hosts file) tctl auth changes: `tctl auth` now outputs to stdout by default. If you set -o to a file, it will use that file instead. Getting rid of -o was not possible because it's still useful if --format is set to `dir` (OpenSSH compatible output). other changes: - added more detailed comments in a few places - restored error handling logic

If '0' is provided as the default and the initial call to create a gateway fails, the user will be presented with an input that says 0. 0 is an invalid value though that this input will not accept. An empty string, on the other hand, is a perfectly valid value (the input doesn't have the `required` attribute). In that scenario, the backend will default the value to '0' and pick a random port.

kontsevoy added 3 commits June 9, 2017 21:25

Minor chnges to tctl (langauge/naming)

91bd5d6

This commit does not change tctl behavior

Merge remote-tracking branch 'origin/master' into ev/i

c0d7ecb

Removed "upserted" language from tctl CLI

7ed43d6

Fixes #857

kontsevoy requested a review from russjones June 10, 2017 04:40

Added a bit more logging to session creation code

efaf2dc

russjones approved these changes Jun 11, 2017

View reviewed changes

kontsevoy force-pushed the ev/i branch from 77f1b05 to 1034498 Compare June 11, 2017 20:35

Implemented identity loading

37319d6

First part of addressing #1033 is ability to load credentials from the credentials file(s). This commit adds -i flag processing, i.e. a certificate can be fed via a cert.file and used to login.

kontsevoy force-pushed the ev/i branch from 1034498 to 37319d6 Compare June 11, 2017 20:37

kontsevoy added 2 commits June 11, 2017 18:52

Added host authorization to identity-based auth

ef790ee

Hooked up -i auth into the proper place

Covered host authentication with tests

3ff5820

kontsevoy changed the title ~~Cosmetic UX changes to tctl CLI~~ -i flag for tsh ssh implementation. Jun 12, 2017

kontsevoy force-pushed the ev/i branch from 3fcfeea to 08951e3 Compare June 12, 2017 04:09

Merge branch 'master' into ev/i

d046eae

kontsevoy force-pushed the ev/i branch from 08951e3 to d046eae Compare June 12, 2017 04:54

russjones reviewed Jun 12, 2017

View reviewed changes

kontsevoy closed this Jun 12, 2017

kontsevoy mentioned this pull request Jun 12, 2017

Identity-based auth #1067

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

-i flag for `tsh ssh` implementation. #1063

-i flag for `tsh ssh` implementation. #1063

kontsevoy commented Jun 10, 2017 •

edited

Loading

kontsevoy commented Jun 10, 2017

kontsevoy commented Jun 12, 2017

russjones Jun 12, 2017

russjones Jun 12, 2017

russjones Jun 12, 2017

russjones Jun 12, 2017

kontsevoy Jun 12, 2017

russjones Jun 12, 2017

russjones Jun 12, 2017

russjones Jun 12, 2017

russjones Jun 12, 2017

kontsevoy Jun 12, 2017

russjones Jun 12, 2017

russjones Jun 12, 2017

russjones Jun 12, 2017

kontsevoy Jun 12, 2017

russjones Jun 12, 2017

-i flag for tsh ssh implementation. #1063

-i flag for tsh ssh implementation. #1063

Conversation

kontsevoy commented Jun 10, 2017 • edited Loading

kontsevoy commented Jun 10, 2017

kontsevoy commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

-i flag for `tsh ssh` implementation. #1063

-i flag for `tsh ssh` implementation. #1063

kontsevoy commented Jun 10, 2017 •

edited

Loading