(master branch) SSH/SFTP: create new openChannel() method to eliminate dupe code

[terrafrost]

@rposky - since you offered to code review in #1888 I'm @'ing you!

[rposky]

What's the nature of the OpenSSH bug? Is it that no more than 1 channel can ever be established, or that multiple channels cannot co-exist? Would it be acceptable to, say, open a shell, close it, and then open the exec PTY?

[terrafrost]

What's the nature of the OpenSSH bug? Is it that no more than 1 channel can ever be established, or that multiple channels cannot co-exist?

Affected OpenSSH servers will kill the connection when you try to open up another channel. See https://marc.info/?l=openssh-unix-dev&m=163409448515619&w=2 for more detail.

[rposky]

If this counter is meant to indicate the number of open channels, as the property doc suggests, then I would postpone incrementing until the channel has been successfully opened.

The data is also already present in the channel_status array, that you'd just need to craft the appropriate query to determine the number of open channels, rather than introduce another variable.

[rposky]

Also, if meant to track number of open channels, I do not see it decremented ever.

[terrafrost]

Superficially, using channel_status seems like a good idea but you'd need to do unset($this->channel_status[$client_channel]) in the close_channel method and unset($this->channel_status[$channel]) in the part of get_channel_packet() where the incoming packet is a NET_SSH2_MSG_CHANNEL_CLOSE packet. And if that's done then get_channel_packet() won't return buffered up packets for closed channels.

Like consider this code:

$ssh->enablePTY();
$ssh->exec('ping -c 3 google.com; exit');
$ssh->write("ping microsoft.com\n", SSH2::CHANNEL_SHELL);
$ssh->setTimeout(3);
echo $ssh->read('', SSH2::READ_SIMPLE, SSH2::CHANNEL_SHELL);
echo $ssh->read('', SSH2::READ_SIMPLE, SSH2::CHANNEL_EXEC);

Right now that throws a "Data is not available on channel" InsufficientDataException exception for the $ssh->read('', SSH2::READ_SIMPLE, SSH2::CHANNEL_EXEC) line since, by that time that line has been called, the server has closed the channel but ideally it wouldn't (and in local modifications I've made it doesn't) since $this->channel_buffers[$channel] isn't empty.

That said, decrementing $this->channelCount is a good idea (esp in lieu of the above) is a good idea, which I've gone ahead and implemented. I've also added a $ssh->getOpenChannelCount() method.

[rposky]

You wouldn't necessarily have to unset($this->channel_status[$channel]). An alternative would be to inspect the actual status values and to make a determination from there, rather than simply tallying up the number of channel status entries.

[terrafrost]

I guess you could do count(array_filter($this->channel_status, function($val) { return $val != NET_SSH2_MSG_CHANNEL_CLOSE; })) but, as a matter of personal preference, I prefer $this->channelCount.

If this was a PR being done by someone else and they did array_filter instead of what I'm doing I prob wouldn't comment on it.

[rposky]

Right, and it can work either way. My preference for channel_status is just in that it involves less state, and mgmt thereof, than the additional channel_count.

[rposky]

$channel should be declared int

[terrafrost]

Good catch!

[rposky]

Can also remove this line, now redundant over use of openChannel()

[rposky]

Generate a log that multiple channels are being disabled due to server's client?

[terrafrost]

phpseclib doesn't really have that kind of logging currently. It has packet logs but it doesn't have event logs like you're describing. It wouldn't be a bad idea for phpseclib to get that kind of logging but that's kinda a project unto it's own and creating an event log that's currently populated by just one event would be kinda weird. And it'd kinda be scope creep too, adding methods that aren't related to the functionality that this PR is specifically concerned with.

Like for that matter it might be good to add multiple logging levels, much like OpenSSH has with it's logs (an example of which can be seen with https://marc.info/?l=openssh-unix-dev&m=163409448515619&w=2) and to then add a Monolog interface for them but that's all well beyond the scope of this PR

[rposky]

Error if channel is already open?

[terrafrost]

Good idea.

As doing this kinda makes lines like this that proceed $this->openChannel() redundant I've removed them:

        if ($this->isShellOpen()) {
            return false;
        }

[rposky]

As doing this kinda makes lines like this that proceed $this->openChannel() redundant I've removed them:

    if ($this->isShellOpen()) {
        return false;
    }

That looks sane to me.

[rposky]

A little awkward to presume that errorOnMultipleChannels=true due to OpenSSH version, in consideration of the flag's generic name and possible future expansion of its use. I'd error with the more generic e.g. "Multiple channel feature has been disabled" or something to that effect.

[terrafrost]

The more generic error message you propose doesn't really explain the situation either. Like anyone affected by the issue is liable to be like "well can you enable the feature?" as tho I'm the reason it's not working when, in reality, it's not working because of a buggy modification that the Ubuntu devs made to their fork of OpenSSH.

Sure, the variable name could be made less generic but, then again, there is something to be said about not having a variable name that's not 1KB in size as well lol. Like maybe I could have it named $this->errorOnMultipleChannelsBecauseUbuntuForkOfOpenSSH58-69IsBroken but that introduces it's own can of worms with it being impractically long.

[rposky]

The reconnect() is probably not desirable in the case that multiple channels have been disabled.

Perhaps more concerning is that with the current changes, ping() can be invoked at most 1 time when multiple channels are disabled, as subsequent pings will throw the multi channel exception.

[terrafrost]

Perhaps more concerning is that with the current changes, ping() can be invoked at most 1 time when multiple channels are disabled, as subsequent pings will throw the multi channel exception.

Decrementing $this->channelCount should resolve that.

[rposky]

Consider exposing a setter for this as well, such that callers can enable the feature or forcibly disable if they know the OpenSSH version match to be a non-issue due to applied workarounds.

[terrafrost]

I've added a forceMultipleChannels() method.

[terrafrost]

Thanks for the feedback! I didn't respond to it earlier, even in spite of responded to other support requests, because this one is a lot more technical than most support requests.

I'll try to get my local changes on git either tonight or tomorrow.

Thanks again!

[terrafrost]

@rposly - I made a new commit if you want to take a look see!

Thanks!

[rposky]

I had previously considered this change to ignore the connectivity requirement if buffers were available but decided against it. The reason being is that, in the READ_SIMPLE mode, connectivity will be required regardless. Once the buffers are drained, we're still executing within the while loop below, and back to needing a connection when we invoke get_channel_packet, or hitting disconnect exception.

We could confine the buffer emptiness check to the READ_NEXT mode for now, or rework the while loop to stop once the buffers are empty (and the channel is closed), but as-is, I think we're just changing the exception bound to be thrown with this change.

[terrafrost]

@rposky - In the unit test I added (testReadingOfClosedChannel() to tests/Functional/Net/SSH2Test.php) it's doing READ_SIMPLE and it's able to read from a closed buffer without issue.

So in the while (true) loop in read() there's this:

            $response = $this->get_channel_packet($channel);
            if ($response === true) {
                return Strings::shift($this->interactiveBuffer, strlen($this->interactiveBuffer));
            }

$response is true because of this (from get_channel_packet):

                $response = $this->get_binary_packet(true);
                if ($response === true && $this->is_timeout) {
                    if ($client_channel == self::CHANNEL_EXEC && !$this->request_pty) {
                        $this->close_channel($client_channel);
                    }
                    return true;
                }

Now that said, it does get a little janky when $ssh->setTimeout(0) is set. Like it outputs some of the output of $ssh->write("ping 127.0.0.2\n", SSH2::CHANNEL_SHELL); when it shouldn't ever output anything as it should essentially be caught in an infinite loop. This is because of of #1440 (comment) . I could undo it but idk I think what we really need are per channel timeout's instead of a global timeout but I think that'd be best for another PR.

Could you give some sample code that demonstrates the problem you were concerned about?

[rposky]

@terrafrost Yes, I'll try to get you that sample code tomorrow (or let you know that I've been unable to reproduce). Sorry it's taking me a bit to get to it.

[rposky]

So here's a unit test I've come up to illustrate my point.

public function testReadFromChannelBufferWhenDisconnected(): void
    {
        $ssh = $this->getSSH2Login();
        $ssh->openShell();
        $sshChannel = $ssh->getInteractiveChannelId();
        // start a command that will run for a bit and then close the channel
        $ssh->write("for i in {1..2}; do echo \"sleep 2...\" && sleep 2; done; exit\n", $sshChannel);

        $ssh->setTimeout(5);
        $ssh->enablePTY();
        $ssh->exec('bash');
        $execChannel = $ssh->getInteractiveChannelId();
        $ssh->write('echo "hello"', $execChannel);
        $ssh->read('', SSH2::READ_SIMPLE, $execChannel);
        $ssh->disablePTY();
        // simulate server disconnect and generate error on read()
        $ssh->disconnect();

        // make channel_buffers public and uncomment
        // $this->assertNotEmpty($ssh->channel_buffers[$sshChannel], 'Buffered data on SSH channel was expected');
        $this->assertFalse($ssh->isInteractiveChannelOpen($sshChannel));

        $ssh->setTimeout(1);
        $readOutput = $ssh->read('', SSH2::READ_SIMPLE, $sshChannel);
        $this->assertStringContainsString('sleep 2...', $readOutput, 'Shell echo output was not found');
        $this->assertFalse($ssh->isInteractiveChannelOpen($sshChannel));
    }

Against master, this generates the error phpseclib3\Exception\InsufficientSetupException : Operation disallowed prior to login() whereas the error phpseclib3\Exception\InvalidArgumentException : fsock is not a resource. is generated by the proposed change to pivot in read() based on channel buffer data.

So in the case that the server has disconnected, we're still throwing an exception, and a less descriptive one at that with the change. However, that's not to suggest that Operation disallowed prior to login() is a good error for this case, and I actually strongly dislike that read() and write() will attempt to auto re-open a shell which has exited (and which can actually succeed if the connection is alive). I would rather receive the error 'Data is not available on channel' like we do for other channel types in the case that the shell is no longer open, and which at least allows for acknowledgement that the prior session is gone. That change could be disruptive however, but perhaps it's not too bad so long as we maintain the shell auto-open (and not re-open) on read() and write().

There is another interesting caveat to the proposed change, and which is highlighted by this test case as well. If removing the call to $ssh->disconnect(), the change does allow for continued read against the closed shell (as intended), albeit only if the connection remains alive, whereas master throws the error phpseclib3\Exception\RuntimeException : Unable to fulfill channel request in a failed attempt to re-open the shell (probably worthy of investigation as well, but I didn't pursue). I'm not sure how likely a scenario it is to maintain an active SSH connection with unexpected closure of the shell though, but it strikes me as rare enough that I wouldn't introduce general logic to support it.

So in summary, I think where I'm at is that I do like enabling access to the buffered channel data, and that read() is probably the appropriate place to grant it, but that we haven't fully arrived at that goal with the proposed change. Technically, the access is already there through $mode=READ_NEXT, though the shell re-open behavior does make that hard to work with.

[terrafrost]

terrafrost@f664ccb should take care of this.

And I do agree that the current behavior is suboptimal but, well, we've already talked about my long term plans lol. But right now my more immediate goals are completely rewriting ASN.1 / X.509. Like with what I have right now processing of ASN.1 stuff is deferred (ie. done on demand). So like if you want to get the total count of how many certs are in a CRL you don't need to parse the whole CRL - you can only parse the relevant parts of it. On a CRL with 40,000 entries this knocked the time down from 4s to 0.2s for that specific operation. I got other changes implemented, as well, but discussing them is a bit off topic in a PR for SSH2 lol

[terrafrost]

I've made updates based on your feedback!

Unless I hear back from you in the next few days I'll prob merge this in the next few days!

[rposky]

Thank ya. Looks good to me.