Validate tools using sha256 checksum

Validate tools using sha256 checksum

The simplestreams tools metadata includes a sha256
checksum and the size of the tarball. When tools are 
unpacked, and when they are downloaded to the bootstrap
node, the checksum and size are matched against the
tarball and an error is raised if there is a mismatch.

Legacy tools loading does not support checksums so tests
needed to be changed accordingly. In so doing, some common
tools helpers were moved to environs/testing/tools.go. This
also has the benefit of keeping the tools related test code
in one place, making it easier to swap out the legacy code
later.

https://code.launchpad.net/~wallyworld/juju-core/check-tools-checksum/+merge/187150

(do not edit description out of merge proposal)

[wallyworld, 2013-09-24 05:51:52]

Please take a look.

[mue, 2013-09-24 10:44:02]

LGTM

[jameinel, 2013-09-24 11:07:07]

a thought

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:155: cfg.Tools.SHA256,
cfg.Tools.Version.String()),
don't we need to check the sha256 *before* we untar the file?

It seems like you want to swap these two lines.

[wallyworld, 2013-09-24 11:17:46]

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:155: cfg.Tools.SHA256,
cfg.Tools.Version.String()),
On 2013/09/24 11:07:07, jameinel wrote:
> don't we need to check the sha256 *before* we untar the file?
> 
> It seems like you want to swap these two lines.

Yeah, I'll swap them. Makes no practical difference but it does seem silly to
untar if the checksum might be bad.

[rog, 2013-09-24 11:48:33]

LGTM modulo the comments below, based on reading the actual code changes. I
haven't looked at the tests yes. Very nice, BTW.

https://codereview.appspot.com/13842044/diff/1/agent/tools/toolsdir.go
File agent/tools/toolsdir.go (right):

https://codereview.appspot.com/13842044/diff/1/agent/tools/toolsdir.go#newcode45
agent/tools/toolsdir.go:45: gzipBytes, err := io.Copy(buf, r)
Storing all this in memory isn't great, and
it's something that was specifically avoided before.

Please could we unpack to a temporary file,
checksumming when we do that (see io.TeeReader), then untar
from that file if the checksum matches.

https://codereview.appspot.com/13842044/diff/1/agent/tools/toolsdir.go#newcod...
agent/tools/toolsdir.go:140: // ReadTools checks that the tools information for
the given version exist
s/exist/exists/

https://codereview.appspot.com/13842044/diff/1/agent/tools/toolsdir.go#newcod...
agent/tools/toolsdir.go:142: // The tools information is json encoded in a text
file named by the toolsFile constant.
That's not a very useful doc comment when the toolsFile constant
so isn't actually available to the reader of the documentation.
Please export the constant or state the path explicitly in the
doc command.

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:144: toolsJson, err := json.Marshal(cfg.Tools)
I'm probably blind, but where is toolsJson used?

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:152: shquote(cfg.Tools.URL),
cfg.Tools.Version.String()),
s/.String()//

(it's redundant, as %s already calls the String method if appropriate)

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:154: fmt.Sprintf(`grep "%s  -"
$bin/juju%s.sha256 || (echo "Tools checksum mismatch"; exit 1)`,
I think I'd just do:

`grep '%s' ...

so that we're not dependent on the two spaces that seem
to be printed by sha256sum. Given that we're grepping
for the full checksum, that's just as safe.

(also, in general I like to see single quotes where possible,
just for the reassurance that no funny shell expansions
are going on, even if it may be theoretically impossible
for that to happen in this case).

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:157: fmt.Sprintf("echo -n %s >
$bin/downloaded-tools.txt", shquote(string(toolsJson))),
I suppose we should probably use printf(1) here now.

https://codereview.appspot.com/13842044/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/13842044/diff/1/state/unit.go#newcode182
state/unit.go:182: if t.URL != "" && (t.Size == 0 || t.SHA256 == "") {
Given the duplication with machine.go, it might be
worth factoring this logic out into function:

// checkToolsValidity checks whether the given
// tools are suitable for passing to SetAgentTools.
func checkToolsValidity(t *tools.Tools) error

https://codereview.appspot.com/13842044/diff/1/testing/targz.go
File testing/targz.go (right):

https://codereview.appspot.com/13842044/diff/1/testing/targz.go#newcode51
testing/targz.go:51: // TarGz returns the given files in gzipped tar-archive
format, along with the tarball size and sha256 checksum.
This comment could do with reformatting.

Also, why return the size? It's always len(data)
and hence redundant (and potentially misleading)
to return it separately.

https://codereview.appspot.com/13842044/diff/1/testing/targz.go#newcode54
testing/targz.go:54: gzw := gzip.NewWriter(&buf)
You could do:

var buf bytes.Buffer
sha256hash := sha256.New()
gzw := gzip.New(io.MultiWriter(&buf, sha256hash))

which saves 4 or 5 lines below.

[wallyworld, 2013-09-25 00:19:15]

https://codereview.appspot.com/13842044/diff/1/agent/tools/toolsdir.go
File agent/tools/toolsdir.go (right):

https://codereview.appspot.com/13842044/diff/1/agent/tools/toolsdir.go#newcode45
agent/tools/toolsdir.go:45: gzipBytes, err := io.Copy(buf, r)
On 2013/09/24 11:48:34, rog wrote:
> Storing all this in memory isn't great, and
> it's something that was specifically avoided before.
> 
> Please could we unpack to a temporary file,
> checksumming when we do that (see io.TeeReader), then untar
> from that file if the checksum matches.

I've done that but we lose the size check. I don't that that matters in practise
because we are using the checksum.

https://codereview.appspot.com/13842044/diff/1/agent/tools/toolsdir.go#newcod...
agent/tools/toolsdir.go:140: // ReadTools checks that the tools information for
the given version exist
On 2013/09/24 11:48:34, rog wrote:
> s/exist/exists/

Done.

https://codereview.appspot.com/13842044/diff/1/agent/tools/toolsdir.go#newcod...
agent/tools/toolsdir.go:142: // The tools information is json encoded in a text
file named by the toolsFile constant.
On 2013/09/24 11:48:34, rog wrote:
> That's not a very useful doc comment when the toolsFile constant
> so isn't actually available to the reader of the documentation.
> Please export the constant or state the path explicitly in the
> doc command.

I'm not sure what effect exporting the constant will have? Why can't the reader
just click through to the constant declaration if they are reading the code if
they want to know what the value is?
I'll replace the reference to the constant with it's actual value, but it means
now there's a place that can get stale if the constant ever changes which is
unfortunate.

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:144: toolsJson, err := json.Marshal(cfg.Tools)
On 2013/09/24 11:48:34, rog wrote:
> I'm probably blind, but where is toolsJson used?

Down below - it's written to downloaded-tools.txt

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:152: shquote(cfg.Tools.URL),
cfg.Tools.Version.String()),
On 2013/09/24 11:48:34, rog wrote:
> s/.String()//
> 
> (it's redundant, as %s already calls the String method if appropriate)

Done.

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:154: fmt.Sprintf(`grep "%s  -"
$bin/juju%s.sha256 || (echo "Tools checksum mismatch"; exit 1)`,
On 2013/09/24 11:48:34, rog wrote:
> I think I'd just do:
> 
> `grep '%s' ...
> 
> so that we're not dependent on the two spaces that seem
> to be printed by sha256sum. Given that we're grepping
> for the full checksum, that's just as safe.
> 
> (also, in general I like to see single quotes where possible,
> just for the reassurance that no funny shell expansions
> are going on, even if it may be theoretically impossible
> for that to happen in this case).

Done.

https://codereview.appspot.com/13842044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:157: fmt.Sprintf("echo -n %s >
$bin/downloaded-tools.txt", shquote(string(toolsJson))),
On 2013/09/24 11:48:34, rog wrote:
> I suppose we should probably use printf(1) here now.

Done.

https://codereview.appspot.com/13842044/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/13842044/diff/1/state/unit.go#newcode182
state/unit.go:182: if t.URL != "" && (t.Size == 0 || t.SHA256 == "") {
On 2013/09/24 11:48:34, rog wrote:
> Given the duplication with machine.go, it might be
> worth factoring this logic out into function:
> 
> // checkToolsValidity checks whether the given
> // tools are suitable for passing to SetAgentTools.
> func checkToolsValidity(t *tools.Tools) error

Done.

https://codereview.appspot.com/13842044/diff/1/testing/targz.go
File testing/targz.go (right):

https://codereview.appspot.com/13842044/diff/1/testing/targz.go#newcode51
testing/targz.go:51: // TarGz returns the given files in gzipped tar-archive
format, along with the tarball size and sha256 checksum.
On 2013/09/24 11:48:34, rog wrote:
> This comment could do with reformatting.
> 
> Also, why return the size? It's always len(data)
> and hence redundant (and potentially misleading)
> to return it separately.

Done.

https://codereview.appspot.com/13842044/diff/1/testing/targz.go#newcode54
testing/targz.go:54: gzw := gzip.NewWriter(&buf)
On 2013/09/24 11:48:34, rog wrote:
> You could do:
> 
> var buf bytes.Buffer
> sha256hash := sha256.New()
> gzw := gzip.New(io.MultiWriter(&buf, sha256hash))
> 
> which saves 4 or 5 lines below.

Done.