cockroach sql --client-certs option fails if node.crt/node.key files without "node" subject are found in <certs dir>

[aaron-crl]

cockroach sql --certs-dir=<certs dir> does not load correct client certs if node cert is present in <certs dir>.

To Reproduce

Create node and user certificates and store them in the same directory. (This behavior ignores the user flag)

$ ls -l client-certs/
total 48
-rw-r--r--  1 aaron  staff  1099 Apr 13 17:58 ca.crt
-rw-r--r--  1 aaron  staff  3909 Apr 13 17:57 client.root.crt
-r--------  1 aaron  staff  1675 Apr 13 17:57 client.root.key
-rw-r--r--  1 aaron  staff  4149 Apr 13 17:58 node.crt
-r--------  1 aaron  staff  1679 Apr 13 17:58 node.key

$ cockroach sql --certs-dir=client-certs/ --host=myhostname --user=root
#
# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.
#
ERROR: cannot load certificates.
Check your certificate settings, set --certs-dir, or use --insecure for insecure clusters.

problem using security settings: client/server node certificate has principals ["" "node.localhost" "myhostname"], expected "node"
Failed running "sql"

However, renaming the certificate files for node.* results in:

$ ls -l client-certs/
total 48
-rw-r--r--  1 aaron  staff  1099 Apr 13 17:58 ca.crt
-rw-r--r--  1 aaron  staff  3909 Apr 13 17:57 client.root.crt
-r--------  1 aaron  staff  1675 Apr 13 17:57 client.root.key
-rw-r--r--  1 aaron  staff  4149 Apr 13 17:58 renamed-node.crt
-r--------  1 aaron  staff  1679 Apr 13 17:58 renamed-node.key

$ cockroach sql --certs-dir=client-certs/ --host=myhostname --user=root
#
# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.
#
W200414 14:53:37.287956 1 security/certificate_loader.go:312  bad filename client-certs/renamed-node.crt: unknown prefix "renamed-node"
# Server version: CockroachDB CCL v20.1.0-beta.4 (x86_64-apple-darwin14, built 2020/03/24 01:31:53, go1.13.5) (same version as client)
# Cluster ID: 765314c3-f249-48d1-8747-7bb59c315ed3
#
# Enter \? for a brief introduction.
#
root@myhostname:26257/defaultdb> 

Expected behavior
Command will walk certificates in the <cert dir> and match the correct one to the implied or supplies principle.

Environment:

• CockroachDB version: cockroach-v20.1.0-beta.4.darwin-10.9
• Server OS: OSX 10.15.4
• Client app cockroach sql

Thanks @Amruta-Ranade for helping identify this.
CC @knz @petermattis

xref: #47300
xref: #47449
xref: cockroachdb/docs#7104
xref: https://www.cockroachlabs.com/docs/dev/connection-parameters.html

[petermattis]

I think this is a dup of #47210. For the sql, cert *, and other "client" commands, I don't think we should be validating the certs in the certs directory, but only using the specific cert. This is another argument for using sql --url which avoids this bit of surprise.

[aaron-crl]

I think this is a dup of #47210. For the sql, cert *, and other "client" commands, I don't think we should be validating the certs in the certs directory, but only using the specific cert. This is another argument for using sql --url which avoids this bit of surprise.

Probably same root cause. Do we want to deprecate --certs-dir or overhaul it to be command context sensitive?

[petermattis]

Probably same root cause. Do we want to deprecate --certs-dir or overhaul it to be command context sensitive?

My preference is to deprecate --certs-dir. I believe that is @knz's preference as well. What is your preference, @aaron-crl?

Also, @bdarnell do you want to weigh in here? I can't recall the history of how we ended up using --certs-dir for client commands.

[Amruta-Ranade]

Are we discussing deprecating --certs-dir for 20.1? That would have a HUGE docs (and probably CockroachCloud) impact.

[petermattis]

Are we discussing deprecating --certs-dir for 20.1? That would have a HUGE docs (and probably CockroachCloud) impact.

No, it is much too late in the 20.1 cycle for that. The earliest we could deprecate it is 20.2.

[aaron-crl]

I vote deprecate as well, if the flag doesn't have a well defined behavior but we have equivalent functionality with common behavior to postgres this sounds like an easy win.

My only misgiving is that the --url connection strings are ugly from a UX standpoint - but this hardly outweighs the cost of redesigning the --certs-dir flag.

[bdarnell]

I think it is well-defined, it just has a bug that it tries to "validate" stuff it finds in that directory that it has no business caring about. I don't see a reason to deprecate; getting to the point where we could remove it would be a pretty heavy lift since this has been the process since the early days. I'd close this as a dupe of #47210

[aaron-crl]

I reread the docs page here: https://www.cockroachlabs.com/docs/v20.1/cockroach-cert.html#node-key-and-certificates and see that most of the expected certificate load paths are called out in an easy to read tabular form though split across a couple sections (with the exception of the ui which is in a sentence below one table).

Maybe I skimmed the docs too quickly when I filed this. But I guess I expected to see a description of all the defaults the flag implied in one place. However, this is a more minor quibble so I retracted the claim that this behavior is ambiguous.

I'm not sure it's a dup of #47210 as the CLI fails to run if the operator follows our previously (and currently) documented pathing convention in conjunction with the new --cert-principal-map flag. If the cockroach sql command finds a node cert that it can't validate - it halts.

Changing issue name to reflect this - pen to additional thoughts though.

[bdarnell]

The cockroach cert page isn't really mean to be the master documentation for the --certs-dir flag as used on other commands (and this page is more comprehensive). We should create a docs page specifically about the certs directory that's not tied to specific CLI subcommands or means of creating certs.

And I still think it's a dupe of #47210 because cockroach sql shouldn't be looking at node certs, or any certs other than the CA and the one for the user it's about to use.

[aaron-crl]

Thanks for the clarification and that makes sense, closing as a dup of #47210.