Skip to content

Add --generate-key flag to createtree command #762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 10, 2017
+87 −31
Merged

Add --generate-key flag to createtree command #762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98fe3b0
Add --generate-key flag to createtree command
Jul 27, 2017
30cc45d
Panic if fakeAdminServer.generatedKey == nil but req.KeySpec != nil
Aug 9, 2017
d0e9f96
Update documentation to promote key generation by Trillian
Aug 9, 2017
2e3a7c4
Enable generating RSA keys using the createtree cmd
Aug 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 34 additions & 11 deletions cmd/createtree/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,7 @@
// command.
//
// Example usage:
// $ ./createtree \
// --admin_server=host:port \
// --pem_key_path=/path/to/pem/file \
// --pem_key_password=mypassword
// $ ./createtree --admin_server=host:port
//
// The command outputs the tree ID of the created tree to stdout, or an error to
// stderr in case of failure. The output is minimal to allow for easy usage in
Expand Down Expand Up @@ -65,7 +62,7 @@ var (
description = flag.String("description", "", "Description of the new tree")
maxRootDuration = flag.Duration("max_root_duration", 0, "Interval after which a new signed root is produced despite no submissions; zero means never")

privateKeyFormat = flag.String("private_key_format", "PrivateKey", "Type of private key to be used (PrivateKey, PEMKeyFile, or PKCS11ConfigFile)")
privateKeyFormat = flag.String("private_key_format", "", "Type of protobuf message to send the key as (PrivateKey, PEMKeyFile, or PKCS11ConfigFile). If empty, a key will be generated for you by Trillian.")
Copy link
Contributor

@gdbelvin gdbelvin Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If empty, the list of valid choices will be printed out?

pemKeyPath = flag.String("pem_key_path", "", "Path to the private key PEM file")
pemKeyPassword = flag.String("pem_key_password", "", "Password of the private key PEM file")
pkcs11ConfigPath = flag.String("pkcs11_config_path", "", "Path to the PKCS #11 key configuration file")
Expand Down Expand Up @@ -131,11 +128,6 @@ func newRequest(opts *createOpts) (*trillian.CreateTreeRequest, error) {
return nil, fmt.Errorf("unknown SignatureAlgorithm: %v", opts.sigAlgorithm)
}

pk, err := newPK(opts)
if err != nil {
return nil, err
}

ctr := &trillian.CreateTreeRequest{Tree: &trillian.Tree{
TreeState: trillian.TreeState(ts),
TreeType: trillian.TreeType(tt),
Expand All @@ -144,9 +136,40 @@ func newRequest(opts *createOpts) (*trillian.CreateTreeRequest, error) {
SignatureAlgorithm: sigpb.DigitallySigned_SignatureAlgorithm(sa),
DisplayName: opts.displayName,
Description: opts.description,
PrivateKey: pk,
MaxRootDuration: ptypes.DurationProto(opts.maxRootDuration),
}}

if opts.privateKeyType != "" {
Copy link
Contributor

@gdbelvin gdbelvin Jul 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should have a --generateKey flag?

Copy link
Contributor Author

@RJPercival RJPercival Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An earlier version of this PR had such a flag, but I removed it because this reduces the number of mutually-exclusive flags and leads to more reasonable default behaviour. I want to encourage letting Trillian generate the key, so I'd like that to be the default. This would require the --generateKey flag defaulting to true. However, that would mean you'd need to pass --generate_key=false when providing your own key else you'd get an error. Without this flag, it automatically won't generate a key if you provide the flags that specify one - this seemed cleaner.

Of course, as another of your comments seemed to suggest, this will clash with PR #734, which makes an empty --private_key_format flag print out the accepted formats. I could add a --list_private_key_formats flag for that, or just not provide that feature (users should probably have researched the protobuf message type they want to use, rather than blindly pick from a list).

Copy link
Contributor

@gdbelvin gdbelvin Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy if we don't implement the list formats feature

pk, err := newPK(opts)
if err != nil {
return nil, err
}
ctr.Tree.PrivateKey = pk
} else {
// Cannot continue if options specifying a key were provided but
// privateKeyType is not set, as there's no way to know what protobuf
// message type was intended.
if opts.pemKeyPath != "" || opts.pemKeyPass != "" || opts.pkcs11ConfigPath != "" {
Copy link
Contributor

@gdbelvin gdbelvin Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little confusing. If we want to generate a key, doesn't that take precedence over the other parameters? Perhaps issue a warning that these options will be ignored?

Copy link
Contributor Author

@RJPercival RJPercival Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user specified these options, I think it indicates that they don't want to generate a key, but have inadvertently omitted the key format flag. Therefore, printing an error and aborting seems better, rather than continuing on the (probably incorrect) assumption that a key should be generated.

return nil, errors.New("must specify private key format")
}

// If no key flags were provided at all, get Trillian to generate a key.
ctr.KeySpec = &keyspb.Specification{}

switch sigpb.DigitallySigned_SignatureAlgorithm(sa) {
case sigpb.DigitallySigned_ECDSA:
ctr.KeySpec.Params = &keyspb.Specification_EcdsaParams{
EcdsaParams: &keyspb.Specification_ECDSA{},
}
case sigpb.DigitallySigned_RSA:
ctr.KeySpec.Params = &keyspb.Specification_RsaParams{
RsaParams: &keyspb.Specification_RSA{},
}
default:
return nil, fmt.Errorf("unsupported signature algorithm: %v", sa)
}
}

return ctr, nil
}

Expand Down
67 changes: 51 additions & 16 deletions cmd/createtree/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ package main

import (
"errors"
"fmt"
"net"
"os"
"testing"
"time"

"github.com/gogo/protobuf/proto"
"github.com/golang/protobuf/proto"
"github.com/golang/protobuf/ptypes"
"github.com/golang/protobuf/ptypes/any"
"github.com/golang/protobuf/ptypes/empty"
Expand Down Expand Up @@ -78,11 +79,10 @@ func TestRun(t *testing.T) {
t.Fatalf("Error starting fake server: %v", err)
}
defer stopFn()
server.generatedKey = anyPrivKey

validOpts := newOptsFromFlags()
validOpts.addr = lis.Addr().String()
validOpts.pemKeyPath = pemPath
validOpts.pemKeyPass = pemPassword

nonDefaultTree := *defaultTree
nonDefaultTree.TreeType = trillian.TreeType_MAP
Expand All @@ -102,16 +102,24 @@ func TestRun(t *testing.T) {
invalidEnumOpts := *validOpts
invalidEnumOpts.treeType = "LLAMA!"

invalidKeyTypeOpts := *validOpts
privateKeyOpts := *validOpts
privateKeyOpts.privateKeyType = "PrivateKey"
privateKeyOpts.pemKeyPath = pemPath
privateKeyOpts.pemKeyPass = pemPassword

emptyKeyTypeOpts := privateKeyOpts
emptyKeyTypeOpts.privateKeyType = ""

invalidKeyTypeOpts := privateKeyOpts
invalidKeyTypeOpts.privateKeyType = "LLAMA!!"

emptyPEMPath := *validOpts
emptyPEMPath := privateKeyOpts
emptyPEMPath.pemKeyPath = ""

emptyPEMPass := *validOpts
emptyPEMPass := privateKeyOpts
emptyPEMPass.pemKeyPass = ""

pemKeyOpts := *validOpts
pemKeyOpts := privateKeyOpts
pemKeyOpts.privateKeyType = "PEMKeyFile"
pemKeyTree := *defaultTree
pemKeyTree.PrivateKey, err = ptypes.MarshalAny(&keyspb.PEMKeyFile{
Expand Down Expand Up @@ -141,8 +149,8 @@ xToc6NWBri0N3VVsswIDAQAB
t.Fatalf("MarshalAny(PKCS11Config): %v", err)
}

emptyPKCS11Path := *validOpts
emptyPKCS11Path.privateKeyType = "PKCS11ConfigFile"
emptyPKCS11Path := pkcs11Opts
emptyPKCS11Path.pkcs11ConfigPath = ""

tests := []struct {
desc string
Expand All @@ -156,12 +164,14 @@ xToc6NWBri0N3VVsswIDAQAB
{desc: "defaultOptsOnly", opts: newOptsFromFlags(), wantErr: true}, // No mandatory opts provided
{desc: "emptyAddr", opts: &emptyAddr, wantErr: true},
{desc: "invalidEnumOpts", opts: &invalidEnumOpts, wantErr: true},
{desc: "emptyKeyTypeOpts", opts: &emptyKeyTypeOpts, wantErr: true},
{desc: "invalidKeyTypeOpts", opts: &invalidKeyTypeOpts, wantErr: true},
{desc: "emptyPEMPath", opts: &emptyPEMPath, wantErr: true},
{desc: "emptyPEMPass", opts: &emptyPEMPass, wantErr: true},
{desc: "PEMKeyFile", opts: &pemKeyOpts, wantErr: false, wantTree: &pemKeyTree},
{desc: "PrivateKey", opts: &privateKeyOpts, wantTree: defaultTree},
{desc: "PEMKeyFile", opts: &pemKeyOpts, wantTree: &pemKeyTree},
{desc: "createErr", opts: validOpts, createErr: errors.New("create tree failed"), wantErr: true},
{desc: "PKCS11Config", opts: &pkcs11Opts, wantErr: false, wantTree: &pkcs11Tree},
{desc: "PKCS11Config", opts: &pkcs11Opts, wantTree: &pkcs11Tree},
{desc: "emptyPKCS11Path", opts: &emptyPKCS11Path, wantErr: true},
}

Expand All @@ -178,17 +188,22 @@ xToc6NWBri0N3VVsswIDAQAB
continue
}

if diff := pretty.Compare(tree, test.wantTree); diff != "" {
t.Errorf("%v: post-createTree diff:\n%v", test.desc, diff)
if !proto.Equal(tree, test.wantTree) {
Copy link
Contributor

@gdbelvin gdbelvin Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good change. consider using the full got, want format

Copy link
Contributor Author

@RJPercival RJPercival Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

t.Errorf("%v: post-createTree diff -got +want:\n%v", test.desc, pretty.Compare(tree, test.wantTree))
}
}
}

// fakeAdminServer that implements CreateTree. If err is nil, the CreateTree
// input is echoed as the output, otherwise err is returned instead.
// fakeAdminServer that implements CreateTree.
// If err is not nil, it will be returned in response to CreateTree requests.
// If generatedKey is not nil, and a request has a KeySpec set, the response
// will contain generatedKey.
// The response to a CreateTree request will otherwise contain an identical copy
// of the tree sent in the request.
// The remaining methods are not implemented.
type fakeAdminServer struct {
err error
err error
generatedKey *any.Any
}

// startFakeServer starts a fakeAdminServer on a random port.
Expand Down Expand Up @@ -218,6 +233,26 @@ func (s *fakeAdminServer) CreateTree(ctx context.Context, req *trillian.CreateTr
return nil, s.err
}
resp := *req.Tree
if req.KeySpec != nil {
if s.generatedKey == nil {
panic("fakeAdminServer.generatedKey == nil but CreateTreeRequest requests generated key")
}

var keySigAlgo sigpb.DigitallySigned_SignatureAlgorithm
switch req.KeySpec.Params.(type) {
case *keyspb.Specification_EcdsaParams:
keySigAlgo = sigpb.DigitallySigned_ECDSA
case *keyspb.Specification_RsaParams:
keySigAlgo = sigpb.DigitallySigned_RSA
default:
return nil, fmt.Errorf("got unsupported type of key_spec.params: %T", req.KeySpec.Params)
}
if treeSigAlgo := req.Tree.GetSignatureAlgorithm(); treeSigAlgo != keySigAlgo {
return nil, fmt.Errorf("got tree.SignatureAlgorithm = %v but key_spec.Params of type %T", treeSigAlgo, req.KeySpec.Params)
}

resp.PrivateKey = s.generatedKey
}
return &resp, nil
}

Expand Down
2 changes: 0 additions & 2 deletions examples/ct/ctmapper/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ go build ./examples/ct/ctmapper/lookup
go build ./cmd/createtree/
tree_id=$(./createtree \
--admin_server=localhost:8090 \
--pem_key_path=testdata/log-rpc-server.privkey.pem \
--pem_key_password=towel \
--signature_algorithm=ECDSA)
./mapper \
-source http://ct.googleapis.com/pilot \
Expand Down
4 changes: 2 additions & 2 deletions integration/log_integration_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ if [[ "${WITH_PKCS11}" == "true" ]]; then
echo 0:${TMPDIR}/softhsm-slot0.db > ${SOFTHSM_CONF}
softhsm --slot 0 --init-token --label log --pin 1234 --so-pin 5678
softhsm --slot 0 --import testdata/log-rpc-server-pkcs11.privkey.pem --label log_key --pin 1234 --id BEEF
KEY_ARGS="--private_key_format PKCS11ConfigFile --pkcs11_config_path testdata/pkcs11-conf.json --signature_algorithm=RSA"
KEY_ARGS="--private_key_format=PKCS11ConfigFile --pkcs11_config_path=testdata/pkcs11-conf.json --signature_algorithm=RSA"
else
KEY_ARGS="--pem_key_path=testdata/log-rpc-server.privkey.pem --pem_key_password=towel --signature_algorithm=ECDSA"
KEY_ARGS="--private_key_format=PrivateKey --pem_key_path=testdata/log-rpc-server.privkey.pem --pem_key_password=towel --signature_algorithm=ECDSA"
fi

echo "Provision log"
Expand Down