Add PIN management functionality

[gierdo]

This PR is an adaption of #161

[MatthiasValvekens]

Hmm, I disagree with the chardet addition on three grounds:

• It's unrelated to the PIN management change (_CK_UTF8CHAR_to_str is not called on any code path that relates to PIN usage)
• The spec is clear that CK_UTF8CHAR is for UTF-8 strings. If a token violates that, the issue is with the token, not the library.
• It introduces a new dependency for the purpose of dealing with noncompliant tokens, and even then chardet is just a heuristic, so it doesn't actually guarantee that the library will process the corrupted data gracefully.

I'd buy that we'd need to "deal" with this if this routine were used on the critical path for cryptographic operations, but that's not the case: _CK_UTF8CHAR_to_str is only invoked calls to decode human-readable token metadata. It's also not used in certificate decoding operations.

---

What's your concrete motivation for introducing this, actually? Do you have a (presumably noncompliant ;) ) token that fails to work without this addition, but does work when this is added? :)

I'm just asking to figure out if there's a better way to deal with your issue than the above.

[gierdo]

Hi Matthias!
Thank you for your quick reply.

We are a relatively large organisation with a large number of tokens. A frustratingly large number of those tokens trigger decoding exceptions, resolved by the change.

I presume that the simple fallback with the "replace" error strategy would be sufficient.
Would you be OK with that?

In any case, I would be happy to create a separate PR for that, to be discussed separately.

[MatthiasValvekens]

I presume that the simple fallback with the "replace" error strategy would be sufficient.
Would you be OK with that?

Works for me, yes!

EDIT: ...but indeed, as a separate PR, if possible. If you're rebasing this branch anyway, I'd be happy to review this PR for the PIN management functionality alone.

[MatthiasValvekens]

Some questions re: possible functionality duplication / unclear API contracts.

[gierdo]

Awesome! Thank you for your review.
I see and agree with your points. Will think about it and adapt!

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (master@bc1b1f3). Learn more about missing BASE report.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #214   +/-   ##
=========================================
  Coverage          ?   90.17%           
=========================================
  Files             ?       15           
  Lines             ?     2625           
  Branches          ?       29           
=========================================
  Hits              ?     2367           
  Misses            ?      255           
  Partials          ?        3           

Flag	Coverage Δ
unittests	90.17% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[MatthiasValvekens]

Nice! Much cleaner changeset now :). If this passes CI it's good to go in my book (just some linting issues, it seems).

EDIT: Ah, right, a test for the logout functionality would also be nice.

[gierdo]

I removed the logout functionality, together with the login. Can readd it, though.

And I will add tests for init_pin

[MatthiasValvekens]

Ah, oops, brain fart. No worries in that case. Covering C_InitPin will be hard due to the way the CI is structured, don't worry about that.

If you're up for adding the logout function back (I do think that one is at least potentially sensible), by all means do, but I'm OK with merging this as-is as soon as the linter's happy.

[gierdo]

ruff format is happy now, and init_pin is tested. it's a bit of a roundtrip, but it works.

[MatthiasValvekens]

Great! I expected those tests with C_InitPin to fail, but I suppose that the spec is indeed silent on whether InitPin is allowed if the PIN is already configured, and SoftHSM2 happens to not care. Oh well, that'll do for a regression test :).

[gierdo]

It was a pleasure! Thank you for being so helpful and clear.

[gierdo]

The commit message of the squashed commit still contains the mention of the additional change that has been removed from this PR.

Not too important, but it would make the history lie.

[MatthiasValvekens]

Yep, I noticed just in time before merging 😅