feat: support read-write device.id in tedge cert/connect #3326

rina23q · 2025-01-10T23:33:50Z

Proposed changes

This PR aims to open up device.id as read-write key. This is the last piece of supporting basic authentication, as it needs device.id but not need for device cert.

TODOs:

tedge cert/connect part

tedge cert create writes device.id in tedge.toml explicitly.
tedge connect returns an error if device.id mismatches the certificate's CN. The only exception is when c8y.auth_mode is basic.
Updated smart_rest_one system test to confirm that tedge connect c8y uses device.id directly when using basic auth.
tedge cert create-csr also sets device.id if not configured.
tedge cert create should work without --device-id option when device.id is already set.
~~Don't overwrite a certificate and device.id when the values of option --device-id and config device.id are conflicting. This case should return an error.~~ If a certificate exists already in the given path, anyway it will not be overwritten (known behavour)

tedge_config part (originally started in the PR #3318 by @jarhodes314 )

Introduce new read-write macro
Clippy warnings & doc test error fix
Move get_device_id_from_config() function to tedge_config to keep config.device.id() as private. refactor: support writable device ids #3318 (comment)

doc part

Create Basic auth and SmartREST1.0 support doc

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
Documentation Update (if none of the other choices apply)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#3242

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA (in all commits with git commit -s)
I ran cargo fmt as mentioned in CODING_GUIDELINES
I used cargo clippy as mentioned in CODING_GUIDELINES
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

didier-wenzek

The command to create a CSR must also be updated to set the device id.

didier-wenzek

If the device id has been set using tedge config set device.id, then the device id should no more be mandatory for tedge cert create.

We also need to take care of the paths for the cloud specific certificates.
For instance, the following sequence of command is error prone:

# Trying to create a certificate for an unknown cloud profile (okay: the error is detected)
$ tedge cert create --device-id demo-c8y c8y --profile foo
Error: missing configuration parameter

Caused by:
Unknown profile `foo` for the multi-profile property c8y

# Creating the cloud profile
$ tedge config set c8y.device.id device-c8y-foo  --profile foo

# Now that the cloud profile exists, the cert creat command runs
# BUT
#    - --device-id is required while set in the config
#    - the value set on the command line erases the config
#    - the certificate is created globally (because no specific paths were configured for this profile)
$ tedge cert create --device-id some-other-device-id c8y --profile foo
Error: failed to create a test certificate for the device some-other-device-id.

Caused by:
A certificate already exists and would be overwritten.
Existing file: "/etc/tedge/device-certs/demo-device-888.pem"
Run `tedge cert remove` first to generate a new certificate.

rina23q · 2025-01-13T10:44:15Z

@didier-wenzek
Actually this error occurs even now if user doesn't set any keys for a profile beforehand.

# Trying to create a certificate for an unknown cloud profile (okay: the error is detected)
$ tedge cert create --device-id demo-c8y c8y --profile foo
Error: missing configuration parameter

Caused by:
Unknown profile `foo` for the multi-profile property c8y

For the rest, I would like to implement to have the consistent behavour as create-csr, which uses the value of device.id if --device-id is not given.

However, on the condition, if device.id is already configured, but --device-id option is given with the different name as you tried, what is the expected behaviour? Returns an error as they are conflictiing?

didier-wenzek · 2025-01-13T11:14:37Z

@didier-wenzek Actually this error occurs even now if user doesn't set any keys for a profile beforehand.

In that case this is a bug. tedge cert create should not create a global certificate when the request if for a specific cloud.

For the rest, I would like to implement to have the consistent behavour as create-csr, which uses the value of device.id if --device-id is not given.

Perfect. One just to be cautious taking the most specific device id (i.e. the id for the requested cloud and profile if given).

However, on the condition, if device.id is already configured, but --device-id option is given with the different name as you tried, what is the expected behaviour? Returns an error as they are conflictiing?

Yes raising an error is the way to go. But make sure the comparison is done on the device id specific for the requested cloud. I.e. if device.id is set but the command provides a different one for c8y, then this is okay.

codecov · 2025-01-13T16:11:35Z

Codecov Report

Attention: Patch coverage is 95.75972% with 36 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
.../tedge_config/src/tedge_config_cli/tedge_config.rs	84.44%	4 Missing and 3 partials ⚠️
crates/core/tedge/src/cli/certificate/cli.rs	88.33%	5 Missing and 2 partials ⚠️
crates/core/tedge/src/cli/connect/command.rs	90.27%	2 Missing and 5 partials ⚠️
...nfig/src/tedge_config_cli/tedge_config_location.rs	0.00%	3 Missing ⚠️
crates/core/tedge/src/cli/certificate/create.rs	95.31%	0 Missing and 3 partials ⚠️
crates/core/tedge/src/cli/certificate/renew.rs	70.00%	3 Missing ⚠️
...rates/core/tedge/src/cli/certificate/create_csr.rs	75.00%	2 Missing ⚠️
crates/core/tedge/src/cli/upload/mod.rs	0.00%	2 Missing ⚠️
...common/tedge_config_macros/impl/src/input/parse.rs	0.00%	1 Missing ⚠️
...mon/tedge_config_macros/impl/src/input/validate.rs	97.91%	0 Missing and 1 partial ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

github-actions · 2025-01-16T15:09:18Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
557	0	2	557	100	1h36m41.704073999s

Signed-off-by: James Rhodes <james.rhodes@cumulocity.com>

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

Examples: `tedge cert create --device-id foo` -> `device.id` is set to foo `tedge cert create --device-id bar c8y` -> `c8y.device.id` is set to bar `tedge cert create --device-id baz c8y --profile new` -> `c8y.profiles.new.device.id` is set to baz Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

* tedge connect c8y uses device.id directly. The CN from certificate is no longer used to determine device.id * tedge connect c8y returns an error if device.id mismatches the certificate's CN when auth method is certificate Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

tests/RobotFramework/tests/tedge/tedge_cert_create.robot

crates/core/tedge/src/error.rs

crates/core/tedge/src/cli/certificate/cli.rs

didier-wenzek · 2025-01-20T13:13:55Z

crates/core/tedge/src/cli/certificate/cli.rs

+                let key = profile.map(|name| name.to_string());
+                let c8y_dto = dto.c8y.try_get(key.as_deref(), "c8y")?;
+                if c8y_dto.device.id.is_some() {
+                    Err(MismatchedDeviceId {


Is this error case really useful?

It's used only in this function

and the error case for no device id at all is raised using anyhow!().

Yeah, I wanted to find a better solution here. Using anyhow!() is definitely fine, even I used it at very first draft. But it will cause repeating the same long error message multiple times in the code. Anyway, I don't like that I had to add MismatchedDeviceId under TEdgeError as well, so using anyhow!() may be better.

Simplified in 99620aa

I'm now surprised that there no more conflict detection at all, when the device id is provided on the command line and in the config, but with different values. Wouldn't be better to raise an error stressing the conflict?

Okay after having read #3326 (comment).

crates/core/tedge/src/cli/certificate/cli.rs

albinsuresh

The feature is working as expected. But, simplifying the device id validation done in the tedge cert commands as suggested by @didier-wenzek would go a long way in making that function comprehendible, and easy to review.

albinsuresh · 2025-01-21T08:36:44Z

crates/core/tedge/src/cli/certificate/cli.rs

                let cmd = CreateCertCmd {
-                    id,
+                    id: get_device_id(id, &config, &context.config_location, &cloud)?,


Although I don't have any objections to this, I'm a bit surprised to see this device.id match validation here. I was under the impression that a value provided in the cli would simply override the existing value in the config and any validation would happen only during tedge connect.

I was under the impression that a value provided in the cli would simply override the existing value in the config

I'm actually now reconsidering this. If user run like below:

tedge config set device.id foo tedge cert create --device-id bar

If cert/key doesn't exist, the command creates cert/key and set device.id to foo.

If cert/key already exists, the command returns an error (known behaviour). No overwriting either id or cert. So, it cannot mess up already existing cert.

Does it really hurt users to overwrite the device id by tedge cert although they configured before? I guess not. Also, the command tells device.id is set to foo.

In that case, error scenario is only there are no input id AND no config id.

any validation would happen only during tedge connect.

The validation required in tedge connect is a different level. It checks if CN matches the configured device.id (or profile version c8y.device.id / c8y.profiles.<profile>.device.id).

It can be simplified like this.

fn get_device_id( id: Option<String>, config: &TEdgeConfig, cloud: &Option<Cloud>, ) -> Result<String, anyhow::Error> { let config_id = config.device_id(cloud.as_ref()).ok(); match (id, config_id) { (None, None) => Err(anyhow!("No device ID is provided. Use `--device-id <name>` option to specify the device ID.")), (None, Some(config_id)) => Ok(config_id.into()), (Some(input_id), None) => Ok(input_id), (Some(input_id), Some(_config_id)) => Ok(input_id), }

IMO, overwriting is the simpler contract to enforce. Keeps things simple at the code level as well.

Simplified in 99620aa

albinsuresh · 2025-01-21T08:44:39Z

crates/core/tedge/src/cli/certificate/cli.rs

+    #[test_case("test", None, None, toml::toml!{
+    [device]
+        id = "test"
+    })]


Formatting suggestion:

Suggested change

#[test_case("test", None, None, toml::toml!{

[device]

id = "test"

})]

#[test_case(

"test",

None,

None,

toml::toml!{

[device]

id = "test"

}

)]

Same for the rest of the test cases as well.

Which formatter are you using and forces this? My IDE doesn't, it's a pain to do all manually 😅

That was just manually formatted as cargo fmt doesn't touch stuff defined inside macros (test_case in this case). I found that laset few test cases really hard to read, hence suggested this.

Addressed in 99620aa

albinsuresh · 2025-01-21T08:47:38Z

crates/core/tedge/src/cli/certificate/renew.rs

@@ -59,6 +59,8 @@ mod tests {
    use std::path::Path;


Wondering if it'd be safer to call set_device_id from this renew command execute() as well so that any configuration from an older version of tedge is updated when the cert is renewed with the newer version.

Why not 👍

Done in 56986f2

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

didier-wenzek

The code looks good and simpler thanks to the latest commits. However, I still need some time to test the updated cli.

crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs

didier-wenzek · 2025-01-21T16:25:57Z

crates/core/tedge/src/cli/certificate/cli.rs

+                let key = profile.map(|name| name.to_string());
+                let c8y_dto = dto.c8y.try_get(key.as_deref(), "c8y")?;
+                if c8y_dto.device.id.is_some() {
+                    Err(MismatchedDeviceId {


I'm now surprised that there no more conflict detection at all, when the device id is provided on the command line and in the config, but with different values. Wouldn't be better to raise an error stressing the conflict?

Okay after having read #3326 (comment).

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

didier-wenzek · 2025-01-21T17:22:24Z

docs/src/operate/c8y/smartrest-one.md

+Specify the external IDs of the SmartREST 1.0 templates as shown below:
+
+```sh
+sudo tedge config set c8y.smartrest1.templates template-1,template-2
+```


suggestion:

You have to register on the device all the SmartREST 1.0 templates you plan to use.

This can be done by providing a comma-separated list of the external IDs of the SmartREST 1.0 templates:

sudo tedge config set c8y.smartrest1.templates template-1,template-2

This can also be done by adding these template IDs one by one;

sudo tedge config add c8y.smartrest1.templates template-1 sudo tedge config add c8y.smartrest1.templates template-2

rina23q requested review from didier-wenzek, albinsuresh, jarhodes314, a team and reubenmiller as code owners January 10, 2025 23:33

rina23q requested a deployment to Test Pull Request January 10, 2025 23:33 — with GitHub Actions Waiting

didier-wenzek reviewed Jan 13, 2025

View reviewed changes

jarhodes314 requested a deployment to Test Pull Request January 13, 2025 16:00 — with GitHub Actions Waiting

jarhodes314 requested a deployment to Test Pull Request January 13, 2025 16:14 — with GitHub Actions Waiting

rina23q requested a deployment to Test Pull Request January 13, 2025 17:30 — with GitHub Actions Waiting

rina23q requested a deployment to Test Pull Request January 13, 2025 17:38 — with GitHub Actions Waiting

rina23q requested a deployment to Test Pull Request January 13, 2025 22:27 — with GitHub Actions Waiting

rina23q requested a deployment to Test Pull Request January 14, 2025 16:26 — with GitHub Actions Waiting

rina23q temporarily deployed to Test Pull Request January 16, 2025 14:37 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto January 16, 2025 14:53 — with GitHub Actions Inactive

rina23q force-pushed the feature/3242/writable-device-id-3 branch from 7dcb459 to 77e1ecd Compare January 16, 2025 15:53

rina23q temporarily deployed to Test Pull Request January 16, 2025 15:53 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto January 16, 2025 16:01 — with GitHub Actions Inactive

jarhodes314 and others added 7 commits January 17, 2025 13:53

Make device.id field private

83ff13d

Update darling to fix clippy issue

a212743

Signed-off-by: James Rhodes <james.rhodes@cumulocity.com>

Add a public function to get dto from TEdgeConfigLocation

2ce1e5a

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

feat: tedge connect validates if device.id is equals as cert's CN

f5d6ca0

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

test: Add robot test for tedge cert create command

34d0121

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

rina23q force-pushed the feature/3242/writable-device-id-3 branch from 77e1ecd to 247d7f8 Compare January 17, 2025 14:03

rina23q requested a deployment to Test Pull Request January 17, 2025 14:03 — with GitHub Actions Waiting

rina23q added 2 commits January 17, 2025 14:07

fixup! feat: tedge cert create writes device.id in tedge.toml

ed40433

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

fixup! Add a public function to get dto from TEdgeConfigLocation

bfc6e14

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

rina23q temporarily deployed to Test Pull Request January 17, 2025 14:09 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto January 20, 2025 08:32 — with GitHub Actions Inactive

didier-wenzek reviewed Jan 20, 2025

View reviewed changes

albinsuresh reviewed Jan 21, 2025

View reviewed changes

fixup! fixup! feat: tedge cert create writes device.id in tedge.toml

99620aa

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

rina23q requested a deployment to Test Pull Request January 21, 2025 13:44 — with GitHub Actions Waiting

fixup! test: Add robot test for tedge cert create command

bb25494

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

rina23q temporarily deployed to Test Pull Request January 21, 2025 15:13 — with GitHub Actions Inactive

rina23q had a problem deploying to Test Auto January 21, 2025 15:28 — with GitHub Actions Failure

rina23q added 2 commits January 21, 2025 16:52

tedge cert renew sets device.id

56986f2

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

fixup! feat: tedge cert create writes device.id in tedge.toml

94ad5f2

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

rina23q temporarily deployed to Test Pull Request January 21, 2025 15:55 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto January 21, 2025 16:34 — with GitHub Actions Inactive

didier-wenzek reviewed Jan 21, 2025

View reviewed changes

doc: add Smartrest 1.0 guide

8dd1021

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

rina23q requested a deployment to Test Pull Request January 21, 2025 17:09 — with GitHub Actions Waiting

fixup! refactor: support writable device ids

9d6dbdb

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

rina23q requested a deployment to Test Pull Request January 21, 2025 17:20 — with GitHub Actions Waiting

didier-wenzek reviewed Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support read-write device.id in tedge cert/connect #3326

feat: support read-write device.id in tedge cert/connect #3326

rina23q commented Jan 10, 2025 •

edited

Loading

didier-wenzek left a comment

didier-wenzek left a comment

rina23q commented Jan 13, 2025

didier-wenzek commented Jan 13, 2025

codecov bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

didier-wenzek Jan 20, 2025

rina23q Jan 20, 2025

rina23q Jan 21, 2025

didier-wenzek Jan 21, 2025

albinsuresh left a comment

albinsuresh Jan 21, 2025

rina23q Jan 21, 2025

rina23q Jan 21, 2025

albinsuresh Jan 21, 2025

rina23q Jan 21, 2025

albinsuresh Jan 21, 2025

rina23q Jan 21, 2025

albinsuresh Jan 21, 2025

rina23q Jan 21, 2025

albinsuresh Jan 21, 2025

rina23q Jan 21, 2025

rina23q Jan 21, 2025

didier-wenzek left a comment

didier-wenzek Jan 21, 2025

didier-wenzek Jan 21, 2025

feat: support read-write device.id in tedge cert/connect #3326

Are you sure you want to change the base?

feat: support read-write device.id in tedge cert/connect #3326

Conversation

rina23q commented Jan 10, 2025 • edited Loading

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

didier-wenzek left a comment

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

rina23q commented Jan 13, 2025

didier-wenzek commented Jan 13, 2025

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

github-actions bot commented Jan 16, 2025 • edited Loading

Robot Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albinsuresh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rina23q commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading