Check PWS slot status before accessng it

[robinkrahl]

The Nitrokey devices to not check whether a PWS slot is programmed
before accessing it (upstream issues [0] [1]). Until this is fixed in
the firmware, we have to manually check the slot status in nitrocli pws
get. This could have been done in libnitrokey or the nitrokey crate,
yet this would lead to unnecessary commands if we check multiple fields
of a slot at the same time.

[0] Nitrokey/nitrokey-pro-firmware#56
[1] Nitrokey/nitrokey-storage-firmware#81

[robinkrahl]

Do you see what causes the checks to fail? Everything’s working fine on my machine, and I don’t see any error message in the cargo output.

[d-e-s-o]

     Running `rustc --crate-name syn /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/syn-0.15.23/src/lib.rs --color never --crate-type lib --emit=dep-info,link -C debuginfo=2 --cfg 'feature="clone-impls"' --cfg 'feature="default"' --cfg 'feature="derive"' --cfg 'feature="full"' --cfg 'feature="parsing"' --cfg 'feature="printing"' --cfg 'feature="proc-macro"' --cfg 'feature="proc-macro2"' --cfg 'feature="quote"' -C metadata=cbd78bf7bedfe2b8 -C extra-filename=-cbd78bf7bedfe2b8 --out-dir /builds/d-e-s-o/nitrocli/nitrocli/target/debug/deps -L dependency=/builds/d-e-s-o/nitrocli/nitrocli/target/debug/deps --extern proc_macro2=/builds/d-e-s-o/nitrocli/nitrocli/target/debug/deps/libproc_macro2-f564b8a685496207.rlib --extern quote=/builds/d-e-s-o/nitrocli/nitrocli/target/debug/deps/libquote-e2eff3b567ce22ab.rlib --extern unicode_xid=/builds/d-e-s-o/nitrocli/nitrocli/target/debug/deps/libunicode_xid-7b9f7590d421a85c.rlib --cap-lints allow`
ERROR: Job failed: exit code 137

I hit that a few times in the past. I believe it's due to the build taking too much memory (that may have increased over time) and being killed (at least that's what a quick search suggested). I plan to check on how to increase the memory size of the runner and see if that helps.

[d-e-s-o]

Thanks for this patch! I'll review this pull request tomorrow. I have two preliminary thoughts, though:

1. Can we add a test?
2. Should we add a change log entry?

[robinkrahl]

1) Can we add a test?

If the slot is empty, the nitrokey crate already returns an error (and we have a nitrocli test case for that). The interesting test case would be after a factory reset as the slot status is reset, but the slot content is not cleared. (We could actually open a separate issue for that.) But I don’t want to add that test case until the problems with the factory reset are fixed (or we know a workaround).

2) Should we add a change log entry?

Done. Let’s see if the Gitlab build error persists.

[robinkrahl]

but the slot content is not cleared

Turns out it is overwritten with random data. Anyway, it’s not emptied.

[d-e-s-o]

1. Can we add a test?
   If the slot is empty, the nitrokey crate already returns an error (and we have a nitrocli test case for that). The interesting test case would be after a factory reset as the slot status is reset, but the slot content is not cleared. (We could actually open a separate issue for that.) But I don’t want to add that test case until the problems with the factory reset are fixed (or we know a workaround).

I'd want to go ahead and merge the factory reset code. I believe then we should add a test along with the check-in. I easily reproduced the issue:

name:     �Ņ�Y��w�tW
login:  ?�YV���BH�3��l

             3���5ޔ
password: �`1�:��¨��6P�[��

and was able to verify that it does work with the fix. So this is a perfect candidate, from my point of view.

2. Should we add a change log entry?

Done. Let’s see if the Gitlab build error persists.

Thanks!

[d-e-s-o]

The reset command got merged. Would you mind adding a test? I'll merge the request once that is done.

[robinkrahl]

Done!

[d-e-s-o]

     Running `rustc --crate-name syn /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/syn-0.15.23/src/lib.rs --color never --crate-type lib --emit=dep-info,link -C debuginfo=2 --cfg 'feature="clone-impls"' --cfg 'feature="default"' --cfg 'feature="derive"' --cfg 'feature="full"' --cfg 'feature="parsing"' --cfg 'feature="printing"' --cfg 'feature="proc-macro"' --cfg 'feature="proc-macro2"' --cfg 'feature="quote"' -C metadata=cbd78bf7bedfe2b8 -C extra-filename=-cbd78bf7bedfe2b8 --out-dir /builds/d-e-s-o/nitrocli/nitrocli/target/debug/deps -L dependency=/builds/d-e-s-o/nitrocli/nitrocli/target/debug/deps --extern proc_macro2=/builds/d-e-s-o/nitrocli/nitrocli/target/debug/deps/libproc_macro2-f564b8a685496207.rlib --extern quote=/builds/d-e-s-o/nitrocli/nitrocli/target/debug/deps/libquote-e2eff3b567ce22ab.rlib --extern unicode_xid=/builds/d-e-s-o/nitrocli/nitrocli/target/debug/deps/libunicode_xid-7b9f7590d421a85c.rlib --cap-lints allow`
ERROR: Job failed: exit code 137

I hit that a few times in the past. I believe it's due to the build taking too much memory (that may have increased over time) and being killed (at least that's what a quick search suggested). I plan to check on how to increase the memory size of the runner and see if that helps.

I checked the two failures that I found and saw no similarities: Different runner and failures in different stages of the build.
Since I don't have my own runner (the entity executing the build/test/lint jobs) I just use the shared ones provided by Gitlab. By their very nature, I don't have access to them. The failures can probably not be mitigated by configuration on either the docker image used or the Gitlab CI integration, at least I did not find anything on that front. And given that the hypothesis is that Linux kills the process the runners may just have been overcommitted by virtue of having too many jobs run on them. I am not sure what we can do about that.

All I can say right now is that if you see this error again feel free to ignore it. I will test the change locally in all likely hood and can retry the job if it failed after check-in. That's not really a satisfactory state, but given the few failures we have it hopefully won't be a big burden either.

Done!

Thanks, I'll hopefully merge it soon!

[d-e-s-o]

Hm. I just hit this problem:

---- tests::pws::set_reset_get_pro stdout ----
thread 'tests::pws::set_reset_get_pro' panicked at 'called `Result::unwrap_err()` on an `Ok` value: "name:     zԗ�N\u{4}\nlogin:    nbn\u{e}���\u{13}x߇�/0Z�u³\u{1f}�3C�h\u{10}��7.\"6\npassword: ��9\u{14}��$-�)��RM�@�\u{e}�\u{1b}\n"', src/libcore/result.rs:1009:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.

in the fourth run of the test. Superficially this could be another timing problem of sorts. But it could also come from somewhere else.

[robinkrahl]

Hm, cannot reproduce on the Storage. :/

[d-e-s-o]

I merged the change after seeing another 53 successful runs and brief double check of the code in question. We'll see whether we hit it again.

Thanks for this fix, Robin!

[d-e-s-o]

Hm, cannot reproduce on the Storage. :/

Have you asked for a Pro device for testing? I would say it would be very helpful and Jan seemed happy to provide hardware that is needed.

[robinkrahl]

Not yet. I first wanted to consider the option of using a development board together with a smart card to simulate a Pro (that I then could also use for firmware testing). But I have to think a bit more about that.