trussed-dev
Put debug-dump-store behind a feature-flag
When debugging https://github.com/Nitrokey/piv-authenticator/pull/12 I realized this was not removed from production firmware. Putting it behind a feature flag will ensure that it does.
I thought the consensus is to not make a feature out of the debug switches (but use --cfg flag instead). Has this changed?
Ref: https://github.com/Nitrokey/trussed-secrets-app/issues/23
I think this is a misunderstanding. We almost never use rustc’s --cfg flag directly, just features. But in the source code, we use the cfg attribute to check for that feature.
Ah, I just saw the review comment you referred to. I’ll have to think about that option.
I think the main difference between the two is that the addition of the debug-dump-store is purely additive, whereas the removal of encryption is not.
This flag does not change the test paths if you use --all-features, since it only adds new code.
For the encryption on the other hand, it removes code, so testing with --all-features does not test encryption.
Also removal of encryption is not something we want to encourage at any point, while features are easily discoverable, and therefore can mistakenly be enabled, which would be highly problematic.
It's also consistent with the std features, that are here only for debugging/testing. They can be features because they're additive, and not that dangerous, contrary to the encryption flag.
dump-store in production firmware sounds dangerous to me, hence the question
It makes the call available but you would still have to call it, and you would have to have the logs somehow accessible on a production device before this becomes really dangerous.
Did you come to consensus? :) Slight tendency to cfg-flag on my side (default "off" naturally).
I moved it behind a CFG. This doesn't matter much if you prefer cfg we can merge it that way.
@sosthene-nitrokey What do you think about removing the syscall implementation entirely and always returning RequestNotAvailable? The current implementation does not do anything anyway and it is not actually used. Then we could drop the syscall in the next breaking trussed-core release.
