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.

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 SmartREST1.0 support doc
Update tedge cert doc
Update tedge conect 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 94.24379% with 51 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/core/tedge/src/cli/certificate/cli.rs	73.58%	23 Missing and 5 partials ⚠️
.../tedge_config/src/tedge_config_cli/tedge_config.rs	84.09%	5 Missing and 2 partials ⚠️
crates/core/tedge/src/cli/connect/command.rs	90.27%	2 Missing and 5 partials ⚠️
crates/core/tedge/src/cli/certificate/create.rs	95.31%	0 Missing and 3 partials ⚠️
...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	1h32m35.761493999s

jarhodes314

From a quick test this seems to be working well. It will be much easier to use the CLI with the persisted device id.

jarhodes314 · 2025-01-16T17:58:32Z

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

+    #[test_case("c8y-foo-test", Some("c8y-foo-test"), Some(CloudArg::C8y{ profile: Some("foo".parse().unwrap()) }), toml::toml!{
+    [device]
+        id = "test"
+    [c8y.device]
+        id = "c8y-test"
+    })]


Does this case actually occur in practice? If someone hasn't got any configurations for a profile, the profile doesn't exist and that will cause an error elsewhere in tedge cert create.

In practice, tedge cert create will return an error because no profile for "foo" exists. So the following toml fie is more realistic. Though the result get_device_id() is the same.

[device] id = "test" [c8y.device] id = "c8y-test" [c8y.profiles.foo.device] cert_path = "/path/to/cert.pem" key_path = "/path/to/key.pem"

jarhodes314 · 2025-01-16T18:00:59Z

crates/core/tedge/src/error.rs

+        r#"The given device ID '{input_id}' doesn't match the one in the config '{config_id}'.
+    Run `tedge config unset {writable_key}` first to unset the device ID."#
+    )]
+    UnmatchedDeviceId {


Suggested change

UnmatchedDeviceId {

MismatchedDeviceId {

fixed in ed40433

jarhodes314 · 2025-01-16T18:08:13Z

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

@@ -102,6 +102,10 @@ impl TEdgeConfigLocation {
        Ok(TEdgeConfig::from_dto(&dto, self))
    }

+    pub fn load_dto_with_file_and_env(&self) -> Result<TEdgeConfigDto, TEdgeConfigError> {


Suggested change

pub fn load_dto_with_file_and_env(&self) -> Result<TEdgeConfigDto, TEdgeConfigError> {

pub fn load_dto_from_toml_and_env(&self) -> Result<TEdgeConfigDto, TEdgeConfigError> {

fixed in bfc6e14

Signed-off-by: James Rhodes <[email protected]>

Signed-off-by: Rina Fujino <[email protected]>

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 <[email protected]>

Signed-off-by: Rina Fujino <[email protected]>

* 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 <[email protected]>

Signed-off-by: Rina Fujino <[email protected]>

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

reubenmiller removed theme:c8y Theme: Cumulocity related topics theme:certificates labels Jan 16, 2025

jarhodes314 reviewed Jan 16, 2025

View reviewed changes

jarhodes314 mentioned this pull request Jan 17, 2025

refactor: support writable device ids #3318

Closed

15 tasks

jarhodes314 and others added 9 commits January 17, 2025 13:46

refactor: support writable device ids

469e250

Make defaults work

6d77448

Make device.id field private

83ff13d

Update darling to fix clippy issue

a212743

Signed-off-by: James Rhodes <[email protected]>

Add a public function to get dto from TEdgeConfigLocation

2ce1e5a

Signed-off-by: Rina Fujino <[email protected]>

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

f5d6ca0

Signed-off-by: Rina Fujino <[email protected]>

test: Add robot test for tedge cert create command

34d0121

Signed-off-by: Rina Fujino <[email protected]>

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 <[email protected]>

fixup! Add a public function to get dto from TEdgeConfigLocation

bfc6e14

Signed-off-by: Rina Fujino <[email protected]>

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

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

jarhodes314 left a comment

jarhodes314 Jan 16, 2025

rina23q Jan 17, 2025

jarhodes314 Jan 16, 2025

rina23q Jan 17, 2025

jarhodes314 Jan 16, 2025

rina23q Jan 17, 2025

	pub fn load_dto_with_file_and_env(&self) -> Result<TEdgeConfigDto, TEdgeConfigError> {
	pub fn load_dto_from_toml_and_env(&self) -> Result<TEdgeConfigDto, TEdgeConfigError> {

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

jarhodes314 left a comment

Choose a reason for hiding this comment

jarhodes314 Jan 16, 2025

Choose a reason for hiding this comment

rina23q Jan 17, 2025

Choose a reason for hiding this comment

jarhodes314 Jan 16, 2025

Choose a reason for hiding this comment

rina23q Jan 17, 2025

Choose a reason for hiding this comment

jarhodes314 Jan 16, 2025

Choose a reason for hiding this comment

rina23q Jan 17, 2025

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