connorkuehl/tftp

An implementation of the Trivial File Transfer Protocol (TFTP; RFC 1350) complete with a client and server

tftp's Issues

Create a constant for the lower bound of the port range

Right now, the Server::random_port and client::Builder::<New>::new methods have different lower bounds for port selection.

Expose a constant (maybe in the connection module) and update both of those places to use that constant instead.

Add a unit test for when the client initiates a connect with a packet that is *not* a read or write request

If Server::serve receives a packet that is not a read/write request, it will create an Error packet and send it to the client.

We should have a test to confirm this behavior.

impl From<Packet<Data>> for Packet<Ack>

Right now any code that sends an Ack packet calls its constructor directly and manually copies the block number from the Data packet. Implementing the From trait might make client code look a little nicer and reduce margin for error.

Ensure requests cannot escape the server's serving directory

This can be done in Server::serve or in one of the Handler methods.

Server should ignore duplicate requests

Originally spotted #46.

The server will need to begin maintaining a record of which TIDs it is currently handling. If it gets a RRQ or WRQ packet in Server::serve for a TID that it is already handling, just ignore the request.

Some notes:

• Due to concurrency concerns, the server will likely need a lock around the data structure it uses for tracking TIDs.
• Since the Handler is the one that handles the request, the Server may want to register a callback function with each Handler so the Handler can notify the server that the request is finished and the request's TID should be removed.

Actually honor mode support

TFTP defines three modes: mail, netascii, and octet. Mail isn't used and shouldn't be implemented. We have it for completeness. However, this crate doesn't actually do anything with the mode. It simply sends and receives bytes, so I guess it just defaults to octet mode regardless of what is specified.

While I'm not sure how immediately helpful it is to distinguish between netascii and octet, but for the sake of accuracy, the mode should probably be honored.

OTOH, I think this is relevant for moving the issue forward:

• The Data packet payload will need to be ran through a filter to convert between netascii and octet
• Relevant unit tests will need to be updated
• Relevant integration tests will need to be updated

Server should transmit an Error packet (illegal operation) if a client's initial packet is NOT a read or write request

This could probably be done in Server::serve.

Add final packet dallying

When a get connection sends its final ACK, it should wait to see if the server sends the last data packet again. This suggests that the server did not receive the final ACK for the final data packet.

Add CI to repo

At the very least, the repo should run cargo test. Probably through Travis CI or GitHub actions?

If other issues like #2 #3 are completed prior to this one, they should be included in the CI as well or additional GitHub issues should be generated to track that work.

Add documentation for using Wireshark to capture TFTP packets

Commit this as Documentation/wireshark.md.

Wireshark is a helpful tool for debugging network protocols.

For example, I can capture the packets sent during the unit tests by opening Wireshark, selecting the loopback interface, and then running cargo test. Then stop capture.

Fix typo "7-bit ASCII" -> "8-bit ASCII" in src/packet/mode.rs

bytes::IntoBytes could take a &mut [u8; MAX_PACKET_SIZE] to avoid allocation

Pretty much what the title says. This is a super nitpick and literally doesn't matter because this is all I/O bound but I am a fan of yeeting as many allocations as possible

Add a Server constructor that chooses a random port

Right now we just have a single constructor for Server that expects the caller to pass in an address + port for it to listen on (for example, 127.0.0.1:6655). This is fine for application writers.

However, this is cumbersome when writing tests as we need to be sure to manually choose a different port to avoid the test failing due to another concurrently executing test having already possibly taken a port.

When this constructor is added, our tests can be updated to take advantage of this.

connection::Connection should transmit the Error packet

Right now it just uses the Error packet to create an io::Error. It'd be best to transmit the courtesy Error packet before returning the io::Error.

Do this for put and get.

impl Clone for client::Builder<ConnectTo>

TFTP doesn't reuse any connections and so the struct Client API is designed to reflect that -- meaning that a Client can't be cloned/copied to attempt to use it again.

However, if one wants to make several requests to a Server, it would be very handy if Builder<ConnectTo> implemented the Clone trait so they can clone this as many times as they want and then construct their own unique clients from that.

This isn't as simple as #[derive(Clone)] on the Builder<ConnectTo> because I specifically want each Builder<ConnectTo> to have its own unique socket. TFTP considers each address:port to represent a unique Transfer ID and therefore if we reused a socket that would mean the transfer ID previously used is no longer unique.

Add support for timeouts

• Client should give up after not hearing from the server after a duration has elapsed
• Server should attempt to resend packets after not receiving an ack for the last one it sent after a certain time period has elapsed
• Server should give up on resending after n resent packets

You can choose the value for 'n' and/or the timeout duration.

Write integration tests

It would be beneficial to have integration tests for the following modules:

• src/client.rs
• src/server.rs
• src/connection.rs

For src/client.rs and src/server.rs I think tests for the handshake process should be sufficient (i.e, the read/write request can simply be for zero bytes)

For src/connection.rs, it would be nice for an integration test to fully transfer a non-zero byte amount. Ideally, coverage would include:

• connection timeouts
• a zero-byte transfer
• an n-byte transfer where n < size of a data packet
• an n-byte transfer where n spans several data packets

Note that the types in src/connection.rs just assume a handshake has already occurred, so the handshake doesn't have to be tested.

Expand CI to run on Windows

Similar to #20, I don't know if Travis will do this for its free tier but if it does, that would be cool.

Clean up examples/client.rs

I think there needs to be a type alias or something for client::Client(or it needs to be refactored anyway). Either way, examples/client.rs is really sloppy. Any improvements to clean up the code quality are welcome.

Document the crate

The following modules require code docs:

• src/client.rs
• src/lib.rs
• src/server.rs
• src/bytes.rs
• src/connection.rs
• src/packet/ack.rs
• src/packet/error.rs
• src/packet/opcode.rs
• src/packet/mod.rs
• src/packet/data.rs
• src/packet/mode.rs
• src/packet/rq/wrq.rs
• src/packet/rq/mod.rs
• src/packet/rq/rrq.rs

Profile the crate with something like flamegraph to see where most of the code spends its time

Handle duplicated packets

Per the title, we should handle duplicate packets such as receiving two Data packets with the same block id or two Ack packets with the same block id. (by silently dropping it, as far as I can see?)

Furthermore, getting two responses for a read/write request should somehow be handled, per the end of §4:

Host A sends a request to host B. Somewhere in the network, the request packet is
duplicated, and as a result two acknowledgments are returned to host
A, with different TID's chosen on host B in response to the two
requests. When the first response arrives, host A continues the
connection. When the second response to the request arrives, it
should be rejected, but there is no reason to terminate the first
connection. Therefore, if different TID's are chosen for the two
connections on host B and host A checks the source TID's of the
messages it receives, the first connection can be maintained while
the second is rejected by returning an error packet.

Replace the assert statement in connection::Connection::put with an error packet and terminate the transmission

A misbehaving peer could send us an ack with the wrong block number; however, panicking via an assert is not a helpful way to deal with this.

We don't need to go out of our way to try to get the connection back on track. Create a courtesy Error packet, send it to the peer, and then return early from this method with an err.

Handler should bind to the same address component as the Server

IIRC right now it chooses 0.0.0.0:port, but if the server chose 127.0.0.1:port then the handler should too.

std::net::UdpSocket::local_addr may be useful here. This could be done in Server::serve.

Add a method called "to_string" for the `packet::error::Code` enum

It should return a String that represents a description of the error.

Hint: it already has a Display trait implementation, so that could probably be used with the format!() macro.

Add a threadpool server example

The way server::Server is implemented, it should be fairly trivial to move server::Handler across thread boundaries. The server::Server instance could listen on the initial thread and then submit server::Handler jobs to the threadpool.

Put it under the examples/ folder.

Remove public access from packet fields and replace them with accessor methods

The following packet modules have visible fields. The pub visibility qualifier should be removed and calling code should be updated to use well-crafted accessor methods.

• src/packet/ack.rs
• src/packet/error.rs
• src/packet/mod.rs
• src/packet/data.rs
• src/packet/rq/wrq.rs
• src/packet/rq/mod.rs
• src/packet/rq/rrq.rs

Write containerized end-to-end tests

It would be cool if we could bring up a server and n-clients with docker-compose or something. They can just run the client and server binaries in the examples/ folder.

Injecting network faults to cover things like timeouts and packet retransmissions would be awesome.

Ultimately, this might be something that could be incorporated into CI (once it exists...)

Server should send an error packet if the client wants to PUT a file that already exists

This could probably be accomplished in server::Handler::put by replacing the create and truncate options with OptionOptions::create_new.

Expand CI to run on MacOS

I haven't checked with Travis to see if this is a part of its free offering, but it would be cool. Off the top of my head there's nothing Linux-specific in this repo.