Several 2.0 regression fixes #876

kontsevoy · 2017-03-26T20:04:39Z

I recommend reviewing this PR by looking at each commit individually. Commits are well-annotated and easy to understand.

The summary:

Fixed BoltDB backend: 'path' is not set, initializing teleport with config file. #867 by "promoting" a top-level data_dir configuration value into back-end's params section. This was the original intent, there was even code which was looking up for this value, but it was never copied.
Fixed Every log message under systemd is logged twice #875 by properly switching between syslog and stderr. Also cleaned up logging initialization, it's much easier to understand now.
Found and killed some dead code: there was a function nobody ever called.
Added comments here and there
Fixed Makefile in build.assets so it doesn't destroy VERSION variable anymore, so when you call make -C build.assets release you get a properly formatted tarball.
Fixed SSH exec unit test on OSX.
Fixed forwarding request denied unit test that failed on macOS.

Notes

I am at Kubecon, so if you see issues with this PR please fix yourself, do not wait for me to return. Thanks!

russjones · 2017-03-27T23:21:15Z

I can reproduce the forwarding request denied issue, let me hunt that down.

russjones · 2017-03-28T00:49:51Z

The reason the agent forwarding test was failing on macOS while passing on Linux was due to the path length restrictions on macOS for Unix sockets. From the the UNIX(4) manpage: UNIX-domain addresses are variable-length file system pathnames of at most 104 characters..

I shortened the UUID to the first 8 characters and also made the tests more cross platform friendly (used os.TempDir instead of /tmp because macOS temporary directories look something like /var/folders/00/l7kftvrn2tx7cv0_w2x_c4_c0000gn/T/).

klizhentas

@russjones spotted some issues

klizhentas · 2017-03-28T05:30:22Z

lib/config/configuration.go

+	if cfg.Auth.StorageConfig.Params == nil {
+		cfg.Auth.StorageConfig.Params = backend.Params{}
+	}
+	cfg.Auth.StorageConfig.Params["data_dir"] = cfg.DataDir


@russjones this overwrites the user-supplied value at all times, what should not be the case. We should only overwrite the value if user haven't supplied the value. You can fix that yourself as @kontsevoy is OOO at the moment.

@klizhentas there are no user-supplied values in StorageConfig.Params [1]. Storage config is a static, backend-specific YAML configuration, with "data_dir" being a reserved word. In fact, we guarantee the presence of "data_dir" in .Params (and back-end code expects it to be there) This needs to be reflected in backend developer README. In other words, this behavior is as-designed.

[1] Bolt used to accept an arbitrary JSON string, so that's why you think there are user-supplied values. This is no longer true since the Great Christmas-2016 Back-end Redesign

I think users can supply Params["path"] for backend, at least what I have observed in configs and the code, so I don't quite understand the fix then. Why is it setting "data_dir" and not "path" like for example "boltdb" requires.

My question basically is - can you explain to me how does this change fixes the case reported here:

#867

@russjones: @kontsevoy explained to me how it works, so this part lgtm, please fix the part with socket path and merge the whole thing

klizhentas · 2017-03-28T05:31:57Z

lib/srv/proxy.go

+	log.Debugf("proxy connecting to host=%v port=%v, exact port=%v\n", t.host, t.port, useExactPort)
+
+	ips, err := net.LookupHost(t.host)
+	log.Debugf("host lookup (%v)=%v, err=%v", t.host, ips, err)


the problem with err= is that it will be 99% of the time equal to err=nil and grep catches it all the time, so we should try to make it less verbose even for debug.

what do you mean by "grep catches it all the time"? adding error logging here helped me solve DNS-related issue on a customer POC.

your error logs look like host lookup (%v)=%v err=nil, so If I grep for gre logs err I will get all these false positives.

k, kill it. I'm not attached to it :)

klizhentas · 2017-03-28T05:32:46Z

lib/srv/sshserver.go

@@ -878,7 +878,7 @@ func (s *Server) handleAgentForward(ch ssh.Channel, req *ssh.Request, ctx *ctx)
 	ctx.setAgent(clientAgent, authChan)

 	pid := os.Getpid()
-	socketPath := filepath.Join(os.TempDir(), fmt.Sprintf("teleport-agent-%v.socket", uuid.New()))
+	socketPath := filepath.Join(os.TempDir(), fmt.Sprintf("teleport-agent-%v.socket", uuid.New()[0:8]))


@russjones it's a bit too late for me now, what are the chances of collision for this given your change?

just a comment: most UNIX tools use their own PID instead of UUID.

they also create random temp dir though, not in root /tmp dir.

Two other options:

We create the socket under /var/run/teleport/. This way our socket files are on the same place on all platforms. We can name the socket itself teleport-{random number}-{pid}.socket and still be under the 104 character limit.

We do this instead: ioutil.TempDir(os.TempDir(), "teleport-") which will create /tmp/teleport-699583643 on Linux and /var/folders/00/l7kftvrn2tx7cv0_w2x_c4_c0000gn/T/teleport-699583643 on macOS. We can name the socket itself teleport-{pid}.socket.

@russjones let's go with option 2. because it protects us from collisions on the implementation level - AFAIK golang checks for collisions

russjones · 2017-03-29T23:50:24Z

@klizhentas Made the changes you suggested, can you approve?

- Added comments to explain the purpose of clientConfig.HostPort - Fixed typo - Fixed docker-based 'make release' to include Teleport version into the produced tarball - More informative logging around host lookups

- Fixed logging. Closes #875 - Removed dead code - Fixed 'exec' tests on OSX

Fixes #867

russjones · 2017-03-30T00:40:14Z

lib/srv/sshserver_test.go

@@ -238,7 +239,8 @@ func (s *SrvSuite) TestAgentForward(c *C) {
 	_, err = io.WriteString(writer, fmt.Sprintf("printenv %v\n\r", teleport.SSHAuthSock))
 	c.Assert(err, IsNil)

-	re := regexp.MustCompile(`/tmp/[^\s]+`)
+	pattern := fmt.Sprintf(`%vteleport-[0-9]+[^\s]+`, os.TempDir())
+	re := regexp.MustCompile(pattern)


This is not actually the line that has the problem, but it's the loop below:

for i := 0; i < 3; i++ { _, err = reader.Read(buf) c.Assert(err, IsNil) matches = re.FindStringSubmatch(string(buf)) if len(matches) != 0 { break } time.Sleep(50 * time.Millisecond) }

On Linux tests hang on the line _, err = reader.Read(buf). Works fine on macOS.

@klizhentas ^

let me try to fix that

russjones · 2017-03-30T05:24:11Z

@klizhentas Tested your fix, works on macOS as well.

klizhentas · 2017-03-30T16:12:14Z

@russjones let's merge then

* Show error when auth settings return `u2f` second factor option - Handles `u2f` cfg returned from v9 - Push users to use webauthn * Conditionally render adding passwordless device based on if pwdless is enabled * Fix brief flash rendering of buttons before attempt success state * Add soft warning for firefox users for passwordless login or register

kontsevoy assigned klizhentas and russjones Mar 26, 2017

russjones requested review from klizhentas and russjones March 27, 2017 23:17

russjones approved these changes Mar 27, 2017

View reviewed changes

klizhentas requested changes Mar 28, 2017

View reviewed changes

russjones force-pushed the ev/fixes branch from 079b6ba to c125f87 Compare March 29, 2017 23:47

kontsevoy and others added 6 commits March 29, 2017 17:12

Minor cosmetic commit

dd9db5e

- Added comments to explain the purpose of clientConfig.HostPort - Fixed typo - Fixed docker-based 'make release' to include Teleport version into the produced tarball - More informative logging around host lookups

Logging fixes and more

1755870

- Fixed logging. Closes #875 - Removed dead code - Fixed 'exec' tests on OSX

Added 'data_dir' value to backend params

35f80ca

Fixes #867

Changes to Unix socket path to make it cross platform.

1cab059

Use ioutil.TempDir to create path to socket instead.

d304e47

Remove log line that contains err.

81b379e

russjones force-pushed the ev/fixes branch from c125f87 to 81b379e Compare March 30, 2017 00:13

russjones reviewed Mar 30, 2017

View reviewed changes

try to fix the test

fd46d88

Fixed Ansible regressions and added documentation for docker build.

835a754

russjones force-pushed the ev/fixes branch from da28310 to 835a754 Compare March 30, 2017 20:52

Added Ansible playbook for testing and docs on how to run it.

e9b1754

klizhentas approved these changes Mar 30, 2017

View reviewed changes

russjones merged commit 176e0f5 into master Mar 30, 2017

russjones deleted the ev/fixes branch March 30, 2017 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several 2.0 regression fixes #876

Several 2.0 regression fixes #876

kontsevoy commented Mar 26, 2017 •

edited by russjones

Loading

russjones commented Mar 27, 2017

russjones commented Mar 28, 2017

klizhentas left a comment

klizhentas Mar 28, 2017

kontsevoy Mar 28, 2017 •

edited

Loading

klizhentas Mar 28, 2017

klizhentas Mar 28, 2017

klizhentas Mar 29, 2017

klizhentas Mar 28, 2017

kontsevoy Mar 28, 2017

klizhentas Mar 28, 2017

kontsevoy Mar 29, 2017

klizhentas Mar 28, 2017

kontsevoy Mar 28, 2017

klizhentas Mar 28, 2017 •

edited

Loading

russjones Mar 28, 2017

klizhentas Mar 29, 2017

russjones commented Mar 29, 2017

russjones Mar 30, 2017

russjones Mar 30, 2017

klizhentas Mar 30, 2017

russjones commented Mar 30, 2017

klizhentas commented Mar 30, 2017

Several 2.0 regression fixes #876

Several 2.0 regression fixes #876

Conversation

kontsevoy commented Mar 26, 2017 • edited by russjones Loading

The summary:

Notes

russjones commented Mar 27, 2017

russjones commented Mar 28, 2017

klizhentas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kontsevoy Mar 28, 2017 • edited Loading

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

klizhentas Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russjones commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russjones commented Mar 30, 2017

klizhentas commented Mar 30, 2017

kontsevoy commented Mar 26, 2017 •

edited by russjones

Loading

kontsevoy Mar 28, 2017 •

edited

Loading

klizhentas Mar 28, 2017 •

edited

Loading