fix: device.id is derived from device certificate if certificate exists by rina23q · Pull Request #3372 · thin-edge/thin-edge.io

rina23q · 2025-01-31T13:21:24Z

Proposed changes

device.id always uses the value from a device certificate. If no certificate exists, it returns a value from the tedge.toml file. Both don't exist, finally it returns an error.
tedge connect doesn't validate if device certficate's CN matches the value from the tedge.toml file. It just consume the value from device.id.
tedge cert create creates a cert, which CN is determined by this order. 1) the value given by the --device-id option, 2) the value from tedge config get <corresponding_device_id_key>

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

#3369

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

reubenmiller · 2025-01-31T13:29:45Z

tedge cert create always creates a cert with the value given by the --device-id option. It doesn't matter if user configure device.id in the tedge config settings explicitly.

The phrasing sounds a little strange here. The precedence of source of the device.id value just follows the cli convention of: argument => env variable => configuration, where the first value present is used.

codecov · 2025-01-31T13:33:01Z

Codecov Report

Attention: Patch coverage is 83.33333% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
.../tedge_config/src/tedge_config_cli/tedge_config.rs	81.81%	4 Missing ⚠️
crates/core/tedge/src/cli/certificate/cli.rs	89.47%	1 Missing and 1 partial ⚠️
crates/core/tedge/src/cli/connect/command.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

github-actions · 2025-01-31T13:43:56Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
570	0	3	570	100	1h32m5.072187s

rina23q · 2025-01-31T14:07:35Z

tedge cert create always creates a cert with the value given by the --device-id option. It doesn't matter if user configure device.id in the tedge config settings explicitly.

The phrasing sounds a little strange here. The precedence of source of the device.id value just follows the cli convention of: argument => env variable => configuration, where the first value present is used.

Yeah I fixed the phrasing. Prioritizing the argument is what this PR fixed. Note that the order of env variable => configuration is the responsibility of the tedge config settings and should be this order for all configs.

didier-wenzek

Okay we have now a more consistent behavior regarding device id / tedge config / tedge connect.

However, it would be good to add 4 independent tests matching exactly the 4 cases described by #3369. The current tests are useful but a bit complicated to follow because very dependent of the whole sequence of check vs tear-down steps.

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

didier-wenzek · 2025-01-31T15:14:30Z

tests/RobotFramework/tests/tedge/tedge_cert_create.robot

+    ${output}=    Execute Command    tedge config get c8y.device.id    exp_exit_code=1
+    ${output}=    Execute Command    tedge config get c8y.device.id --profile foo    exp_exit_code=1


I was expecting a cascading behavior here: i.e. if there is no specific device id for a cloud profile, then the general device id is used.

~~Is this because explicit paths have set for the misc cloud profiles?~~

I got it: this is because no device id has been set in the config file.

I would be good to have a more consistent cascading behavior.

I would be good to have a more consistent cascading behavior.

If no specific device.cert_path is set for a cloud profile, the result here will be different.

# Remove specific paths $ tedge config unset c8y.device.cert_path $ tedge config unset c8y.device.cert_path --device foo # create a new cert for the generic one $ tedge config cert create --device-id input # get device ids $ tedge config get device.id input $ tedge config get c8y.device.id input $ tedge config get c8y.device.id --profile foo input

This behaviour also caught me by surprise as setting a cloud profile certificate path changes the device.id value returned, even if the cloud profile certificate does not exist...though after reviewing it, it seems to make sense as the act of creating a different cert path for a cloud profile, will decouple it from the generic/default certificate...so it makes it easier for system users to control the device.id inheritance without forcing them to create the certificate.

tests/RobotFramework/tests/tedge/tedge_cert_create.robot

rina23q · 2025-01-31T17:39:06Z

However, it would be good to add 4 independent tests matching exactly the 4 cases described by #3369. The current tests are useful but a bit complicated to follow because very dependent of the whole sequence of check vs tear-down steps.

Added a new suite (tests/RobotFramework/tests/tedge/device_id.robot) to cover all the four cases in ef49eeb

didier-wenzek

Approved. Thanks for taking the time to make more consistent how the device id is used across the misc tedge cert / config and connect API.

didier-wenzek · 2025-01-31T18:01:24Z

tests/RobotFramework/tests/tedge/tedge_cert_create.robot

+    ### All cert/key paths are the default
+    ${output}=    Execute Command    tedge cert create --device-id input
+    Should Contain    ${output}    CN=input
+    Validate device IDs    input    input    input
+    Execute Command    tedge cert remove


This is the cascading behavior I was expecting.

tests/RobotFramework/tests/tedge/device_id.robot

reubenmiller

Approved (only 1 minor test case renaming required)

Overall it would benefit from splitting the "Device ID derivation" into individual test cases (and possibly moving it to a new Test Suite so it can use a single "Suite Setup" and reuse the same device for all test cases with suitable cleanup logic between tests)...however this is not a blocker for merging the PR.

tests/RobotFramework/tests/tedge/device_id/device_id_usecases.robot

tests/RobotFramework/tests/tedge/device_id/device_id_derivation.robot

* device.id always uses a value from device certificate. If no certificate exists, returns a value from tedge.toml file. Both don't exist, finally it returns an error. * tedge connect doesn't validate if device certficate's CN matches the value from tedge.toml file. It just consume the value from device.id. * The CN of `tedge cert create` is determined by this order: 1) the value of --device-id option 2) the value of tedge config get <device_id_key> Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

rina23q requested review from a team, albinsuresh, didier-wenzek and jarhodes314 as code owners January 31, 2025 13:21

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

rina23q temporarily deployed to Test Auto January 31, 2025 13:29 — with GitHub Actions Inactive

didier-wenzek reviewed Jan 31, 2025

View reviewed changes

reubenmiller reviewed Jan 31, 2025

View reviewed changes

tests/RobotFramework/tests/tedge/tedge_cert_create.robot Outdated Show resolved Hide resolved

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

rina23q temporarily deployed to Test Auto January 31, 2025 17:44 — with GitHub Actions Inactive

didier-wenzek approved these changes Jan 31, 2025

View reviewed changes

reubenmiller reviewed Feb 3, 2025

View reviewed changes

tests/RobotFramework/tests/tedge/device_id.robot Outdated Show resolved Hide resolved

reubenmiller approved these changes Feb 3, 2025

View reviewed changes

reubenmiller added theme:cli Theme: cli related topics theme:certificates Theme: Device certificate topics and removed theme:certificates Theme: Device certificate topics labels Feb 3, 2025

rina23q temporarily deployed to Test Pull Request February 3, 2025 11:13 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto February 3, 2025 11:20 — with GitHub Actions Inactive

rina23q had a problem deploying to Test Pull Request February 3, 2025 11:22 — with GitHub Actions Failure

rina23q force-pushed the fix/3369/tedge-connect-always-use-cn-as-device-id branch from a93847a to 88e81bc Compare February 3, 2025 11:32

rina23q temporarily deployed to Test Pull Request February 3, 2025 11:33 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto February 3, 2025 12:03 — with GitHub Actions Inactive

reubenmiller reviewed Feb 3, 2025

View reviewed changes

tests/RobotFramework/tests/tedge/device_id/device_id_usecases.robot Outdated Show resolved Hide resolved

reubenmiller reviewed Feb 3, 2025

View reviewed changes

tests/RobotFramework/tests/tedge/device_id/device_id_derivation.robot Outdated Show resolved Hide resolved

rina23q force-pushed the fix/3369/tedge-connect-always-use-cn-as-device-id branch from 88e81bc to 83a82b2 Compare February 3, 2025 13:03

rina23q temporarily deployed to Test Pull Request February 3, 2025 13:03 — with GitHub Actions Inactive

rina23q had a problem deploying to Test Auto February 3, 2025 13:10 — with GitHub Actions Failure

rina23q temporarily deployed to Test Auto February 3, 2025 14:13 — with GitHub Actions Inactive

rina23q added this pull request to the merge queue Feb 3, 2025

Merged via the queue into thin-edge:main with commit f236a10 Feb 3, 2025

rina23q deleted the fix/3369/tedge-connect-always-use-cn-as-device-id branch August 8, 2025 11:58

		${output}= Execute Command tedge config get c8y.device.id exp_exit_code=1
		${output}= Execute Command tedge config get c8y.device.id --profile foo exp_exit_code=1

Conversation

rina23q commented Jan 31, 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!

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

Uh oh!

Uh oh!

codecov bot commented Jan 31, 2025

Codecov Report

Uh oh!

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

Uh oh!

Robot Results

Uh oh!

rina23q commented Jan 31, 2025

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

didier-wenzek Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

rina23q Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

reubenmiller Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

reubenmiller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rina23q commented Jan 31, 2025 •

edited

Loading

reubenmiller commented Jan 31, 2025 •

edited

Loading

github-actions bot commented Jan 31, 2025 •

edited

Loading

rina23q commented Jan 31, 2025 •

edited

Loading