feat: support read-write device.id in tedge cert/connect by rina23q · Pull Request #3326 · thin-edge/thin-edge.io

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.~~ Reverted.
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.~~ Reverted.
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 97.20745% with 21 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
.../tedge_config/src/tedge_config_cli/tedge_config.rs	30.00%	0 Missing and 7 partials ⚠️
crates/core/tedge/src/cli/connect/command.rs	91.86%	2 Missing and 5 partials ⚠️
crates/core/tedge/src/cli/certificate/cli.rs	96.29%	1 Missing and 1 partial ⚠️
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 ⚠️
crates/core/tedge/src/cli/certificate/create.rs	88.88%	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
560	0	2	560	100	1h31m30.663062999s

rina23q

I was trying to fix the error message that @didier-wenzek highlighted. There are some points discovered why it's not easy.

rina23q · 2025-01-27T15:04:21Z

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

-        .subject_common_name()
-        .map_err(|err| cert_error_into_config_error(ReadableKey::DeviceId.to_cow_str(), err))?;
+    let pem = PemCertificate::from_pem_file(cert_path).map_err(|err| {
+        cert_error_into_config_error(ReadableKey::DeviceId.to_cow_str(), err, cert_path)


This device_id_from_cert() is called by all device id default functions (device_id(), c8y_device_id(), az_device_id(), and aws_device_id(). However, here gives ReadableKey::DeviceId as key. This is not 100% correct. It can be ReadableKey::C8yDeviceId(None), ReadableKey::AzDeviceId(Some("abc")) and so on.

The difficulty is, the default functions cannot know which profile is used. That's why it is impossible to parse the precise ReadableKey to this fuction from the default device id functions.

So the issue to improve the error message is that device_id_from_cert has no clue on the actual key. But can this key be passed as a parameter with the appropriate value by the caller (i.e device_id(), c8y_device_id(), az_device_id(), and aws_device_id()`) ?

So the issue to improve the error message is that device_id_from_cert has no clue on the actual key.

Right.

But can this key be passed as a parameter with the appropriate value by the caller (i.e device_id(), c8y_device_id(), az_device_id(), and aws_device_id()) ?

Also no unfortunately. Look at this function. This function is called by the macro. No chance to get the profile.

thin-edge.io/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs

Lines 1375 to 1383 in 91eb9bd

fn c8y_device_id(

c8y_device: &TEdgeConfigReaderC8yDevice,

dto_value: &OptionalConfig<String>,

) -> Result<String, ReadError> {

match dto_value.or_none() {

Some(id) => Ok(id.clone()),

None => device_id_from_cert(&c8y_device.cert_path),

}

}

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

didier-wenzek

Since improving error messages and hints when the device id cannot be derived from the config, I propose to give up on this point - at least for now, and to focus on the main point of this PR: the ability to configure a device id with or with a certificate.

Configuring a device to use several cloud profiles each with its specific certificate is working, although this is for now a complicated and error prone task. We can improve the user experience in a second step with the broader scope of configuring multi-profile connection . @reubenmiller do you agree on that?

didier-wenzek · 2025-01-27T16:15:21Z

crates/core/tedge/tests/main.rs

        remove_cmd.assert().success();

-        // We start we no certificate, hence no device id
+        // We start with no certificate, hence no device id


This test has to be updated with the new error message.

// We start with no certificate, hence no device id get_device_id_cmd .assert() .failure() .stderr(predicate::str::contains("The device certificate cannot be read from"));

didier-wenzek · 2025-01-27T16:38:27Z

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

-        .subject_common_name()
-        .map_err(|err| cert_error_into_config_error(ReadableKey::DeviceId.to_cow_str(), err))?;
+    let pem = PemCertificate::from_pem_file(cert_path).map_err(|err| {
+        cert_error_into_config_error(ReadableKey::DeviceId.to_cow_str(), err, cert_path)


So the issue to improve the error message is that device_id_from_cert has no clue on the actual key. But can this key be passed as a parameter with the appropriate value by the caller (i.e device_id(), c8y_device_id(), az_device_id(), and aws_device_id()`) ?

didier-wenzek

Approved, although the user experience could be simpler (see here for the complicate list of commands required when the use case is not the regular one: #3326 (comment)).

didier-wenzek · 2025-01-28T08:59:32Z

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

+    /// The tedge.toml file location, required to access to TEdgeConfigDto
+    pub config_location: TEdgeConfigLocation,


Okayish as this is here only to provide a key path in error messages.

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

Also, small refactoring in tedge cert, makes --device-id optional tedge connect * tedge connect <cloud> fails when the common name from the device certificate does not match the device id configured in tedge config. * this validation applies only when auth_method is certificate tedge cert * tedge cert create can be run without --device-id if device.id is set Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

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

This reverts commit ad2209f.

* update the smartrest_one test to consume device.id directly * add tests for tedge connect for CN/device id validation * add tests for tedge cert command Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

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

albinsuresh

The feature is working fine for the most common cases. The error messages could be improved in some cases though, but don't need to block this PR anymore for that. One simple example of such an error message is when trying to create a certificate with a different --device-id from what is set as device.id:

Error: missing configuration parameter

Caused by:
    `--device-id` option conflicts with tedge config settings.
    Configured value: 'albin_test', but input: 'albin_latest'
    
    Please either update the configuration using `tedge config set <key> <new_id>`
    or provide the correct value with the `--device-id` option.

The highlighted error is Error: missing configuration parameter whereas this is a pure case of "conflicting configuration". Even though that generic error is misleading, th rest of the message that suggests ways to fix is clear enough for the user. So, this is fine. If I spot more inconsistencies like this for the common use-cases, I'll compile and share it separately, even after this PR is merged.

rina23q · 2025-01-28T14:59:08Z

The highlighted error is Error: missing configuration parameter whereas this is a pure case of "conflicting configuration".

I'm actually aware of that. However, since this error message comes from very high level, I decided not to touch. Here the message is coming from.

thin-edge.io/crates/core/tedge/src/main.rs

Line 80 in e44a0ff

.with_context(|| "missing configuration parameter")?;

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

rina23q had a problem deploying to Test Pull Request January 10, 2025 23:33 — with GitHub Actions Failure

didier-wenzek reviewed Jan 13, 2025

View reviewed changes

jarhodes314 had a problem deploying to Test Pull Request January 13, 2025 16:00 — with GitHub Actions Failure

jarhodes314 had a problem deploying to Test Pull Request January 13, 2025 16:14 — with GitHub Actions Failure

rina23q had a problem deploying to Test Pull Request January 13, 2025 17:30 — with GitHub Actions Failure

rina23q had a problem deploying to Test Pull Request January 13, 2025 17:38 — with GitHub Actions Failure

rina23q had a problem deploying to Test Pull Request January 13, 2025 22:27 — with GitHub Actions Failure

rina23q had a problem deploying to Test Pull Request January 14, 2025 16:26 — with GitHub Actions Failure

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

rina23q had a problem deploying to Test Pull Request January 27, 2025 14:58 — with GitHub Actions Failure

rina23q commented Jan 27, 2025

View reviewed changes

rina23q force-pushed the feature/3242/writable-device-id-3 branch from 953fcaa to 91eb9bd Compare January 27, 2025 15:10

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

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

didier-wenzek reviewed Jan 27, 2025

View reviewed changes

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

rina23q had a problem deploying to Test Auto January 27, 2025 17:53 — with GitHub Actions Failure

didier-wenzek approved these changes Jan 28, 2025

View reviewed changes

jarhodes314 and others added 9 commits January 28, 2025 13:38

refactor: support writable device ids

c3fa4c3

Make defaults work

95722fa

Make device.id field private

501162f

Update darling to fix clippy issue

79e07c4

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

tedge cert create prints the contents of tedge cert show

75e13b2

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

Revert "Hide cloud specific device configurations"

5adf4d4

This reverts commit ad2209f.

test: system tests update related to read-write device id

a004817

* update the smartrest_one test to consume device.id directly * add tests for tedge connect for CN/device id validation * add tests for tedge cert command Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

doc: add Smartrest 1.0 guide

2f61a0a

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

rina23q force-pushed the feature/3242/writable-device-id-3 branch from 9a3587d to 2f61a0a Compare January 28, 2025 13:58

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

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

albinsuresh approved these changes Jan 28, 2025

View reviewed changes

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

rina23q mentioned this pull request Jan 28, 2025

feat: read device.id from credentials.toml when basic auth mode #3242

Closed

11 tasks

rina23q added this pull request to the merge queue Jan 28, 2025

Merged via the queue into thin-edge:main with commit 74947b0 Jan 28, 2025

rina23q mentioned this pull request Jan 28, 2025

allow setting the device.id when using c8y basic auth #3240

Closed

rina23q deleted the feature/3242/writable-device-id-3 branch August 8, 2025 11:58

	fn c8y_device_id(
	c8y_device: &TEdgeConfigReaderC8yDevice,
	dto_value: &OptionalConfig<String>,
	) -> Result<String, ReadError> {
	match dto_value.or_none() {
	Some(id) => Ok(id.clone()),
	None => device_id_from_cert(&c8y_device.cert_path),
	}
	}

		/// The tedge.toml file location, required to access to TEdgeConfigDto
		pub config_location: TEdgeConfigLocation,

Conversation

rina23q commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

rina23q commented Jan 13, 2025

Uh oh!

didier-wenzek commented Jan 13, 2025

Uh oh!

codecov bot commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Robot Results

Uh oh!

rina23q left a comment

Choose a reason for hiding this comment

Uh oh!

rina23q Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

rina23q Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albinsuresh left a comment

Choose a reason for hiding this comment

Uh oh!

rina23q commented Jan 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rina23q commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

rina23q Jan 27, 2025 •

edited

Loading

didier-wenzek Jan 28, 2025 •

edited

Loading