Compare trimmed token serial numbers (PKCS11 provider)

[mohamedasaker-arm]

Fixes: #615

Signed-off-by: Mohamed Omar Asaker mohamed.omarasaker@arm.com

[ionut-arm]

Looks good! Could you add a config test to prevent regressions?

[mohamedasaker-arm]

Thanks, @ionut-arm!
An (e2e) config test will require knowledge of the token serial number to compare it against the provided one.
The token's serial number should be less than 16 characters to have a meaningful test.
Given the above info, here are the challenges/limitations I found

• The token serial number the system will use as a reference in the comparison (can be retrieved using std::Command) https://rust-lang-nursery.github.io/rust-cookbook/os/external.html.
• The token generated via softhsm-util is 16 characters.
• This test couples with softhsm.
• Using cryptoki needs to extend the e2e framework to have the ability to extract the pkcs11 backend implementation from the config file.

Suggestion:

• Create a mock backend to fully control the provider backend, which allows driving logic/data from the test.
  for instance, this mock can set the token info
  then by running the service with this backend implementation
  it will use the token data set in the test.
• Rely on the softhsm for now and use the command line approach

[ionut-arm]

Thanks! 👍

[ionut-arm]

(As for the test, you can leave it for another PR. Though now that the CI is failing... :( )

[mohamedasaker-arm]

Failures: It's the new version of the Clippy issue, It's better to fix them, then rebase this PR.
As for the test, Which one from the above suggestions do you think is better, or do you have another idea?

[ionut-arm]

I'd go for the CLI approach - don't think it's particularly likely we'll move to some other PKCS11 implementation, and we already have stuff in the CI pipeline that expects softhsm for setup. And it's a lot easier. If you want you can create some sort of tagging/tracking mechanism for the places where we make explicit use of a specific backend.

[mohamedasaker-arm]

Rethinking the test, we will depend on the softhsm-util in generating the tokens, which generates a 16 char token.
I wonder how the test will be different from the serial_number_only test in e2e_tests/tests/all_providers/config
Could you give me your insights @ionut-arm?

[ionut-arm]

Rethinking the test, we will depend on the softhsm-util in generating the tokens, which generates a 16 char token. I wonder how the test will be different from the serial_number_only test in e2e_tests/tests/all_providers/config Could you give me your insights @ionut-arm?

Hey! Well, serial_number_only does not check the trimming functionality IIUC, it passes whether or not we trim the value in the config file. What I'd like is some guarantee that we can identify the SN successfully even when it's padded with spaces, to protect against any regressions on it. The service doesn't know that a SN has to be 16 characters, so we can put as many spaces as we'd like. Ideally, softhsm would let us set whatever serial number we want so that we can test that side too, but I don't think that's something we can do.

What we want to support here is for an admin to give a serial number of (abcd + 2 spaces) even when the real serial number is (abcd + 12 white spaces) - and have some way to prove that it works.

[mohamedasaker-arm]

Hey @ionut-arm
About this statement,

"Ideally, softhsm would let us set whatever serial number we want so that we can test that side too, but I don't think that's something we can do."

I agree, and I am not aware of such an option to set the serial number ourselves. So relaying on softhsm in this test won't add any value.
So the conclusion, we want to go with other approaches in which we can set/manipulate the token serial number.

[ionut-arm]

we want to go with other approaches in which we can set/manipulate the token serial number.

Do we have such an option?

[ionut-arm]

So what my aim would be with the test is to verify if the trimming works correctly. Because the operation is symmetrical (on both what's in the config file and what's coming from the token), it doesn't really matter which one we can control. It would be nice to be able to test both of them (setting the SN in the config file to something with whitespace around in one test, and the serial number of the token to something with whitespace around in another), but if we can only do one of them that would still be preferable - though maybe there's something preventing us from doing that?

[mohamedasaker-arm]

Nothing is preventing us from doing that (Adding spaces to the provided serial number), but that won't fulfil the following statement

What we want to support here is for an admin to give a serial number of (abcd + 2 spaces) even when the actual serial number is (abcd + 12 white spaces) - and have some way to prove that it works.

I got from this statement that you want to validate both sides.
For clarity:
SN_A: Provided Token SN has written in config.toml provided by the user
SN_B: The Token SN (generated by softhsm2-util)

Option1: SN_A is set to SN_B with extra spaces
The test should pass and is testing that SN_A.trim is okay
Option2: SN_B is less than 16 char and has spaces ex: SN_B = "abcd "
SN_A is SN_B with different spaces ex: SN_A = " abcd "
Option 2 isn't achievable with the current testing framework and system assumptions
I am okay with option1 only for now.
Is that what you meant?
Sorry for the misunderstanding 😄

[anta5010]

In real world SN_B might be "abcdef123 ". i.e 16 chars with spaces at the end. For example with Nitrokey USB HSM.
Ideally we need the test to pass when SN_A either "abcdef123 " or "abcdef123".
But, it looks like it's not straightforward to test with softhsm2-util.
Although as we build softhsm2-util from sources for CI tests we can patch it to produce the required SN_B

[ionut-arm]

Yeah, option 1 should be fine, testing at least part of it.

[mohamedasaker-arm]

@ionut-arm, Could you retrigger this test.
I think it's just unstable.
Is there an issue to tackle this instability?

[ionut-arm]

Is there an issue to tackle this instability?

No, I don't think so, and we have multiple issues, mostly in the stress test it seems. One affects the "Mbed Crypto derivatives", and another the PKCS11 provider. The PKCS11 provider issues we've tried to fix and it's definitely better now than before, but Softhsm probably still has multithreading issues.

Feel free to open issues!