Accept SSH environment variable passing

[cjwatson]

Contributor Checklist:

• There is an associated ticket in Trac. https://twistedmatrix.com/trac/ticket/9315
• The changes pass minimal style checks
• I have created a newsfragment in src/twisted/conch/newsfragments/
• I have updated the automated tests.

[twm]

Hi Colin,

This is a great feature, and it's great to see it happening! I noted a few things in the review comments below. You'll need to fix the incompatible interface change and add the missing test coverage before we can land. I'll add that I'm not too familiar with Conch myself, so I'm mostly looking at this from a general Twisted background and might miss Conch-specific stuff.

The ticket mentions implementing OpenSSH's environment filtering behavior. It seems like Conch's default server should do this, though if you prefer to file a follow-up ticket for that it's fine.

Thanks for contributing to Twisted!

[cjwatson]

Regarding the behaviour of Conch's default server: OpenSSH's default behaviour is to reject all environment variables, and the only default server that I know of that Conch ships is twisted.conch.tap which only has very minimal configuration. My understanding is that the typical way to use Conch as a server is to use it as a library of components from which you can assemble a server with the custom behaviour you want (that's certainly how we use it in Launchpad). But I could be wrong: perhaps @markrwilliams can elaborate on their requirements?

Note that the cited behaviour of OpenSSH in accepting LANG and LC_* is not the default behaviour upstream, but rather a Debian-specific customisation.

[glyph]

FWIW I’d love it if Conch’s tap command line were a bit more robust in terms of supporting this sort of configuration.

[glyph]

Sorry to have a secondary review that piles on more feedback, but there are a couple of public-interface concerns here which I would really like to see addressed before landing. Thanks again for doing this, this is a hugely useful feature for conch!

When you have addressed feedback, feel free to @ me here, and I'll do my best to re-review promptly to get this integrated.

[glyph]

This seems like a good candidate for the "active nothing" pattern, where it's never None but sometimes it's a specialized no-op implementation that refuses all variables? This pushes some of the conditional logic below into the stub implementation, making it less complex.

[cjwatson]

I'd prefer not to do this in this case, because that loses the distinction for logging purposes between "can't set environment variables because doing that isn't implemented" and "can't set environment variables because the implementation said no", which seems to me to be likely to be useful. (Or, if I were to preserve the extra logging, as far as I can see it would make no difference to the complexity of the conditional logic below it: it still needs to cope with the three cases of OK, explicitly-forbidden, and error.)

[cjwatson]

@glyph Thanks for the review. I think I've addressed all your comments in one way or another; please re-review?

[twm]

Indeed, all the review feedback has been addressed. I will go ahead and merge (the test failure is a recurring Azure issue unrelated to these changes). Thank you very much!