1
\$\begingroup\$

Last December I interviewed with Storj and was unsuccessful after my code challenge. Storj were good enough to pay me for the time it took to complete the challenge, but have refused to give any kind of feedback as to where or why my submission was not up to their standards.

The repo for this code challenge can be found here: https://github.com/Samyoul/storj-file-sender

The question summary states:

Suppose you have two laptops (A and B) and a server. The laptops are in different houses and each are behind a firewall, so laptop A can't talk to laptop B and vice versa. The server is in a datacenter somewhere, and both laptops can talk to it.

The user of laptop A wants to send a file to the user of laptop B. The users are talking on the phone, so they can exchange information, but not the file itself.

You'll need to write three programs:

  • The sender - this program will run on laptop A.
  • The receiver - this program will run on laptop B.
  • The relay - this program will run on the server, which both laptops can reach.

My Sender code :

package main

import (
    "encoding/binary"
    "errors"
    "io"
    "log"
    "net"
    "os"

    "github.com/Samyoul/storj-file-sender/common"
    "github.com/Samyoul/storj-file-sender/sender/codegen"
)

func main() {
    // get arguments
    args := os.Args
    err := validateArgs(args)
    if err != nil {
        log.Fatalf("error - validating arguments : %s", err)
    }

    // checksum file
    h, err := common.HashFile(args[2])
    if err != nil {
        log.Fatalf("error - checksumming file %s : %s", args[2], err)
    }

    // generate secret code
    // Use the int64 encoded checksum of the file as part of the random seed
    code := codegen.Make(int64(binary.BigEndian.Uint64(h.Sum(nil))))

    // display secret code
    println(code)

    // open connection with relay
    conn, err := net.Dial("tcp", args[1])
    if err != nil {
        log.Fatalf("error - making a connection : %s", err)
    }
    defer conn.Close()

    // Set write buffer size
    err = conn.(*net.TCPConn).SetWriteBuffer(common.BufferLimit)
    if err != nil {
        log.Fatalf("error - setting write buffer : %s", err)
    }

    // Write and send header on connection, send checksum and filename with header
    hdr := common.MakeRequestHeaderSend(args[2], code, h.Sum(nil))
    conn.Write(hdr)

    // Open file hold ready to transfer
    f, err := os.Open(args[2])
    if err != nil {
        log.Fatal(err)
    }
    defer f.Close()
}

func validateArgs(args []string) error {
    if len(args) != 3 {
        return errors.New(
            "invalid number of arguments.\n" +
                "expected : ./sender <relay-host>:<relay-port> <file-to-send>\n" +
                "example  : ./sender localhost:9021 corgis.mp4")
    }

    return nil
}

My Relay code:

package main

import (
    "errors"
    "fmt"
    "io"
    "log"
    "net"
    "os"
    "sync"

    "github.com/Samyoul/storj-file-sender/common"
)

type stream struct {
    checksum []byte
    filename []byte
    sendConn chan net.Conn
    wg       sync.WaitGroup
}

func (s *stream) Close() {
    close(s.sendConn)
}

type streamMap map[string]*stream

func main() {
    // get init argument
    args := os.Args
    err := validateArgs(args)
    if err != nil {
        log.Fatalf("error - validating arguments : %s", err)
    }

    // open TCP server
    l, err := net.Listen("tcp", args[1])
    if err != nil {
        log.Fatalf("error - starting tcp server : %s", err)
    }
    defer l.Close()

    // Create streams map
    sm := streamMap{}

    // wait for connections
    for {
        conn, err := l.Accept()
        if err != nil {
            log.Println(err)
            return // return don't exit because you don't want to kill your whole server over a single fail connection
        }

        // serve connections with successful connection
        go handle(&sm, conn)
    }
}

func handle(sm *streamMap, conn net.Conn) {
    defer conn.Close()

    // Get the connection header
    // Added header so that I connection parameters can be exchanged between client and server
    hdr, err := common.GetRequestHeader(conn)
    if err != nil {
        log.Printf("error - getting request header - %s", err)
        return
    }

    // determine the type of connection coming in.
    // switch logic to give handler for each the send and receive requests
    switch string(hdr["Type"]) {
    case common.HeaderSend:
        err = send(sm, conn, hdr)
        if err != nil {
            log.Printf("error - processing send request - %s", err)
            return
        }
        break
    case common.HeaderReceive:
        err = receive(sm, conn, hdr)
        if err != nil {
            log.Printf("error - processing receive request - %s", err)
            return
        }
        break
    default:
        log.Println(hdr)
    }
}

func validateArgs(args []string) error {
    if len(args) != 2 {
        return errors.New(
            "invalid number of arguments.\n" +
            "expected : ./relay :<port>\n" +
            "example  : ./relay :9021")
    }

    return nil
}

func send(sm *streamMap, conn net.Conn, hdr common.Header) error {
    s := &stream{}
    (*sm)[string(hdr["Code"])] = s

    s.filename = hdr["Filename"]
    s.checksum = hdr["Checksum"]
    s.sendConn = make(chan net.Conn)
    s.wg = sync.WaitGroup{}
    defer s.Close()

    err := conn.(*net.TCPConn).SetReadBuffer(common.BufferLimit)
    if err != nil {
        return err
    }

    s.wg.Add(1)
    s.sendConn <- conn
    s.wg.Wait()

    delete(*sm, string(hdr["Code"]))

    return nil
}

func receive(sm *streamMap, conn net.Conn, hdr common.Header) error {
    // Check the stream exists in the stream map
    s, ok := (*sm)[string(hdr["Code"])]
    if !ok {
        return errors.New(fmt.Sprintf("unrecognised secret code '%s'", hdr["Code"]))
    }

    rh := common.MakeResponseHeaderReceive(s.filename, s.checksum)
    _, err := conn.Write(rh)
    if err != nil {
        return err
    }

    _, err = io.Copy(conn, <-s.sendConn)
    if err != nil {
        return err
    }

    s.wg.Done()

    return nil
}

My Receiver code :

package main

import (
    "bytes"
    "errors"
    "io"
    "log"
    "net"
    "os"

    "github.com/Samyoul/storj-file-sender/common"
)

func main() {
    // get arguments
    args := os.Args
    err := validateArgs(args)
    if err != nil {
        log.Fatalf("error - validating arguments : %s", err)
    }

    // open connection with relay
    conn, err := net.Dial("tcp", args[1])
    if err != nil {
        log.Fatalf("error - making a connection : %s", err)
    }
    defer conn.Close()

    // make receive request to relay
    reqH := common.MakeRequestHeaderReceive(args[2])
    conn.Write(reqH)

    // get receive response header from relay with checksum and filename
    resH, err := common.GetResponseHeader(conn)
    if err != nil {
        log.Fatalf("error - reading response header : %s", err)
    }

    // start to receive data stream
    fn := args[3] + string(resH["Filename"])

    f, err := os.Create(fn)
    if err != nil {
        log.Fatalf("error - creating file : %s", err)
    }
    defer f.Close()

    // write data to file
    _, err = io.Copy(f, conn)
    if err != nil {
        log.Fatalf("error - creating file : %s", err)
    }

    // check file complete with checksum comparison.
    h, err := common.HashFile(fn)
    if err != nil {
        log.Fatalf("error - checksumming file %s : %s", fn, err)
    }

    if bytes.Compare(h.Sum(nil), resH["Checksum"]) != 0 {
        log.Fatalf("error - checksum does not match")
    }
}

func validateArgs(args []string) error {
    if len(args) != 4 {
        return errors.New(
            "invalid number of arguments.\n" +
                "expected : ./receiver <relay-host>:<relay-port> <secret-code> <output-directory>\n" +
                "example  : ./receiver localhost:9021 this-is-a-secret-code out/")
    }

    return nil
}

There are a few other files for handling common functionality and random code gen but this is the core of the applications I've written. What could I have done better? Thank you.

\$\endgroup\$
  • \$\begingroup\$ i think the code is racy regarding the sm variable of type streamMap. i would have written a node.go and a relay.go. relay codes are missing timeout. why hdr := *new([]byte) ? what happens if two receive of the same file occurs before first send finishes ? the server can be flooded as it does not limit number of active connections. due to a poor api abstractions, you are not testing very much of the program, there should be something like a client and a server, and some tests that spawns each to trigger some use cases. \$\endgroup\$ – mh-cbon Feb 4 at 16:01
  • 2
    \$\begingroup\$ For other internet travelers, some good feedback also exists on reddit.com/r/golang/comments/eyphsm/… \$\endgroup\$ – jtolds Feb 4 at 16:19
  • \$\begingroup\$ The current question title is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Feb 5 at 12:44
1
\$\begingroup\$

because OP code is using poor abstractions, the code review that should be produced involves writing a whole new solution from scratch to demonsrate what should have been done going down the path chosen by OP.

it is a lot of work, with chances that the end code does not even demonstrate specific solution to some specific flaws of OP code, because the new implementation would probably be inherently not affected by this or that flaw.

here are some comments about OP code, then an alternative approach proposal

  • sm variable usage seems racy, you should go run your code with the -race argument.
  • relay codes management appears weak to flooding. they lack a maximum count limit and lifetime system involving timeout durations.
  • send/receive operations might be weak to code stealing
  • the headers are a map[string], i would use a well defined struct
  • this is weird hdr := *new([]byte), just write hdr := []byte{}
  • tests are not testing much of the use cases, the current api does not allow it.

dirty code, but functional, userA shares files with userB. the third party rdv point is provided by the tor network, users has to exchange both EP and credentials.

package main

import (
    "context"
    "crypto"
    "crypto/ed25519"
    "crypto/rand"
    "crypto/x509"
    "encoding/pem"
    "flag"
    "fmt"
    "io/ioutil"
    "log"
    "net/http"
    "os"
    "os/signal"
    "time"

    "github.com/99designs/basicauth-go"
    "github.com/clementauger/tor-prebuilt/embedded"
    "github.com/cretz/bine/tor"
)

func main() {

    var privateKey crypto.PrivateKey
    if _, err := os.Stat("onion.pk"); os.IsNotExist(err) {
        _, privateKey, err = ed25519.GenerateKey(rand.Reader)
        if err != nil {
            log.Fatal(err)
        }
        x509Encoded, err := x509.MarshalPKCS8PrivateKey(privateKey)
        if err != nil {
            log.Fatal(err)
        }
        pemEncoded := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: x509Encoded})
        ioutil.WriteFile("onion.pk", pemEncoded, os.ModePerm)
    } else {
        d, _ := ioutil.ReadFile("onion.pk")
        block, _ := pem.Decode(d)
        x509Encoded := block.Bytes
        privateKey, err = x509.ParsePKCS8PrivateKey(x509Encoded)
        if err != nil {
            log.Fatal(err)
        }
    }

    d, err := ioutil.TempDir("", "")
    if err != nil {
        log.Fatal(err)
    }

    // Start tor with default config (can set start conf's DebugWriter to os.Stdout for debug logs)
    fmt.Println("Starting and registering onion service, please wait a couple of minutes...")
    t, err := tor.Start(nil, &tor.StartConf{TempDataDirBase: d, ProcessCreator: embedded.NewCreator(), NoHush: true})
    if err != nil {
        log.Panicf("Unable to start Tor: %v", err)
    }
    defer t.Close()
    // Wait at most a few minutes to publish the service
    listenCtx, listenCancel := context.WithTimeout(context.Background(), 3*time.Minute)
    defer listenCancel()
    // Create a v3 onion service to listen on any port but show as 80
    onion, err := t.Listen(listenCtx, &tor.ListenConf{Key: privateKey, Version3: true, RemotePorts: []int{80}})
    if err != nil {
        log.Panicf("Unable to create onion service: %v", err)
    }
    defer onion.Close()
    fmt.Printf("Open Tor browser and navigate to http://%v.onion\n", onion.ID)
    // fmt.Println("Press enter to exit")
    // Serve the current folder from HTTP
    errCh := make(chan error, 1)
    go func() {
        errCh <- mainn(onion)
    }()
    // End when enter is pressed
    // go func() {
    //  if _, err := fmt.Scanln(); err == nil {
    //      errCh <- nil
    //  }
    // }()

    c := make(chan os.Signal, 1)
    signal.Notify(c)
    select {
    case err = <-errCh:
        log.Panicf("Failed serving: %v", err)
    case s := <-c:
        fmt.Println("Got signal:", s)
    }
}

func mainn(onion *tor.OnionService) error {
    var dir string
    var user string
    var pwd string
    flag.StringVar(&dir, "d", ".", "directory path to serve")
    flag.StringVar(&user, "u", "user", "username")
    flag.StringVar(&pwd, "p", "pass", "password")
    flag.Parse()

    users := map[string][]string{}
    users[user] = []string{pwd}
    middleware := basicauth.New(onion.ID, users)

    h := middleware(http.FileServer(http.Dir(dir)))

    return http.Serve(onion, h)
}

because the underlying implementation is using the http std go library, one can use all sorts of middleware to improve security or other aspects.

using tor you have a zero configuration JIT bi directionnal connectivity to work with out of the box, although some enterprise level network environment might give problems.

I don't know if that would pass the test (...)

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.