Skip to content

fix: pkcs11 signing with a wrong key and a ready check when TedgeP11Client is created#3602

Merged
reubenmiller merged 2 commits intothin-edge:mainfrom
Bravo555:fix/3601/wrong-key
May 12, 2025
Merged

fix: pkcs11 signing with a wrong key and a ready check when TedgeP11Client is created#3602
reubenmiller merged 2 commits intothin-edge:mainfrom
Bravo555:fix/3601/wrong-key

Conversation

@Bravo555
Copy link
Copy Markdown
Member

@Bravo555 Bravo555 commented May 9, 2025 •
edited
Loading

Proposed changes

This PR has 2 parts:

  1. Fix a bug where we didn't keep the handle to the key we found using the URI if it was declared, but used another search to find a new key capable of signing. Sometimes it worked, because the library could technically return the least recently used key as "first", which worked sometimes but not always. Now, within the session we keep track of the object handle in the key struct so the key is guaranteed to be correct.
  2. Add a ready ready check when creating the TedgeP11Client. During a shared debugging session with @reubenmiller, we discovered that on certain setups loading the PKCS11 library can be unreasonably slow, so we decided to make a request that starts the server before the actual handshake to ensure the server is warm. More rationale in the method doc comment.

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

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

@Bravo555
fix: pkcs11 signing with a wrong key
6d61dd7 
Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
@Bravo555 Bravo555 had a problem deploying to Test Pull Request May 9, 2025 09:55 — with GitHub Actions Failure
@codecov
Copy link
Copy Markdown

codecov bot commented May 9, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 32.14286% with 38 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/extensions/tedge-p11-server/src/pkcs11.rs 0.00% 25 Missing ⚠️
crates/extensions/tedge-p11-server/src/client.rs 60.71% 5 Missing and 6 partials ⚠️
crates/extensions/tedge-p11-server/src/signer.rs 0.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Bravo555 Bravo555 had a problem deploying to Test Pull Request May 9, 2025 12:17 — with GitHub Actions Failure
@Bravo555 Bravo555 temporarily deployed to Test Pull Request May 9, 2025 12:34 — with GitHub Actions Inactive
@reubenmiller reubenmiller added theme:security Theme: Security related topics theme:certificates Theme: Device certificate topics theme:hsm Hardware Security Module related topics and removed theme:security Theme: Security related topics theme:certificates Theme: Device certificate topics labels May 9, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented May 9, 2025 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
629 0 3 629 100 1h53m17.304606s

@Bravo555 Bravo555 force-pushed the fix/3601/wrong-key branch from 48222f2 to f494b0c Compare May 12, 2025 10:29
@Bravo555 Bravo555 temporarily deployed to Test Pull Request May 12, 2025 10:29 — with GitHub Actions Inactive
@Bravo555 Bravo555 changed the title fix: pkcs11 signing with a wrong key fix: pkcs11 signing with a wrong key and a ready check when TedgeP11Client is created May 12, 2025
@Bravo555 Bravo555 marked this pull request as ready for review May 12, 2025 10:37
reubenmiller
reubenmiller approved these changes May 12, 2025
View reviewed changes
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. We tested this on the device where the initial problem was discovered and it worked as expected.

didier-wenzek
didier-wenzek approved these changes May 12, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved

@Bravo555 Bravo555 added this pull request to the merge queue May 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 12, 2025
@reubenmiller reubenmiller added this pull request to the merge queue May 12, 2025
github-merge-queue bot pushed a commit that referenced this pull request May 12, 2025
@reubenmiller
Merge pull request #3602 from Bravo555/fix/3601/wrong-key
e6c539a 
fix: pkcs11 signing with a wrong key and a ready check when `TedgeP11Client` is created
@Bravo555
perform ready check when creating tedge-p11-client
c0f1bfa 
Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
@Bravo555 Bravo555 force-pushed the fix/3601/wrong-key branch from f494b0c to c0f1bfa Compare May 12, 2025 13:48
@Bravo555 Bravo555 temporarily deployed to Test Pull Request May 12, 2025 13:48 — with GitHub Actions Inactive
Bravo555
Bravo555 commented May 12, 2025
View reviewed changes
crates/extensions/tedge-p11-server/src/server.rs
};

tokio::task::spawn_blocking(move || {
let client = TedgeP11Client::with_ready_check(socket_path.into());
Copy link
Copy Markdown
Member Author

@Bravo555 Bravo555 May 12, 2025

Choose a reason for hiding this comment

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

NOTE: this is the fix for #3606

@reubenmiller reubenmiller removed this pull request from the merge queue due to a manual request May 12, 2025
@reubenmiller reubenmiller added this pull request to the merge queue May 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 12, 2025
@reubenmiller reubenmiller added this pull request to the merge queue May 12, 2025
Merged via the queue into thin-edge:main with commit 608e8d7 May 12, 2025
34 checks passed
@Bravo555 Bravo555 deleted the fix/3601/wrong-key branch May 12, 2025 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@reubenmiller reubenmiller reubenmiller approved these changes

@didier-wenzek didier-wenzek didier-wenzek approved these changes

@rina23q rina23q rina23q approved these changes

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

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

Assignees

No one assigned

Labels

theme:hsm Hardware Security Module related topics

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Bravo555 @reubenmiller @didier-wenzek @rina23q