Skip to content

feat: Key selection using PKCS #11 URI#3555

Merged
Bravo555 merged 2 commits intothin-edge:mainfrom
Bravo555:feat/pkcs11-uri
Apr 23, 2025
Merged

feat: Key selection using PKCS #11 URI#3555
Bravo555 merged 2 commits intothin-edge:mainfrom
Bravo555:feat/pkcs11-uri

Conversation

@Bravo555
Copy link
Copy Markdown
Member

@Bravo555 Bravo555 commented Apr 10, 2025 •
edited
Loading

TODO

  • PKCS11 URI scheme parsing
  • selection of token by serial number and label and private key by label
  • select private key by id (need to handle percent encoding correctly)
  • tests
  • require warn a user to provide a URI that uniquely identifies key to use if there are multiple tokens/keys and URI is either not provided or not sufficiently precise (feat: Key selection using PKCS #11 URI #3555 (comment))

Follow-up

  • warn user if used URI points to an object that is not a private key but the private key exists in the token
  • code cleanup

Proposed changes

Allows the user to denote private key that will be used by tedge-p11-server for signing by a new optional --uri parameter.

The PKCS #⁠11 URI is explained in RFC 7512. It can be used to identify any cryptoki resource by its properties. In our case it's used to select a key, but the URI given by the user doesn't have to identify a key, but can identify e.g a token which we can select and find its first if the token contains only one private key, use that 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

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 Bravo555 mentioned this pull request Apr 10, 2025
Closed
11 tasks
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 10, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 86 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/extensions/tedge-p11-server/src/pkcs11.rs 0.00% 74 Missing ⚠️
crates/extensions/tedge-p11-server/src/signer.rs 0.00% 6 Missing ⚠️
crates/extensions/tedge-p11-server/src/service.rs 0.00% 5 Missing ⚠️
crates/extensions/tedge-p11-server/src/main.rs 0.00% 1 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.

reubenmiller
reubenmiller reviewed Apr 11, 2025
View reviewed changes
@Bravo555 Bravo555 temporarily deployed to Test Pull Request April 14, 2025 17:44 — with GitHub Actions Inactive
@Bravo555 Bravo555 marked this pull request as ready for review April 15, 2025 08:30
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 15, 2025 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
616 0 3 616 100 1h47m33.714582999s

@Bravo555 Bravo555 temporarily deployed to Test Pull Request April 15, 2025 11:36 — with GitHub Actions Inactive
@Bravo555 Bravo555 temporarily deployed to Test Pull Request April 15, 2025 11:47 — with GitHub Actions Inactive
reubenmiller
reubenmiller reviewed Apr 15, 2025
View reviewed changes
reubenmiller
reubenmiller reviewed Apr 15, 2025
View reviewed changes
reubenmiller
reubenmiller reviewed Apr 15, 2025
View reviewed changes
@reubenmiller reubenmiller temporarily deployed to Test Pull Request April 15, 2025 12:39 — with GitHub Actions Inactive
@reubenmiller reubenmiller added theme:certificates Theme: Device certificate topics skip-release-notes Don't include the ticket in the auto generated release notes labels Apr 15, 2025
didier-wenzek
didier-wenzek reviewed Apr 16, 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.

I will be happy to approve. There are small points still to address though.

@Bravo555 Bravo555 temporarily deployed to Test Pull Request April 16, 2025 10:35 — with GitHub Actions Inactive
didier-wenzek
didier-wenzek reviewed Apr 16, 2025
View reviewed changes
tests/RobotFramework/tests/pkcs11/private_key_storage.robot Outdated
Comment on lines +61 to +66
# create tokens with no keys on them, so key selection fails if wrong token is selected
Execute Command softhsm2-util --init-token --free --label token2 --pin "123456" --so-pin "123456"
Execute Command softhsm2-util --init-token --free --label token3 --pin "123456" --so-pin "123456"
Execute Command softhsm2-util --init-token --free --label token4 --pin "123456" --so-pin "123456"
Execute Command softhsm2-util --init-token --free --label token5 --pin "123456" --so-pin "123456"
Execute Command softhsm2-util --init-token --free --label token6 --pin "123456" --so-pin "123456"
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek Apr 16, 2025

Choose a reason for hiding this comment

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

I don't get the point of these fake tokens. The tests pass without those.

Copy link
Copy Markdown
Member Author

@Bravo555 Bravo555 Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

removed in a8a732e.

This is a leftover I forgot to remove from the previous approach which I abandoned because it was bad. Currently when there are many keys, we just use the first one returned to us by the PKCS#11 implementation. The idea was to create many new tokens, such that one of them would be picked up as "first" and used and later return an error because it does not contain a key.

But relying on an arbitrary implementation-specific order is bad. In situations where there are many tokens/keys to be used and we can't automatically select a good one, we should fail right there and require a user to provide a URI that uniquely identifies a key to be used.

So the test was changed to explicitly point to a token without the key and then to a token with the correct key. As for actually returning an error where a key cannot be uniquely identified, it will be implemented in the next commit.

EDIT: On a today's daily call it was deemed that a warning will be sufficient.

Copy link
Copy Markdown
Member Author

@Bravo555 Bravo555 Apr 17, 2025

Choose a reason for hiding this comment

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

warning implemented in 7aa3921

@Bravo555 Bravo555 temporarily deployed to Test Pull Request April 16, 2025 17:17 — with GitHub Actions Inactive
@Bravo555 Bravo555 linked an issue Apr 17, 2025 that may be closed by this pull request
allow users to specify which pkcs11 private key to use by setting a uri #3538
Closed
@Bravo555 Bravo555 temporarily deployed to Test Pull Request April 17, 2025 16:08 — with GitHub Actions Inactive
didier-wenzek
didier-wenzek approved these changes Apr 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approved

Edited. A system test was failing for a silly error. Fixed: 4141e33

Bravo555 added 2 commits April 18, 2025 10:33
@Bravo555
refactor: Extract cryptoki loading
4815365 
Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
@Bravo555
feat: PKCS#11 URI parsing and token/key selection
def1922 
Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
@Bravo555 Bravo555 temporarily deployed to Test Pull Request April 18, 2025 10:52 — with GitHub Actions Inactive
@Bravo555 Bravo555 added this pull request to the merge queue Apr 23, 2025
Merged via the queue into thin-edge:main with commit 3821a79 Apr 23, 2025
51 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@reubenmiller reubenmiller reubenmiller left review comments

@didier-wenzek didier-wenzek didier-wenzek 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

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

Assignees

No one assigned

Labels

skip-release-notes Don't include the ticket in the auto generated release notes theme:certificates Theme: Device certificate topics theme:security Theme: Security related topics

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

allow users to specify which pkcs11 private key to use by setting a uri

3 participants

@Bravo555 @reubenmiller @didier-wenzek