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, authz checks #84371

Conversation

abarganier
Copy link
Contributor

Backport:

Please see individual PRs for details.

/cc @cockroachdb/release

--
This PR backports the client cert generation command,
as well as the associated authz logic to use said client
cert, for tenant scoped certificates.

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.

Informs #77958

The backport also pulls in multiple PRs that enable roachtest to
identify if tenant scoped certificates are in play, and makes use of them
if so. Now, the test logic searches the help on the command to see if it
supports the tenant-scoped certs flag. If it does, we assume we will
need them.

This is all being backported into v22.1 so that we can make use of the
tenant scoped client certificates in upcoming serverless deployments.
The availability of these new certificates will enable better o11y into
tenant clusters by enabling secure debug.zip generation against
tenants.

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:

  1. 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.
  2. 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.
  3. roachtest: run workload from the tenant node #83707 (included in the backport) made alterations to the smoketest/secure/multitenant roachtest. However, smoketest/secure/multitenant as of today does not yet exist on the v22.1 release branch. Since it was minimal effort to import this test, and 22.1 is likely going to be the next deployed serverless version, it doesn't hurt to have another multi-tenant test on this release branch. Therefore, we pulled this test into the backport.
  4. There are various diffs made to test specs within pkg/cmd/roachtest/tests/multitenant_fairness.go (e.g. in roachtest,roachprod: detect tenant-scope certs via help, unskip tests #83703). However, similar to the above mentioned smoketest for secure mt clusters, this entire file does not yet exist on the v22.1 release branch. For now, I have decided not to pull this file in, but if anyone feels we should include it, I can do so.
  5. In the original PRs, pkg/security contained subpackages such as pkg/security/username. These subpackages however do not yet exist on the v22.1 branch. However, the contents of these new subpackages are simply lifed from the original pkg/security, so instead of opting to port over all the subpackages, we instead use the analogous constants/types from the old pkg/security.

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.

@abarganier abarganier requested review from a team as code owners July 13, 2022 17:58
@abarganier abarganier requested a review from a team July 13, 2022 17:58
@abarganier abarganier requested a review from a team as a code owner July 13, 2022 17:58
@blathers-crl
Copy link

blathers-crl bot commented Jul 13, 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

Note: this is a replacement for the now closed #84313

@abarganier abarganier force-pushed the backport22.1-79064-79065-82927-83703-83707 branch from 834f87e to f644bfd Compare July 14, 2022 16:30
@abarganier
Copy link
Contributor Author

See cockroachdb/cockroach-go#142 for relevant cockroach-go changes. This allows the test server to dynamically check if tenant scoped client certificates are available in the same way that #83703 does, via looking for --tenant-scope in the output of cockroach cert create-client --help.

@abarganier abarganier force-pushed the backport22.1-79064-79065-82927-83703-83707 branch 3 times, most recently from e17a37f to cdca332 Compare July 19, 2022 18:38
rimadeodhar and others added 3 commits July 20, 2022 10:46
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>
This PR extends the TLS code to use a tenant scoped
client cert to authenticate a client for specific tenant.

Release note (security update): We introduce a new
tenant scoped client certificate to authenticate a client
on a specific tenant. A tenant scoped client certificate
contains the client name within the CN and the tenant ID,
to which the certificate is being scoped to, as the SAN.
The tenant ID is embedded within the URI section with the
format "crdb://tenant/<tenant_id>/user/<username>".
For example, a root client certificate scoped to a tenant
with ID 123 will contain "root" in the CN field and the URI
"crdb://tenant/123/user/root" in the URI section of the
certificate. This certificate will authorize the root
client on the tenant with the ID 123. It will result
in an authorization error if used to authenticate the
root client on any other tenant.
In semver, v22.2.0 is greater than v22.2.0-alpha. Until the 22.2.0
release is cut, all nightly builds will have pre-release versions that
would fail the previous condition.

Here, I've used an alpha version that I believe is the first alpha
version that required tenant-scoped certs.

Note that we can only do this because we happened to already ship the
version that requires tenant-scoped certs.  Otherwise, we would need a
less exact comparison that would be wrong for some 22.2.0 alpha
versions. An alternative to this type of version comparison might be
if the cockroach binary could report its capabilities via some
standard interface.

Release note: None

Release note: None
This is a hacky fix for build failures related to the new
tenant-scoped certificates. We cache a single copy of the certs to
create connections locally, so each time we regenerate them, we also
need to refetch them. Further, since we want to connect to multiple
tenants, when we regenerate the certs we need to know about all
tenants we might connect to.

Release note: None
The secure URL refers to paths on disk on the clusters in the
node. Since we only create the tenant-scoped certs on the tenant node,
we need to run workload from that node.

BACKPORT NOTES: Also adds the `multi-tenant` owner to the roachtest
registry and TEAMS.yml, which were not yet added to the release branch.

Fixes cockroachdb#82266
Depends on cockroachdb#83703

Release note: None
@abarganier abarganier force-pushed the backport22.1-79064-79065-82927-83703-83707 branch from cdca332 to 0e6e974 Compare July 20, 2022 14:55
@abarganier
Copy link
Contributor Author

After some necessary changes to cockroachdb/cockroach-go#142 and cockroachdb/examples-orms#149, roachtests are now properly using tenant-scoped client certificates in tests when appropriate, and CI is now passing 🎉 .

PTAL - if possible, we'd like to get this backport merged in time for the v22.1.5 patch release.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 40 of 45 files at r1, 16 of 16 files at r6, 8 of 8 files at r7, 1 of 1 files at r8, 5 of 5 files at r9, 5 of 5 files at r10, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @catj-cockroach, @jeffswenson, @rimadeodhar, and @stevendanna)

Copy link
Collaborator

@rimadeodhar rimadeodhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 45 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @catj-cockroach, @jeffswenson, and @stevendanna)

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.

5 participants