openssh_* modules: check return code on ssh(-keygen) invocations; fail if comment cannot be updated

[felixfontein]

SUMMARY

Fixes #645.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

openssh_cert
openssh_keygen

[felixfontein]

On CentOS 6, comment change fails because of the -o option. I have no idea what -o does, my version of ssh-keygen (OpenSSH_9.3p2) doesn't document it and simply ignores it. It seems that -o was necessary for some ssh-keygen versions for certain types of keys (https://superuser.com/questions/361764/how-can-i-change-the-comment-field-of-an-rsa-key-ssh#comment2608595_1368078).

[felixfontein]

The -o option has been added in openssh/openssh-portable@bcd00ab#diff-28638b900664fcd9b95d7f1d8ff8015345626120df46b0baa1c2ca4a931b1cdaR2178, and made redundant in openssh/openssh-portable@ed7bd5d#diff-28638b900664fcd9b95d7f1d8ff8015345626120df46b0baa1c2ca4a931b1cdaR2445. If I understand GitHub correctly, it was added in OpenSSH 6.5p1, and made redundant in OpenSSH 7.8p1.

[felixfontein]

I think this is working now. Please ignore the failing FreeBSD CI jobs, the failures are completely unrelated (and happen in other repos as well).

[felixfontein]

Please note that this might also start reporting errors in other cases that worked before (or looked like working - whether it's an error now reported or a wrongly reported error needs to be determined on a case-by-case basis).

[Ajpantuso]

Please note that this might also start reporting errors in other cases that worked before (or looked like working - whether it's an error now reported or a wrongly reported error needs to be determined on a case-by-case basis).

I'm okay with the module explicitly reporting errors rather than silently continuing.

These changes were not made previously to avoid breaking behavior post-refactor. Also the idea at the time was to eventually deprecate/remove the opensshbin backend, but achieving feature parity with ssh-keygen is not likely given how messy the openssh revision history is.

[Ajpantuso]

LGTM

[felixfontein]

These changes were not made previously to avoid breaking behavior post-refactor.

Makes total sense. I've been thinking a bit about it while working on this PR, especially whether this is more a feature (the user should know if something doesn't work) or a breaking change. I guess it is both, but I think it's more a feature, so I added a minor_changes changelog fragment.

Also the idea at the time was to eventually deprecate/remove the opensshbin backend, but achieving feature parity with ssh-keygen is not likely given how messy the openssh revision history is.

Yes, unfortunately... At least it's not as messy as GnuPG handling... ;-)

[felixfontein]

@Ajpantuso thanks a lot for reviewing this!