Hostenv file storage #489

egor-duda · 2022-06-05T17:23:53Z

Part of implementation for #485

This PR implements a storage, backed by file on host OS, for persisting authenticator data between restarts of OpenSK application used for FIDO2 emulation for QEMU.

After changes to storage API there's no more need for double buffering of storage data.

Current read_slice() method signature declares storage object as immutable. This conflicts with File object being mutated by read operations (current file position is changed for and after read). To keep read_slice() signature unchanged, interior mutability via RefCell is used.

This allows running ctap2 authenticator application on non-embedded host OS to implement virtual FIDO2 authenticator for QEMU

read_slice(...) can return Cow::Owned buffer to the caller

coveralls · 2022-06-05T17:31:37Z

Coverage increased (+0.1%) to 90.027% when pulling cc1fb25 on egor-duda:hostenv-file-storage into 12f6ed6 on google:develop.

ia0

This overall looks good to me. I made a first pass of high-level comments. I left the more style-related comments for later.

libraries/persistent_store/Cargo.toml

libraries/persistent_store/src/storage.rs

libraries/persistent_store/src/file.rs

libraries/persistent_store/src/lib.rs

libraries/persistent_store/src/file.rs

ia0

Thanks for the PR!

kaczmarczyck · 2022-06-07T05:48:07Z

libraries/persistent_store/src/file.rs

+
+        if file_len == 0 {
+            file.seek(SeekFrom::Start(0))?;
+            let buf = vec![0xffu8; options.page_size];


We have generally used 0xFF. Is the u8 annotation is required?

Other instances of this exist below.

Right, for consistency with the rest of the library let's use 0xff if Rust is able to infer the type.

kaczmarczyck · 2022-06-07T05:52:33Z

libraries/persistent_store/src/lib.rs

@@ -365,6 +365,8 @@ extern crate alloc;
 mod buffer;
 #[cfg(feature = "std")]
 mod driver;
+#[cfg(feature = "std")]


Does this std condition correctly interact with the rest of the code? We use std for tests and fuzzing only so far. Therefore, I was wondering if you can just compile the OpenSK library with std later for non-testing purposes?

Yes, std means that we compile with the standard library (and thus an OS). We use std for tests and fuzzing because that's the only reason we need to compile on an OS. And this example is another reason now.

Note that once CTAP is a library we will compile it with std for non-testing purposes too. This library is just one step ahead because it's already a library and doesn't depend on Tock.

kaczmarczyck · 2022-06-07T05:53:34Z

libraries/persistent_store/src/storage.rs

@@ -36,6 +36,13 @@ pub enum StorageError {
    CustomError,
 }

+#[cfg(feature = "std")]
+impl From<std::io::Error> for StorageError {


Same here, another use of std that is actually necessary outside of tests.

kaczmarczyck · 2022-06-07T05:58:02Z

I resolved all of @ia0 's comments since approved. I don't mind merging, just had a nit and an open question about `std``, and if you already tried using this PR in your QEMU project and it all worked.

ia0

Not merging to first get a confirmation that this PR solves the problem of running with QEMU.

ia0 · 2022-06-07T08:19:30Z

libraries/persistent_store/src/file.rs

+
+        if file_len == 0 {
+            file.seek(SeekFrom::Start(0))?;
+            let buf = vec![0xffu8; options.page_size];


Right, for consistency with the rest of the library let's use 0xff if Rust is able to infer the type.

ia0 · 2022-06-07T08:21:50Z

libraries/persistent_store/src/lib.rs

@@ -365,6 +365,8 @@ extern crate alloc;
 mod buffer;
 #[cfg(feature = "std")]
 mod driver;
+#[cfg(feature = "std")]


Yes, std means that we compile with the standard library (and thus an OS). We use std for tests and fuzzing because that's the only reason we need to compile on an OS. And this example is another reason now.

Note that once CTAP is a library we will compile it with std for non-testing purposes too. This library is just one step ahead because it's already a library and doesn't depend on Tock.

egor-duda · 2022-06-07T13:15:32Z

Regarding std, I've tested my prototype code:

I run cargo run --features=std on linux host system and ssh-keygen -t ecdsa-sk on QEMU guest, then restarted qemu virtual fido2 device and opensk application and connected via ssh using generated ecdsa-sk key. Everything worked fine.

However, I've encountered two problems:

Current implementations of Store tries to convert max_page_erases and max_word_writes to Nat type, which fails in current implementation of this PR on 64-bit platform, trying to convert usize::MAX to u32
Format::is_storage_supported function disallows storages with max_page_erases > 65535

Should I change FileStorage::max_word_writes and max_page_erases to return Nat::MAX and 65535 respectively, or change functions in Format to accommodate current FileStorage max values?

ia0 · 2022-06-07T13:28:17Z

Should I change FileStorage::max_word_writes and max_page_erases to return Nat::MAX and 65535 respectively, or change functions in Format to accommodate current FileStorage max values?

Oops my bad, I forgot about that. Yes, your suggestion sounds good. Let's also document for each of them why we use those values because I'm pretty I'm gonna forget again.

fn max_word_writes(&self) -> usize {
    // We can write an unlimited amount of times in a file, but the store arithmetic
    // uses `Nat` so the value should fit in a `Nat`.
    Nat::MAX as usize
}

fn max_page_erases(&self) -> usize {
    // We can "erase" an unlimited amount of times in a file, but the store format
    // encodes the number of erase cycles on 16 bits.
    u16::MAX as usize
}

egor-duda added 2 commits June 5, 2022 15:08

Implement file-backed persistent storage

db26f61

This allows running ctap2 authenticator application on non-embedded host OS to implement virtual FIDO2 authenticator for QEMU

With new Storage API there's no need to double-buffer file data

1cf7373

read_slice(...) can return Cow::Owned buffer to the caller

ia0 reviewed Jun 5, 2022

View reviewed changes

egor-duda added 3 commits June 5, 2022 22:39

Use StorageError::CustomError for implementations-specific (I/O) errors

f2cb2f7

No need for specific feature, use std instead

c0299c3

Separate options type for FileStorage, remove hardcoded constants

4e47968

ia0 reviewed Jun 5, 2022

View reviewed changes

egor-duda added 2 commits June 6, 2022 15:06

Make FileOptions available to library's users

660b6b7

Improve variable names and comments readability

2708074

ia0 approved these changes Jun 6, 2022

View reviewed changes

kaczmarczyck reviewed Jun 7, 2022

View reviewed changes

ia0 previously approved these changes Jun 7, 2022

View reviewed changes

Use 0xff for consistency

0158cc8

egor-duda dismissed ia0’s stale review via 0158cc8 June 7, 2022 13:15

Accommodate Store requirements for max_word_writes and max_page_erases

cc1fb25

ia0 approved these changes Jun 7, 2022

View reviewed changes

Merge branch 'develop' into hostenv-file-storage

aea4c56

ia0 merged commit 4763c3a into google:develop Jun 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hostenv file storage #489

Hostenv file storage #489

egor-duda commented Jun 5, 2022

coveralls commented Jun 5, 2022 •

edited

Loading

ia0 left a comment

ia0 left a comment

kaczmarczyck Jun 7, 2022

ia0 Jun 7, 2022

egor-duda Jun 7, 2022

kaczmarczyck Jun 7, 2022

ia0 Jun 7, 2022

kaczmarczyck Jun 7, 2022

kaczmarczyck commented Jun 7, 2022

ia0 left a comment

ia0 Jun 7, 2022

ia0 Jun 7, 2022

egor-duda commented Jun 7, 2022 •

edited

Loading

ia0 commented Jun 7, 2022

Hostenv file storage #489

Hostenv file storage #489

Conversation

egor-duda commented Jun 5, 2022

coveralls commented Jun 5, 2022 • edited Loading

ia0 left a comment

Choose a reason for hiding this comment

ia0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaczmarczyck commented Jun 7, 2022

ia0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egor-duda commented Jun 7, 2022 • edited Loading

ia0 commented Jun 7, 2022

coveralls commented Jun 5, 2022 •

edited

Loading

egor-duda commented Jun 7, 2022 •

edited

Loading