Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Storage changes #17

Merged
merged 5 commits into from
Mar 26, 2024
Merged

Storage changes #17

merged 5 commits into from
Mar 26, 2024

Conversation

simo5
Copy link
Member

@simo5 simo5 commented Mar 13, 2024

Refactor storage to allow multiple backends.
Add a sqlite based storage backend (see more info on the commit).
Rework some PIN operations to simplify and address storage/masking issues.

also
Fixes #23

@simo5 simo5 marked this pull request as draft March 13, 2024 21:45
@simo5 simo5 force-pushed the storage branch 3 times, most recently from a616fdf to 61a9b25 Compare March 21, 2024 22:27
@simo5 simo5 requested a review from Jakuje March 21, 2024 22:27
@simo5 simo5 marked this pull request as ready for review March 21, 2024 22:27
@simo5 simo5 marked this pull request as draft March 22, 2024 00:06
@simo5
Copy link
Member Author

simo5 commented Mar 22, 2024

@Jakuje I have a few more changes I want to make, you can start to review the general change if you want, but expect some API changes in the next rebase, so feel free to hold off.

@simo5 simo5 force-pushed the storage branch 5 times, most recently from fb1f325 to 518d910 Compare March 22, 2024 08:57
@simo5 simo5 marked this pull request as ready for review March 22, 2024 08:57
@simo5
Copy link
Member Author

simo5 commented Mar 22, 2024

I think it is basically ready for review.

I am not happy with the need to take a write lock for token for so many operations now (due to the caches needing updates on object fetching which are technically just reads).

One idea is to change the object caches in the storage implementations to have themselves locks, so for most operations the locking can be in the caches themselves instead of locking the whole token object.

Another option is to move the caches into the session objects, but the reason I moved them in the token is that some non-token objects still need to be available across sessions, and that would make matters complicated still requiring a caching layer at the token level ...

@simo5 simo5 force-pushed the storage branch 2 times, most recently from 109b58a to 5d606ea Compare March 22, 2024 13:48
src/lib.rs Outdated Show resolved Hide resolved
src/object.rs Show resolved Hide resolved
src/object.rs Show resolved Hide resolved
src/storage/sqlite.rs Outdated Show resolved Hide resolved
src/storage/sqlite.rs Outdated Show resolved Hide resolved
src/token.rs Show resolved Hide resolved
src/token.rs Show resolved Hide resolved
src/token.rs Outdated Show resolved Hide resolved
src/token.rs Show resolved Hide resolved
src/token.rs Outdated Show resolved Hide resolved
simo5 added 5 commits March 25, 2024 17:28
In various cases changeing data in the cache would not cause a flush to
the json file. Writes are rare so even for the very inefficient json
file storage it is ok to just flush (and therefore rewrite the entire
file) all the changes as soon as any modification operation happens.

Signed-off-by: Simo Sorce <[email protected]>
Add an sqlite storage backend and change the existing code to become a
memory and a json backend (with the latter using the former as temporary
storage).

Change token to address caching storage changes, which require a
mutable object to update the cache on reads as well. Introduce
the conept of CheckedHandle to force correct use of the API.
It needed to be split into a "prefetch" call which takes a mutable
reference to the storage, and an actual call to get the object that does
not.
This is because the borrow checkers holds a mutable reference to the
objects whenever a reference (even immutable) is returned as a result.
This then prevents making further calls to the token object.
Once theborrow checker is improved to stop inferring incorrect things
about the need to keep mutable references around, this can be changed
back to be a slightly saner access pattern.

Change test infrastructure to use the files in testdata/ to generate
separate test files that are always removed after the test completes.
Switch most tests to use the sql storage.

Signed-off-by: Simo Sorce <[email protected]>
The secrets would enver be revealead, but the object could confuse an
application nonetheless, never attach a handle to them so they can never
be seen at the application level.
Streamline supporting functions as well.

Signed-off-by: Simo Sorce <[email protected]>
@simo5
Copy link
Member Author

simo5 commented Mar 25, 2024

@Jakuje I think I addressed all of the issues you raised

Copy link
Contributor

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to go!

@simo5 simo5 merged commit e178cf1 into latchset:main Mar 26, 2024
4 checks passed
@simo5
Copy link
Member Author

simo5 commented Mar 26, 2024

merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The PIN objects should not be propagated to the PKCS#11 interface
2 participants