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

Delete user k8s, etc. certificates on re-issue #6492

Merged
merged 5 commits into from
May 6, 2021
Merged

Conversation

tcsc
Copy link
Contributor

@tcsc tcsc commented Apr 19, 2021

Prior to this change, user certificates for services like kubernetes were
preserved across a certificate re-issue. This led to issues where elevated
privileges granted by an access request were not applied to the service
certificates as they were not updated during the reissue process.

This patch changes the certificate re-issue process such that:

  • certificates for services (like Kuberenetes) are not preserved
    over a certificate re-issue. It is expected that they will be recreated
    on the first access to the service in question, and
  • the local keystore files for these certificates services are explicitly
    deleted so that the now-invalid cached certificates are not returned
    on keystore queries.

See-Also: #5047

lib/client/client.go Outdated Show resolved Hide resolved
lib/client/client.go Outdated Show resolved Hide resolved
lib/client/client.go Outdated Show resolved Hide resolved
@tcsc tcsc requested a review from nklaassen April 23, 2021 06:11
lib/auth/permissions.go Outdated Show resolved Hide resolved
lib/client/client.go Outdated Show resolved Hide resolved
@tcsc tcsc requested review from awly and andrejtokarcik April 26, 2021 06:32
Copy link
Contributor

@andrejtokarcik andrejtokarcik left a comment

Choose a reason for hiding this comment

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

I'm worried the proposed logic is incorrect as it assumes the impact of a role change to be limited to a single cluster whereas I think a role change should discard certs cached for all clusters so that requests against any cluster would trigger a reissue with the new roles.

Further, since SSH certs are now also cached per cluster, they should get discarded with the other service certs as well.

tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/access_request_test.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request_test.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
@tcsc
Copy link
Contributor Author

tcsc commented Apr 27, 2021

Further, since SSH certs are now also cached per cluster, they should get discarded with the other service certs as well.

From wading around in tsh, the SSH certs appear to be special; they seem to be the ultimate source of a lot of profile information for tsh (e.g. Roles & Active Requests), and a lot of code depends on them just existing at all times. That is not to say that they shouldn't be discarded with the other certs, just that it opens up a lot more complexity than one might think.

I'm not sure of the best way forward with this.

Maybe the correct thing is to stop trying to cobble together a solution out the existing key management primitives, and write something that:

  1. Enumerates all the known clusters for the active user behind the given proxy
  2. Deletes the service certs for all of them
  3. Generates (somehow!) a new SSH cert for the user/cluster

@tcsc tcsc requested a review from andrejtokarcik April 27, 2021 11:36
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request.go Outdated Show resolved Hide resolved
tool/tsh/access_request_test.go Outdated Show resolved Hide resolved
@tcsc
Copy link
Contributor Author

tcsc commented May 3, 2021

@awly's suggestion to go back to unconditionally deleting the certificates without
checking for a role change simplifies everything. This, hopefully last, patch makes
the following changes

  • Deletes all of the role change detection code
  • Adds a parameter to ProxyClient.ReissueUserCerts that indicates how existing
    certificates in the cache should be treated (i.e. Kept or Dropped)
  • Adds handling ProxyClient.ReissueUserCerts() that takes the appropriate cache action.
  • Adds handling to ProxyClient.reissueUserCerts() that changes how cached certificates
    are preserved across a re-issue.

I've added this to the ProxyClient as the cache cleanup needs to happen before the re-issued Key is added to the key store, otherwise it becomes very difficult to differentiate between the old certificates (that need purging) and the new certs (that need to be preserved).

Given that the rest of the system currently expects the new certs to be in the key store before ProxyClient.ReissueUserCerts() returns, this looked like the minimal change required to add the desired behaviour.

@tcsc tcsc force-pushed the trent/k8s-roles-request branch from eff061a to bb2916a Compare May 4, 2021 10:55
@tcsc tcsc requested a review from awly May 4, 2021 10:56
Copy link
Contributor

@nklaassen nklaassen left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@andrejtokarcik andrejtokarcik left a comment

Choose a reason for hiding this comment

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

BTW, does this really work as intended since it doesn't seem to be performing any real deletion of the stored certs anymore?

lib/client/client.go Outdated Show resolved Hide resolved
lib/client/client.go Show resolved Hide resolved
Copy link
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

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

Bot.

tcsc added 4 commits May 6, 2021 12:06
Prior to this change, user certificates for services like kubernetes were
preserved across a certificate re-issue. This led to issues where elevated
privileges granted by an access request were not applied to the service
certificates as they were not updated during the reissue process.

This patch changes the certificate re-issue process such that:
 * certificates for services (like Kuberenetes) are not preserved
   over a certificate re-issue. It is expected that they will be recreated
   on the first access to the service in question, and
 * the local keystore files for these certificates services are explicitly
   deleted so that the now-invalid cached certificates are not returned
   on keystore queries.

See-Also: #5047
Andrew's feedback that it's OK delete the cached certificates without
checking for a role change simplifies everything. This, hopefully last,
patch makes the following changes

 - Deletes all of the role change detection code
 - Adds a parameter to ReissueUserCerts that indicates how existing
   certificates in the cache should be treated (i.e. Kept or Dropped)
 - Adds code to ProxyClient.reissueUserCerts() that takes the
   appropriate cache action.

I've added this to the ProxyClient as the cache cleanup needs to
happen _before_ the new key is added to the key store, otherwise it
becomes very difficult to differentiate between the old certificates
that need purging and new certs that need to be preserved. Given that
the rest of the system currently expects the new certs to be in the key
store before ProxyClient.reissueUserCerts() returns, this looked like
the minimal change required to add the desired behaviour.
Co-authored-by: Andrej Tokarčík <[email protected]>
@tcsc tcsc force-pushed the trent/k8s-roles-request branch from e53b7df to 8643e16 Compare May 6, 2021 02:06
@tcsc tcsc enabled auto-merge (squash) May 6, 2021 02:07
@tcsc tcsc merged commit 2f51af0 into master May 6, 2021
@tcsc tcsc deleted the trent/k8s-roles-request branch May 6, 2021 02:20
tcsc added a commit that referenced this pull request May 6, 2021
Delete user k8s, etc. certificates on re-issue

Prior to this change, user certificates for services like kubernetes were
preserved across a certificate re-issue. This led to issues where elevated
privileges granted by an access request were not applied to the service
certificates as they were not updated during the reissue process.

This patch changes the certificate re-issue process such that:
 * certificates for services (like Kuberenetes) are not preserved
   over a certificate re-issue. It is expected that they will be recreated
   on the first access to the service in question, and
 * the local keystore files for these certificates services are explicitly
   deleted so that the now-invalid cached certificates are not returned
   on keystore queries.

See-Also: #5047
tcsc added a commit that referenced this pull request May 7, 2021
Delete user k8s, etc. certificates on re-issue

Prior to this change, user certificates for services like kubernetes were
preserved across a certificate re-issue. This led to issues where elevated
privileges granted by an access request were not applied to the service
certificates as they were not updated during the reissue process.

This patch changes the certificate re-issue process such that:
 * certificates for services (like Kuberenetes) are not preserved
   over a certificate re-issue. It is expected that they will be recreated
   on the first access to the service in question, and
 * the local keystore files for these certificates services are explicitly
   deleted so that the now-invalid cached certificates are not returned
   on keystore queries.

See-Also: #5047
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