Skip to content
This repository has been archived by the owner on Jul 2, 2023. It is now read-only.

Support for PKCS11 #3

Open
wants to merge 146 commits into
base: main
Choose a base branch
from

Conversation

portersrc
Copy link
Member

@portersrc portersrc commented Jun 28, 2021

Initial pkcs11 support for ocicrypt-rs, ported from ocicrypt's golang version.

portersrc and others added 10 commits June 17, 2021 18:48
Signed-off-by: Arron Wang <[email protected]>
Signed-off-by: Arron Wang <[email protected]>
Signed-off-by: Chris Porter <[email protected]>
Signed-off-by: Chris Porter <[email protected]>
Signed-off-by: Chris Porter <[email protected]>
Initial implementation for module spec/config/helper

Signed-off-by: Chris Porter <[email protected]>
Initial implementation for module spec/config/helper

Signed-off-by: R. Jantz <[email protected]>
Signed-off-by: Chris Porter <[email protected]>
src/utils.rs Outdated Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
src/keywrap/pkcs11/mod.rs Outdated Show resolved Hide resolved
src/keywrap/pkcs11/mod.rs Outdated Show resolved Hide resolved
src/keywrap/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

At some point when this is ready to merge, it may be good for @stefanberger to take a quick look through as well as he was the main author of the PKCS11 component.

src/keywrap/pkcs11/mod.rs Outdated Show resolved Hide resolved
@@ -10,3 +10,11 @@ edition = "2018"

[dependencies]
anyhow = ">=1.0"
pkcs11 = ">=0.5.0"
pkcs11-uri = ">=0.1.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@stefanberger are these libraries using the same standard for PKCS11 URIs? This library says it's implementing RFC 7512, so it seems like there shouldn't be any issue here, but deferring to your opinion on the intricacies

Choose a reason for hiding this comment

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

Following this page here it does:

PKCS#11 URI

Bare bones implementation of the RFC 7512 URI scheme for locating keys and other PKCS#11 objects.

This library is patched together from existing libraries, namely pkcs11, uriparse and percent-encoding, and is a work in progress.

So this library seems to want to do the same as mine does. Differences may be in the details.

@portersrc
Copy link
Member Author

@lumjjb @fitzthum this pkcs11 support never got merged in August, but I want to find out if it's still wanted/needed in ocicrypt-rs. If so, what's a good next set of steps? There's a lot of code here. I can confirm that the happy-path test replicated from golang's ocicrypt works.

Also, what is the slack URL for discussing ocicrypt development? I cannot seem to find it.

Lastly, testing environment is a problem in this PR, and I'm open to suggestions. I disabled a test that's unrelated to my PR but which was breaking the automatic github push checkers (caf91a4). I think this is due to an environment variable problem from here:
https://github.com/containers/ocicrypt-rs/blob/main/src/encryption.rs#L26-L28
Similarly, tests for the pkcs11 support in this PR depend on softhsm and create temporary files in /tmp, for example. These non-unit tests break the github checkers, as well, so I've disabled them.

@portersrc
Copy link
Member Author

@arronwy @sameo - Please feel free to comment on my previous message. But in particular, if you have a pointer to the slack URL for confidential containers and ocicrypt-rs, that would be very helpful. Thanks.

@fitzthum
Copy link
Member

@rudyjantz I think we have an ocicrypt channel in the Kata slack workspace.

@jodh-intel
Copy link
Member

@portersrc (or any other commenter) - please could you give an update on the status of this PR. It's pretty huge meaning it's going to be difficult to land I fear. Also, could these 131 commits be squashed down to... 1? 😄

@portersrc
Copy link
Member Author

portersrc commented Jun 2, 2022

@jodh-intel

"give an update": I need to fix the latest conflicts. I will look at it next week after Tuesday.

"it's pretty huge, hard to land": Agreed. I intentionally followed the pkcs11 support in the (golang) ocicrypt repo closely when porting, thinking this was least risk. I glanced at issue confidential-containers/guest-components#182 today, and it looks like this project has made good progress. This feature might be a nice add. I'm OK with closing or fixing any and all requests before merging.

"131 commits squashed to 1": I can do that before a merge, no problem.

Signed-off-by: chris porter <[email protected]>
Signed-off-by: chris porter <[email protected]>
Signed-off-by: chris porter <[email protected]>
Signed-off-by: chris porter <[email protected]>
@portersrc
Copy link
Member Author

portersrc commented Jun 13, 2022

The "nightly" github job may have a small bug which is causing it to fail. Here's its error:

error: you are deriving `PartialEq` and can implement `Eq`
Error:  --> src/utils/keyprovider.rs:1:17
  |
1 | #[derive(Clone, PartialEq, ::prost::Message)]
  |                 ^^^^^^^^^ help: consider deriving `Eq` as well: `PartialEq, Eq

But I've already made this change here. Note that clippy modifies the keyprovider.rs file, so what may be happening is a prior github job (i.e. stable or beta) modifies the source, which then gets (incorrectly) reused in the nightly job.

@portersrc
Copy link
Member Author

@jodh-intel
This PR is up-to-date again. Is there a lead developer on the repo now? Or: Is there a customer/user for pkcs11 that you know of?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants