3
\$\begingroup\$

This is my solution to challenge 1.6 from Matasano/NCC Cryptopals.

Here is my repository so the code can be cloned/ran and the bitutils/futils code can also be looked at if necessary (I've created the branch tag 'for-review' to pin this code version for this post) but here is the main code we're concerned with:

package main

import (
    "encoding/base64"
    "github.com/tomatopeel/pals/bitutils"
    "github.com/tomatopeel/pals/futils"
    "io/ioutil"
    "log"
    "os"
    "strings"
)

var (
    remote_file string = "http://cryptopals.com/static/challenge-data/6.txt"
    local_file  string = "secrets_01_06.txt"
    alphabet    string = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ \n\r,.'0123456789!()?\";:"
)

func main() {
    file, err := os.Open(local_file)
    if err != nil {
        futils.DownloadFile(local_file, remote_file)
        file, err = os.Open(local_file)
        if err != nil {
            log.Fatal(err)
        }
    }
    defer file.Close()

    decoder := base64.NewDecoder(base64.StdEncoding, file)
    data, err := ioutil.ReadAll(decoder)
    if err != nil {
        log.Fatal(err)
    }

    k := KeySize(1, 50, data)

    blocks := Blocks(k, data)
    transd := make([][]byte, k)

    for _, block := range blocks {
        for j, byte := range block {
            transd[j] = append(transd[j], byte)
        }
    }

    key := []byte{}
    for _, block := range transd {
        c, _ := TopCharacter(block)
        key = append(key, byte(c))
    }
    log.Printf("KEY: %s", string(key))

    plaintext := bitutils.RepeatingKeyXOR(data, key)
    log.Printf("PLAINTEXT: %s", string(plaintext))
}

// Determine most likely keysize between x and y
func KeySize(x, y int, data []byte) (keysize int) {
    var ham float64

    for i := x; i <= y; i++ {

        temp_ham, counter := float64(0), 0
        blocks := Blocks(i, data)

        for j, init := range blocks {
            for _, rem := range blocks[j+1:] {

                result, err := bitutils.Hamming(init, rem)
                if err != nil {
                    log.Fatal(err)
                }

                temp_ham += (float64(result) / float64(i))
                counter++
            }
        }

        temp_ham /= float64(counter)
        if temp_ham < ham || ham == 0 {
            ham = temp_ham
            keysize = i
        }
    }
    return keysize
}

// Split data into k len []byte's and return the [][]byte
func Blocks(k int, data []byte) (blocks [][]byte) {
    for b, rem := Block(k, data); b != nil; b, rem = Block(k, rem) {
        blocks = append(blocks, b)
    }
    return
}

// Return k length []byte and the remaining []byte
func Block(k int, data []byte) ([]byte, []byte) {
    if len(data) < k {
        return nil, nil
    } else {
        return data[:k], data[k:]
    }
}

// Determine most likely character as part of key
func TopCharacter(secret []byte) (c rune, score int) {
    tester := make([]byte, len(secret))

    for _, a := range alphabet {
        for i := 0; i < len(secret); i++ {
            tester[i] = byte(a)
        }
        for i := range secret {
            tester[i] ^= secret[i]
        }
        temp_score := Score(tester)
        if temp_score > score {
            score = temp_score
            c = rune(a)
        }
    }
    return
}

func Score(line []byte) (score int) {
    for i := range line {
        if strings.ContainsRune(alphabet, rune(line[i])) {
            score++
        }
    }
    return
}

I'm actually quite happy with this, I spent many hours working through this challenge, I think the code is quite well arranged in terms of the splitting of the functions, and I'm particularly pleased with the Blocks and Block functions. What I'm looking for in review:

  1. To be shot down on what I'm happy with.

  2. I'm pretty new to Golang, this is my first project (going through the Cryptopals challenges). Suggestions for anything I could be doing in a more "idiomatic" fashion would be useful.

  3. I was hesitant to use ioutil.ReadAll until very late on, as I've read some articles discouraging use of this function. Previous incarnations of this code involved passing the reader and the *os.File around and having them make the Read or ReadFull calls - it was really messy like this and it seems using ReadAll on the ciphertext file here makes sense, is it so?

  4. Is the code dealing with the opening/getting of the local/remote file alright? And how about the error-handling?

  5. If I was going to attempt to make this code leverage Golang's concurrency features, what approach would be suggested? (I haven't played much with concurrency yet, and I don't think the challenges I'm doing are going to require any concurrent programming, which I feel like I should be practising).

\$\endgroup\$
1
\$\begingroup\$

Code should be readable, correct, maintainable, robust, and reasonably efficient. Readability is a prerequisite for the other characteristics.

Start with Go Code Review Comments.

You write:

func Block(k int, data []byte) ([]byte, []byte) {
    if len(data) < k {
        return nil, nil
    } else {
        return data[:k], data[k:]
    }
}

The unnecessary else conditional path and indentation are a distraction.

The most important path is the correct path. It should be easy to read the correct path.

func Block(k int, data []byte) ([]byte, []byte) {
    return data[:k], data[k:]
}

Now, in an unobtrusive fashion, check for and handle errors:

func Block(k int, data []byte) ([]byte, []byte) {
    if len(data) < k {
        return nil, nil
    }
    return data[:k], data[k:]
}

See Indent Error Flow

You use log.Fatal(err) to panic on errors. By design, Go returns errors. For example,

file, err := os.Open(localFile)
if err != nil {
    err = downloadFile(localFile, remoteFile)
    if err != nil {
        return nil, err
    }
    file, err = os.Open(localFile)
    if err != nil {
        return nil, err
    }
}
defer file.Close()

With goroutines, an unrecovered panic crashes the program for all (100,000 or more) users. Program for Google scale robustness and concurrency. Return errors; don't panic.

See Don't Panic

And so forth.

Here's a simple, more readable, and robust example, and a pattern that can be used elsewhere.

var (
    localFile  string = "secrets_01_06.txt"
    remoteFile string = "http://cryptopals.com/static/challenge-data/6.txt"
)

func main() {
    data, err := readData(localFile, remoteFile)
    if err != nil {
        log.Fatal(err)
    }
    // ...
}

func readData(localFile, remoteFile string) ([]byte, error) {
    file, err := os.Open(localFile)
    if err != nil {
        err = downloadFile(localFile, remoteFile)
        if err != nil {
            return nil, err
        }
        file, err = os.Open(localFile)
        if err != nil {
            return nil, err
        }
    }
    defer file.Close()

    decoder := base64.NewDecoder(base64.StdEncoding, file)
    data, err := ioutil.ReadAll(decoder)
    if err != nil {
        return nil, err
    }
    return data, nil
}

func downloadFile(local string, url string) error {
    resp, err := http.Get(url)
    if err != nil {
        return err
    }
    defer resp.Body.Close()

    f, err := os.Create(local)
    if err != nil {
        return err
    }
    defer f.Close()
    _, err = io.Copy(f, resp.Body)
    if err != nil {
        return err
    }
    err = f.Close()
    if err != nil {
        return err
    }
    return nil
}

Go uses camelCase: remoteFile. Go uses a capital letter for the first character to export identifiers.

Use functions to encapsulate complexity: readData.

Bubble errors up so that each level can handle them appropriately.

Local variables such as file and decoder are not exposed outside the function.

The defer f.Close() function runs at the end of the function instead of the end of the program.

For output files, check for errors on Close.

Don't panic in packages: futils. downloadFile is a rewrite of futils,DownloadFile.

Note that downloadFile now truncates the local file only if there is no error reading the remote file. From a design point of view, we may want to download the latest version of the remote file if available. If there is an error, common on remote connections, we can elect to fall back to the last local file. Robust code gracefully handles failure.

And so forth.

There is more, but I'm out of time.

\$\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.