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

kb for renewing expired harvester cloud credentials #60

Merged
merged 3 commits into from
May 28, 2024

Conversation

ibrokethecloud
Copy link
Contributor

@ibrokethecloud ibrokethecloud commented May 17, 2024

KB detailing steps on how to renew expired new harvester cloud credentials on harvester

ref: harvester/harvester#5827

Copy link

netlify bot commented May 17, 2024

Deploy Preview for harvester-home-preview ready!

Name Link
🔨 Latest commit 551b3c7
🔍 Latest deploy log https://app.netlify.com/sites/harvester-home-preview/deploys/66553a57e3d019000898f372
😎 Deploy Preview https://deploy-preview-60--harvester-home-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@innobead innobead left a comment

Choose a reason for hiding this comment

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

In general, LGTM.

Should we also provide a resolution for users who are going to create a cloud credential for their managed Harvester cluster? By updating the global ttl setting? However, as this is a global setting, it will be applied to other cases as well rather than Harvester only.

Also, replace crd with CRD.

kb/2024-05-17/modify_cloud_credential_ttl.md Outdated Show resolved Hide resolved
kb/2024-05-17/modify_cloud_credential_ttl.md Outdated Show resolved Hide resolved
@ibrokethecloud
Copy link
Contributor Author

@innobead i guess this can be used to set a custom expiry on harvester cloud credentials manually while retaining normal expiry intervals for non cloud credentials.

@innobead
Copy link
Contributor

@FrankYang0529 @Vicente-Cheng please review this.

Copy link
Contributor

@Vicente-Cheng Vicente-Cheng left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. Just add a suggestion, we could also provide the one-line command for the user.

kb/2024-05-17/modify_cloud_credential_ttl.md Show resolved Hide resolved
Copy link
Member

@FrankYang0529 FrankYang0529 left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Thank you.

Copy link
Contributor

@m-ildefons m-ildefons left a comment

Choose a reason for hiding this comment

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

This KB article should also mention the two other cases:

  1. The workaround how to avoid the tokens from expiring in the first place, for new clusters, i.e. setting the default- and maximum-token-ttl settings to 0
  2. The workaround to recover a cluster whose token has expired:
    Create new cloud credentials (preferably such that the token doesn't expire, see 1) and patch the cluster's yaml such that the .spec.cloudCredentialSecretName references the new cloud credential.

These two workarounds would still need to be confirmed by someone on the Rancher team though, as I'm not sure they are completely free of other subtle bugs.

Another problem with this workaround in general is that it relies on the token still being there. This isn't guaranteed, because expired tokens are garbage-collected after a while, so if the user can't react to the tokens expiry within a short time, this workaround won't work for them at all. See case 2 on how to recover in that case.

m-ildefons added a commit to m-ildefons/docs that referenced this pull request May 17, 2024
Highlight known issue about token TTL in Harvester node driver/Rancher
integration. The highlighting also links to the Knowledgebase article
describing the temporary workaround.

depends-on: harvester/harvesterhci.io#60

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
@macedogm
Copy link

macedogm commented May 20, 2024

@m-ildefons:

This KB article should also mention the two other cases:

  1. The workaround how to avoid the tokens from expiring in the first place, for new clusters, i.e. setting the default- and maximum-token-ttl settings to 0

You mean setting the TTLs to 0 in Rancher's default config https://ranchermanager.docs.rancher.com/api/api-tokens#setting-ttl-on-kubeconfig-tokens?

If yes, then this is very risk as explained in #60 (comment).

@m-ildefons
Copy link
Contributor

@m-ildefons:

This KB article should also mention the two other cases:

  1. The workaround how to avoid the tokens from expiring in the first place, for new clusters, i.e. setting the default- and maximum-token-ttl settings to 0

You mean setting the TTLs to 0 in Rancher's default config https://ranchermanager.docs.rancher.com/api/api-tokens#setting-ttl-on-kubeconfig-tokens?

If yes, then this is very risk as explained in #60 (comment).

There are three different problems this KB article should cover:

  1. For new installations it should cover how users can avoid running into the problem of expiring tokens in the first place (i.e. restore behavior of Rancher v2.7 wrt. this feature)

  2. For an existing installation whose token has not yet expired, this KB article should cover how users can avoid an outage from the impending expiry

  3. For an existing installation with an already expired token, this article should cover how a user can recover and regain control of the downstream cluster

That's all I'm asking. I don't care what the exact solution to each of the three problems is, but these three points should be covered.
I've suggested editing the default- and maximum-TTL-settings, because I've tested that it works and I'm confident that it's a solution to the first point.
If you have a solution to the first point, that doesn't involve changing the default- and maximum-TTL-settings, then please share it here, but please also keep the time to implement it in mind, since telling users to wait until Rancher v2.10 releases in 2026 with an all-new token-design is hardly a workaround worth putting into a KB article.
If it's such a big no-no to set the default- and maximum-TTL-settings to 0, then why are they user-editable in the first place?
We could also advise users to set them to a very long period instead (e.g. 10 years), but I don't think this would be significantly better for the security posture[1], I also don't think this would be more favorable for garbage-collecting old tokens and I don't think it's really all that advisable for users to set themselves up with a problem years into the future, when the tokens finally expire.

[1]: A short side note on security in this context: I've found absolutely zero discussion on expiring tokens and their improvement of security, besides that the original PR, which changed the token TTL for Rancher v2.8.x, claims that it does.
Without an analysis that includes a threat-model and talks through the process of dealing with expired tokens, I'm skeptical at best that this change really has the positive impact on the security posture that you think it has.

@innobead innobead requested a review from jillian-maroket May 24, 2024 03:39
@ibrokethecloud ibrokethecloud changed the title kb for ttl-workaround kb for renewing expired harvester cloud credentials May 24, 2024
update instructions for patching cloud credentials

updated title
Copy link
Contributor

@jillian-maroket jillian-maroket left a comment

Choose a reason for hiding this comment

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

I reused content from doc PR 567, which is ready for merging. Let me know if you have concerns about the changes.

kb/2024-05-17/modify_cloud_credential_ttl.md Outdated Show resolved Hide resolved
kb/2024-05-17/modify_cloud_credential_ttl.md Outdated Show resolved Hide resolved
include feedback from Jilllian.

Co-authored-by: Jillian <67180770+jillian-maroket@users.noreply.github.com>
Copy link
Contributor

@jillian-maroket jillian-maroket left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@innobead innobead merged commit 196d067 into harvester:main May 28, 2024
5 checks passed
m-ildefons added a commit to m-ildefons/docs that referenced this pull request May 29, 2024
Highlight known issue about token TTL in Harvester node driver/Rancher
integration. The highlighting also links to the Knowledgebase article
describing the temporary workaround.

depends-on: harvester/harvesterhci.io#60

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
jillian-maroket pushed a commit to harvester/docs that referenced this pull request May 29, 2024
* Docs: Highlight known issue about token TTL

Highlight known issue about token TTL in Harvester node driver/Rancher
integration. The highlighting also links to the Knowledgebase article
describing the temporary workaround.

depends-on: harvester/harvesterhci.io#60

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>

* re-format after code-review suggestion

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>

* update KB article URL

Update KB article URL to the final published one.

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>

---------

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
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.

10 participants