Skip to content

rpc: *almost* use tenant client certs#51498

Closed
tbg wants to merge 5 commits intocockroachdb:masterfrom
tbg:tenant-rpc-ctx
Closed

rpc: *almost* use tenant client certs#51498
tbg wants to merge 5 commits intocockroachdb:masterfrom
tbg:tenant-rpc-ctx

Conversation

@tbg
Copy link
Copy Markdown
Member

@tbg tbg commented Jul 16, 2020

  • rpc: add TenantID to rpc.ContextOptions
  • security: slight test improvement
  • rpc: pass TenantID to SecurityContext to CertManager
  • security: use a single test_certs di
  • rpc: almost use tenant client certs (on tenants)

As of the last commit, tenants would use their proper tenant client certs if it
weren't for a manual override that was added.

This override exists because the KV layer can not yet authenticate
tenant client certs (this will change soon, in a follow-up to #50503).

However, uncommenting both the override and the hack in
pkg/security/securitytest/test_certs/regenerate.sh to make the tenant
client certs match those used by the KV nodes gives early validation
that this "will work" once the KV side plays ball.

Touches #47898.

tbg added 5 commits July 16, 2020 10:17
This is currently unused, but already initialized correctly: it's always
SystemTenantID except on SQL tenant servers, where it's the tenant's ID.

Release note: None
I worried for a short while that this would always return nil, but it
does not.

Release note: None
This makes sure that a certificate manager for an `rpc.Context` for a
given tenant is aware of the tenant ID.

This is not used yet, but a new TODO (to be addressed shortly) hints
at where this will be used during dialing: when we're a tenant, use
the tenant client certs instead of the client certs.

Release note: None
I had previously made sure that the multi-tenancy certs were in a
different subdirectory, but in hindsight this is not worth the
hassle.

Release note: None
As of this commit, tenants would use their proper tenant client certs if
it weren't for a manual override that was added.

This override exists because the KV layer can not yet authenticate
tenant client certs (this will change soon, in a follow-up to cockroachdb#50503).

However, uncommenting both the override and the hack in
`pkg/security/securitytest/test_certs/regenerate.sh` to make the tenant
client certs match those used by the KV nodes gives early validation
that this "will work" once the KV side plays ball.

Touches cockroachdb#47898.

Release note: None
@tbg tbg requested a review from a team as a code owner July 16, 2020 08:18
@tbg tbg added the A-multitenancy Related to multi-tenancy label Jul 16, 2020
@tbg tbg requested a review from nvb July 16, 2020 08:18
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@tbg
Copy link
Copy Markdown
Member Author

tbg commented Jul 16, 2020

Subsumed into #51503

@tbg tbg closed this Jul 16, 2020
@tbg tbg deleted the tenant-rpc-ctx branch July 16, 2020 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-multitenancy Related to multi-tenancy

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants