Skip to content

fix: tedge cert renew returning success exit code on error #3525

Merged
didier-wenzek merged 4 commits intothin-edge:mainfrom
didier-wenzek:fix/cert-renew-exit-code
Apr 2, 2025
Merged

fix: tedge cert renew returning success exit code on error #3525
didier-wenzek merged 4 commits intothin-edge:mainfrom
didier-wenzek:fix/cert-renew-exit-code

Conversation

@didier-wenzek
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek commented Apr 2, 2025

Proposed changes

  • tedge cert renew exits with status code 1 on error
  • tedge cert renew returning a confusing error message when the private key cannot be read

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

#3524

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s. You can activate automatic signing by running just prepare-dev once)
  • I ran just format as mentioned in CODING_GUIDELINES
  • I used just check 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
Copy link
Copy Markdown

codecov bot commented Apr 2, 2025

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 2, 2025

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
604 0 3 604 100 1h43m4.138806999s

@didier-wenzek didier-wenzek force-pushed the fix/cert-renew-exit-code branch from cbceba9 to ec2bf68 Compare April 2, 2025 09:20
@didier-wenzek didier-wenzek temporarily deployed to Test Pull Request April 2, 2025 09:20 — with GitHub Actions Inactive
Comment on lines +169 to +175
.or_else(|err| {
if key_path.exists() {
Err(err)
} else {
Ok(KeyKind::New)
}
})
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The root cause of the error-prone hint was to conflate all error cases into a file not found error.

@didier-wenzek didier-wenzek added bug Something isn't working theme:cli Theme: cli related topics theme:certificates Theme: Device certificate topics labels Apr 2, 2025
Copy link
Copy Markdown
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

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

I'm not sure the behaviour of tedge cert renew for self-signed certificate is intended.

Copy link
Copy Markdown
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

Approved. Works nicely now.

Copy link
Copy Markdown
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
When run without enough privileges
`tedge cert renew` was returning an error-prone message:

```
A private key already exists and would be overwritten.
Run `tedge cert remove` first to generate a new certificate and private key.
```

With this fix the error points to the correct cause:

```
Error: failed to renew the device certificate from https://127.0.0.1

Caused by:
    0: I/O error accessing the private key: "/etc/tedge/device-certs/demo-device-888.key"
    1: Permission denied (os error 13)
```

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@didier-wenzek didier-wenzek force-pushed the fix/cert-renew-exit-code branch from 758b169 to f34c00a Compare April 2, 2025 18:37
@didier-wenzek didier-wenzek temporarily deployed to Test Pull Request April 2, 2025 18:38 — with GitHub Actions Inactive
@didier-wenzek didier-wenzek enabled auto-merge April 2, 2025 18:38
@didier-wenzek didier-wenzek added this pull request to the merge queue Apr 2, 2025
Merged via the queue into thin-edge:main with commit e727c2d Apr 2, 2025
34 checks passed
@didier-wenzek didier-wenzek deleted the fix/cert-renew-exit-code branch April 2, 2025 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working theme:certificates Theme: Device certificate topics theme:cli Theme: cli related topics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants