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

release-22.1: cert: Add CLI to create tenant scope client cert #84313

Closed

Conversation

abarganier
Copy link
Contributor

Backport 1/1 commits from #79064.

/cc @cockroachdb/release


This PR extends the client cert generation command
to generate add tenant scoping for the certificate.
Tenant scoping a client certificate is used to authenticate
a client a specific set of tenant IDs as indicated in the
URI SAN section of the certificate. A single client could
be authorized for multiple tenants. A client allowed to authorize
on all tenants (global client certificate) is indicated by scoping
a certificate to the system tenant.
Subsequent PRs stacked on top of this one will implement the
authentication component for this certificate. The first use
case for such tenant scoped certificates will be the debug
zip command.

Informs #77958

Release note (cli change): The client cert generation command is
being extended to create a tenant scoped client certificates.
Tenant scoped certs will authenticate a client for a specific tenant.

Release note (security update): This PR introduces tenant scoping
for a client certificate. This allows for client certificates to
be used to authenticate a client on a specific tenant or set of tenants. It
contains the username within the CN of the certificate. The tenant
IDs are embedded in the URIs section of Subject Alternate Name (SAN)
values. The format of the URI SAN is
crdb://tenant/<tenant_id>.

--

Backport notes:

  • Moved certain roachtest multitenant helper functions into pkg/cmd/roachtest/tests/multitenant_utils.go from pkg/cmd/roachtest/tests/multitenant_upgrade.go and updated tests accordingly. This is the current state on master.
  • Functions such as username.RootUserName() that exist on master today, are still within the security package on v22.1. The analogous functions in the security package are effectively equivalent, though.
  • Changes in pkg/cli/flags.go make use of package-local implementations of functions such as StringFlag. BoolFlag, etc. In the original PR, these functions were moved to the cliflagcfg package, but their functionality is effectively equivalent.

Release justification: Provides necessary tenant-scoped TLS certificate updates, which will be required when upgrading serverless clusters to v22.1 to enable secure access to tenant-scoped debug.zip bundles. This is necessary to improve tenant-level observability for SREs, TSEs, etc.

This PR extends the client cert generation command
to generate add tenant scoping for the certificate.
Tenant scoping a client certificate is used to authenticate
a client to a specific set of tenants as indicated in the
URI SAN section of the certificate.
Subsequent PRs stacked on top of this one will implement the
authentication component for this certificate. The first use
case for such tenant scoped certificates will be the debug
zip command.

Informs cockroachdb#77958

Release note (cli change): The client cert generation command is
being extended to create a tenant scoped client certificates.
Tenant scoped certs will authenticate a client for a specific tenant.

Release note (security update): This PR introduces tenant scoping
for a client certificate. This allows for client certificates to
be used to authenticate a client on a specific tenant only. It
contains the username within the CN of the certificate. The tenant
ID and username is embedded in the URIs section of Subject Alternate
Name (SAN) values. The format of the URI SAN is
crdb://tenant/<tenant_id>/user/<username>
@abarganier abarganier requested review from a team as code owners July 12, 2022 20:14
@abarganier abarganier requested a review from a team July 12, 2022 20:14
@abarganier abarganier requested a review from a team as a code owner July 12, 2022 20:14
@blathers-crl
Copy link

blathers-crl bot commented Jul 12, 2022

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • 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

@abarganier
Copy link
Contributor Author

cc @cucaroach & @stevendanna. We will be backporting the tenant-scoped client certificates & associated authz logic into CRDB v22.1. This will require updates to the version gates used in Example-ORM and cockroach-go to ensure that the proper certificates are used for v22.1. Are you able to assist us in updating these version gates appropriately? We'll be sure to avoid merging any backports until we have this squared away.

@abarganier abarganier removed request for a team July 12, 2022 20:18
@stevendanna
Copy link
Collaborator

@abarganier On master, I've now removed all the version-based gates in the tests and in roachprod in favor of a small hack that checks the --help output. I think our best bet is to pull those commits (along with the ones that fixed up the tests) into this backport.

I think the PRs you'll want to include are:

You'll likely hit some conflicts in roachprod when you pull those in. I think we can just drop any of the roachprod changes since the relevant feature (secure start-tenant) isn't likely needed on the 22.1 branch (that feature isn't currently used in roachtests and those of us who are using it can build roachprod from master)

@abarganier
Copy link
Contributor Author

abarganier commented Jul 13, 2022

@stevendanna perfect, thanks for the guidance. I'll begin revising the backport (I'll likely open a new one) with these additional PRs. I also need to pull in an additional PR that actually updates the authz logic. I'll link the replacement backport here once it's up.

@abarganier
Copy link
Contributor Author

Closing in favor of #84371

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants