-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
docs: add note about user CA rotation + desktop access #10126
Conversation
CA pin when adding nodes after rotation. | ||
</Admonition> | ||
|
||
<Admonition type="warning" title="Desktop Access"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it poor form to have two Adminition
s back to back like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've suggested moving the CA pinning warning since we use it twice in the page.
Started](../getting-started.mdx) guide. | ||
<Admonition type="note" title="User CA Rotation."> | ||
These steps will need to be repeated if Teleport's user certificate | ||
authority is [rotated](../setup/operations/ca-rotation.mdx). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we can keep the reader's focus on this guide, I think we should remove the link here and provide any additional info we need from this link within this sentence. We can link to the page on CA rotation in a "Next steps" or "Further reading" section if it helps.
Take a look at the [Certificates chapter](../../architecture/authentication.mdx#authentication-in-teleport) in the | ||
architecture document to learn how the certificate authority rotation works. | ||
Take a look at the | ||
[Certificates chapter](../../architecture/authentication.mdx#authentication-in-teleport) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is reading this guide required before carrying on, or is this additional context? If it's additional context, I think we should put this in a "Next steps" or "Further reading" section to keep the reader's focus on this guide. If it's a prerequisite, maybe we should put it in the "Prerequisites" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, to be honest. This is a very old page that predates my employment at Teleport. On first glance, I don't see a problem moving this to a "further reading" section.
I guess getting such a thorough review is what I get for wrapping lines that aren't related to the actual change I'm making 😅
|
||
This section will show you how to implement certificate rotation in practice. | ||
|
||
During manual and semi-automatic certificate authority rotation, Teleport generates a new certificate | ||
authority and issues certificates for auth servers, proxies, nodes and users. | ||
During manual and semi-automatic certificate authority rotation, Teleport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we know what "manual" and "semi-automatic" mean in this sentence before we get to the relevant sections, I think we should add something like,
There are two kinds of CA rotation:
- **Manual:** this is the definition.
- **Semi-automatic:** this is the definition.
We can put this sentence below that one.
- `update_servers` Servers will reload and would start serving | ||
TLS and SSH certificates signed by the new certificate authority, but will still accept certificates | ||
issued by old certificate authority. | ||
- `init` - All components are notified of the rotation. A new certificate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `init` - All components are notified of the rotation. A new certificate | |
- `init` All components are notified of the rotation. A new certificate |
For consistency
authority is issued, but not used. It is necessary for remote trusted clusters | ||
to fetch the new certificate authority, otherwise the new clients will reject | ||
it. | ||
- `update_clients` - internal clients certs are updated and reloaded. Servers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `update_clients` - internal clients certs are updated and reloaded. Servers | |
- `update_clients` Internal clients certs are updated and reloaded. Servers |
During the grace period, certificates issued both by old and new certificate authority work. | ||
This will trigger a rotation process for both hosts and users with a *grace | ||
period* of 48 hours. During the grace period, certificates issued both by old | ||
and new certificate authority work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and new certificate authority work. |
If we accept my previous suggestion, we'll need to remove this line.
rotation and come back only after the grace period has ended, they will be | ||
forced to leave the cluster, i.e. users will no longer be allowed to SSH | ||
into them. | ||
Be careful when choosing a grace period when rotating host certificates. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a long admonition, what would you think of removing this text from the Admonition
component and making it regular body text, then wrapping the sentence, "Be careful when choosing a grace period when rotating host certificates." in a Notice
component with the same type
and title
?
If you are using [CA Pinning](../admin/adding-nodes.mdx#untrusted-auth-servers) when adding new nodes, the CA pin will change after the rotation. Make sure you use the | ||
*new* CA pin when adding nodes after rotation. | ||
If you are using | ||
[CA Pinning](../admin/adding-nodes.mdx#untrusted-auth-servers) when adding new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a place we can put this warning so we only need to include it once in the page?
|
||
<Admonition type="warning" title="Desktop Access"> | ||
Teleport signs Windows Desktop certificates with the user certificate authority. | ||
If the user CA is rotated, the new certificate will need to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than add the link on "exported", which could break the user's focus, I should we should add a "Read more about exporting the Teleport CA" link at the bottom of this admonition box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am definitely not the expert, so I defer to you on the final decision, but in this case I think the link makes sense.
As a reader, I'm thinking "you're telling me I have to do this export thing, but I have no idea what that means or how to do it. Why make me search for it?"
CA pin when adding nodes after rotation. | ||
</Admonition> | ||
|
||
<Admonition type="warning" title="Desktop Access"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've suggested moving the CA pinning warning since we use it twice in the page.
@ptgott after thinking on this, many of your comments should be applied to the docs for all recent releases, but the note about desktop access + cert rotation only applies to v8 and up. I will open a separate PR to address the comments that are not related to desktop access. Once we get that merged and backported to all appropriate branches, I'll update this PR so that it contains only the desktop access change. Sound reasonable? |
@zmb3 It does! |
17cd116
to
63358c5
Compare
@ptgott I finally got back to this. Should be a much simpler review now that the rest of your comments have already been addressed and merged. PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with small comments
use the *new* CA pin when adding nodes after rotation. | ||
|
||
<Admonition type="warning" title="Desktop Access"> | ||
Teleport signes Windows Desktop services with the user certificate authority. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Teleport signes Windows Desktop services with the user certificate authority. | |
Teleport signs Windows Desktop services with the user certificate authority. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should this be "Windows Desktop Service instances" (or "Windows Desktop Service nodes" or "Windows Desktop Service agents")?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be "Teleport signs Windows Desktop certificates with the..."
certificates, not services. Good catch!
<Admonition type="warning" title="Desktop Access"> | ||
Teleport signes Windows Desktop services with the user certificate authority. | ||
If the user CA is rotated, the new CA certificate will need to be exported and | ||
configured in group policy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be an article/possessive adjective before "group policy"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think so. "Group Policy" is standard terminology in Windows land.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
In the future we'll move desktop access to a separate CA, but for now Windows certs are generated with the user CA, so Active Directory will need to be updated to trust the new certificate when a rotation takes place.
63358c5
to
5fd3b0b
Compare
In the future we'll move desktop access to a separate CA, but for
now Windows certs are generated with the user CA, so Active Directory
will need to be updated to trust the new certificate when a rotation
takes place.