cli: add --cert-principal-map to client commands

[petermattis]

Add support for the --cert-principal-map flag to the certs and client
commands. Anywhere we were accepting the --certs-dir flag, we now also
accept the --cert-principal-map flag.

Fixes #47300

Release note (cli change): Support the --cert-principal-map flag in
the cert * and "client" commands such as sql.

[cockroach-teamcity]

This change is 

[petermattis]

This needs unit tests, but it seems to work for me in manual testing. Before pushing on that, I want to make sure this is directionally correct.

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 7b9b37bb.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[knz]

I am not too fond of this approach, it really over-emphasizes the --certs-dir flag and its idiosyncrasies.

Why not deprecate the --certs-dir altogether and emphasize --url with explicit certs instead? This way our doc on this topic would work equally well for the cockroach CLI commands and also psql or other SQL clients (this would simplify overall).

[petermattis]

I am not too fond of this approach, it really over-emphasizes the --certs-dir flag and its idiosyncrasies.

I agree. In my manual testing of this PR I confused myself several times.

Why not deprecate the --certs-dir altogether and emphasize --url with explicit certs instead? This way our doc on this topic would work equally well for the cockroach CLI commands and also psql or other SQL clients (this would simplify overall).

My major complaint with --url is the need for %2F in the URL is quite awkward. On the plus side, --url makes it nicely explicit which certs and keys are being used, which is not at all true of --certs-dir.

[petermattis]

My major complaint with --url is the need for %2F in the URL is quite awkward. On the plus side, --url makes it nicely explicit which certs and keys are being used, which is not at all true of --certs-dir.

I'm going to partially retract my complaint. Both "postgresql://root@localhost:26257?sslcert=certs%2Fclient.foo.crdb.io.crt&sslkey=certs%2Fclient.foo.crdb.io.key&sslmode=verify-full&sslrootcert=certs%2Fca.crt" and "postgresql://root@localhost:26257?sslcert=certs/client.foo.crdb.io.crt&sslkey=certs/client.foo.crdb.io.key&sslmode=verify-full&sslrootcert=certs/ca.crt" work fine in the --url parameter. Is the Go URL parser more permissive than required in regards to quoting?

[knz]

The URL quoting is not mandatory over HTTP or anything network. It is available for convenience so that the result of fetching a URL can be stored in a file named after the entire last portion of URL (after quoting).

[petermattis]

The URL quoting is not mandatory over HTTP. It is available for convenience so that the result of fetching a URL can be stored in a file named after the entire last portion of URL (after quoting).

Ah. Perhaps we should adjust the help text shown at start:

sql:                 postgresql://root@Peter's-Macbook-Pro:26257?sslmode=verify-full&sslrootcert=certs%2Fca.crt

Should the help text for other client commands use the --url flag instead of --certs-dir?

RPC client flags:    ./cockroach <client cmd> --host=Peter's-Macbook-Pro:26257 --certs-dir=certs

[knz]

Yes I agree these improvements would be worthwhile.

(However meanwhile I am horrified that macOS puts single quotes in hostnames, and that Go's parser allows them. I'm 99.999% confident this is a giant no-no by all IETF standards)

[dbist]

I'm +1 on this approach myself, just ran through the flow and this approach also fixes the issue with having additional directories to split node certs from client certs.

mkdir certs my-safe-directory
openssl genrsa -out my-safe-directory/ca.key 2048
chmod 400 my-safe-directory/ca.key
openssl req -new -x509 -config ca.cnf -key my-safe-directory/ca.key -out certs/ca.crt -days 365 -batch
rm -f index.txt serial.txt
touch index.txt
echo '01' > serial.txt
openssl genrsa -out certs/node.key 2048
chmod 400 certs/node.key
openssl req -new -config node.cnf -key certs/node.key -out node.csr -batch
openssl ca -config ca.cnf -keyfile my-safe-directory/ca.key -cert certs/ca.crt -policy signing_policy -extensions signing_node_req -out certs/node.crt -outdir certs/ -in node.csr -batch
openssl x509 -in certs/node.crt -text | grep "X509v3 Subject Alternative Name" -A 1
openssl genrsa -out certs/client.root.key 2048
chmod 400 certs/client.root.key
openssl req -new -config client.cnf -key certs/client.root.key -out client.root.csr -batch
openssl ca -config ca.cnf -keyfile my-safe-directory/ca.key -cert certs/ca.crt -policy signing_policy -extensions signing_client_req -out certs/client.root.crt -outdir certs/ -in client.root.csr -batch
openssl x509 -in certs/client.root.crt -text | grep CN=
# removing .pem files is only required once with a single directory
rm -rf certs/*.pem
cockroach start-single-node --certs-dir=certs --cert-principal-map=node.example.com:node,user.example.com:root --background
cockroach sql --url 'postgresql://root@node.example.com:26257?sslcert=certs%2Fclient.root.crt&sslkey=certs%2Fclient.root.key&sslmode=verify-full&sslrootcert=certs%2Fca.crt'

[petermattis]

I just revived this PR as a stop-gap for 20.1.

I am not too fond of this approach, it really over-emphasizes the --certs-dir flag and its idiosyncrasies.

Agreed, yet I'm worried that properly fixing the "client" commands to not use CertificateManager will be too invasive to backport to 20.1. I'm still plugging away on that, but it is taking longer to untangle than I expected. As long as --certs-dir exists for a command, I think we should also allow specification of --cert-principal-map. I'm open to better suggestions, but as #48116 shows, we have some brokenness in 20.1 right now that it would be good to provide a solution for.

[knz]

As long as --certs-dir exists for a command, I think we should also allow specification of --cert-principal-map

Ok I can stand behind this for consistency's sake.

[petermattis]

Ok I can stand behind this for consistency's sake.

Great. Let me add some additional testing to this PR.

[knz]

I re-reviewed this. LGTM modulo a test that the flag is processed. I would recommend not adding testing in more places than where --certs is tested already.

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl)

[aaron-crl]

As long as --certs-dir exists for a command, I think we should also allow specification of --cert-principal-map

Ok I can stand behind this for consistency's sake.

Sounds like the best option for the near term. Let's start tracking these hiccups so we can make sure we get a cleaner separation later.

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 04c0b124.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[petermattis]

Sounds like the best option for the near term. Let's start tracking these hiccups so we can make sure we get a cleaner separation later.

Ack. #47210 is where I'm currently tracking these hiccups. Let me retitle that issue and expand the description.

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl)

---

pkg/cli/interactive_tests/test_cert_advisory_validation.tcl, line 87 at r2 (raw file):

expect $prompt
send "$argv node ls --certs-dir=$certs_dir --cert-principal-map=foo.bar:node"
eexpect "1 row"

I think I cargo-culted something wrong here as the test fails at this line. Perhaps I should be adding this test to test_secure.tcl instead and using the existing start_secure_server. I'd need to create an additional cert which doesn't validate without --cert-principal-map, which is why I initially went with the addition here.

@knz I hope you can spot what I've done wrong and advise on the appropriate fix. Or give me guidance on how to debug this.

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 77baebe2.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[knz]

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl and @petermattis)

---

pkg/cli/interactive_tests/test_cert_advisory_validation.tcl, line 87 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I think I cargo-culted something wrong here as the test fails at this line. Perhaps I should be adding this test to test_secure.tcl instead and using the existing start_secure_server. I'd need to create an additional cert which doesn't validate without --cert-principal-map, which is why I initially went with the addition here.

@knz I hope you can spot what I've done wrong and advise on the appropriate fix. Or give me guidance on how to debug this.

The magic incantation that I use is

killall -9 cockroach cockroachshort && rm -rf logs && expect -d -f pkg/cli/interactive_tests/xxxx.tcl ./cockroach

(expect -d helps. However, it's possible that the default expect in macOS gives you trouble. YMMV.)

---

pkg/cli/interactive_tests/test_cert_advisory_validation.tcl, line 83 at r3 (raw file):

start_test "Check that the client commands can use cert principal map."
system "$argv start-single-node --certs-dir=$certs_dir --cert-principal-map=foo.bar:node --advertise-addr=localhost --background >>expect-cmd.log 2>&1"
send "$argv sql --certs-dir=$certs_dir --cert-principal-map=foo.bar:node -e \"select 'hello'\""

You forgot \r at the end of the line.

---

pkg/cli/interactive_tests/test_cert_advisory_validation.tcl, line 86 at r3 (raw file):

eexpect "hello"
expect $prompt
send "$argv node ls --certs-dir=$certs_dir --cert-principal-map=foo.bar:node"

ditto.

---

pkg/cli/interactive_tests/test_cert_advisory_validation.tcl, line 89 at r3 (raw file):

eexpect "1 row"
expect $prompt
send "$argv quit --certs-dir=$certs_dir --cert-principal-map=foo.bar:node"

\r here too

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl and @knz)

---

pkg/cli/interactive_tests/test_cert_advisory_validation.tcl, line 87 at r2 (raw file):

Previously, knz (kena) wrote…

The magic incantation that I use is

killall -9 cockroach cockroachshort && rm -rf logs && expect -d -f pkg/cli/interactive_tests/xxxx.tcl ./cockroach

(expect -d helps. However, it's possible that the default expect in macOS gives you trouble. YMMV.)

Thanks.

---

pkg/cli/interactive_tests/test_cert_advisory_validation.tcl, line 83 at r3 (raw file):

Previously, knz (kena) wrote…

You forgot \r at the end of the line.

Ah-ha! I've made exactly that mistake before. Thank you!

Done.

---

pkg/cli/interactive_tests/test_cert_advisory_validation.tcl, line 86 at r3 (raw file):

Previously, knz (kena) wrote…

ditto.

Done.

---

pkg/cli/interactive_tests/test_cert_advisory_validation.tcl, line 89 at r3 (raw file):

Previously, knz (kena) wrote…

\r here too

Done.

[knz]

lemme hand hold this for you

[aaron-crl]

Should have marked my earlier comment approved.

[knz]

@petermattis the proximate cause of the failure is that you can't use a node cert for a SQL client connection.

[knz]

Ok there's something uncanny going on with cert create-client too now.

[petermattis]

@petermattis the proximate cause of the failure is that you can't use a node cert for a SQL client connection.

Yep, I just stumbled on this myself. I've added creation of a root cert. Will see what the next problem is.

[petermattis]

Ok there's something uncanny going on with cert create-client too now.

Just hit this myself. It will need a --cert-principal-map flag as well.

[petermattis]

Ok, I think I've got the test working, @knz. Thanks for your help!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz)

[knz]

Trying your code made me discover that the cert mapping only works for the node cert, not for SQL or RPC client certs. It's not possible to remap root.myorg.com to root with the code you've implemented so far.

This comes as a surprise (to me). I have said something different to @Amruta-Ranade in the past. I had expected the cert mapping to be universal for all uses of certs, not just for node-node connections.

[petermattis]

Trying your code made me discover that the cert mapping only works for the node cert, not for SQL or RPC client certs. It's not possible to remap root.myorg.com to root with the code you've implemented so far.

Huh, this doesn't sound right to me. I'm pretty sure you can map the principal in a client cert, but you have to have the client cert named client.<db-principal>.crt even though the principal in the cert inside is named something different.

[knz]

oh I had mixed up the filename maybe. I'll check again.

[aaron-crl]

Huh, this doesn't sound right to me. I'm pretty sure you can map the principal in a client cert, but you have to have the client cert named client..crt even though the principal in the cert inside is named something different.

That was the case for the OpenSSL generated certs. Does the crdb client do something different?

[knz]

(I don't see a test for it though.)

[petermattis]

(I don't see a test for it though.)

I verified it works manually just now. Yeah, there isn't a test for it. Let me add one to this PR.

[knz]

ok I checked, it works (with a warning):

kena@kenax ....com/cockroachdb/cockroach % ./cockroach cert create-client foobaz --certs-dir=my-safe-directory --cert-principal-map=foo.bar:node --ca-key=my-safe-directory/ca.key

kena@kenax ....com/cockroachdb/cockroach % cd my-safe-directory
kena@kenax ...ockroach/my-safe-directory % ls
ca.crt             ca.key             client.foobaz.crt  client.foobaz.key  node.crt           node.key
kena@kenax ...ockroach/my-safe-directory % cp client.foobaz.crt client.root.crt
kena@kenax ...ockroach/my-safe-directory % cp client.foobaz.key client.root.key
kena@kenax ...ockroach/my-safe-directory % cd ..

kena@kenax ....com/cockroachdb/cockroach % ./cockroach node ls --certs-dir=my-safe-directory --cert-principal-map=foo.bar:node,foobaz:root
W200429 16:40:02.586752 1 security/certificate_loader.go:327  could not parse certificate for my-safe-directory/client.foobaz.crt: failed to validate certificate 0 in file client.foobaz.crt: client certificate has principals ["root"], expected "foobaz"
  id
------
   1
(1 row)

(I started the server with --cert-principal-map=foo.bar:node,foobaz:root)

I'm a bit confused by the text of the warning (I'd have expected the opposite really, something along the lines of client cert has principal foobaz, expected root) but I'm glad it works.

We'll need to document the file names though. it's a pitfall.

[knz]

Reviewed 1 of 1 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @petermattis)

---

pkg/cli/interactive_tests/test_cert_advisory_validation.tcl, line 82 at r3 (raw file):

start_test "Check that the client commands can use cert principal map."
system "$argv start-single-node --certs-dir=$certs_dir --cert-principal-map=foo.bar:node --advertise-addr=localhost --background >>expect-cmd.log 2>&1"

Another thing: can you make the certs_dir a subdir of logs/
as well as add a -s=path=logs/db to the command line here and other start-single-node

This is because the test hardness only collects artifacts from the logs directory (specifically, that's a mounted volume and the artifact collection happens outside of docker)

[petermattis]

We'll need to document the file names though. it's a pitfall.

Yes, it is a pitfall. Requiring the filenames to have a particular structure seems burdensome in hindsight. I don't think it is adding anything as we load all of the certs anyways a check what the principal inside says.

I've changed the test to create a root cert with the principal root.crdb.io and then map that principal to root.

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @petermattis)

---

pkg/cli/interactive_tests/test_cert_advisory_validation.tcl, line 82 at r3 (raw file):

Previously, knz (kena) wrote…

Another thing: can you make the certs_dir a subdir of logs/
as well as add a -s=path=logs/db to the command line here and other start-single-node

This is because the test hardness only collects artifacts from the logs directory (specifically, that's a mounted volume and the artifact collection happens outside of docker)

Done.

[knz]

Reviewed 1 of 1 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @petermattis)

[petermattis]

TFTR!

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)