Fix regression in parsing tokeninfo

[Jakuje]

Some of the recent changes refactoring tokeninfo storing broke the loading of the objects from database. The problem is that the unit tests are ran only from single invocation of C_Initialize() so all what is used is pulled from the cache and never from DB.

This adds a test using JSON DB backend, which looks like working after this change, but I still had some issues with the sqlite backend which returns a CKR_DEVICE_MEMORY for any operation after second C_Initialize(), but I was not yet able to track the source of this error.

[Jakuje]

The new test calls c_finalize which breaks other tests running in parallel. I tried to workaround it with serial_test, but we would likely have to refactor the tests or prefix each of them with #]parallel] otherwise they are not guaraneed to run at different time than the serial test. The other option might be to spawn these tests in separate process (there is a rusty-fork crate). If you have some other idea, it would be welcomed.

[simo5]

Why do you need to re-initialize the whole token to check data? I think it would make more sense to figure out if we can detect if there is a need to reload data from the DB, and maybe always force that via an appropriate test ? A cache has the big issue of risking concurrent access modifying stuff and we not seeing it.

[simo5]

(I think it is ok to mark all tets btw, I find it valuable to test the re-init, but I think it is not sufficient)

[Jakuje]

if you have some suggestion how to flush the cache without calling the finalize, it might do that too without the need to go with the annotating all the tests. I just did what I knew will do (obviously with the side-effects) and which will likely keep working without modifications in the long term through refactorings and changes.

[simo5]

Please annotate the tests and lets get this merged

[Jakuje]

The problem is not only with the tests annotations. This fix is for the json storage, but there is still some issue with the sql storage that i have not been able to track down yet ...

On Mon, Jun 17, 2024, 20:56 Simo Sorce @.***> wrote:

Please annotate the tests and lets get this merged

— Reply to this email directly, view it on GitHub https://github.com/latchset/kryoptic/pull/58#issuecomment-2174210464, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAUR2WPMM3LDHO34ZGDIH3LZH4WPVAVCNFSM6AAAAABJNLPZX6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZUGIYTANBWGQ . You are receiving this because you authored the thread.Message ID: @.***>

[Jakuje]

Update. It fails on this line: https://github.com/latchset/kryoptic/blob/055ad8bec64ab893bd9254456f507a24cd2fd924/src/storage/sqlite.rs#L121

When it tries to convert the token info label:

(gdb) p id
$12 = 2
(gdb) p atype
$13 = 3
(gdb) p val
$14 = rusqlite::types::value_ref::ValueRef::Blob(&[u8](size=32) = {32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 
    32, 32, 32, 32, 32, 32, 32, 32, 32, 32})
(gdb) n
102             if let Some(obj) = objects.last_mut() {
(gdb) 
102             if let Some(obj) = objects.last_mut() {
(gdb) 
103                 let attrtype = attribute::attr_id_to_attrtype(atype)?;
(gdb) 
103                 let attrtype = attribute::attr_id_to_attrtype(atype)?;
(gdb) 
104                 let attr = match attrtype {
(gdb) 
119                     AttrType::StringType => match val
(gdb) 
119                     AttrType::StringType => match val
(gdb) 
121                         .map_err(bad_storage)?

The type is Blob (not null terminated) so the rust fails to convert it to string, I think. Will try to dig into that later today.

[simo5]

Update. It fails on this line:

https://github.com/latchset/kryoptic/blob/055ad8bec64ab893bd9254456f507a24cd2fd924/src/storage/sqlite.rs#L121

When it tries to convert the token info label:

(gdb) p id
$12 = 2
(gdb) p atype
$13 = 3
(gdb) p val
$14 = rusqlite::types::value_ref::ValueRef::Blob(&[u8](size=32) = {32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 
    32, 32, 32, 32, 32, 32, 32, 32, 32, 32})
(gdb) n
102               if let Some(obj) = objects.last_mut() {
(gdb) 
102               if let Some(obj) = objects.last_mut() {
(gdb) 
103                   let attrtype = attribute::attr_id_to_attrtype(atype)?;
(gdb) 
103                   let attrtype = attribute::attr_id_to_attrtype(atype)?;
(gdb) 
104                   let attr = match attrtype {
(gdb) 
119                       AttrType::StringType => match val
(gdb) 
119                       AttrType::StringType => match val
(gdb) 
121                           .map_err(bad_storage)?

The type is Blob (not null terminated) so the rust fails to convert it to string, I think. Will try to dig into that later today.

Ahh these are fixed length strings, we can treat them as blobs or as strings, but we should be consistent.

[Jakuje]

The current version has the one failing test with the sql storage. I probably miss the whole picture so without diving too deep I was not able to figure out a good fix for this yet.

There are options with the way forward:

• Figure out the sql storage issue
• Mark the test as failing and merge before we will be able to figure out the issue
• Go ahead with the cache disable/removal as discussed last time (it might unearth more issues)

I will mark it as a draft for now.

[simo5]

Ok, I already have code for cache removal, but was still adding more stuff, will open a PR soon.