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

Certificate authentication with colon-separated SAN URI fails in 22.1.6 #87235

Closed
jonstjohn opened this issue Aug 31, 2022 · 9 comments
Closed
Assignees
Labels
branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. T-server-and-security DB Server & Security

Comments

@jonstjohn
Copy link
Collaborator

jonstjohn commented Aug 31, 2022

Describe the problem

If a client certificate that includes a colon-separated SAN URI is used to authenticate with CRDB in 22.1.6, the following errors occurs:

ERROR: invalid tenant URI SAN mycompany:sv:rootclient:dev:usw1

This issue appeared in 22.1.6 and looks like it is related to #84371 .

To Reproduce

Generate a client certificate with a colon-separated SAN URI.

Setup a simple directory structure to hold keys and configuration materials:

mkdir auth auth/certs auth/openssl auth/safe

Put the cluster's ca.key in auth/safe and ca.crt in auth/certs.

Initialize the openssl index and serial files:

touch auth/openssl/index.txt
echo '01' > auth/openssl/serial.txt

Create a ca configuration file in auth/openssl/ca.cnf:

# OpenSSL CA configuration file
[ ca ]
default_ca = CA_default

[ CA_default ]
default_days = 365
database = auth/openssl/index.txt
serial = auth/openssl/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

Create a certificate configuration file inauth/openssl/sanuri.client.cnf:

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

[ distinguished_name ]
organizationName = Cockroach
commonName = sanuri

[ extensions ]
subjectAltName = DNS:root,URI:mycompany:sv:rootclient:dev:usw1

Next, generate a certificate using openssl and the cluster's ca key:

openssl genrsa -out auth/certs/client.sanuri.key 2048
openssl req -new -config auth/openssl/sanuri.client.cnf -key auth/certs/client.sanuri.key -out auth/openssl/client.sanuri.csr -batch
openssl ca -config auth/openssl/ca.cnf -keyfile auth/safe/ca.key -cert auth/certs/ca.crt -policy signing_policy -extensions signing_client_req -out auth/certs/client.sanuri.crt -outdir auth/certs/ -in auth/openssl/client.sanuri.csr -batch

Inspect the certificate:

openssl x509 -in auth/certs/client.sanuri.crt -text -noout | less

Create the user on the cluster:

CREATE USER sanuri;
GRANT ADMIN to sanuri;

Try to login using the cockroach client (where $CRDB_IP_EXTERNAL is the accessible IP):

cockroach sql --url "postgres://$CRDB_IP_EXTERNAL:26257?sslcert=.%2Fauth%2Fcerts%2Fclient.sanuri.crt&sslkey=.%2Fauth%2Fcerts%2Fclient.sanuri.key&sslmode=verify-full&sslrootcert=.%2Fauth%2Fcerts%2Fca.crt"

Note the error message.

ERROR: invalid tenant URI SAN mycompany:sv:rootclient:dev:usw1

For the login to be successful in 22.1.5 and earlier, a cert-principal-map and/or identity map needs to be configured, but the error is present even without that.

Expected behavior

This should work as it does in 22.1.5 and earlier versions.

Additional data / screenshots

None

Environment:

  • CockroachDB version: 22.1.6
  • Server OS: n/a
  • Client app: cockroach client

Additional context

Unable to upgrade to 22.1.6.

Certificates are generated via a company-wide policy and the format cannot be changed.

Jira issue: CRDB-19225

@jonstjohn jonstjohn added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Aug 31, 2022
@abarganier
Copy link
Contributor

Thanks for the repro steps - I have a fix prepared & should have a PR up shortly. We'll have to backport this into v22.1.7 or do an extraordinary release to get it into v22.1. @jonstjohn let me know if you have a preference here (if they can fallback to v22.1.5 until v22.1.7, that's preferable, but if it's high priority we can expedite).

The v22.1.7 release is scheduled for publish on September 12th currently, but it depends on whether we can get the original PR + backport merged before September 6th (might be cutting it close...).

If we miss it, we can try pushing for an extraordinary release if it's time sensitive, or we'd wait for v22.1.8, which is scheduled for publish on October 3rd.


The fix itself is to simply relax our requirement on the URI SAN scheme/format. If we don't recognize the format, instead of erroring out and exiting, we'll fallback to the legacy behavior.

I did some tests - first, to reproduce:

$ ./cockroach sql --url "postgres://localhost:26257?sslcert=.%2Fauth%2Fcerts%2Fclient.sanuri.crt&sslkey=.%2Fauth%2Fcerts%2Fclient.sanuri.key&sslmode=verify-full&sslrootcert=.%2Fauth%2Fcerts%2Fca.crt"
#
# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.
#
ERROR: invalid tenant URI SAN mycompany:sv:rootclient:dev:usw1
SQLSTATE: 28000
Failed running "sql"

Then, with my proposed changes:

$ ./cockroach sql --url "postgres://localhost:26257?sslcert=.%2Fauth%2Fcerts%2Fclient.sanuri.crt&sslkey=.%2Fauth%2Fcerts%2Fclient.sanuri.key&sslmode=verify-full&sslrootcert=.%2Fauth%2Fcerts%2Fca.crt"
#
# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.
#
# Server version: CockroachDB CCL v22.2.0-alpha.1-256-g9911916662-dirty (x86_64-apple-darwin21.6.0, built 2022/09/01 17:27:18, go1.18.4) (same version as client)
# Cluster ID: 2ab40009-e4cd-4e7d-8588-c87a88d9adcc
#
# Enter \? for a brief introduction.
#
root@localhost:26257/defaultdb>

I'm going to polish this up and get a PR out - I'll do my best to move quickly!

@jonstjohn
Copy link
Collaborator Author

Thanks for the update @abarganier ! Yes, they can use 22.1.5 until the fix comes out. Ideally, we would have the fix in 22.1.7 but if that isn't possible, a fix in 22.1.8 is acceptable as there isn't a pressing need to upgrade at the moment.

@blathers-crl
Copy link

blathers-crl bot commented Sep 5, 2022

Hi @knz, please add branch-* labels to identify which branch(es) this release-blocker affects.

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

@knz knz added release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. and removed GA-blocker labels Sep 5, 2022
@blathers-crl
Copy link

blathers-crl bot commented Sep 5, 2022

Hi @knz, please add branch-* labels to identify which branch(es) this release-blocker affects.

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

@knz knz added branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 labels Sep 5, 2022
@knz
Copy link
Contributor

knz commented Sep 5, 2022

this is a release blocker.

@blathers-crl blathers-crl bot added the T-server-and-security DB Server & Security label Sep 6, 2022
craig bot pushed a commit that referenced this issue Sep 7, 2022
87302: pkg/security: relax requirement to follow CRDB URI SAN cert scheme r=knz a=abarganier

Recently, a customer upgraded to v22.1.6, a recent patch release which
contains the new tenant-scoped client certificates and asssociated
authorization logic updates.

The new authz logic *required* that the SAN URIs included in the client
certificate followed the URI SAN scheme:
`crdb://tenant/<tenant_id>/user/<tenant_username>`

However, for customers that use URI SANs that do not follow this
convention or do not have the flexibility to alter the URI SAN,
this was preventing them from using their existing certificates.
This would generate an error when attempting to connect to a
SQL shell.

One example URI SAN is as follows:
`mycompany:sv:rootclient:dev:usw1`

This is a certificate that worked with the legacy behavior, but
is rejected by the new authz logic.

We should update the authz logic to be less strict about the URI SAN
following our own scheme. If we are unable to parse the URI SAN then
we should fallback to using the globally scoped client certificate
instead, enabling backwards compatibility.

This patch does just that, logging an error in the case where we are
unable to parse the URI SAN and instead falling back to the legacy
behavior, producing a global user scope for the certificate.

Release note: none

Release justification: low risk, necessary fix to enable customers
using custom URI SAN schemes to continue using their existing certificates
on newer CRDB versions.

Addresses #87235

87311: backupccl: parallelize loading of manifests from External Storage r=benbardin a=adityamaru

This change is a targetted change to parallelize the loading
of backup manifests for each incremental layer of a backup.
This method is shared by both restore as well as `SHOW BACKUP`.

Fixes: #87183

Release note: None

Release justification: low risk performance improvement required
for making `SHOW BACKUP` in the presence of many incremental layers
more performant

87316: sctest: Augmented BACKUP/RESTORE tests with table-level restore r=Xiang-Gu a=Xiang-Gu

Commit 1: non-code minor changes (added comments, moved function a little bit)
Commit 2:
   Previously, the Backup test in declarative schema changer backups the
    whole database and restore the whole database with `RESTORE DATABASE`.
    This PR augments the test by adding another flavor to restore:
    `RESTORE TABLE tbl1,...,tblN` where `tblx` are *all* the tables in the
    backup. This will nicely give us coverage for `RESTORE TABLE` when
    a declarative schema changer job is restored.

   Note that ideally we want to randomly restore only a subset of all the
    table. Indeed I tried to implement that but realize it was blocked by
    one limitation in the declarative shcema changer: We don't yet support
    restore schema changer job that skips missing sequences (E.g. if we
    have a table `t` and a sequence `s`, and I want to
    `ALTER TABLE t ADD COLUMN c DEFAULT nextval('s')`, we backup database
    in PostCommit phase. Later when we restore just `t`, the schema changer
    job will run into error `error executing 'missing rewrite for id 109 in
    <column_default_expression:<table_id:108 column_id:2
    embedded_expr:<expr:"nextval(109:::REGCLASS)" uses_sequence_ids:109 >)`)
    This issue is tracked in #87518.

Fixes: #86835

   Release justification: test-only changes
   Release note: None

87446: authors: add faizaanmadhani to authors r=faizaanmadhani a=faizaanmadhani

Release note: None
Release justification: non-production code change

87459: scbuildstmt: fallback if adding a virtual column with NOT NULL r=Xiang-Gu a=Xiang-Gu

We found a regression in the new schema changer for the following stmt: `ALTER TABLE t ADD COLUMN j INT AS (NULL::INT) VIRTUAL NOT NULL;` incorrectly succeeded. This PR made `ADD COLUMN` fall back if the to-be-added column is a virtual column with NOT NULL constraint.

Surprisingly, we actually have logic tests in place for this case but it has incorrect expected output so we also changed the exsiting tests.

Fix: #87457

Release justification: bug fix for GA blocker.

Release note: None

87506: ci: add some extra environment variables for sqllogic corpus nightly r=healthy-pod a=rickystewart

Without these extra environment variables, GitHub posting doesn't work correctly.

Release justification: Non-production code changes
Release note: None

Co-authored-by: Alex Barganier <[email protected]>
Co-authored-by: adityamaru <[email protected]>
Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: Faizaan Madhani <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
@abarganier
Copy link
Contributor

abarganier commented Sep 8, 2022

@jonstjohn this has been resolved for both 22.2, and the next 22.1 release (22.1.8 22.1.7).

We now look through all the URI SANs included in the certificate for one with the expected crdb:// prefix. If we find a prefix match, we attempt to parse the URI SAN as before, in which case we take a strict approach where we throw an error if the format isn't the expected crdb://tenant/<tenant_id>/user/<tenant_username>.

In the case where none of the provided URI SANs have the expected crdb:// prefix, such as in your example, only then do we fallback to the global cert.

Thanks for reporting this issue. Marking as closed.

@jonstjohn
Copy link
Collaborator Author

Thanks @abarganier and others! Really appreciate the turn-around!

@rafiss
Copy link
Collaborator

rafiss commented Sep 9, 2022

update: v22.1.7 will include this change, since the release schedule shifted slightly which allowed this fix to make it in. it's currently targeted to go out next week.

@abarganier
Copy link
Contributor

Great news @rafiss! Thanks for clarifying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. T-server-and-security DB Server & Security
Projects
None yet
Development

No branches or pull requests

5 participants