Change security flags

[mberhault]

Addresses #4269

Replace --certs with individual flags for CA cert and key, and
server/client cert and key.
One notable difference is that we now have a single certificate for
nodes with double role as server and client authentication.

A few notes:

• this turned out to be big enough, so I'll leave the default --insecure value to the next PR.
• removed cmd/monitor, it isn't being used anywhere
• I'll probably collapse all the path building into helder funcs.

[petermattis]

Looks good besides the {ssl,SSL} prefix. @spencerkimball, @bdarnell might want to weigh in on the flag naming.

---

Review status: 0 of 43 files reviewed at latest revision, 2 unresolved discussions.

---

cli/flags.go, line 153 [r1] (raw file):
Looks like these flag names are inspired by postgres. I'm wondering if we want the ssl- prefix here. We're using TLS, not SSL. Postgres likely uses ssl for historical reasons. But newer programs like docker use tls for a prefix. etcd omits a prefix entirely and uses --ca-file and --cert-file.

---

security/x509.go, line 151 [r1] (raw file):
What does this change do?

---

Comments from the review on Reviewable.io

[spencerkimball]

Why don't we rename these keys so they're shorter and more consistent:

--ca-cert
--ca-key
--cert
--key

[bdarnell]

Review status: 0 of 43 files reviewed at latest revision, 6 unresolved discussions.

---

cli/flags.go, line 153 [r1] (raw file):
Users will still need to see names with ssl: the postgres connection url uses parameters like sslcert and sslkey. I think it's probably better to just use those names (without hyphens) than to use different names between our own tools and postgres drivers (in fact, we should have the option to just pass in a postgres url for all of our client-side commands). If we're going to deviate from postgresql's naming then I'd prefer to drop the tls/ssl prefix entirely (and cert-key is weird; it's just a key)

---

resource/test_certs/README.md, line 23 [r1] (raw file):
Specifying these separately is really tedious. I get that we want to move away from our old assumption that everything is in one place (especially for the CA key, which really shouldn't be copied all over the place), but I think it would be good if we could still infer the locations of the other files from the location of one.

---

Comments from the review on Reviewable.io

[tamird]

Reviewed 42 of 43 files at r1.
Review status: 42 of 43 files reviewed at latest revision, 8 unresolved discussions.

---

acceptance/cluster/localcluster.go, line 398 [r1] (raw file):
I think this function (and perhaps the others) should take a struct as their argument, so that the fields are more clearly defined in the caller.

---

base/context.go, line 55 [r1] (raw file):
this could use the same struct described in my other comment

---

Comments from the review on Reviewable.io

[petermattis]

Review status: 42 of 43 files reviewed at latest revision, 8 unresolved discussions.

---

cli/flags.go, line 153 [r1] (raw file):
Yes, the postgres connection url uses parameters with the ssl prefix, but I'm not sure if that is enough justification to use the ssl prefix for our flags. My preference is to try the prefix entirely and go with --{,ca-}{cert,key}. Sounds like this is @spencerkimball's preference as well. Let's finalize this decision today.

---

Comments from the review on Reviewable.io

[spencerkimball]

Review status: 42 of 43 files reviewed at latest revision, 8 unresolved discussions.

---

cli/flags.go, line 153 [r1] (raw file):
I agree we should be able to accept a URL on the sql command!

---

Comments from the review on Reviewable.io

[petermattis]

Review status: 42 of 43 files reviewed at latest revision, 8 unresolved discussions.

---

cli/flags.go, line 153 [r1] (raw file):
cockroach start --url <url> already works. Is there something more you're looking for.

---

Comments from the review on Reviewable.io

[petermattis]

Review status: 42 of 43 files reviewed at latest revision, 8 unresolved discussions.

---

cli/flags.go, line 153 [r1] (raw file):
Err, cockroach sql --url <url>.

---

Comments from the review on Reviewable.io

[bdarnell]

Review status: 42 of 43 files reviewed at latest revision, 8 unresolved discussions.

---

cli/flags.go, line 153 [r1] (raw file):
Ah, I missed that we already had that. I would like to take it a step further, though, and use it for all client commands, including those that are based on the KV api under the hood (unless all of those are doomed to go away or be replaced by sql)

---

Comments from the review on Reviewable.io

[petermattis]

Review status: 42 of 43 files reviewed at latest revision, 8 unresolved discussions.

---

cli/flags.go, line 153 [r1] (raw file):
The --url flag also works with the cockroach {zone,user} commands. I suppose we can make the {range,node,kv,exterminate,quit} commands also accept --url. File a bug.

---

Comments from the review on Reviewable.io

[mberhault]

Review status: 42 of 43 files reviewed at latest revision, 8 unresolved discussions.

---

acceptance/cluster/localcluster.go, line 398 [r1] (raw file):
it's just a function call, putting all args into a struct doesn't particularly make it clearer.

---

base/context.go, line 55 [r1] (raw file):
ditto, adding an extra level of addressing seems odd.

---

cli/flags.go, line 144 [r1] (raw file):
discussing in main thread about this

---

cli/flags.go, line 145 [r1] (raw file):
Done.

---

cli/flags.go, line 153 [r1] (raw file):
about --url: it's fine for sql addresses, and to some extent for the kv layer comands as well, although there is some question of what to do with the security mode. use sslmode or the scheme part of the url?
This still leaves the start and certs commands which need actual flags (URLs don't make sense here).

I like using ssl to make it clear what cert/key we're talking about. Having simply key or ca is too ambiguous.
As for tls, I don't think it makes more sense. people equate ssl with secure communications and certs, often using the term as a superset of both actual ssl and tls.

Overall, it would be nice to have agreement between url args and cmdline args, but I really don't like the non-hyphened versions in postgres, and sslrootcert is also a terrible name.

So here are a few proposals:

1. stick with postgres names (sslrootcert, sslcert, sslkey) and add sslrootkey when signing certs.
2. customize names: ssl-ca-cert, ssl-ca-key, ssl-cert, ssl-key
3. custom, but dropping ssl entirely: ca-cert, ca-key, cert, key.

---

cli/kv.go, line 40 [r1] (raw file):
these should end up matching whatever we decide as the final names.

---

resource/test_certs/README.md, line 23 [r1] (raw file):
I agree, but the old "custom directory + hard-coded filenames" was really odd and inflexible.
I'm not sure picking something in-between would be much better. We could either allow both directory and custom filenames, or deduce the directory from one of the filenames but the question becomes which.

---

security/x509.go, line 151 [r1] (raw file):
this allows the node certificate to be used both as server and client certificates.
before this change, we generated two separate ones, but we can combine them.

---

Comments from the review on Reviewable.io

[tamird]

Review status: 42 of 43 files reviewed at latest revision, 7 unresolved discussions.

---

acceptance/cluster/localcluster.go, line 398 [r1] (raw file):
What do you mean? If you made it a struct it would be

security.RunCreateNodeCert(security.CreateNodeCertConfig{
  CertPath: ..,
  CAPath: ...,
 ....
})

Seems much clearer to me.

---

Comments from the review on Reviewable.io

[mberhault]

per discussion, renamed to ca-cert, ca-key, cert, and key

---

Review status: 38 of 43 files reviewed at latest revision, 7 unresolved discussions.

---

acceptance/cluster/localcluster.go, line 398 [r1] (raw file):
this doesn't really buy anything. with decent indentation and clear filenames as we have here, it's clear enough. Creating a new struct for each of those functions really won't help.

---

Comments from the review on Reviewable.io

[tamird]

Reviewed 4 of 5 files at r2.
Review status: 42 of 43 files reviewed at latest revision, 7 unresolved discussions.

---

acceptance/cluster/localcluster.go, line 398 [r1] (raw file):
OK. I disagree, but we don't have guidelines on this. Flagging for the style guide @bdarnell @petermattis

---

Comments from the review on Reviewable.io

[bdarnell]

LGTM

---

Review status: 34 of 43 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

acceptance/cluster/localcluster.go, line 398 [r1] (raw file):
-1 on using a struct for this. It's not clear to me that it's even an improvement (The struct makes it possible to name your arguments but it also means that they are all optional as far as the compiler is concerned.), and even if it were an improvement it's such a minor thing that it wouldn't make sense to mandate in the style guide.

---

cli/cli_test.go, line 169 [r3] (raw file):
Isn't --insecure=false implied by the presence of the cert/key flags?

---

cli/flags.go, line 153 [r1] (raw file):
The scheme part of the url is always postgresql and doesn't indicate security; the security mode would always come from sslmode when a url is used.

I think I like the bare names (option 3) best, followed by postgres-style (option 1), prefixed with tls- (not listed), and prefixed by ssl- (option 2)

---

Comments from the review on Reviewable.io

[tamird]

Reviewed 8 of 9 files at r3.
Review status: 42 of 43 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

Comments from the review on Reviewable.io

[petermattis]

LGTM

---

Review status: 42 of 43 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

base/context.go, line 54 [r3] (raw file):
Should the SSL prefix be removed from these fields? I see utility in having the prefix, but SSLCA is quite a mouthful.

---

Comments from the review on Reviewable.io

[mberhault]

Review status: 28 of 42 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

base/context.go, line 54 [r3] (raw file):
This is a case where I'd much rather be very clear. SSLCA may be annoying from a capitalization point of view, but I think the clearer meaning in callers makes it worth it.

---

cli/cli_test.go, line 169 [r3] (raw file):
nope. Right now, specifying --insecure=false and empty --cert on the server is an error.
On the client, it gets turned into InsecureSkipVerify mode.

---

cli/flags.go, line 153 [r1] (raw file):
Done.

---

Comments from the review on Reviewable.io

[jseldess]

Docs updated with cockroachdb/docs#117.