Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Parse root certificate from either file or directory in tedge cert upload #2953

Merged
merged 17 commits into from
Jul 26, 2024
Merged

fix: Parse root certificate from either file or directory in tedge cert upload #2953

merged 17 commits into from
Jul 26, 2024

Conversation

jarhodes314
Copy link
Contributor

@jarhodes314 jarhodes314 commented Jun 19, 2024 •
edited
Loading

Proposed changes

This PR ensures the c8y.root_cert_path setting is respected in all places tedge makes an HTTP connection to Cumulocity. The setting previously existed but wasn't widely respected, meaning that operations such as retrieving a configuration file or remote access didn't work unless the Cumulocity root certificate was trusted by the system. Following these changes, it should be sufficient to set c8y.root_cert_path to point to the Cumulocity root certificate and all operations as well as commands like tedge cert upload should work.

This also fixes a separate issue with tedge cert upload where it did respect the setting, but always read the configuration from /etc/tedge/tedge.toml, rather than using the provided custom configuration directory.

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

#2951

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

@jarhodes314 jarhodes314 temporarily deployed to Test Pull Request June 19, 2024 11:48 — with GitHub Actions Inactive
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 19, 2024 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
484 0 2 484 100 1h25m35.181485s

@jarhodes314 jarhodes314 temporarily deployed to Test Pull Request June 19, 2024 15:43 — with GitHub Actions Inactive
@jarhodes314 jarhodes314 temporarily deployed to Test Pull Request June 20, 2024 11:02 — with GitHub Actions Inactive
@codecov Codecov
Copy link

codecov bot commented Jun 20, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 67.85714% with 108 lines in your changes missing coverage. Please review.

Project coverage is 78.2%. Comparing base (706cdea) to head (e637b25).
Report is 3 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
crates/common/axum_tls/src/acceptor.rs 96.4% <ø> (ø)
crates/common/axum_tls/src/files.rs 90.6% <100.0%> (ø)
crates/common/certificate/src/lib.rs 89.7% <ø> (ø)
crates/common/upload/src/upload.rs 89.1% <100.0%> (+0.4%) ⬆️
crates/core/plugin_sm/src/plugin_manager.rs 67.0% <100.0%> (+0.1%) ⬆️
crates/core/tedge/src/cli/certificate/error.rs 36.3% <ø> (ø)
...core/tedge_agent/src/file_transfer_server/actor.rs 81.7% <ø> (ø)
crates/extensions/c8y_auth_proxy/src/server.rs 89.5% <100.0%> (-0.3%) ⬇️
crates/extensions/c8y_http_proxy/src/tests.rs 93.5% <100.0%> (+<0.1%) ⬆️
...rates/extensions/tedge_downloader_ext/src/tests.rs 97.2% <100.0%> (+<0.1%) ⬆️
... and 22 more

... and 4 files with indirect coverage changes

@reubenmiller reubenmiller changed the title Parse root certificate from either file or directory in tedge cert upload fix: Parse root certificate from either file or directory in tedge cert upload Jun 21, 2024
@reubenmiller reubenmiller added theme:c8y Theme: Cumulocity related topics theme:configuration Theme: Configuration management labels Jun 21, 2024
@gligorisaev
Copy link
Contributor

gligorisaev commented Jul 3, 2024 •
edited
Loading

@jarhodes314 I created test case that should cover the bug #2951, but not being yet able to check if it will pass after installing the final build

@reubenmiller reubenmiller mentioned this pull request Jul 18, 2024
Merged
11 tasks
@jarhodes314 jarhodes314 temporarily deployed to Test Pull Request July 19, 2024 08:11 — with GitHub Actions Inactive
@jarhodes314 jarhodes314 temporarily deployed to Test Pull Request July 22, 2024 10:10 — with GitHub Actions Inactive
@jarhodes314 jarhodes314 marked this pull request as ready for review July 22, 2024 11:13
@jarhodes314 jarhodes314 requested review from gligorisaev and a team as code owners July 22, 2024 11:41
@jarhodes314 jarhodes314 temporarily deployed to Test Pull Request July 22, 2024 11:41 — with GitHub Actions Inactive
@jarhodes314 jarhodes314 temporarily deployed to Test Pull Request July 24, 2024 20:33 — with GitHub Actions Inactive
@albinsuresh albinsuresh temporarily deployed to Test Pull Request July 25, 2024 06:19 — with GitHub Actions Inactive
albinsuresh
albinsuresh approved these changes Jul 25, 2024
View reviewed changes
Copy link
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

jarhodes314 and others added 17 commits July 26, 2024 10:38
@jarhodes314
Parse root certificate from either file or directory in tedge cert up…
f36ed3e 
…load

Signed-off-by: James Rhodes <[email protected]>
@jarhodes314
Respect config-dir in tedge cert upload
de1919b 
Signed-off-by: James Rhodes <[email protected]>
@jarhodes314
Create a mechanism for deriving root certificates from tedge_config
a7acdb4 
Signed-off-by: James Rhodes <[email protected]>
@jarhodes314
Use RootCertClient in uploader
c5393d9 
Signed-off-by: James Rhodes <[email protected]>
@jarhodes314
Update tests to use
c2850df 
Signed-off-by: James Rhodes <[email protected]>
@jarhodes314
Add support for hyper clients to use root cert path
13ffb83 
Signed-off-by: James Rhodes <[email protected]>
@jarhodes314
Add support to remote access plugin for root_cert_path
942e39e 
Signed-off-by: James Rhodes <[email protected]>
@jarhodes314
Add test to verify remote access works with a custom root cert path
30388dc 
Signed-off-by: James Rhodes <[email protected]>
@gligorisaev @jarhodes314
proposal test suite for this PR
9cf7376 
Signed-off-by: gligorisaev <[email protected]>
@jarhodes314
Verify certificates can be uploaded with custom root cert path
1db32b4
@jarhodes314
Address some review comments
66aaa80
@jarhodes314
Move the certificate logic from tedge_utils to certificate crate
716e330
@jarhodes314
Run formatter
587a4b7
@jarhodes314
Fix error message
3445e44
@jarhodes314
Simplify test as suggested
3daae97
@jarhodes314
Replace unwrap with expect
b61ddbc
@jarhodes314
Fix system test
e637b25
@jarhodes314 jarhodes314 force-pushed the bug/c8y-root-cert-upload branch from 963bd5e to e637b25 Compare July 26, 2024 09:49
@jarhodes314 jarhodes314 temporarily deployed to Test Pull Request July 26, 2024 09:49 — with GitHub Actions Inactive
@jarhodes314 jarhodes314 enabled auto-merge July 26, 2024 09:52
@jarhodes314 jarhodes314 added this pull request to the merge queue Jul 26, 2024
Merged via the queue into thin-edge:main with commit c33692a Jul 26, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gligorisaev gligorisaev gligorisaev left review comments

@reubenmiller reubenmiller reubenmiller left review comments

@albinsuresh albinsuresh albinsuresh approved these changes

@didier-wenzek didier-wenzek Awaiting requested review from didier-wenzek didier-wenzek is a code owner

@rina23q rina23q Awaiting requested review from rina23q rina23q is a code owner

Assignees
No one assigned
Labels
theme:c8y Theme: Cumulocity related topics theme:configuration Theme: Configuration management
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jarhodes314 @gligorisaev @albinsuresh @reubenmiller