feat: Connect summary cryptoki show mode and key URI#3623
feat: Connect summary cryptoki show mode and key URI#3623Bravo555 merged 2 commits intothin-edge:mainfrom
Conversation
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
It might make more sense to show the same value as used in the tedge.toml, e.g. |
7184394 to
f29ff36
Compare
Robot Results
|
didier-wenzek
left a comment
There was a problem hiding this comment.
Approved, with minor comments.
| let cryptoki_key_uri = config.device.cryptoki_config(cloud)?.map(|c| match c { | ||
| CryptokiConfig::Direct(d) => d.uri, | ||
| CryptokiConfig::SocketService { uri, .. } => uri, | ||
| }); | ||
| let cryptoki_mode = config.device.cryptoki.mode.clone(); | ||
| let cryptoki_status = match cryptoki_key_uri { | ||
| None => "off".to_string(), | ||
| Some(None) => format!("{cryptoki_mode} (key: unspecified)"), | ||
| Some(Some(key)) => format!("{cryptoki_mode} (key: {key})"), | ||
| }; |
There was a problem hiding this comment.
This is working but I'm surprised by the mode extracted twice via two different paths (indirectly with config.device.cryptoki_config and then explicitly with config.device.cryptoki.mode). Isn't there a risk of inconsistency?
There was a problem hiding this comment.
fixed in c057753
Yeah, I've overengineered it a bit, I was using cryptoki_config to get the correct URI, but I could simply get URI from cloud.key_uri. It should now be much simpler.
There was a problem hiding this comment.
I had to revert this because I forgot why I did that originally: cloud.device.key_uri is empty if we use device.key_uri, and .cryptoki_config() actually read both and applied device.key_uri if the one from cloud was None.
|
@Bravo555 I noticed that we currently don't have a system test (which is run by default) for setting the It would make sense adding a new system tests which just uses Cumulocity and sets the PKCS11 URI on the client side (and not on the tedge-p11-server)...having this test would also make it easier to see a real example of how the |
f29ff36 to
c057753
Compare
@reubenmiller New test added in f3ac02b. Is it sufficient? |
a21f01b to
a309757
Compare
reubenmiller
left a comment
There was a problem hiding this comment.
Approved. Nice improvement to make it more transparent about the settings being used.
Change `cryptoki:` line in connection summary from simple `true/false` to display mode and key URI: - If mode is off: `cryptoki: off` - If mode is not off and `key_uri` is not set: `cryptoki: MODE` - If mode is not off and `key_uri` is set: `cryptoki: MODE (key: KEY)` Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
094c495 to
a2de92a
Compare
Proposed changes
Change
cryptoki:line in connection summary from simpletrue/falseto display mode and key URI:cryptoki: offkey_uriis not set:cryptoki: MODEkey_uriis set:cryptoki: MODE (key: KEY)Types of changes
Paste Link to the issue
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments
I'm not sure if this is the best way to format the information in the summary, I'm open to suggestions for changes.
An alternative would be to instead of separate
cryptoki:line, to add aprivate key:line, and put therekey_uriandkey_pathconfig settings, depending whether cryptoki is enabled or not.