-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
not_before_duration added to SSH #15250
not_before_duration added to SSH #15250
Conversation
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, my understanding is:
You've add a new parameter to the role, called not_before_duration
that's a time.Duration == int64
type. (Good!)
You then use this parameter when setting the ValidAfter
value. (Good!)
You've added tests & docs & changelog and this is also good. :-)
But there's one small thing missing that I forgot earlier, due to the new role parameter: migration :/
If you have an existing role (under an older Vault version), because the new field will take on the zero value, we'll change behavior from backdating by 30 seconds to only 0 seconds, which might break some deployments. :/
So in net, you'll probably want to:
- Bump the role version indicator to 3:
vault/builtin/logical/ssh/path_roles.go
Lines 28 to 31 in 436e7f3
// Present version of the sshRole struct; when adding a new field or are // needing to perform a migration, increment this struct and read the note // in checkUpgrade(...). roleEntryVersion = 2 - Add a new role version check, that if the version is less than three, you'll mark it modified and set the
NotAfterDuration
field to 30 seconds:vault/builtin/logical/ssh/path_roles.go
Line 687 in 436e7f3
Otherwise, looks great to me!
"ttl": "2h", | ||
"cert_type": "host", | ||
"valid_principals": "dummy.example.org,second.example.com", | ||
}), |
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.
As an aside: wow, I know that passes make fmt
but that's kinda weird formatting. (Not suggesting you change it or anything, just find it funny).
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.
Not sure how is the formatting weird there but it seems to be in accordance with go fmt
so I don't know how could it be improved.
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.
Unless I'm misreading this diff, it looks like there's an extra level of indentation. It starts by placing all parameters on the same line, indents for the map. Next map goes to the same indentation level as the function invocation (fine), but then we indent for a non-map parameter, add the next map parameter, and indent that again!
Seems like we should outdent 1716 and line everything up nicely, or expand the function invocation so all parameters are on the same level (indented one like 1716), forcing all map's k=v entries to be on the level of 1717 instead of 1712.
But like you said, go fmt
is happy with it and other tests in the same file look like that, so I don't really care :D
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 see, thanks for pointing it out.
Looking great @Gabrielopesantos! Thank you for your contribution :-) Merging... |
Description
Added a configurable field that allows users to specify the amount of seconds an SSH certificate is backdated.
Issue: #13608