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.

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

Add support for vendor defined attributes #237

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

Conversation

jrozner
Copy link

@jrozner jrozner commented Dec 21, 2024

No description provided.

@jrozner jrozner force-pushed the add-vendor-defined-attributes branch from ca4983b to 697a4bf Compare December 21, 2024 09:48
Copy link
Member

@hug-dev hug-dev left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! This will complete nicely vendor-defined mechanisms already added here: #232

cryptoki/src/object.rs Outdated Show resolved Hide resolved
cryptoki/src/object.rs Outdated Show resolved Hide resolved
cryptoki/src/object.rs Outdated Show resolved Hide resolved
@hug-dev
Copy link
Member

hug-dev commented Dec 21, 2024

Does SoftHSM support them? Would be nice to add tests if possible.

@jrozner
Copy link
Author

jrozner commented Dec 21, 2024

I'll look into whether SoftHSM supports any vendor defined attributes and can write some tests if it does. I've been testing this with a Yubikey which does rely on them for specifying the touch/pin policy. This set of changes seems to have broken something around the attribute type value, it's working with some values but not others. Need to debug.

@jrozner
Copy link
Author

jrozner commented Dec 21, 2024

Looks like I just missed a compilation for my test code. Everything is working

hug-dev
hug-dev previously approved these changes Dec 22, 2024
Copy link
Member

@hug-dev hug-dev left a comment

Choose a reason for hiding this comment

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

Too bad there are no tests in SoftHsm but good that you tested with Yubikey! Thanks for the changes!

cryptoki/src/object.rs Show resolved Hide resolved
@hug-dev
Copy link
Member

hug-dev commented Dec 22, 2024

All good on my side! Seems that the CI is failing (can you see the logs btw? Could be a permission thing). Seems you need to cargo fmt all :)

Signed-off-by: Joe Rozner <[email protected]>
@jrozner
Copy link
Author

jrozner commented Dec 22, 2024

Formatting fixed

hug-dev
hug-dev previously approved these changes Dec 22, 2024
@hug-dev hug-dev enabled auto-merge December 22, 2024 20:45
Copy link
Collaborator

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

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

Looks good, just one question though.

@@ -405,7 +413,6 @@ impl TryFrom<CK_ATTRIBUTE_TYPE> for AttributeType {
}

#[derive(Debug, Clone, PartialEq, Eq)]
#[non_exhaustive]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this got removed? Correct me if I'm wrong but vendor defined doesn't cover all space of possible values? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, fixed that. You're correct that it's not exhaustive.

auto-merge was automatically disabled December 24, 2024 05:15

Head branch was pushed to by a user without write access

@hug-dev
Copy link
Member

hug-dev commented Dec 24, 2024

You forgot to sign-off the last commit 🍏

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

Successfully merging this pull request may close these issues.

3 participants