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.

Sign up for GitHub

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

Jump to bottom

release-21.2: rpc,security: use the tenant client cert for pod-pod communication #71402

Merged
merged 1 commit into from
Oct 11, 2021
Merged

release-21.2: rpc,security: use the tenant client cert for pod-pod communication #71402

merged 1 commit into from
Oct 11, 2021

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Oct 11, 2021 •
edited by knz
Loading

Backport 1/1 commits from #71248 on behalf of @knz.

/cc @cockroachdb/release

Release note (security update): Multitenant SQL servers now reuse
the tenant client certificate (client-tenant.NN.crt) for SQL-to-SQL
communication. Existing deployments must regenerate the certificates
with dual purpose (client and server authentication).

Release justification: prevents a significant security risk in CC serverless

@knz
rpc,security: use the tenant client cert for pod-pod communication
82b8465 
As of this patch, we have the following file usage:

- KV nodes on host cluster:
  - ui.crt (optional):
    - used as server cert for HTTP
  - ui-ca.crt (optional):
    - used in unit tests to verify the server's identity for HTTP conns
  - node.crt:
    - used as client cert for node-to-node comms
    - used as server cert for node-to-node comms
    - used as server cert for SQL clients
    - used as server cert for incoming conns from SQL tenant servers
    - used as server cert for HTTP, if ui.crt doesn't exist
  - tenant-client-ca.crt (optional):
    - used to verify certificates from SQL tenant servers connecting as clients
  - client-ca.crt (optional);
    - used to verify client certs for SQL clients
    - used to verify client certs for SQL tenant servers, if tenant-client-ca.crt doesn't exist
  - ca.crt:
    - used to verify other node client certs for node-to-node comms
    - used in unit tests to verify the server's identity for SQL and RPC conns
    - used to verify client certs for SQL clients, if client-ca.crt doesn't exist
    - used to verify client certs for SQL tenant servers, if neither tenant-client.ca.crt nor client-ca.crt exist

- SQL servers:
  - ui.crt (optional):
    - used as server cert for HTTP
  - ui-ca.crt (optional):
    - used in unit tests to verify the server's identity for HTTP conns
  - client-tenant.NN.crt:
    - used as client cert for node-to-node comms (SQL server to SQL server)
    - used as server cert for node-to-node comms (SQL server to SQL server)
    - used as client cert for conns to KV nodes
    - used as server cert for SQL clients
    - used as server cert for HTTP, if ui.crt doesn't exist
  - tenant-client-ca.crt (optional):
    - used to verify certs from other SQL tenant servers
  - client-ca.crt (optional);
    - used to verify client certs for SQL clients
    - used to verify client certs for SQL tenant servers, if tenant-client-ca.crt doesn't exist
  - ca.crt:
    - used to verify other SQL server certs for node-to-node comms, if tenant-client-ca.crt doens't exist
    - used to verify client certs for SQL clients, if client-ca.crt doesn't exist
    - used to verify client certs for SQL tenant servers, if neither tenant-client.ca.crt nor client-ca.crt exist
    - used in unit tests to verify the server's identity for SQL and  RPC conns

Release note (security update): Multitenant SQL servers now reuse
the tenant client certificate (`client-tenant.NN.crt`) for SQL-to-SQL
communication. Existing deployments must regenerate the certificates
with dual purpose (client and server authentication).
@blathers-crl blathers-crl bot requested review from a team as code owners October 11, 2021 15:26
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-21.2-71248 branch from 4dbfb46 to 82b8465 Compare October 11, 2021 15:26
@blathers-crl blathers-crl bot requested review from catj-cockroach and knz October 11, 2021 15:26
@blathers-crl
Copy link
Author

blathers-crl bot commented Oct 11, 2021 •
edited by knz
Loading

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz merged commit fad7454 into release-21.2 Oct 11, 2021
@knz knz deleted the blathers/backport-release-21.2-71248 branch October 11, 2021 17:26
@cockroach-teamcity cockroach-teamcity mentioned this pull request Oct 11, 2021
Open
@catj-cockroach catj-cockroach mentioned this pull request Oct 14, 2021
Merged
@blathers-crl blathers-crl bot mentioned this pull request Oct 14, 2021
Merged
craig bot pushed a commit that referenced this pull request Oct 15, 2021
@catj-cockroach
Merge #71548
bbffd4c 
71548: security: prevent PEM decoding errors by careful newline insertation r=catj-cockroach a=catj-cockroach

Fixes #71588

> Previously, we assumed that the Tenant CA certificate and the CA
> certificate would not be the same, and that they would be saved as
> files with an end of file newline character.
> 
> This change adds support for not loading the Tenant CA Certificate
> if it will just be a second copy of the CA certificate.
> 
> This change also adds a helper function to concatenate certificates
> with a newline separating each, to ensure that the easiest way to
> make a certificate bundle is the correct way.
> 
> Release note: bugfix for certificate loading logic

The bug in question was introduced in #71248 (and back ported in #71402).

Co-authored-by: Cat J <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@catj-cockroach catj-cockroach catj-cockroach approved these changes

@knz knz Awaiting requested review from knz

Assignees

@knz knz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cockroach-teamcity @catj-cockroach @knz