Add scionFTP

[cneukom]

This adds scionFTP from https://github.com/elwin/scionFTP.

Changes in ScionFTP:

• Adaptions to work with scion-apps v0.6.0
• Integrate Hecules (use -hercules to specify the Hercules binary)

Changes in scion-apps:

• Add scion-ftp and scion-ftpserver
• Add appquic.Listen()

---

This change is 

[cneukom]

Reviewable status: 2 of 55 files reviewed, 5 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

a discussion (no related file):

Previously, matzf (Matthias Frei) wrote…

Yes, please!

While refactoring the internal/ftp/scion package, I did not really find anything that I think would be an improvement for appquic. What's left in sockquic (which is what I renamed the refactored package to), is really only needed to make the GridFTP extension work in scion-ftp.

---

---

ftp/README.md, line 12 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Oops.

Done.

---

ftp/client/scionftp/scionftp.go, line 49 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

I would propose an hcfg command to specify the configuration file.

This seems like the right direction. But why even bother with this command? Just read the hercules configuration file from, one or multiple fixed paths. For example readhercules.cfg in the cwd and if that doesn't exist, /etc/hercules.cfg, or whatever is appropriate.

👍 for simplicity.

[matzf]

Reviewed 1 of 62 files at r1, 5 of 61 files at r2, 37 of 56 files at r3, 1 of 4 files at r4, 21 of 21 files at r5.
Reviewable status: all files reviewed, 25 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

a discussion (no related file):

Previously, cneukom (Cédric Neukom) wrote…

While refactoring the internal/ftp/scion package, I did not really find anything that I think would be an improvement for appquic. What's left in sockquic (which is what I renamed the refactored package to), is really only needed to make the GridFTP extension work in scion-ftp.

Much better, thanks! After looking closer again, I think this can still be "condensed" and improved a bit. Commented in the individual files as well, but quick summary is I think we should drop the sockquic package and move this code out to the client or server application code respectively. The socket can also be cleaned up by moving the "MultiSocket" and its components to the striping package (where it belongs, I think) and then completely removing it because it only contains unnecessary or broken abstractions.

---

---

ftp/client/scionftp/scionftp.go, line 49 at r1 (raw file):

Previously, cneukom (Cédric Neukom) wrote…

👍 for simplicity.

💯

---

ftp/ftpd/main.go, line 24 at r2 (raw file):

Previously, cneukom (Cédric Neukom) wrote…

Yes, I like this idea. I added another authenticator for anonymous FTP

👍

---

ftp/internal/ftp/client_test.go, line 44 at r4 (raw file):

func testConn(t *testing.T, disableEPSV bool) {

	mock, c := openConn(t, "127.0.0.1", DialWithTimeout(5*time.Second), DialWithDisabledEPSV(disableEPSV))

These tests are all failing (127.0.0.1 is not a valid address: invalid address: regex match failed addr="127.0.0.1").
I'm not sure what's the best thing to do here; it tries to run a mock FTP server (see conn_test.go) and connect to it. With SCION, this would require a running dispatcher (at least). We could mock more stuff (the connections to the dispatcher and sciond) or maybe we could convert these tests to run under the integration test framework? As a last resort, we could drop the tests.

---

ftp/internal/ftp/cmd.go, line 36 at r5 (raw file):

	"github.com/netsec-ethz/scion-apps/internal/ftp/hercules"
	mode2 "github.com/netsec-ethz/scion-apps/internal/ftp/mode"

Nit: mode2 is a bit odd, but i guess it's to avoid name clashes. I've seen code using a lib prefix for this, i.e. libmode which would perhaps be clearer here.

---

ftp/internal/ftp/cmd.go, line 187 at r5 (raw file):

				defer wg.Done()

				conn, err := sockquic.DialAddr(addrs[i])

This should just create new streams on the same quic session, no? That's exactly what the streams are for. If so, the server side also needs to be adapted.

---

ftp/internal/ftp/cmd.go, line 211 at r5 (raw file):

		remote.Host.Port = port

		conn, err := sockquic.DialAddr(remote.String())

Same here, just create a new stream.
Except, not for hercules; in the hercules mode, don't call this method and rather explicitly just open a dummy UDP to allocate an ephemeral port.

---

ftp/internal/ftp/ftp.go, line 17 at r4 (raw file):

// Copyright 2020-2021 ETH Zurich modifications to add support for SCION

// Package ftp implements a FTP scionftp as described in RFC 959.

Search and replace gone wrong here? "an FTP client"
Maybe mention that it supports SCION and some extensions that are definitely not in RFC 959.

---

ftp/internal/ftp/ftp.go, line 50 at r4 (raw file):

	conn                  *textproto.Conn
	keepAliveConn         *textproto.Conn
	local, remote         string // local and remote address (without port!)

remote is unused

---

ftp/internal/ftp/ftp.go, line 50 at r5 (raw file):

type ServerConn struct {
	options           *dialOptions
	socket            *socket.ScionSocket

As noted in socket.ScionSocket, we should track the quic session here explicitly to close it after use.

---

ftp/internal/ftp/ftp.go, line 56 at r5 (raw file):

	features          map[string]string // Server capabilities discovered at runtime
	mlstSupported     bool
	herculesSupported bool

These feature bools seem to be redundant. Instead, check features in IsHerculesSupported and add an analogous IsMLSTSupported method.

---

ftp/internal/ftp/README.md, line 4 at r5 (raw file):

Client package for FTP + GridFTP extension, adapted to the SCION network. Forked
from [elwin/scionFTP](https://github.com/elwin/scionFTP).

Nit: "Forked from jlaffaye/ftp, with SCION support originally added in elwin/scionFTP."

---

ftpd/internal/core/go.sum, line 1 at r5 (raw file):

cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=

Delete this.

---

ftpd/internal/driver/file/mock_driver.go, line 30 at r5 (raw file):

var _ core.Driver = &MockDriver{}

type MockDriver struct {

Unused?

---

internal/ftp/hercules/utils.go, line 93 at r5 (raw file):

sudo

That's not super great and it limits this to users with passwordless sudo which is rather bad for the system's security.
Perhaps it might be worth looking into setting the correct capabilities to allow running this as non-root.

Anyway ok to me for now as this is clearly an experimental feature.

---

internal/ftp/hercules/utils.go, line 125 at r5 (raw file):

//  - /etc/scion-ftp/
//  - /etc/
func ResolveConfig() (*string, error) {

Nit: go doc comments should start with the name of the thing they are documenting:

// ResolveConfig checks for ...
func ResolveConfig() ...

---

internal/ftp/queue/queue.go, line 15 at r5 (raw file):

// limitations under the License.

package queue

If it's not too much effort to change, I'd drop this entire package and just use container/heap instead.

---

internal/ftp/queue/queue.go, line 30 at r5 (raw file):

}

var _ Queue = &Implementation{}

Drop this line (asserted by NewQueue)

---

internal/ftp/queue/queue.go, line 35 at r5 (raw file):

// Rather appropriate wrapper method should be created
// that prevent the wrong types from being added
type Implementation struct {

Just don't export the name and remove the comment. queue? Or queueImpl. Same for Item --> item.

---

internal/ftp/socket/multisocket.go, line 23 at r5 (raw file):

)

type Socket interface {

Unused, drop it.

---

internal/ftp/socket/multisocket.go, line 29 at r5 (raw file):

}

type MultiSocket struct {

I would move this MultiSocket, together with ReaderSocket and WriterSocket to the striping package. And don't export the the reader/writer socket and worker types.

---

internal/ftp/socket/socket.go, line 16 at r5 (raw file):

// DataSocket describes a data socket is used to send non-control data between the scionftp and
// server.
type DataSocket interface {

Isn't this is just net.Conn minus SetReadDeadline? Is it really worth it having a separate interface for this?

---

internal/ftp/socket/socket.go, line 41 at r5 (raw file):

	quic.Session
	quic.Stream
}

First, the name ScionSocket is not accurate as it's not related to SCION but related to QUIC.
More importantly, it contains a session but never closes it, Close only closes the stream. Also, when multiple of these things refer to the same session, the "ownership" of the session is not clear anymore -- who is responsible for closing it?
I would propose to remove this struct and keep or pass around the session and streams explicitly where needed. I believe this would not affect too much code but really make things a bit cleaner here.

---

internal/ftp/sockquic/client.go, line 15 at r5 (raw file):

// limitations under the License.

package sockquic

It looks like this should not be a separate package but be part of socket or better yet just move this code directly to where it's used in the application.
See also the somewhat related comment on socket.ScionSocket.

---

internal/ftp/sockquic/client.go, line 31 at r5 (raw file):

	tlsConfig := &tls.Config{
		InsecureSkipVerify: true,
		NextProtos:         []string{"scionftp"},

Nit: I'd just use "ftp".

---

internal/ftp/sockquic/client.go, line 63 at r5 (raw file):

	msg := []byte{200}

	return binary.Write(rw, binary.BigEndian, msg)

This is just weird, byte order for a something that is already []byte (of length 1 even!)? Just rw.Write(msg).

---

internal/ftp/sockquic/listener.go, line 81 at r5 (raw file):

	msg := make([]byte, 1)
	err := binary.Read(rw, binary.BigEndian, msg)

Same comment as on sending side. Just use rw.Read(msg), there is no byte order to take care of.

[cneukom]

Reviewable status: all files reviewed, 22 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

ftp/internal/ftp/client_test.go, line 44 at r4 (raw file):

Previously, matzf (Matthias Frei) wrote…

These tests are all failing (127.0.0.1 is not a valid address: invalid address: regex match failed addr="127.0.0.1").
I'm not sure what's the best thing to do here; it tries to run a mock FTP server (see conn_test.go) and connect to it. With SCION, this would require a running dispatcher (at least). We could mock more stuff (the connections to the dispatcher and sciond) or maybe we could convert these tests to run under the integration test framework? As a last resort, we could drop the tests.

The integration test framework seems to be the natural approach to me. This would avoid that the mocked behavior diverges from the real behavior.

---

ftp/internal/ftp/cmd.go, line 187 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

This should just create new streams on the same quic session, no? That's exactly what the streams are for. If so, the server side also needs to be adapted.

I'm not so sure about this one. I agree that this would lead to a nicer FTP-QUIC integration. However, this would introduce another difference to the standard FTP. Additionally, it would make the GridFTP extension useless, as the session sending rate does not change with the number of streams. Also, having multiple QUIC sessions with GridFTP would allow to use different paths (though, not currently implemented) and get additional benefits from distributing the load across different bottlenecks.

---

ftp/internal/ftp/ftp.go, line 17 at r4 (raw file):

Previously, matzf (Matthias Frei) wrote…

Search and replace gone wrong here? "an FTP client"
Maybe mention that it supports SCION and some extensions that are definitely not in RFC 959.

Done.

---

ftp/internal/ftp/ftp.go, line 50 at r4 (raw file):

Previously, matzf (Matthias Frei) wrote…

remote is unused

Done.

---

ftp/internal/ftp/ftp.go, line 56 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

These feature bools seem to be redundant. Instead, check features in IsHerculesSupported and add an analogous IsMLSTSupported method.

Done.

---

ftpd/internal/core/go.sum, line 1 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

Delete this.

Done.

---

ftpd/internal/driver/file/mock_driver.go, line 30 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

Unused?

Indeed. Deleted it.

---

internal/ftp/hercules/utils.go, line 93 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

sudo

That's not super great and it limits this to users with passwordless sudo which is rather bad for the system's security.
Perhaps it might be worth looking into setting the correct capabilities to allow running this as non-root.

Anyway ok to me for now as this is clearly an experimental feature.

I agree. I'll look into this, if time permits.

---

internal/ftp/socket/multisocket.go, line 23 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

Unused, drop it.

Done.

---

internal/ftp/sockquic/client.go, line 31 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit: I'd just use "ftp".

Wouldn't that imply that the FTP layer of our implementation is (somewhat) compatible to standard FTP?

---

internal/ftp/sockquic/client.go, line 63 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

This is just weird, byte order for a something that is already []byte (of length 1 even!)? Just rw.Write(msg).

Done.

---

internal/ftp/sockquic/listener.go, line 81 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

Same comment as on sending side. Just use rw.Read(msg), there is no byte order to take care of.

Done.

[cneukom]

Reviewable status: 39 of 53 files reviewed, 21 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

internal/ftp/queue/queue.go, line 15 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

If it's not too much effort to change, I'd drop this entire package and just use container/heap instead.

Yes, I like this, done.

---

internal/ftp/queue/queue.go, line 30 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

Drop this line (asserted by NewQueue)

(queue replaced with go heap)

---

internal/ftp/queue/queue.go, line 35 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

Just don't export the name and remove the comment. queue? Or queueImpl. Same for Item --> item.

(queue replaced with go heap)

---

internal/ftp/socket/multisocket.go, line 29 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

I would move this MultiSocket, together with ReaderSocket and WriterSocket to the striping package. And don't export the the reader/writer socket and worker types.

Done.

[cneukom]

Reviewable status: 39 of 53 files reviewed, 21 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

a discussion (no related file):

Previously, matzf (Matthias Frei) wrote…

Much better, thanks! After looking closer again, I think this can still be "condensed" and improved a bit. Commented in the individual files as well, but quick summary is I think we should drop the sockquic package and move this code out to the client or server application code respectively. The socket can also be cleaned up by moving the "MultiSocket" and its components to the striping package (where it belongs, I think) and then completely removing it because it only contains unnecessary or broken abstractions.

Thank you for your feedback, I like the direction this is going :)

I did not yet remove the ScionSocket, as I'd like to hear your opinion on a "MultiSession" first (see below).

---

---

internal/ftp/socket/socket.go, line 16 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

Isn't this is just net.Conn minus SetReadDeadline? Is it really worth it having a separate interface for this?

That's true, I extended the MultiSocket and switched to using net.Conn instead.

---

internal/ftp/socket/socket.go, line 41 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

First, the name ScionSocket is not accurate as it's not related to SCION but related to QUIC.
More importantly, it contains a session but never closes it, Close only closes the stream. Also, when multiple of these things refer to the same session, the "ownership" of the session is not clear anymore -- who is responsible for closing it?
I would propose to remove this struct and keep or pass around the session and streams explicitly where needed. I believe this would not affect too much code but really make things a bit cleaner here.

I've thought about this too, actually. The striping (MultiSocket) prevented me from changing it. In stream mode, a "data connection" would be the tuple of a quic.Session and a quic.Stream, which we would pass around separately. In extended mode, a "data connection" would then be a set of quic.Sessions and a set of quic.Streams, each set passed around separately. While I agree that it would not be much to change and some things get clearer, the notion of a MultiSession just does not make much sense to me. It would just be the "thing" you need to close after closing the MulitStream - at least as long as we stick to multiple QUIC sessions for extended mode, so that different paths could be used.
I think, wherever we have a session with exactly one stream, a QuicSocket (with an adapted Close()) is a suitable abstraction. However, I'm not 100% happy with it because we cannot enforce that it's only used with single-stream sessions.
I'm not really satisfied with both options... What do you think of the MultiSession?

---

internal/ftp/sockquic/client.go, line 15 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

It looks like this should not be a separate package but be part of socket or better yet just move this code directly to where it's used in the application.
See also the somewhat related comment on socket.ScionSocket.

Done.

[matzf]

Reviewed 6 of 9 files at r6, 7 of 16 files at r7.
Reviewable status: 43 of 53 files reviewed, 11 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

a discussion (no related file):

Previously, cneukom (Cédric Neukom) wrote…

Thank you for your feedback, I like the direction this is going :)

I did not yet remove the ScionSocket, as I'd like to hear your opinion on a "MultiSession" first (see below).

Ok. See the other comment.

---

---

internal/ftp/striping/heap.go, line 10 at r7 (raw file):

type segmentHeap struct {
	segments *[]*Segment

I think just []*Segments will work fine and would safe you line noise below. Note that a slice is just a (fat) pointer already.
And btw. it might even make sense to pass the Segments by value too, because they are also just small structs with pointers, and I believe they do not need to be modified once they are in the queue.

---

internal/ftp/striping/multisocket.go, line 25 at r7 (raw file):

	*readerSocket
	*writerSocket
	closed bool

closed unused

---

ftp/internal/ftp/cmd.go, line 187 at r5 (raw file):

Previously, cneukom (Cédric Neukom) wrote…

I'm not so sure about this one. I agree that this would lead to a nicer FTP-QUIC integration. However, this would introduce another difference to the standard FTP. Additionally, it would make the GridFTP extension useless, as the session sending rate does not change with the number of streams. Also, having multiple QUIC sessions with GridFTP would allow to use different paths (though, not currently implemented) and get additional benefits from distributing the load across different bottlenecks.

Ok, fair point about this GridFTP extension.

---

ftp/internal/ftp/cmd.go, line 211 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

Same here, just create a new stream.
Except, not for hercules; in the hercules mode, don't call this method and rather explicitly just open a dummy UDP to allocate an ephemeral port.

This point still stands, I would prefer not to use this method for the hercules case. I think doing the quic handshake just to allocate a port is excessive and rather confusing.

---

ftp/internal/ftp/README.md, line 6 at r7 (raw file):

Forked from [jlaffaye/ftp](https://github.com/jlaffaye/ftp), with SCION support originally added in [elwin/scionFTP](https://github.com/elwin/scionFTP).
Forked
from .

Oops?

---

ftpd/internal/core/cmd.go, line 1344 at r7 (raw file):

	for i := range listener {
		listener[i], err = conn.NewListener()

Is this thing ever closed? And btw, does appquic ever close the underlying socket? It looks like it doesn't :/

Also, why do we need a separate Listener for each connection here? Just one listener (== port) can accept all these sessions.

---

internal/ftp/queue/queue.go, line 15 at r5 (raw file):

Previously, cneukom (Cédric Neukom) wrote…

Yes, I like this, done.

💯

---

internal/ftp/socket/socket.go, line 41 at r5 (raw file):

I think, wherever we have a session with exactly one stream, a QuicSocket (with an adapted Close()) is a suitable abstraction. However, I'm not 100% happy with it because we cannot enforce that it's only used with single-stream sessions.
I'm not really satisfied with both options... What do you think of the MultiSession?

This MultiSession does not sound convincing to me. If we stick to one-stream-per-session, such a single stream abstraction seems clearer. You can enforce to only have a single stream, by hiding the session.

type SingleStream struct {
   quic.Stream
   session quic.Session
}

func (s SingleStream) LocalAddr() net.Addr {
	return s.session.LocalAddr()
}

func (s SingleStream) RemoteAddr() net.Addr {
	return s.session.RemoteAddr()
}

func (s SingleStream) Close() error {
	s.Stream.Close()
	return s.session.CloseWithError(0, "") // error code?
}

var _ net.Conn = SingleStream{}

func DialAddr(remoteAddr string) (SingleStream, error) {


	session, err := appquic.Dial(...)
	...
	stream, err := session.OpenStreamSync(context.Background())  // *Sync*, see comment on sendHandshake below
        ...
	return SingleStream{
		Stream:  stream,
		session: session,
	}, nil
}

// ... and in the listener ...
func (listener *Listener) Accept() net.Conn {
    session := listener.QuicListener.Accept(...)
    stream := session.AcceptStream()
    return SingleStream {
       Stream: stream,
       session: session,   
   }
}

Note: currently there seems to be one place where a session created here still opens another stream for the keepalive connection. I'm quite sure that this can just be deleted entirely, just set KeepAlive: true in the quic.Config.

---

internal/ftp/sockquic/client.go, line 51 at r6 (raw file):

	}

	err = sendHandshake(stream)

Only noticed now (sorry for first letting you fix the style in sendHandshake): this OpenStream function and the handshake code is completely unnecessary. Just call OpenStreamSync instead of OpenStream, then you get the desired behaviour without custom shenanigans.

---

internal/ftp/sockquic/listener.go, line 81 at r5 (raw file):

Previously, cneukom (Cédric Neukom) wrote…

Done.

Nit: maybe not use len (name of a builtin function) for this. The common name for this seems to be just n.

[matzf]

Reviewed 20 of 20 files at r8.
Reviewable status: all files reviewed, 9 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

ftpd/internal/core/quic.go, line 52 at r8 (raw file):

func (listener *Listener) Accept() (net.Conn, error) {
	return socket.Accept(listener.QuicListener)
}

Move all of this to this socket package? Dial is there, Accept is there, so move this whole thing there, too?

---

internal/ftp/socket/delayedclosersocket.go, line 5 at r8 (raw file):

// license that can be found in the LICENSE file.
//
// Copyright 2020 ETH Zurich modifications to add support for SCION

Nit: this is new code, just slap on the default appache license header.

---

ftpd/internal/core/cmd.go, line 21 at r5 (raw file):

	"github.com/netsec-ethz/scion-apps/internal/ftp/mode"
	socket2 "github.com/netsec-ethz/scion-apps/internal/ftp/socket"
	"github.com/netsec-ethz/scion-apps/internal/ftp/sockquic"

Nit: imports are usually grouped: std library first, then third party, then own.

---

ftpd/internal/core/cmd.go, line 394 at r8 (raw file):

		return
	}
	conn.dataConn = socket.DelayedCloserSocket{Conn: sock, Closer: listener, Duration: 120 * time.Second}

Can you explain why this is needed? I may be missing the context, but this just looks like a hack.

---

internal/ftp/striping/queue.go, line 22 at r8 (raw file):

type SegmentQueue struct {
	internal    heap.Interface

Make this *segmentHeap explicitly, then you don't need to silently cast it back.

[cneukom]

Reviewable status: all files reviewed, 7 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

ftpd/internal/core/quic.go, line 52 at r8 (raw file):

Previously, matzf (Matthias Frei) wrote…

Move all of this to this socket package? Dial is there, Accept is there, so move this whole thing there, too?

Done, much cleaner :)

---

internal/ftp/striping/heap.go, line 10 at r7 (raw file):

Previously, matzf (Matthias Frei) wrote…

I think just []*Segments will work fine and would safe you line noise below. Note that a slice is just a (fat) pointer already.
And btw. it might even make sense to pass the Segments by value too, because they are also just small structs with pointers, and I believe they do not need to be modified once they are in the queue.

Yep, moving the pointer to the outermost level works too :)

---

internal/ftp/striping/multisocket.go, line 25 at r7 (raw file):

Previously, matzf (Matthias Frei) wrote…

closed unused

Done.

---

ftp/internal/ftp/cmd.go, line 211 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

This point still stands, I would prefer not to use this method for the hercules case. I think doing the quic handshake just to allocate a port is excessive and rather confusing.

Again, I agree in principle, but: the issue here is that the Hercules mode is actually some kind of mixed mode. It does not make sense to transfer directory listings via Hercules, so these are transferred the same way as in stream mode and the QUIC handshake is indeed required in that case.
Also, the way this data connection establishment works in FTP requires that the client opens a connection, as this is the only point where the client "tells" the server its port number (which is required for Hercules downloads, as the sender initiates the transfer). This does not work with plain UDP, as there's no notion of a connection in UDP. Hence, we would need to send a dummy handshake to make that work. In my opinion, it's not worth the additional complexity, what do you think?

---

ftp/internal/ftp/ftp.go, line 50 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

As noted in socket.ScionSocket, we should track the quic session here explicitly to close it after use.

Done via SingleStream, see the other comment regarding the closing of a session.

---

ftp/internal/ftp/README.md, line 6 at r7 (raw file):

Previously, matzf (Matthias Frei) wrote…

Oops?

Oops! Thanks for spotting!

---

ftpd/internal/core/cmd.go, line 1344 at r7 (raw file):

Previously, matzf (Matthias Frei) wrote…

Is this thing ever closed? And btw, does appquic ever close the underlying socket? It looks like it doesn't :/

Also, why do we need a separate Listener for each connection here? Just one listener (== port) can accept all these sessions.

This was probably left-over from IP based GridFTP... I've fixed that: there's only one listener now and that gets closed (after some timeout) after the data has been sent

---

ftpd/internal/core/cmd.go, line 394 at r8 (raw file):

Previously, matzf (Matthias Frei) wrote…

Can you explain why this is needed? I may be missing the context, but this just looks like a hack.

We want to close the listener eventually, after use. Now, we cannot just close it with the socket, as at that point there's still data buffered (at the sender or receiver), closing the listener would immediately terminate the associated QUIC session(s), causing that memory to be freed and the data to be lost. Also, there's no API to find out at what point the data has been sent (and consumed at the receiver). Hence, we let the listener "expire" after some timeout ...

---

internal/ftp/socket/socket.go, line 41 at r5 (raw file):

Previously, matzf (Matthias Frei) wrote…

I think, wherever we have a session with exactly one stream, a QuicSocket (with an adapted Close()) is a suitable abstraction. However, I'm not 100% happy with it because we cannot enforce that it's only used with single-stream sessions.
I'm not really satisfied with both options... What do you think of the MultiSession?

This MultiSession does not sound convincing to me. If we stick to one-stream-per-session, such a single stream abstraction seems clearer. You can enforce to only have a single stream, by hiding the session.

type SingleStream struct {
   quic.Stream
   session quic.Session
}

func (s SingleStream) LocalAddr() net.Addr {
	return s.session.LocalAddr()
}

func (s SingleStream) RemoteAddr() net.Addr {
	return s.session.RemoteAddr()
}

func (s SingleStream) Close() error {
	s.Stream.Close()
	return s.session.CloseWithError(0, "") // error code?
}

var _ net.Conn = SingleStream{}

func DialAddr(remoteAddr string) (SingleStream, error) {


	session, err := appquic.Dial(...)
	...
	stream, err := session.OpenStreamSync(context.Background())  // *Sync*, see comment on sendHandshake below
        ...
	return SingleStream{
		Stream:  stream,
		session: session,
	}, nil
}

// ... and in the listener ...
func (listener *Listener) Accept() net.Conn {
    session := listener.QuicListener.Accept(...)
    stream := session.AcceptStream()
    return SingleStream {
       Stream: stream,
       session: session,   
   }
}

Note: currently there seems to be one place where a session created here still opens another stream for the keepalive connection. I'm quite sure that this can just be deleted entirely, just set KeepAlive: true in the quic.Config.

Yes, kind of. That way, one could still have the idea to wrap a quic.Stream (of a quic.Session they have access to) in a SingleStream, but since that would be quite useless, I think it's good enough :)

Re closing of the session: as far, as I can see, quic-go does not allow closing a session gracefully. I think, we don't need to close it... That's also what ssh does:

// Close closes the connection.
// Any blocked Read or Write operations will be unblocked and return errors.
func (mc *QuicConn) Close() error {
	return mc.Stream.Close()
}

---

internal/ftp/sockquic/client.go, line 51 at r6 (raw file):

Previously, matzf (Matthias Frei) wrote…

Only noticed now (sorry for first letting you fix the style in sendHandshake): this OpenStream function and the handshake code is completely unnecessary. Just call OpenStreamSync instead of OpenStream, then you get the desired behaviour without custom shenanigans.

Unfortunately, that does not work: AcceptStream() on the listener side keeps blocking in that case. However, in FTP, the server is supposed to welcome the client when a connection is opened, so we need to unblock AcceptStream() somehow. I assume that this was the reason for introducing the handshake in the first place...

---

internal/ftp/striping/queue.go, line 22 at r8 (raw file):

Previously, matzf (Matthias Frei) wrote…

Make this *segmentHeap explicitly, then you don't need to silently cast it back.

👍

[matzf]

Reviewed 12 of 12 files at r9.
Reviewable status: all files reviewed, 4 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

ftp/internal/ftp/cmd.go, line 211 at r5 (raw file):

Previously, cneukom (Cédric Neukom) wrote…

Again, I agree in principle, but: the issue here is that the Hercules mode is actually some kind of mixed mode. It does not make sense to transfer directory listings via Hercules, so these are transferred the same way as in stream mode and the QUIC handshake is indeed required in that case.
Also, the way this data connection establishment works in FTP requires that the client opens a connection, as this is the only point where the client "tells" the server its port number (which is required for Hercules downloads, as the sender initiates the transfer). This does not work with plain UDP, as there's no notion of a connection in UDP. Hence, we would need to send a dummy handshake to make that work. In my opinion, it's not worth the additional complexity, what do you think?

Ok

---

ftpd/internal/core/cmd.go, line 394 at r8 (raw file):

Previously, cneukom (Cédric Neukom) wrote…

We want to close the listener eventually, after use. Now, we cannot just close it with the socket, as at that point there's still data buffered (at the sender or receiver), closing the listener would immediately terminate the associated QUIC session(s), causing that memory to be freed and the data to be lost. Also, there's no API to find out at what point the data has been sent (and consumed at the receiver). Hence, we let the listener "expire" after some timeout ...

Sender should Close the stream after sending. The receiver reads until EOF, then closes the session (with "no error" error code, see other comment). When the session is closed, the listener can be closed safely. There is a (unstable?) API Context on the session that can be used to wait until the session is closed.

Ok for me to keep the timeout though, but add a // HACK comment explaining why this is here.

---

internal/ftp/socket/socket.go, line 41 at r5 (raw file):

Previously, cneukom (Cédric Neukom) wrote…

Yes, kind of. That way, one could still have the idea to wrap a quic.Stream (of a quic.Session they have access to) in a SingleStream, but since that would be quite useless, I think it's good enough :)

Re closing of the session: as far, as I can see, quic-go does not allow closing a session gracefully. I think, we don't need to close it... That's also what ssh does:

// Close closes the connection.
// Any blocked Read or Write operations will be unblocked and return errors.
func (mc *QuicConn) Close() error {
	return mc.Stream.Close()
}

As far as I understand you should still close the session, otherwise you're leaving (a lot!) of internal state dangling until the session times out. The API is a bit weird; you call session.CloseWithError(errorNoError, "") with a suitably defined error code errorNoError (see e.g. Close of http3 client in quic-go).

---

internal/ftp/sockquic/client.go, line 51 at r6 (raw file):

Previously, cneukom (Cédric Neukom) wrote…

Unfortunately, that does not work: AcceptStream() on the listener side keeps blocking in that case. However, in FTP, the server is supposed to welcome the client when a connection is opened, so we need to unblock AcceptStream() somehow. I assume that this was the reason for introducing the handshake in the first place...

All clear. Ugly, but ok.