Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cockroach sql CLI does not correctly support mapped principle certificate usage. #47300

Closed
aaron-crl opened this issue Apr 9, 2020 · 17 comments · Fixed by #47449
Closed

cockroach sql CLI does not correctly support mapped principle certificate usage. #47300

aaron-crl opened this issue Apr 9, 2020 · 17 comments · Fixed by #47449
Labels
C-investigation Further steps needed to qualify. C-label will change.

Comments

@aaron-crl
Copy link

Describe the problem

Attempting to log into a single node cluster with a certificate for notroot and --cert-principle-map=notroot:root doesn't seem to work. But issuing a similar certificate with the name root but leaving the map in place does allow login.

To Reproduce

OpenSSL Configs:

ca.cnf

# OpenSSL CA configuration file
[ ca ]
default_ca = CA_default

[ CA_default ]
default_days = 365
database = index.txt
serial = serial.txt
default_md = sha256
copy_extensions = copy
unique_subject = no

# Used to create the CA certificate.
[ req ]
prompt=no
distinguished_name = distinguished_name
x509_extensions = extensions

[ distinguished_name ]
organizationName = Cockroach
commonName = Cockroach CA

[ extensions ]
keyUsage = critical,digitalSignature,nonRepudiation,keyEncipherment,keyCertSign
basicConstraints = critical,CA:true,pathlen:1

# Common policy for nodes and users.
[ signing_policy ]
organizationName = supplied
commonName = optional

# Used to sign node certificates.
[ signing_node_req ]
keyUsage = critical,digitalSignature,keyEncipherment
extendedKeyUsage = serverAuth,clientAuth

# Used to sign client certificates.
[ signing_client_req ]
keyUsage = critical,digitalSignature,keyEncipherment
extendedKeyUsage = clientAuth

node.cnf

# OpenSSL node configuration file
[ req ]
prompt=no
distinguished_name = distinguished_name
req_extensions = extensions

[ distinguished_name ]
organizationName = Cockroach

[ extensions ]
subjectAltName = critical,DNS:localhost,DNS:node,IP:0.0.0.0

client.cnf

[ req ]
prompt=no
distinguished_name = distinguished_name

[ distinguished_name ]
organizationName = Cockroach
commonName = notroot

For the notroot denied login case:

USERNAME=notroot

# create CA
mkdir certs
mkdir 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

# create certs for nodes
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

# create client config
openssl genrsa -out certs/client.$USERNAME.key 2048
chmod 400 certs/client.$USERNAME.key
openssl req -new -config client.cnf -key certs/client.$USERNAME.key -out client.$USERNAME.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.$USERNAME.crt -outdir certs/ -in client.$USERNAME.csr -batch

# start database in background mode with principle map
cockroach start-single-node --certs-dir=certs --cert-principal-map=notroot:root --background
$ cockroach sql --certs-dir=certs/ --user=notroot
#
# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.
#
ERROR: password authentication failed for user notroot
Failed running "sql"

For the root case with the flag still present:

Set USERNAME to root in the above script and change the commonName in the client.cnf file to root

client.cnf

[ req ]
prompt=no
distinguished_name = distinguished_name

[ distinguished_name ]
organizationName = Cockroach
commonName = root
$ cockroach sql --certs-dir=certs/ --user=root
#
# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.
#
# 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: c6f32f12-b85f-4162-9ada-5f283544681c
#
# Enter \? for a brief introduction.
#
root@:26257/defaultdb> 

Expected behavior

  1. Principle map entry for root user should allow remapping of root user.
  2. Login should fail if principle mapping is supplied but root is presented.

Environment:

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

Many thanks to @Amruta-Ranade for surfacing this issue.

cc @knz, @petermattis

@knz
Copy link
Contributor

knz commented Apr 10, 2020

Friends have you been careful to distinguish the behavior of the client from that of the server ?

What happens if you take your notroot client cert and populate it in, say, a UI tool like dBeaver?

(I suspect a bug in our client-side CLI code, but then the CLI was largely out of scope for the feature at hand.)

@petermattis
Copy link
Collaborator

Login should fail if principle mapping is supplied but root is presented.

This isn't the way the --cert-principal-map flag works today. There is an implicit rule to map <db-principal>:<db-principal> regardless of the other mappings. If you want to prohibit a cert containing "root" from working, you need to explicitly map into to something invalid. For example, --cert-principal-map=root:-,notroot:root. I haven't actually tried this, but it should work.

Do we want to change the behavior of --cert-principal-map? What semantics are you expecting? Perhaps every <db-principal> specified in the map should have an implicit <db-principal>:- mapping added.

Principle map entry for root user should allow remapping of root user.

I still need to walk through this example to see what is going on (thanks for the detailed reproduction steps).

@knz knz added the C-investigation Further steps needed to qualify. C-label will change. label Apr 10, 2020
@aaron-crl
Copy link
Author

Do we want to change the behavior of --cert-principal-map? What semantics are you expecting? Perhaps every specified in the map should have an implicit :- mapping added.

My thinking was that this would be a step towards removing magic principles - so I mentally jumped from aliasing (which is what we have) to reassigning the principles completely. I think there's room for discussion about doing this as we review principle usage throughout the app but I retract (2) here, caveated that we highlight in docs that this feature only adds aliases.

@knz
Copy link
Contributor

knz commented Apr 10, 2020

If we move towards looking at what pg does (#47196 ) we may want to look at what pg does in this case.

@petermattis
Copy link
Collaborator

If we move towards looking at what pg does (#47196 ) we may want to look at what pg does in this case.

Agreed.

@aaron-crl
Copy link
Author

Thank you @knz !

I was able to connect with psql by specifying the username of root but providing the certificates for notroot - so definitely a client thing.

psql "host=$(hostname) user=root sslcert=./certs/client.notroot.crt sslkey=./certs/client.notroot.key sslrootcert=./certs/ca.crt sslmode=verify-ca port=26257"
psql (12.2, server 9.5.0)
SSL connection (protocol: TLSv1.3, cipher: TLS_AES_128_GCM_SHA256, bits: 128, compression: off)
Type "help" for help.

root=# 

It might be worth updating the docs to clarify that it still expects the username to be the original principle?

I changed the names of the certs in the ./certs dir to match root but still have the notroot SAN and our CLI tool balked at providing a different principle for username and SAN. Do we have any other way to override this for our CLI?

cockroach sql --certs-dir=certs/ --user=root
#
# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.
#
W200410 14:08:34.937630 1 security/certificate_loader.go:327  could not parse certificate for certs/client.root.crt: failed to validate certificate 0 in file client.root.crt: client certificate has principals ["notroot"], expected "root"
Enter password: 

@dbist
Copy link
Contributor

dbist commented Apr 13, 2020

I have a customer requirement to include FQDN as part of CN in client.cnf file.

[ req ]
prompt=no
distinguished_name = distinguished_name

[ distinguished_name ]
organizationName = Cockroach
commonName = node.example.com

unless I create root user first and then follow the same steps to create a user node.example.com, I cannot login to the cluster with the scenario above.

cockroach start-single-node --certs-dir=node-certs --cert-principal-map=node.example.com:node --background
cockroach sql --certs-dir=client-certs
> create user 'node.example.com';
 vi client.cnf # changing CN=root to CN=node.example.com
 openssl genrsa -out client-certs/client.node.example.com.key 2048
 chmod 400 client-certs/client.node.example.com.key
 openssl req -new -config client.cnf -key client-certs/client.node.example.com.key -out client.node.example.com.csr -batch
 openssl ca -config ca.cnf -keyfile my-safe-directory/ca.key -cert client-certs/ca.crt -policy signing_policy -extensions signing_client_req -out client-certs/client.node.example.com.crt -outdir client-certs/ -in client.node.example.com.csr -days 1830 -batch
 rm -rf client-certs/*.pem
 cockroach sql --certs-dir=client-certs --user=node.example.com

CC: @Amruta-Ranade @aaron-crl @knz @petermattis

@dbist
Copy link
Contributor

dbist commented Apr 13, 2020

wondering if this can help here, #43847

@aaron-crl
Copy link
Author

@dbist I've been digging into this one and it looks like a client CLI bug. Specifically:
CRDB CLI expects the --user to be found in the certificate principles (so it does not respect that a mapping might have occurred).

Postgres client doesn't care if you provide it with a mismatched principle and certificate pair as it passes this to the server for validation.

This can be made to work for existing clients by adding root to the SAN and providing root as the username.

I still name my client cert client.root.crt but my client.cnf for this case looks like:

[ req ]
prompt=no
distinguished_name = distinguished_name
req_extensions = extensions

[ distinguished_name ]
organizationName = Cockroach
commonName = notroot

[ extensions ]
subjectAltName = DNS:root

With my command to connect being:
$ cockroach sql --certs-dir=certs/ --user=root

Alternatively, the postgres invocation above will also work if you simply need to test.

@aaron-crl
Copy link
Author

wondering if this can help here, #43847

No, this is unrelated to this issue.

@aaron-crl aaron-crl changed the title start-single-node --cert-principal-map doesn't seem to work for root principle cockroach sql CLI does not correctly support mapped principle certificate usage. Apr 13, 2020
@dbist
Copy link
Contributor

dbist commented Apr 13, 2020

I see two options.

  1. create user root cert, add a non-root user as described in my repro steps above.
  2. leverage subjectAltName = DNS:root.

I linked the issue above as it explains the reasoning for having a root user as the first user in the db. I will test your approach as I think it avoids the CN=root directly. Out of curiosity, which of the two approaches do you feel are more widely accepted? Do you see enterprises have access to a single root user that's stored somewhere for safe keeping while all end-users, with and without admin rights created as children of the root. Or, do you see the 2nd approach as more widely used?

@aaron-crl
Copy link
Author

Neither. I would expect the mapping of an arbitrary certificate principle to work. For example:

CN=john.smith@example.com
SAN=email:john.smith@example.com,DNS:dbadmin.user.example.com

Where my mapping would be dbadmin.user.example.com:root

If I had to pick, then 2 is the lesser of the two evils. I would never want a single root key to everything in the enterprise and I haven't seen a security conscious enterprise intentionally deploy such a pattern.

CRDB server already supports this correctly (as verified with postgres above). This is a client CLI bug.

@petermattis
Copy link
Collaborator

CRDB CLI expects the --user to be found in the certificate principles (so it does not respect that a mapping might have occurred).

This is only true when using sql --certs-dir=<dir>. If you specify a sql --url postgres://..., you can have the user name and the filenames be anything you like.

~ ./cockroach cert create-ca --certs-dir=certs --ca-key=certs/ca.key
~ ./cockroach cert create-client foo.crdb.io --certs-dir=certs --ca-key=certs/ca.key
~ COCKROACH_CERT_NODE_USER=node.crdb.io ./cockroach cert --certs-dir=certs/ --ca-key=certs/ca.key create-node localhost 127.0.0.1
~ ./cockroach start-single-node --http-port 26258 --certs-dir=certs --cert-principal-map=node.crdb.io:node,foo.crdb.io:root
~ ./cockroach sql --url "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"
#
# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.
#
# Server version: CockroachDB CCL v20.1.0-beta.4-675-g52a6bee613-dirty (x86_64-apple-darwin19.3.0, built 2020/04/13 20:17:29, go1.13.9) (same version as client)
# Cluster ID: 4cb907e2-40e2-4e9e-843a-beb13fa60a9c
#
# Enter \? for a brief introduction.
#
root@localhost:26257/defaultdb>

Note how the use of --url also avoids any problems with the node cert.

#47449 adds --cert-principal-map support to the certs and client commands, but sometimes it might be preferable to be explicit.

@dbist
Copy link
Contributor

dbist commented Apr 13, 2020

your approach with the following client.cnf works

[ req ]
prompt=no
distinguished_name = distinguished_name
req_extensions = extensions

[ distinguished_name ]
organizationName = Example
commonName = john.smith@example.com

[ extensions ]
subjectAltName = email:john.smith@example.com,DNS:user.example.com,DNS:root

cockroach start-single-node --certs-dir=node-certs --cert-principal-map=node.example.com:node,user.example.com:root --background

removing DNS:root from the client.cnf file results in CLI prompting for password but I think my customer is unblocked as CN= no longer shows root.

        Issuer: O=Example, CN=Example CA
        Subject: O=Example, CN=john.smith@example.com

@knz
Copy link
Contributor

knz commented Apr 14, 2020

I would recommend completely avoiding the --certs-dir / --certs-principal-map for clients and instead use --url in every case.

@dbist
Copy link
Contributor

dbist commented Apr 14, 2020

+1

@dbist
Copy link
Contributor

dbist commented Apr 14, 2020

Including the whole example for posterity

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'
# OpenSSL CA configuration file
[ ca ]
default_ca = CA_default

[ CA_default ]
default_days = 365
database = index.txt
serial = serial.txt
default_md = sha256
copy_extensions = copy
unique_subject = no

# Used to create the CA certificate.
[ req ]
prompt=no
distinguished_name = distinguished_name
x509_extensions = extensions

[ distinguished_name ]
organizationName = ExampleCorp
commonName = ExampleCorp CA

[ extensions ]
keyUsage = critical,digitalSignature,nonRepudiation,keyEncipherment,keyCertSign
basicConstraints = critical,CA:true,pathlen:1

# Common policy for nodes and users.
[ signing_policy ]
organizationName = supplied
commonName = optional

# Used to sign node certificates.
[ signing_node_req ]
keyUsage = critical,digitalSignature,keyEncipherment
extendedKeyUsage = serverAuth,clientAuth

# Used to sign client certificates.
[ signing_client_req ]
keyUsage = critical,digitalSignature,keyEncipherment
extendedKeyUsage = clientAuth
# OpenSSL node configuration file
[ req ]
prompt=no
distinguished_name = distinguished_name
req_extensions = extensions

[ distinguished_name ]
organizationName = ExampleCorp

[ extensions ]
subjectAltName = critical,DNS:localhost,DNS:node.example.com,IP:0.0.0.0
[ req ]
prompt=no
distinguished_name = distinguished_name
req_extensions = extensions

[ distinguished_name ]
organizationName = ExampleCorp
commonName = john.smith@example.com

[ extensions ]
subjectAltName = email:john.smith@example.com,DNS:user.example.com,DNS:root

petermattis added a commit to petermattis/cockroach that referenced this issue Apr 29, 2020
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 cockroachdb#47300
Fixes cockroachdb#48116

Release note (cli change): Support the `--cert-principal-map` flag in
the `cert *` and "client" commands such as `sql`, `init`, and `quit`.
petermattis added a commit to petermattis/cockroach that referenced this issue Apr 29, 2020
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 cockroachdb#47300
Fixes cockroachdb#48116

Release note (cli change): Support the `--cert-principal-map` flag in
the `cert *` and "client" commands such as `sql`, `init`, and `quit`.
petermattis added a commit to petermattis/cockroach that referenced this issue Apr 29, 2020
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 cockroachdb#47300
Fixes cockroachdb#47754
Fixes cockroachdb#48116

Release note (cli change): Support the `--cert-principal-map` flag in
the `cert *` and "client" commands such as `sql`, `init`, and `quit`.
petermattis added a commit to petermattis/cockroach that referenced this issue Apr 29, 2020
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 cockroachdb#47300
Fixes cockroachdb#47754
Fixes cockroachdb#48116

Release note (cli change): Support the `--cert-principal-map` flag in
the `cert *` and "client" commands such as `sql`, `init`, and `quit`.
craig bot pushed a commit that referenced this issue Apr 29, 2020
46992: sql: Add Logical Column ID field to ColumnDescriptor r=rohany a=RichardJCai

The LogicalColumnID field mimics the ColumnID field however LogicalColumnID may be swapped
between two columns whereas ColumnID cannot. LogicalColumnID is referenced for virtual tables
(pg_catalog, information_schema) and most notably affects column ordering for SHOW COLUMNS.

This LogicalColumnID field support swapping the order of two columns - currently only used for
ALTER COLUMN TYPE when a shadow column is created and swapped with it's original column.

Does not affect existing behaviour.

Release note: None

47449: cli: add --cert-principal-map to client commands r=petermattis a=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`.

48138: keys: support splitting Ranges on tenant-id prefixed keys r=nvanbenschoten a=nvanbenschoten

Fixes #48122.
Relates to #47903.
Relates to #48123.

This PR contains a series of small commits that work towards the introduction of tenant-id prefixed keyspaces and begin the removal of some `keys.TODOSQLCodec` instances. This should be the only time we need to touch C++ throughout this work.

48160: storage,libroach: Check for MaxKeys when reading from intent history r=itsbilal a=itsbilal

We weren't checking for MaxKeys (or TargetBytes) being reached
in the case where we read from intent history in the MVCC scanner.
All other cases go through addAndAdvance(), which had these checks.

Almost certainly fixes #46652. Would be very surprised if it was
something else.

Release note (bug fix): Fixes a bug where a read operation in a transaction
would error out for exceeding the maximum count of results returned.

48162: opt: add rule to eliminate Exists when input has zero rows r=rytaft a=rytaft

This commit adds a new rule, `EliminateExistsZeroRows`, which
converts an `Exists` subquery to False when it's known
that the input produces zero rows.

Informs #47058

Release note (performance improvement): The optimizer can now
detect when an Exists subquery can be eliminated because the input
has zero rows. This leads to better plans in some cases.

Co-authored-by: richardjcai <caioftherichard@gmail.com>
Co-authored-by: Peter Mattis <petermattis@gmail.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Bilal Akhtar <bilal@cockroachlabs.com>
Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
@craig craig bot closed this as completed in c360bd4 Apr 29, 2020
petermattis added a commit to petermattis/cockroach that referenced this issue May 2, 2020
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 cockroachdb#47300
Fixes cockroachdb#47754
Fixes cockroachdb#48116

Release note (cli change): Support the `--cert-principal-map` flag in
the `cert *` and "client" commands such as `sql`, `init`, and `quit`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-investigation Further steps needed to qualify. C-label will change.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants