Add --generate-key flag to createtree command #762
Conversation
RJPercival
force-pushed
the
cmd_createtree_keygen
branch
3 times, most recently
from
64a8515 to
91c7ac9
Compare
| 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.") |
There was a problem hiding this comment.
If empty, the list of valid choices will be printed out?
| MaxRootDuration: ptypes.DurationProto(opts.maxRootDuration), | ||
| }} | ||
|
|
||
| if opts.privateKeyType != "" { |
There was a problem hiding this comment.
Perhaps we should have a --generateKey flag?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I'm happy if we don't implement the list formats feature
|
|
||
| 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) { |
There was a problem hiding this comment.
good change. consider using the full got, want format
|
Love the shortness of this PR. We seem to have some extra flag processing that would be nice to simplify. |
cmd/createtree/main_test.go
Outdated
| return nil, s.err | ||
| } | ||
| resp := *req.Tree | ||
| if req.KeySpec != nil && s.generatedKey != nil { |
There was a problem hiding this comment.
Is it worth to error if req.KeySpec != nil && s.generatedKey == nil ?
There was a problem hiding this comment.
In the past, I've gotten the impression that you'd rather avoid having test code that simply detects whether the test is misconfigured. However, I'm happy to add this if you'd prefer in this case.
There was a problem hiding this comment.
True, but in this case I think it's more about mimicking AdminServer and less about the test checking itself. This is a suggestion that doesn't change much in practice right now, so feel free to ignore it.
There was a problem hiding this comment.
I've made it panic in this case. While I know panic is to be avoided, it seems the best option here as I don't want tests to accidentally interpret this mistake in test setup as an expected error (in those test cases where fakeAdminServer.err != nil).
| // 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 != "" { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Makes the Trillian admin server generate the private key, rather than requiring the user to provide one. This may be more secure, depending on how the server stores the generated key. It is hard-coded to generate an ECDSA key using the default parameters. Users that require more control over the key specification should communicate with the admin server directly.
RJPercival
force-pushed
the
cmd_createtree_keygen
branch
from
91c7ac9 to
98fe3b0
Compare
This seems the best option as I don't want tests to accidentally interpret this mistake in test setup as an expected error (in those test cases where fakeAdminServer.err != nil).
|
Travis will only be happy with this PR once google/certificate-transparency-go#55 is merged. |
|
Travis is now happy. |
4 participants
Makes the Trillian admin server generate the private key, rather than requiring the user to provide one. This may be more secure, depending on how the server stores the generated key.
It is hard-coded to generate a key using the default parameters. Users that require more control over the key specification should communicate with the admin server directly.