-
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
Add host fields and protos for ssh identities #51024
Conversation
6a0c7c7
to
f4fcde4
Compare
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.
LGTM, just a couple questions out of curiosity
lib/auth/keygen/keygen.go
Outdated
// 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") |
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.
Just curious what the value of explicitly rejecting the request is when the valid before/after is set.
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 would this make more or less sense to put in the req.Check()
method? More just asking for my own edification.
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.
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()
.
f02998f
to
fabe0fa
Compare
fabe0fa
to
a4b94e2
Compare
Updates
sshca.Identity
to support host identity fields and addsSSHIdentity
to thedecision
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
andUserIdentity
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 commontlsca.Identity
is already monolithic).Part of ongoing work related to the PDP proposal (https://github.com/gravitational/teleport.e/pull/5736).