Undocumented breaking changes for "get pubkey" and "get wrapped", behavior changes between yubihsm-shell 2.5.0 and 2.6.0

[eliasrudberg]

For the "get pubkey" command, a new "t:key_type=asymmetric-key" argument was added, which is a breaking change for users who are using the subsequent file argument:

yubihsm-shell$ git checkout 2.5.0
yubihsm-shell$ git grep -A1 "yh_com_get_pubkey,"
src/main.c:  register_subcommand(*c, (Command){"pubkey", yh_com_get_pubkey,
src/main.c-                                    "e:session,w:key_id,F:file=-", fmt_nofmt,

yubihsm-shell$ git checkout 2.6.0
yubihsm-shell$ git grep -A1 "yh_com_get_pubkey,"
src/main.c:  register_subcommand(*c, (Command){"pubkey", yh_com_get_pubkey,
src/main.c-                                    "e:session,w:key_id,t:key_type=asymmetric-key,F:file=-", fmt_nofmt,

Similarly, for the "get wrapped" command, another new argument was added in a way that becomes a breaking change for users who are using the file argument:

yubihsm-shell$ git checkout 2.5.0
yubihsm-shell$ git grep -A1 "yh_com_get_wrapped,"
src/main.c:                      (Command){"wrapped", yh_com_get_wrapped,
src/main.c-                                "e:session,w:wrapkey_id,t:type,w:id,F:file=-",

yubihsm-shell$ git checkout 2.6.0
yubihsm-shell$ git grep -A1 "yh_com_get_wrapped,"
src/main.c:                      (Command){"wrapped", yh_com_get_wrapped,
src/main.c-                                "e:session,w:wrapkey_id,t:type,w:id,b:include_seed=0,F:file=-",

Both changes were done in the commit 080e32d

In the changelog https://github.com/Yubico/yubihsm-shell/blob/master/CHANGELOG that change is described as "Add support for asymmetric wrap".

Could the new functionality be added in a way that does not break existing yubihsm-shell usage?

Breaking changes like this make it more cumbersome to maintain things relying on yubihsm-shell. Example: https://git.glasklar.is/sigsum/core/key-mgmt/-/merge_requests/28

The changes regarding "get pubkey" and "get wrapped" should at least be documented as breaking changes to alert users to the fact that existing scripts may stop working when yubihsm-shell is upgraded?

[aveenismail]

The changes should not break yubihsm-shell usage. Before 2.6.0, get pubkey only returned public keys of asymmetric keys. After 2.6.0, if no type is mentioned, the type is assumed to be asymmetric-key specifically so as not to break yubihsm-shell from earlier versions. Same with get wrapped

What errors are you getting when not specifying the type?

[eliasrudberg]

Hi, thanks for answering!

What errors are you getting when not specifying the type?

For the "get pubkey" command, my attempted command looks like this:

get pubkey 0 500 "$log_pubkey_file"

where $log_pubkey_file is the filename "tmp.logsrv.pem".

That works for 2.5.0 but for 2.6.0 it fails with the following error message:

Invalid argument 4: tmp.logsrv.pem (t:key_type=asymmetric-key)

See https://git.glasklar.is/sigsum/core/key-mgmt/-/issues/27

As I understand it, the problem is that for 2.5.0 the meaning of the three arguments is "session, key_id, file" while for 2.6.0 if there are three arguments then those are interpreted as "session, key_id, key_type" so the filename that I specify as third argument is interpreted as a key_type. To make it work for 2.6.0 it is necessary to add the key_type argument before the filename argument, so that the list of arguments becomes "session, key_id, key_type, file". I can do that, but then my command no longer works for 2.5.0. There seems to be no way to write it that would work for both 2.5.0 and 2.6.0.

[aveenismail]

Hi @eliasrudberg

OK, I see the issue. Unfortunately, positional arguments are kind of tricky when several of them are optional and I'm not sure I agree that the change is breaking backwards compatibility since interactive mode would always require user interaction. The equivalent of both get pubkey and get wrapped commands are available in non-interactive mode that is more script friendly and in non-interactive mode, arguments are not expected to be given in a specific order and completely omitting the extra/new argument would make the command behave in the exact way it behaved in previous versions.

Having said that, I now created an internal ticket to evaluate whether the order of the arguments in interactive mode should be altered now that this order has been in official releases for almost a year.

[eliasrudberg]

OK, I see the issue. Unfortunately, positional arguments are kind of tricky when several of them are optional and I'm not sure I agree that the change is breaking backwards compatibility since interactive mode would always require user interaction.

In our case (for the Sigsum project) we have scripts that do not require user interaction, where we get this problem.

For example here: https://git.glasklar.is/sigsum/core/key-mgmt/-/blob/main/scripts/yhp-keygen#L29

As you can see there, we use a bash script with a construction like this:

yubihsm_shell << EOF
	connect
	session open 1 password
	put      authkey [...]
	put      wrapkey [...]
	generate asymmetric [...]
	generate asymmetric [...]
	get pubkey 0 "$LOGSRV_SIGNING_KEY_ID"  "$log_pubkey_file"
        [...]
EOF

When a script like the above is used, all the commands until "EOF" are run without user interaction. We have several scripts that work in that way, and they do break for 2.6.0. So from my point of view the change is breaking backwards compatibility.

The equivalent of both get pubkey and get wrapped commands are available in non-interactive mode that is more script friendly and in non-interactive mode, arguments are not expected to be given in a specific order and completely omitting the extra/new argument would make the command behave in the exact way it behaved in previous versions.

Thanks for explaining this. It sounds like it would have been better if we had used non-interactive mode in our scripts from the beginning, then we would not have encountered this problem. But still, the current situation is that we have something that works for yubihsm-shell 2.5.0 and we would have preferred if it kept working regardless of yubihsm-shell being upgraded.