not_before_duration added to SSH

[Gabrielopesantos]

Description

Added a configurable field that allows users to specify the amount of seconds an SSH certificate is backdated.
Issue: #13608

[cipherboy]

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!

[cipherboy]

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).

[Gabrielopesantos]

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.

[cipherboy]

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

[Gabrielopesantos]

I see, thanks for pointing it out.

[cipherboy]

Looking great @Gabrielopesantos! Thank you for your contribution :-) Merging...