Allow setting pReserved in Initialize

[satoqz]

Allow setting the pReserved field in CK_C_INITIALIZE_ARGS via InitializeWithReserved(), keeping Initialize() as-is.

This follows the same motivation as parallaxsecond/rust-cryptoki#321 and would be a nice to have.

Since the data pointed at is library-specific, it is probably best to accept an unsafe.Pointer and let advanced users construct the payload as needed, e.g. via C.CString. I tested that configuring https://github.com/latchset/kryoptic via pReserved works with this change.

[miekg]

is this something generic? So that other bits can also be set?
Regardless of the answer, this could also (and saves a method in the api), be done as

func (c *Ctx) Initialize(whatever ...unsafe.Pointer) error {
    if len(whatever) > 0  {
	   return c.InitializeWithReserved(whatever[0])
   }
	return c.InitializeWithReserved(nil)
}

[satoqz]

Fair points!

Looking at the definition of CK_C_INITIALIZE_ARGS:

typedef struct CK_C_INITIALIZE_ARGS {
  CK_CREATEMUTEX CreateMutex;
  CK_DESTROYMUTEX DestroyMutex;
  CK_LOCKMUTEX LockMutex;
  CK_UNLOCKMUTEX UnlockMutex;
  CK_FLAGS flags;
  CK_VOID_PTR pReserved;
} CK_C_INITIALIZE_ARGS;

Available flags are:

CKF_LIBRARY_CANT_CREATE_OS_THREADS
CKF_OS_LOCKING_OK

This library always sets flags = CKF_OS_LOCKING_OK, which I assume will remain the case because of the Go runtime's implications (?)

This struct and available flags have not changed with PKCS#11 v3+, so no concern in that regard, unless we anticipate additional flags past the current PKCS#11 v3.2.

So the only value we'd really want to add configurability to for now is pReserved unless you think users should be able to control the various locking-related options. If you think an option-like pattern would be preferable (it does cover the rare chance that additional flags or fields are added in the future) I can do that.

[miekg]

This library always sets flags = CKF_OS_LOCKING_OK,

can't remember...

I think an option pattern is an indea, but what about just extending Ctx with a flags and pRevered bits, that get applied when you initialize

[satoqz]

what about just extending Ctx with a flags and pRevered bits, that get applied when you initialize

IMHO this seems a bit out of place as pReserved is a pointer you might want to free after calling Initialize, and keeping it dangling inside of Ctx feels... wrong.

I think a separate struct that already resembles the C struct would really be the simplest, least-surprise path:

type InitializeArgs struct {
	Flags    uint
	Reserved unsafe.Pointer
}

func (c *Ctx) Initialize() error {
	return c.InitializeWith(InitializeArgs{
		Flags: CKF_OS_LOCKING_OK,
	})
}

func (c *Ctx) InitializeWith(args InitializeArgs) error {
	e := C.Initialize(c.ctx, C.CK_FLAGS(args.Flags), C.CK_VOID_PTR(args.Reserved))
	return toError(e)
}

... or alternatively as just Initialize(args ...InitializeArgs) where we expect 0-1 args, if you'd prefer.

[satoqz]

I think https://github.com/softhsm/SoftHSMv2/blob/7ff177d678a253171fbe220b5df02ee231207667/src/lib/SoftHSM.cpp#L456 holds the answer as to why CKF_OS_LOCKING_OK is the sensible default, it will assume no multi-threaded access otherwise. Passing function pointers (CreateMutex/DestroyMutex/...) that call back into Go's own Mutex implementation is likely counterproductive in terms of complexity and performance or might have led to direct problems?

[miekg]

locking was added in b0a4dd3

a struct (option struct) looks cleanest. On the fence is we need a whole new method, or use an optional argument. I'm leaning towards the latter

[satoqz]

Updated, assuming you were asking for functional options. Let me know if I misinterpreted 😄

locking was added in b0a4dd3

I think this only moved the creation of the CK_C_INITIALIZE_ARGS struct over to the C side, but didn't change the values that are ultimately passed.

[miekg]

I can live with that.