genodelabs / genode

Genode OS Framework
https://genode.org/
Other
1.05k stars 248 forks source link

A graphical control interface for the CBE #4032

Closed m-stein closed 2 years ago

m-stein commented 3 years ago

In an offline discussion with @nfeske we created a plan for a first graphical CBE control interface. There will be a new component, let's call it CBE Manager that connects to the filesystem root of the CBE (for now, we assume that CBE device and Trust Anchor are presented through files), input, and graphical output. The component will use the sandbox API to start children for tasks it wants to delegate, much like, for instance, the Text Area application.

As a starting point, the CBE Manager will start an fs_query child in order to read the state of the filesystem root. Initially, it will be empty. This means that Trust Anchor and CBE device are still missing and the component has to start with initializing the Trust Anchor. In order to do so, it requires a passphrase from the user - a requirement that it communicates through a Menu View child. As soon as the user responds, the Manager will start a Trust Anchor Init child

If, however, the Trust Anchor is present on startup but the CBE device is missing, the CBE Manager will want to initialize a CBE device. In order to do so, it communicates through the Menu View child, that it first requires the passphrase to unlock the Trust Anchor, an then the parameters for CBE device initialization. As soon as the user responds, the manager will start a CBE Init child and wait for it to finish.

This approach will also apply when both Trust Anchor and CBE device are present at startup. Then the manager will unlock the Trust Anchor as depicted above, start the CBE driver, request it to read some device info and display this info in the menu view together with available CBE device commands.

These will include long-running commands like rekeying or resizing, that are reflected through files in the FS. These files can again be used by the manager to reflect the command state in the Menu View.

nfeske commented 3 years ago

Thanks for posting this plan. I'm eager to see how it will work out.

m-stein commented 3 years ago

I have the initialization of trust anchor and cbe device image in place and could create the main window for the cbe device controls today. In this course I reworked my dialog rom generation to use generic utilities. The state can be found on my branch "4032_A_graphical_control_interface_for_the_CBE".

nfeske commented 3 years ago

Thank you for the update.

BTW, your issue with the removal of fs_query should now be resolved with the fix of issue #4044. Maybe, you can give it try?

m-stein commented 3 years ago

@nfeske Just tried it out and it works like a charme :) Thanks for fixing this so fast!

m-stein commented 3 years ago

This commit series provides a version of the CBE manager that can be deployed in Sculpt (see my depot index) and used to persistently store data on the automatically created ext2 fs that is provided by the manager through an FS service. This version, however, is not thought for productive use and you should refrain from storing sensitive data with it!. Please also read the "Open issues" paragraph in gems/src/app/cbe_manager/README before using the package.

d670efcd1b cbe_manager: GUI for controlling CBE devices 7d429543fb trust_anchor_vfs.h: fix bug in key decrypt b566aaf7f9 vfs/cbe_trust_anchor: sync keyfile-handle close 0a06215e06 vfs/cbe_trust_anchor: fix unlocking 874810cd4d vfs/cbe_trust_anchor: close handles correctly e7d1dffff8 vfs/cbe_trust_anchor: sync hashfile-handle close 91b445748a vfs/cbe: control/deinitialize file f365fe96b2 vfs/cbe: fix ID argument on discard_snap 9c3fdf889c vfs/cbe: support watching the snapshots fs 43aa1c0f4e vfs/cbe: fix result of SnapshotsFS.num_dirent("/") 412e257457 vfs/cbe: support watching "rekey" file 8561624f95 vfs/cbe: support watching "extend" file b868b08695 vfs/cbe: mark extend/rekey fs readable 01bcff975a vfs/cbe: fix size of extend/rekey fs c6681ede3b cbe/types: INVALID_GENERATION, Generation_string d6ebf9c6a9 recipes/src/cbe: build also cbe_init_trust_anchor 1d331f60a7 gems/recipes: api/cbe 29ad3c5100 gems: update cbe port 17f7e0a7b8 fs_tool: add operation fd6fcb1360 fs_query: gracefully deal with missing directory 9c718ee92a fs_query: watch only readable files 8b0e953f91 fs_query: read content only from readable files 3a0eb0b9cd vfs lib: move implementations from header to unit b4d3447228 os/recipes: src/pointer 3abff8df7e text_area: typo fix in child_state.h

nfeske commented 3 years ago

Thanks for sharing, @m-stein! I just gave your package found in your sculpt index a try.

The initial test to deploy the component, route both the trust anchor and the back-end file systems to the recall fs, write to the encrypted file system using another system-shell instance, restarting the system, typing in my passphrase again, and getting back to my system-shell prompt with access to the unencrypted data. That is really cool! The CBE suddenly becomes tangible. Thank you so much!

I also clicked on the other buttons and could observe some activity, like a long-taking resize operation. :-) But I haven't stressed those other buttons too much.

The current state is already very nice to test-drive the CBE. In the longer run, in particular once we start offering the component the Sculpt users, I would love to see the following points for improvement covered. (of course, I'd offer a helping hand)

Please don't take this rather long list as complaints. I just wanted to write down my initial thoughts after giving your package a try. That said, I hope that we can address those points over time to make this component a regular companion of Sculpt.

m-stein commented 3 years ago

@nfeske Thanks a lot for trying out the new package so fast and for sharing your thoughts in such a detailed way!

The component is still a bit too chatty to embed it into a regular
work scenario. I think the status messages may better be presented
as status info in the dialog, if needed at all.

This is only because it is still a premature version of the program. My plan would be that the final version doesn't print anything to the log when not explicitely asked to (similar to the NIC router). Of course, this includes the graceful handling of most (better all) error conditions with reflections to the UI if needed.

I wondered about finding my passphrase stored in the trust anchor's
keyfile. I can only guess that this is a debugging feature?

As stated in the "open issues" section of the README, the key management is still very primitive. I'm already working at this as it seems the most important problem to me. My current approach is that the file 'keyfile' will be replaced by a file 'encrypted_private_key' which contains a PRNG-generated private key that has been encrypted with AES256 using the SHA256 hash of your passphrase.

Unlocking the CBE device will then consist in first decrypting the private key with the hash of the entered passphrase and second decrypting the symmetric block-encryption keys (found in the superblock of the CBE device) using the plain private key.

What I'm not sure about, so far, is how to detect that a given passphrase was wrong. In the depicted unlocking process, a bad passphrase would simply lead to reading bogus data from the CBE device. But I'm doing research regarding this issue.

The UI does not follow the patterns of the Sculpt manger very
closely. Be should strive for more consistency.

  o The monospaced font looks out of place.
  o The Sculpt UI follows some kind of principle that I would
    subsume under the term "calmness". Go out of the way. Don't be
    chatty. Avoid repetition. Show only what is really needed and
    important. Avoid text. If text cannot be avoided, make it as
    short as possible. The current version of the CBE manager
    (understandably) does not adhere to these guiding principles
    because they are not documented anywhere.
  o The use of buttons to accentuate title text is misguiding. Such
    text should better be presented using the title font (it is
    slightly larger than the regular font). But maybe (speaking of
    "calmness"), the titles can just be dropped?
  o The dialog is too busy for the common case. With this case, I
    mean unlocking the crypto fs (entering a passphrase), using the
    storage (here the dialog should fold to just two buttons, "lock"
    and "manage"), locking the crypto fs (going back to the
    passphrase entry). Only when pressing the "manage" button, the
    advanced options should appear, similar to how the storage
    dialog of the leitzentrale works.
  o Thougout Sculpt's leitzentrale, I try to center dialog elements
    whenever possible to give the dialogs a balanced look, roughly
    following Apple's style guidelines for Mac OS X. In contrast,
    your dialogs tend to align most things to the left.

Regarding the font, I'm definitely with you - it's an artifact. Regarding your principles of text usage and style I sense that I have a slightly different idea. Let's better talk about this in person. As a general thought, I struggle with the use of absolute terms (e.g. "The dialog is too busy for the common case") when I have the feeling you're actually expressing your personal taste instead of an agreement we came to as a group.

When creating a crypto fs, it would be nice to request the
passphrase twice to lower the change for typing mistakes. Or,
alternatively, the passphrase entry could be presented in clear
text. After all, this procedure is done only once at install time.
So the risk of eavesdropping seems rather contrived, in my opinion.
(or the clear-text display could be toggleable)

I'm absolutely with you. This was the plan for the next version and I forgot to list it in the "open issues" section.

Maybe, the current size could be displayed in the device info?
Speaking of the term "device", it seems a bit misplaced here. I'd
try avoiding this term because the component does not touch any
hardware.

Regarding the size info, definetly for the next version. But it requires a bit of background work and so I dropped it for the first version. Regarding "device", I'll try to come up with something else.

In the context of the resizing operation, it would be nice to see
the current size displayed.

This is done (although not always correctly yet as stated in "open issues").

It is nice that tab works to move the cursor to the next entry field.

Thanks. I'd like to be able to control the whole program using a keyboard.

I like the on-the-fly calculation in the creation dialog while
typing in the size. The size dialog takes the number of bytes as
arguments but the resize dialog does not show the size in bytes
though. Could both dialogs become more similar?

In my resize dialog the current/future image size is shown in bytes. If you're referring to the input value, however, this is done in number of blocks as the granularity of this operation is the size of a block. The value could, of course also be entered in bytes and the program would then divide and ignore the rest (e.g. 14K would result in a resizing by 3 blocks ignoring the remaining 2K).

Input validation seems largely be missing. For example, I can type
any letters in an entry field that should accept only numbers. In
the creation dialog, the entry dialog should better not accept
characters other than numbers and 'K', 'M', and 'G'. The entered
value should satisfy a minimum bound as a precondition for the
create button. E.g., when entering '40' (bytes), the mkfs step fails
and the dialog gets stuck. When restarting the CBE manager
afterwards, it aborts early with an |Unexpected_state| exception. I
think such exceptions must eventually be reflected to the user (by
showing a message) along with potentially presenting a way to take
action (re-initialize?).

I dropped this for the first version. Forgot to mention it in the "open issues" but will implement it in the next versions.

The package should have a descriptive name. The term "CBE manager"
does not draw the connection to the purpose of the package. A name
like "file vault" would make this much more obvious.

I've spent some time thinking about it but didn't find one that I like, so far. Will change this.

The file-system session label "back_end" should better be named
"data". When routed to the recall fs (that is what I would be
doing), the corresponding directory would then be named "data",
which makes intuitively more sense.

Good idea! But which directory you're referring to?

The state file contains a number. Wouldn't it be nicer to have the
state displayed in text form?

I also thought that. It was, however, easier with numbers while the state names still changed often, but this isn't the case anymore. Should I also embed the value in an XML node (e.g., <state name="init_obtain_parameters" />)?

The shutdown button should better lead back to the passphrase entry.
I would probably use it for locking the system, not necessarily
shutting it down. So maybe "Lock" would be a better name for the button?

I'm using the button for actually shutting down the application and I sense that hinting the user with such a button that simply killing the program can cause data loss may be a good idea. But I understand your need for a lock semantic and would opt for a dedicated button (side note: it seems to me that locking isn't exactly the same as just going back the passphrase dialog as we should keep the client FS session open, shouldn't we!?).

It took me a while to find out why nothing happened when pressing
the discard snapshot button. One needs to select a snapshot first.
It is not obvious that snapshots can be selected. The selected state
is presented as a text selection rather than a selected checkbox
button. I think it would be better to just have button
"Snapshots..." (the three dots show that another dialog is behind
the button). Once when pressing it, the existing snapshots would
appear under the botton as a list of buttons or radio buttons. Only
when selecting one, a discard button appears. This would foster the
consistency with the Leitzentrale, e.g., the way a wifi accesspoint
is selected and activated.

I'll give it a try. I also found that the "discard" button could be displayed only when having a snapshot selected.

Please don't take this rather long list as complaints. I just wanted to write down my initial thoughts after giving your package a try. That said, I hope that we can address those points over time to make this component a regular companion of Sculpt.

Thanks for communicating this to me! I value your input a lot and didn't sense the length of your list inappropriate. I'll try to prioritize each issue in a sensible way!

chelmuth commented 3 years ago

I share your sentiment that the key protection must get top priority before the CBE can be applied in practical use cases. Therefore I checked my favorite key management and cloud vault tools for technical documentation of the key protection mechanisms. Cryptomator brings detailed documentation of its security architecture including key protection and was also independently audited by security researchers.

As stated in the "open issues" section of the README, the key management is still very primitive. I'm already working at this as it seems the most important problem to me. My current approach is that the file 'keyfile' will be replaced by a file 'encrypted_private_key' which contains a PRNG-generated private key that has been encrypted with AES256 using the SHA256 hash of your passphrase.

Unlocking the CBE device will then consist in first decrypting the private key with the hash of the entered passphrase and second decrypting the symmetric block-encryption keys (found in the superblock of the CBE device) using the plain private key.

What I'm not sure about, so far, is how to detect that a given passphrase was wrong. In the depicted unlocking process, a bad passphrase would simply lead to reading bogus data from the CBE device. But I'm doing research regarding this issue.

The algorithm used by Cryptomator is AES key wrap RFC 3394, which also solves the Key Data Integrity issue in Chapter 2.2.3.

It may be too time-consuming to implement a full standard mechanism at the moment, but we definitely have to put RFC 3394 or an alternative like Argon2 (used by Keepass/X/XC) on the agenda. For now, the information above could serve as an inspiration.

m-stein commented 3 years ago

@chelmuth Wow, splendid! Thanks for this helpful input! I'm already done with my above mentioned approach and it works fine but I will dig right into the documentation you sent and see what I can improve.

m-stein commented 3 years ago

The next version of the package that can now be found under "mstein -> Tools -> File Vault" is out. The package should be compatible to Sculpt 21.03b. It would be really cool, if everyone could give it a try and give me some feedback! Note that this version isn't compatible with vaults created with the last version.

These are some of the improvements since the last version:

m-stein commented 3 years ago

My merge_to_staging branch contains the commits ready to be merged.

nfeske commented 3 years ago

Thanks @m-stein for the new version and the convenient package!

I'm writing this comment via the Falkon web browser that stores its entire state (config, cookies, bookmarks) in a file-vault directory. I have added a copy of the launcher/recall_fs that I call launcher/vault_recall_fs with the file-system route going to the file vault. This way, I can route arbitrate file-system sessions to subdirectories of the file fault by simply selecting vault_recall_fs.

To answer your request for feedback:

Thanks again for the new version. I'll keep using the Falkon setup for my GitHub activity. In case of data loss, this would not be an issue because my browser state is not precious. Once it stands the test of time, I'll entrust more and more sensitive data in the hands of the file vault (in different recall-fs directories of course).

m-stein commented 3 years ago

I found that we need another fixup ae40c28fa9 in order to make run/cbe_tester succeed again.

Btw., @nfeske do you think we can merge the file vault commits on my merge_to_staging branch?

I would have loved to add an automatic test before merging but, AFAICT, this would be some work from the point where we are now.

nfeske commented 3 years ago

Btw., @nfeske do you think we can merge the file vault commits on my merge_to_staging branch?

I have no reservations but I need to schedule enough time to review the commit series. Please bear with me.

nfeske commented 3 years ago

I merged 190f475abc2a4afcc1dca13609ac6f4a35b4c83f and 96fda25ad501d3c22585255e89b27c4f63589764 to staging but wondered if we shouldn't better remove the diagnostic messages instead of leaving them in the form of dead code for the rare event that they may be useful. @cnuke do you deem the messages worth keeping or could you live with adding such instrumentation on the fly when needed in the future?

cnuke commented 3 years ago

Sure, if those messages are now guarded by a compile-time flag I agree that removing them completely seems more reasonable.

(That being said, some of them have informational value that for the time being is not provided otherwise, e.g. space available in the file system. For various reasons the LOG is not the right place for that but I just want to point out to keep that in mind.)

nfeske commented 3 years ago

Merged ebc22565b6577b866a7cd5b19947dd16fdbd043b as 4de2d27cd26fc2ba95565c7077532a5158d91a4f with the braces around the single statement removed.

Merged bbf3d3d1ace498eecb40f2662570a2de8e3edef6 as 18a243edd8ce62638305dc13650b74bbce904f43 with a similar style adjustment.

Commit fdb1b3d4bb70faa30691f1d77a1e866c466a090a results in an empty report if one query (of potentially many) refers to a non-existing directory. It would be better to produce a report with the remaining query results covered.

nfeske commented 3 years ago

Merged 939b3f4391c7549ee7a1132c45ed0b74e35d22de as e392bee4d493aab260c78c23145e00a5742ce50d with braces removed.

nfeske commented 3 years ago

I have added a commit d9eea75d2282ccf93da773c84795d451ee520fa7 moving the New_file utility from the gems/src/app/text_area/new_file.h to os/vfs.h.

Based on this commit, I reworked the fs_tool commit in 3628e7e323d137cca75afe1fc09dc6dea3b65525, simplifying the code, extending the test-fs_tool runtime. @m-stein, could you rebase the remaining commit series to the state on staging and give it a spin?

m-stein commented 3 years ago

@nfeske I'll rebase test and come up with a new branch with my adaptions.

m-stein commented 3 years ago

@nfeske I've updated my merge_to_staging branch.

nfeske commented 3 years ago

I force-pushed staging with the re-arranged commits regarding the New_file utility. I also extended the test in the fs_tool commit.

@m-stein, please excuse that I have to ask you again for rebasing your branch.

m-stein commented 3 years ago

@nfeske No problem, rebased, file vault still works fine.

m-stein commented 3 years ago

Found a fixup for rump fbdd9f3c1b.

m-stein commented 3 years ago

Updated the fixup to 51e52fe2db according to a suggestion of @chelmuth

chelmuth commented 3 years ago

Thanks, merged to staging.

jschlatow commented 3 years ago

@m-stein I just gave your file_vault package (2021-05-17) a try. First of all, I really enjoyed the ease of use. I am going to use it with the falkon browser for testing.

I made a few observations though, which I'd like to share:

m-stein commented 3 years ago

@jschlatow Thanks for integrating the file vault in your workflow!

Regarding your observations:

jschlatow commented 3 years ago

@m-stein Regarding the font sizing, you're right it's consistent with the "+" menu. Interestingly, I haven't noticed this so far, probably because the "+" menu has more items and is vertically centered. In the file vault, the menu title appears right at the place where a normal item has been before and on which my eyes are currently focused. I believe this is why it feels different for me.

nfeske commented 3 years ago

The frame around the menu-view dialogs is caused by the PNG used as default frame style (https://github.com/genodelabs/genode/blob/master/repos/gems/src/app/menu_view/styles/frame/default/background.png). This makes the frame pretty whenever window decorations are absent, like in the leitzentrale. But when hosting a menu view in a window with window decorations, there is gap. To solve this issue, we'd need to externalize the menu view style (I hinted at this direction in https://github.com/genodelabs/genode/issues/4135#issuecomment-834139491) analogously to the fonts_fs approach. So when deploying an application with a window manager, we could route its "style fs" to a file system where the top-level frame has no gap.

That said, I would not make this topic a priority for now.

nfeske commented 3 years ago

Regarding the style of the file-fault menu, I think that the "+" menu is not a suitable template to follow in general. It is a special case of a deeply nested structure with any items on most levels. So the notion of "browsing" the menu springs into mind, which is somehow reflected by the symbols used.

However, if a menu structure is rater simple, the notion of "browsing" does not apply. The concept that works for the "+" menu feels a bit forced in this case. Hence, in https://github.com/genodelabs/genode/issues/4032#issuecomment-836440714 I suggested using plain buttons instead, following the pattern used in the storage dialog of the leitzentrale. Maybe it could be considered in future version?

m-stein commented 3 years ago

I just noticed that my answer to Normans last feedback on the UI wasn't added to this issue but only send via mail. I'm sorry for that! Here's the missing message in which I make my suggestion regarding the menu structure:

@nfeske It's cool to see that you already integrated the vault into your daily workflow! Thanks also for the quick and detailed feedback!

  • The user interface of the new version is much improved! Especially the setup dialog has become a pleasure to use. Thanks for putting so much thought into it. Thanks, I'm glad you enjoy it!

  • Maybe you could consider changing the order of || nodes in the runtime file? The first entry has a special meaning. It defines the anchor where the component should appear in the component graph. Right now, this is the |rm| route, which has no representation in the graph. So the file fault appears floating at the left. I think it would be most sensible to make the data file system the first entry. So the connection of the file fault to the backing store is always visible at first glance.

Ah, I didn't know this magic :) Of course, I will change it. It annoyed me as well that the node would appear disconnected.

  • Even though the main window has become more handy, it still feels and looks a bit unnatural to me and takes away too much screen space. To explain my concern, I'd like to add the window to my always-visible overlay layer, like I do for the battery applet and the top view (in minimized mode. Right now, the size of the dialog rules out this option. Here are some suggestions: o I'd skip the title because there is already a window title.

I'm fine with that.

  o The title font should better not be used for the menu items.

Ok.

  o As far as I can see, there are only three functions "Expand
    client fs", "Expand journalling buffer", and "Replace
    block-encryption key". I like the sensible naming of these
    functions but I would prefer three simple buttons over the
    nested menu structure.

As far as I am concerned, the number of options could soon increase. Replacing the passphrase and the master key are, as I sense it, important features and relatively easy to achieve. The latter also applies for accessing and controlling snapshots.

Moving from the exclusive-opening menus to integrated menus, however, is some work as a good part of the internal state model of the vault is tailored to the way menus work. Maybe we can skip this transition for now?

  o The inability to expand the client fs is revealed only after two
    mouse clicks (click on the "Dimensions" item, click on the
    "Expand client fs" item). I think the whole topic
    (Dimensions/Expand) should better disappear from the GUI
    whenever it is inaccessible, reducing the size of the dialog
    when in use.

Expanding the journalling buffer is always possible, so, the "Dimensions" menu wouldn't disappear. However, I admit that letting the "Expand Client FS" menu disappear sounds cool at first sight, but this would completely hide a feature without hinting the user about it (I personally wouldn't deduce from your suggested "2 users" info that there is something hidden unless I remove the users).

Also, why would I need to see from the top view that expanding the client FS isn't possible if the operation is, as you already mentioned, very specific and required only in rare cases?

On the other hand, if I made the effort to "dive down" to the expand menu and see that it's not possible currently, I don't have to exit the menu again in order to fix this problem. As soon as I remove the user, the opened menu will switch immediately.

  o The status could be put on a single line to save screen space.
    "Image: 20 MiB, File system: 16 MiB, 2 users". The number of
    users/clients is useful to keep in sight because it restricts
    the expand options.

Ok, I'll give it a try. May we change "users" to "clients" in order to stay conform with the name "client FS"? Or should we change "client FS" to "user FS"?

Thanks again for the new version. I'll keep using the Falkon setup for my GitHub activity. In case of data loss, this would not be an issue because my browser state is not precious. Once it stands the test of time, I'll entrust more and more sensitive data in the hands of the file vault (in different recall-fs directories of course).

Sounds very good to me! I'll integrate the vault also into my workflow.

nfeske commented 2 years ago

I finally merged your commit series to staging today. Please excuse the long delay. While browsing through the commits, I found the clean separation between the cbe, trust anchor, tools, and file fault very nice. Thank you for the impressive work!

In the future we may consider consolidating the bureaucracy of the various VFS plugins. The code looks quite repetitive at places, which is not your fault but rather hints at current limitations of the VFS-internal interfaces and utilities.

We may also consider replacing the hardwired use of jitterentropy by the use of random numbers obtained from a separate file in the cbe_trust_anchor. So it would be up to the integrator to supply a random number generator like the vfs_jitterentropy plugin or alternatively a driver for a dedicated random-number device.

m-stein commented 2 years ago

@nfeske Thanks for having reviewed the code! I like your suggestions a lot. Especially cleaning up the VFS plugins further was already on my TODO list.