Skip to content

ssh: Unify logic to handle git-upload-pack(1) and git-upload-archive(1)

Patrick Steinhardt requested to merge pks-ssh-unify-upload-command-execution into master

The logic to run git-upload-pack(1) and git-upload-archive(1) is quite complex. Despite the need to splice several standard streams, we also need to handle the case where negotiation of the data that is to be sent does not finish, which is done to address time-of-check-time-of-use attacks.

Right now, much of the logic to do this setup is split up across the callers and monitorStdinCommand(). As there are multiple callers of the latter function, this means that some of the logic is duplicated. Furthermore, it is quite hard to change monitorStdinCommand() to alter its semantics, as it is not at all a self-contained building block.

Refactor this to move much of the logic into monitorStdinCommand(), which brings us a bunch of advantages:

- The monitoring logic is more self-contained so that we can amend
  it at a later point. This was the original motivation of this
  patch as it will be required to fix cases where we don't correctly
  detect that the mentioned negotiation has completed.

- It allows us to use the same counting writer for SSHUploadArchive
  that we already use for SSHUploadPack. We thus get better insight
  into how much data we're typically returning in this RPC.

- It allows us to use the same large buffer reader that we already
  use for SSHUploadPack. This reader uses a larger-than-usual buffer
  so that we can avoid many syscalls and should thus help to make
  SSHUploadArchive more efficient.

- We can reuse the bits to detect cancellation via our pktline
  monitor in case negotiation does not finish in time.

- We can reuse the error handling where the remote side hangs up
  unexpectedly, which is thrown by Git's pktline protocol in case
  the remote side hangs up in the middle of a write. This allows us
  to label user-cancelled requests as `codes.Canceled` instead of
  `codes.Internal` in SSHUploadArchive.

So overall we gain quite a lot of benefits with this refactoring while also simplifying our code base.

Part of #5351 (closed).

Merge request reports