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

Add host fields and protos for ssh identities #51024

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

fspmarshall
Copy link
Contributor

Updates sshca.Identity to support host identity fields and adds SSHIdentity to the decision family of protos.

The original sshca.Identity object was added in #50787 and was initially specialized to user identities only. At the time I thought I'd be able to mostly avoid caring about host identities. It turns out that there is enough ssh certificate handling logic that is intended to work with host and user certs, that it is best to be able to handle both using the same identity type.

I'm not completely happy with having host and user identity fields living alongside one another, and I did experiment a bit with the idea of having separate HostIdentity and UserIdentity types, with a common interface so that logic that is generic over both can continue to operate on identities as a single object. This ended up being a bit too much scope creep for the purposes of these changes (especially since the more common tlsca.Identity is already monolithic).

Part of ongoing work related to the PDP proposal (https://github.com/gravitational/teleport.e/pull/5736).

@fspmarshall fspmarshall added the no-changelog Indicates that a PR does not require a changelog entry label Jan 14, 2025
@fspmarshall fspmarshall force-pushed the fspmarshall/pdp-ssh-identity-proto branch from 6a0c7c7 to f4fcde4 Compare January 14, 2025 18:00
@fspmarshall fspmarshall mentioned this pull request Jan 14, 2025
21 tasks
Copy link
Contributor

@mvbrock mvbrock left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple questions out of curiosity

// since this method ignores the supplied values for ValidBefore/ValidAfter, avoid confusing by
// rejecting identities where they are set.
if ident.ValidBefore != 0 {
return nil, trace.BadParameter("ValidBefore should not be set in calls to GenerateUserCert")
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious what the value of explicitly rejecting the request is when the valid before/after is set.

Copy link
Contributor

Choose a reason for hiding this comment

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

And would this make more or less sense to put in the req.Check() method? More just asking for my own edification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is that the values are intended to be derived from TTL by this function rather than supplied by the caller, so a nonzero value in one of those fields may indicate human error. Basically, this check is just intended to help catch mistakes.

In terms of the location of the check, no real reason... I was just working on this part of the code when I first decided that it would be good to have a check like this. Moved it to req.Check().

@fspmarshall fspmarshall force-pushed the fspmarshall/pdp-ssh-identity-proto branch 2 times, most recently from f02998f to fabe0fa Compare January 24, 2025 22:14
@fspmarshall fspmarshall enabled auto-merge January 24, 2025 22:16
@fspmarshall fspmarshall force-pushed the fspmarshall/pdp-ssh-identity-proto branch from fabe0fa to a4b94e2 Compare January 27, 2025 17:30
@fspmarshall fspmarshall added this pull request to the merge queue Jan 27, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 27, 2025
@fspmarshall fspmarshall added this pull request to the merge queue Jan 27, 2025
Merged via the queue into master with commit 5791a98 Jan 27, 2025
44 checks passed
@fspmarshall fspmarshall deleted the fspmarshall/pdp-ssh-identity-proto branch January 27, 2025 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants