Client/domaininfo #752
Client/domaininfo #752
Conversation
gdbelvin
force-pushed
the
client/domaininfo
branch
from
25788c8 to
6a36afd
Compare
Codecov Report
@@ Coverage Diff @@
## master #752 +/- ##
==========================================
+ Coverage 48.92% 49.18% +0.25%
==========================================
Files 30 30
Lines 2520 2507 -13
==========================================
Hits 1233 1233
+ Misses 1090 1077 -13
Partials 197 197
Continue to review full report at Codecov.
|
- Use der.ToPublicProto for generating and parsing keyspb.PublicKey - Add insecure flag so people can try out the test server without messing with the self signed cert. - Set the test server as the default server URL. - Cleaned up the dial function by separating transport from client credentials.
gdbelvin
force-pushed
the
client/domaininfo
branch
from
6a36afd to
0742884
Compare
Also return interfaces instead of concrete types.
There was a problem hiding this comment.
Thanks a lot for this PR!
Besides a few nits this LGTM.
I think you can ignore the comments about the documenting how the different flags depend on each other (autoconfig,vrf,log-key, and map-key as well as fake-auth-userid, client-secret and service-key) and merge it as it is.
If it really turns out to be confusing we can fix this in a separate pull!
| RootCmd.PersistentFlags().String("kt-cert", "genfiles/server.crt", "Path to public key for Key Transparency") | ||
| RootCmd.PersistentFlags().Bool("autoconfig", true, "Fetch config info from the server's /v1/domain/info") | ||
| RootCmd.PersistentFlags().Bool("insecure", false, "Skip TLS checks") | ||
|
|
||
| RootCmd.PersistentFlags().String("vrf", "genfiles/vrf-pubkey.pem", "path to vrf public key") | ||
|
|
||
| RootCmd.PersistentFlags().String("log-key", "genfiles/trillian-log.pem", "Path to public key PEM for Trillian Log server") | ||
| RootCmd.PersistentFlags().String("map-key", "genfiles/trillian-map.pem", "Path to public key PEM for Trillian Map server") |
There was a problem hiding this comment.
Suggestion: Maybe add a comment stating that these flags will be set automatically if --autoconfig=true? Or, even add this to the viper description string. Otherwise, it might be unclear what will be set automatically and what not.
(If --autoconfig=true, which is default, it doesn't matter what you set for the vrf, log-key, and map-key)
| RootCmd.PersistentFlags().String("vrf", "genfiles/vrf-pubkey.pem", "path to vrf public key") | ||
|
|
||
| RootCmd.PersistentFlags().String("log-key", "genfiles/trillian-log.pem", "Path to public key PEM for Trillian Log server") | ||
| RootCmd.PersistentFlags().String("map-key", "genfiles/trillian-map.pem", "Path to public key PEM for Trillian Map server") | ||
|
|
||
| RootCmd.PersistentFlags().String("client-secret", "", "Path to client_secret.json file for user creds") | ||
| RootCmd.PersistentFlags().String("service-key", "", "Path to service_key.json file for anonymous creds") | ||
| RootCmd.PersistentFlags().String("fake-auth-userid", "", "userid to present to the server as identity for authentication. Only succeeds if fake auth is enabled on the server side.") |
There was a problem hiding this comment.
Suggestion: Document the "priority" here as well? If fake-auth-userid is set, client-secret and service-key will be ignored, etc. I wonder if viper provides and automatic way to warn you that it doesn't make sense to set all of these 3.
|
|
||
| case ktCert != "": // Custom CA Cert. | ||
| return credentials.NewClientTLSFromFile(ktCert, host) | ||
|
|
| return credentials.NewTLS(&tls.Config{ | ||
| InsecureSkipVerify: true, | ||
| }), nil | ||
|
|
There was a problem hiding this comment.
remove superfluous newline?
There was a problem hiding this comment.
the newlines visually break up the cases
Configure the client using the info published at
v1/domain/infoThis saves users from needing to do a lot of complicated configuration.
Changes:
--autoconfigflag, defaulting totrue, which reads configuration information fromv1/domain/info--insecureflag in case users want to connect to a server with a self-signed certificate.crypto.PublicKeyas the return type forvrf.Public()