Opened 2 months ago

Closed 6 weeks ago

#9760 release blocker: release process bug closed fixed (fixed)

azure pipelines builds are failing on Windows due to OpenSSH integration tests

Reported by: Glyph Owned by: Glyph
Priority: highest Milestone:
Component: conch Keywords:
Cc: Branch: 9760-windows-build-failures
branch-diff, diff-cov, branch-cov, buildbot
Author:

Change History (11)

comment:1 Changed 2 months ago by Glyph

I set up a local Windows development environment and can't reproduce these.

comment:2 Changed 2 months ago by Glyph

After tweaking the tests to be just the tiniest bit verbose, I see this:

Failure: twisted.conch.error.ConchError: ('exit code was not 0: 255 (b\'@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
r
n@ WARNING: UNPROTECTED PRIVATE KEY FILE! @
r
n@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
r
nPermissions for
\'dsa_test
\' are too open.
r
nIt is required that your private key files are NOT accessible by others.
r
nThis private key will be ignored.
r
nLoad key "dsa_test": bad permissions
r
ntestuser@127.0.0.1: Permission denied (publickey).
r
n\')', None)

which clearly suggests the problem is that something about the permissions we're explicitly setting in the tests is getting ignored in this configuration.

This Stack Overflow question https://superuser.com/questions/1296024/windows-ssh-permissions-for-private-key-are-too-open indicates that this might be happening because our tmpdir now inherits undesirable permissions.

comment:3 Changed 2 months ago by Colin Watson

If it helps, I think this is the implementation of the relevant security check on Windows:

https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/win32compat/w32-sshfileperm.c

comment:4 Changed 6 weeks ago by Glyph

Branch: 9760-windows-build-failures

comment:5 Changed 6 weeks ago by Glyph

Further investigation has yielded that the problem user is S-1-5-32-545, which is BUILTIN_USERS according to https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-dtyp/81d92bba-d22b-4a8c-908a-554ab29148ab

comment:6 Changed 6 weeks ago by Glyph

Component: coreconch
Keywords: review added

OK, I think this gets across the desired semantics for the test skip; hopefully CI will be happy with the result.

comment:8 Changed 6 weeks ago by Colin Dunklau

Owner: set to Colin Dunklau

comment:9 Changed 6 weeks ago by Colin Dunklau

Keywords: review removed
Owner: changed from Colin Dunklau to Glyph

I'm vaguely dissatisfied with the conditional skip in ConchServerSetupMixin._createFiles, but not enough to dig in my feet. I suggest a new ticket asking for help verifying that the tests that wind up calling that method on various platforms and versions.

LGTM, please merge

comment:10 Changed 6 weeks ago by Glyph

That's the idea with the "for real" followup I already filed / linked in the skip :). I am also not happy with this, but also; I really want to unblock the 20 other things that are backed up behind this.

Thanks for the review!

comment:11 Changed 6 weeks ago by Glyph <glyph@…>

Resolution: fixed
Status: newclosed

In b719119:

Merge pull request #1220 from twisted/9760-windows-build-failures

Author: glyph

Reviewer: cdunklau

Fixes: ticket:9760

Get continuous integration working again by fixing up some minor issues related to filesystem permissions and path configuration in tests.

Note: See TracTickets for help on using tickets.