Update sig_stfl.h document for #1650

[ducnguyen-sb]

This PR address the comments in #1650

This PR rewrite the documentation in sig_stfl.h header for a few APIs, make simpler and (somewhat) easier to read.

• Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
• Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

[baentsch]

Same question as above: When does (an application) need to call this function?

[ducnguyen-sb]

So I added:

* This operation is essential in multi-threaded or multi-process contexts
 * in order to prevent simultaneous operations that could compromise the stateful signature security.
 
 ...
* @note It's not required to use lock and unlock functions in a single-threaded environment.

So, it's should be call in multi-threaded environment.
I wonder if the current documentation is good enough, let me know if it's unclear.

[baentsch]

I wonder if the current documentation is good enough, let me know if it's unclear.

I can only judge from my perspective and cannot know what other's may (mis)understand, so here goes my question now: Which (liboqs API) calls ("operations" in your wording above) need to be guarded with this function in multi-threaded environments? And a much more basic question: If there are such library functions failing to properly operate in a multi-threaded environment, wouldn't it be better to document them accordingly? And as follow-on question: Does it need such liboqs-based "lock/unlock" functions at all if such documentation exists? Why isn't use of any external, e.g. as provided by the application-chosen threading library, lock/unlock mechanisms sufficient?

I guess what I'd like to suggest in a more "brutal question": Should we not strive to implement a thread-safe library API OR completely get out of the way (giving the app developer full control of what they want/need to do)? Anything in between IMO has the potential for a "support nightmare" ("use API XYZ if you are using threading library ABC; if not using any threading, disregard XYZ but use API XZZ", etc....)

[ducnguyen-sb]

I answer each question below.

1. What function need to be guarded with this function in a concurrent environment?

Only the Signing operation.

Thanks to your question, I updated this note:

* @note It's not necessary to use this function in either Keygen or Verifying operations.
 *       In a concurrent environment, the user is responsible for locking and unlocking the private key,
 *       to make sure that only one thread can access the private key during a Signing operation.

2. Why such lock/unlock APIs are there? Is it meant we strive to be a thread-safe library?

These APIs intention is to limit to one owner of private key at a time in a concurrent environment.
If users supply lock/unlock logic with a mutex, we acquire the lock to be the only owner of the private key, and unlock after the Signing operation complete.
If users don't supply the lock/unlock logic or a mutex, they are simply NOP operations.

I think the key different here is: We don't execute stateful signature in multi-thread at the library itself, but we provide a way to let users use multi-thread securely, in the right way.

3. Does it need such liboqs-based "lock/unlock" functions at all if such documentation exists?

I think to have lock/unlock functions help liboqs in:

• Testing. Someone has to foreseen the usage of our proposed APIs in multi-thread environment, so we need lock/unlock to test if it's still safe in such if our low-level implementation change.

So we have it in test, well, why not exposed them to library user, it's convenience anyway. So there are liboqs-based lock/unlock functions.

4. Why isn't use of any external, e.g. as provided by the application-chosen threading library, lock/unlock mechanisms sufficient?

Actually we don't concretely define the lock/unlock mechanisms in liboqs, if the function pointers of lock/unlock are non-NULL then we will run them to perform lock/unlock the private key.

So, we open to any application-chosen threading library, and it's user responsibility to create a proper lock/unlock functions, and it's up to them to define their mutex.

5. Anything in between IMO has the potential for a "support nightmare" ("use API XYZ if you are using threading library ABC; if not using any threading, disregard XYZ but use API XZZ", etc....)

I totally agree with you, we don't want to get into "support nightmare". I want to run away from it as far as possible. :))

You have more experience than me in this matter, please let me know if there is something I need to change to get out of "support nightmare".

[ducnguyen-sb]

@baentsch before I merge this PR, do I answer your questions?

[baentsch]

You have more experience than me in this matter, please let me know if there is something I need to change to get out of "support nightmare".

I'm not sure I do -- I did not use these APIs -- yet. My "general" question regarding minimizing support effort is: Could we completely do away with those functions in the external OQS API? If we could, we won't have to support them.

So we have it in test, well, why not exposed them to library user, it's convenience anyway. So there are liboqs-based lock/unlock functions.

That's what I'm concerned with: If it's mere "convenience" (for us) we shouldn't externally make it available. Any API externally visible creates more support effort. Please see the discussion in #1648 and decide whether mutex/lock/unlock is another candidate for this internal API.

[ducnguyen-sb]

I double check, these APIs just for testing purpose, so I moved them to internal APIs.
Updated in 8b0e15e

[ducnguyen-sb]

The set_lock, set_mutex, and set_unlock should still be public APIs.
@baentsch

[baentsch]

:-( I won't respond to user questions about them.

[baentsch]

Thanks for the updates addressing many of my original comments, @ducnguyen-sb ! I only re-reviewed the changes in the PR, not the merge-to-main PR again: Please let me know when I should do that (to close out/mark as resolved my comments there).

[ducnguyen-sb]

Thanks @baentsch , I addressed your comments.
I believe it's simpler to address a smaller changes here rather than the big PR in #1650 , once it's resolved here we can mark the comments in the main branch as resolved.

[baentsch]

Thanks for the clarification.