refactor: Untangle certificate request, creation and renewal #3268

didier-wenzek · 2024-11-26T21:31:20Z

Proposed changes

Prepare the integration with Cumulocity CA, by putting apart the code to create self-signed certificate, to create signing request and to renew a certificate.

These different tasks was implemented by a single structure using misc flags to drive checks and file generation.
Now each PEM operation is control by a specific flow.

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

#3248

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

codecov · 2024-11-26T21:41:57Z

Codecov Report

Attention: Patch coverage is 79.24528% with 33 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/core/tedge/src/cli/certificate/create.rs	82.92%	0 Missing and 14 partials ⚠️
crates/core/tedge/src/cli/certificate/cli.rs	40.00%	6 Missing ⚠️
...rates/core/tedge/src/cli/certificate/create_csr.rs	80.00%	0 Missing and 5 partials ⚠️
crates/core/tedge/src/cli/certificate/renew.rs	80.76%	0 Missing and 5 partials ⚠️
crates/common/certificate/src/lib.rs	81.25%	0 Missing and 3 partials ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

github-actions · 2024-11-26T21:52:44Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
527	0	2	527	100	1h31m48.934624s

didier-wenzek · 2024-11-27T09:29:34Z

crates/common/certificate/src/lib.rs


        let not_after = not_before + Duration::days(config.validity_period_days.into());
        params.not_before = not_before;
        params.not_after = not_after;

-        params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); // IsCa::SelfSignedOnly is rejected by C8Y


This is the change failing the system tests.

I forgot to inject this back to self-signed certificate creation.

Fixed by 950ebcb

Bravo555 · 2024-11-27T13:18:19Z

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

+/// Create self-signed device certificate and signing request
 pub struct CreateCertCmd {
    /// The device identifier
    pub id: String,

-    /// The path where the device certificate will be stored
+    /// The path where the device certificate / request will be stored
    pub cert_path: Utf8PathBuf,

    /// The path where the device private key will be stored
    pub key_path: Utf8PathBuf,

-    /// The path where the device CSR file will be stored
-    pub csr_path: Option<Utf8PathBuf>,
-
-    /// The component that is configured to host the MQTT bridge logic
-    pub bridge_location: BridgeLocation,
+    /// The owner of the private key
+    pub user: String,
+    pub group: String,
 }


question: if CreateCertCmd creates certificate signing requests, what does the CreateCsrCmd do?

Is this a leftover of some previous changes, or am I not understanding something?

Oops. This is a leftover. CreateCertCmd is now only used to create self signed certificate.

This now confusing comment has been added during an intermediate step and has to be removed.

Fixed: 4cf7c88

And improved: 0de8244

albinsuresh

LGTM.

albinsuresh · 2024-11-28T03:44:09Z

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


-        create_cmd.renew_test_certificate(config)
+        override_public_key(cert_path, cert.certificate_pem_string()?)


The explicit removal of the certificate in line. 40 seems redundant now, because of this "override" mechanism. But not harm either.

I thought the same but it is not. Indeed, a certificate is in practice write protected (0o444).

Signed-off-by: Didier Wenzek <[email protected]>

This field was used to hack the CreateCertCmd making it creating a CSR instead of a self-signed certificate. Signed-off-by: Didier Wenzek <[email protected]>

Previously, this user was derived from the bridge type introducing an unrelated domain, even if in practice thin-edge mainly uses certificates to authenticate the bridge. Signed-off-by: Didier Wenzek <[email protected]>

Signed-off-by: Didier Wenzek <[email protected]>

Two fields were used to pass a device id to a CSR command, one being only used if the other was not suitable. Now the device id is provided by the caller. Signed-off-by: Didier Wenzek <[email protected]>

didier-wenzek requested review from albinsuresh, jarhodes314 and rina23q as code owners November 26, 2024 21:31

didier-wenzek temporarily deployed to Test Pull Request November 26, 2024 21:31 — with GitHub Actions Inactive

didier-wenzek added refactoring Developer value theme:certificates labels Nov 26, 2024

didier-wenzek had a problem deploying to Test Auto November 26, 2024 21:37 — with GitHub Actions Failure

didier-wenzek commented Nov 27, 2024

View reviewed changes

didier-wenzek requested review from reubenmiller, Bravo555, gligorisaev and a team as code owners November 27, 2024 10:38

didier-wenzek temporarily deployed to Test Pull Request November 27, 2024 10:38 — with GitHub Actions Inactive

didier-wenzek had a problem deploying to Test Auto November 27, 2024 10:44 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Auto November 27, 2024 11:02 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Auto November 27, 2024 12:13 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Auto November 27, 2024 12:50 — with GitHub Actions Failure

didier-wenzek force-pushed the refactor/simplify_csr_creation branch from ddff773 to 950ebcb Compare November 27, 2024 13:13

didier-wenzek temporarily deployed to Test Pull Request November 27, 2024 13:13 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto November 27, 2024 13:19 — with GitHub Actions Inactive

Bravo555 reviewed Nov 27, 2024

View reviewed changes

didier-wenzek temporarily deployed to Test Pull Request November 27, 2024 14:10 — with GitHub Actions Inactive

didier-wenzek had a problem deploying to Test Auto November 27, 2024 14:17 — with GitHub Actions Failure

didier-wenzek temporarily deployed to Test Pull Request November 27, 2024 14:40 — with GitHub Actions Inactive

Bravo555 approved these changes Nov 27, 2024

View reviewed changes

didier-wenzek had a problem deploying to Test Auto November 27, 2024 14:46 — with GitHub Actions Failure

didier-wenzek temporarily deployed to Test Auto November 27, 2024 16:04 — with GitHub Actions Inactive

albinsuresh approved these changes Nov 28, 2024

View reviewed changes

didier-wenzek added 6 commits November 28, 2024 09:05

Remove duplicated code

4a2eb41

Signed-off-by: Didier Wenzek <[email protected]>

Untangle impl for cert/csr creation/renewal

3047ac2

Signed-off-by: Didier Wenzek <[email protected]>

Remove misleading field: CreateCertCmd::csr_path

7ebf3c2

This field was used to hack the CreateCertCmd making it creating a CSR instead of a self-signed certificate. Signed-off-by: Didier Wenzek <[email protected]>

Directly provice the cert owner to CreateCertCmd

22fab3c

Previously, this user was derived from the bridge type introducing an unrelated domain, even if in practice thin-edge mainly uses certificates to authenticate the bridge. Signed-off-by: Didier Wenzek <[email protected]>

Fully untangle certificate request, creation and renewal

58f8dbb

Signed-off-by: Didier Wenzek <[email protected]>

Make CreateCsrCmd fields less confusing

88bcaad

Two fields were used to pass a device id to a CSR command, one being only used if the other was not suitable. Now the device id is provided by the caller. Signed-off-by: Didier Wenzek <[email protected]>

didier-wenzek force-pushed the refactor/simplify_csr_creation branch from 0de8244 to 88bcaad Compare November 28, 2024 08:10

didier-wenzek temporarily deployed to Test Pull Request November 28, 2024 08:10 — with GitHub Actions Inactive

didier-wenzek had a problem deploying to Test Auto November 28, 2024 08:16 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Auto November 28, 2024 08:38 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Auto November 28, 2024 09:34 — with GitHub Actions Failure

didier-wenzek temporarily deployed to Test Auto November 28, 2024 09:56 — with GitHub Actions Inactive

didier-wenzek enabled auto-merge November 28, 2024 09:56

didier-wenzek added this pull request to the merge queue Nov 28, 2024

Merged via the queue into thin-edge:main with commit 649e4ed Nov 28, 2024
33 checks passed

didier-wenzek deleted the refactor/simplify_csr_creation branch November 28, 2024 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Untangle certificate request, creation and renewal #3268

refactor: Untangle certificate request, creation and renewal #3268

didier-wenzek commented Nov 26, 2024

codecov bot commented Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading

didier-wenzek Nov 27, 2024

didier-wenzek Nov 27, 2024

Bravo555 Nov 27, 2024

didier-wenzek Nov 27, 2024

didier-wenzek Nov 27, 2024

didier-wenzek Nov 27, 2024

albinsuresh left a comment

albinsuresh Nov 28, 2024 •

edited

Loading

didier-wenzek Nov 28, 2024


		create_cmd.renew_test_certificate(config)
		override_public_key(cert_path, cert.certificate_pem_string()?)

refactor: Untangle certificate request, creation and renewal #3268

refactor: Untangle certificate request, creation and renewal #3268

Conversation

didier-wenzek commented Nov 26, 2024

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

github-actions bot commented Nov 26, 2024 • edited Loading

Robot Results

didier-wenzek Nov 27, 2024

Choose a reason for hiding this comment

didier-wenzek Nov 27, 2024

Choose a reason for hiding this comment

Bravo555 Nov 27, 2024

Choose a reason for hiding this comment

didier-wenzek Nov 27, 2024

Choose a reason for hiding this comment

didier-wenzek Nov 27, 2024

Choose a reason for hiding this comment

didier-wenzek Nov 27, 2024

Choose a reason for hiding this comment

albinsuresh left a comment

Choose a reason for hiding this comment

albinsuresh Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

didier-wenzek Nov 28, 2024

Choose a reason for hiding this comment

codecov bot commented Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading

albinsuresh Nov 28, 2024 •

edited

Loading