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

[4.2] Write vm-specific config values to qubesdb #936

Closed
3 tasks done
rocodes opened this issue Jan 16, 2024 · 21 comments · Fixed by #1051
Closed
3 tasks done

[4.2] Write vm-specific config values to qubesdb #936

rocodes opened this issue Jan 16, 2024 · 21 comments · Fixed by #1051
Assignees

Comments

@rocodes
Copy link
Contributor

rocodes commented Jan 16, 2024

Tracking/discussion. Let's make use of .vm-config part of qubesdb (internal to a VM) to store vm-specific configuration. Eventually this will allow us to get rid of config.json.

Upstream documentation:
https://dev.qubes-os.org/projects/core-admin-client/en/latest/manpages/qvm-features.html#vm-config

Configurable components

Status Value dom0[1] sd-app sd-gpg sd-log sd-proxy sd-whonix
Drafted $QUBES_GPG_DOMAIN $QUBES_GPG_DOMAIN[2]
n/a config.json:.environment /etc/apt/sources.list.d/securedrop_workstation.list
Drafted config.json:.hidserv.hostname /home/user/.securedrop_proxy/sd-proxy.yaml[2] /var/lib/tor/keys/app_journalist.auth_private[3]
Drafted config.json:.hidserv.key /var/lib/tor/keys/app_journalist.auth_private[3]
Drafted config.json:.submission_key_fpr /home/user/.securedrop_client/config.json[2]
n/a config.json:.vmsizes.sd_app qvm-volume resize sd-app:private ...
n/a config.json:.vmsizes.sd_log qvm-volume resize sd-log:private ...
salt://sd/sd-journalist.sec /home/user/.gnupg/sd-journalist.sec[4]
  1. Out of scope of this ticket; just documenting here for completeness.
  2. Program reads directly
  3. Helper program templates into file contents
  4. Secret: install directly into domain and remove from dom0

NB. Only testable on Qubes 4.2, where QubesDB starts to return vm-config.* as /vm-config/*.

Related

@cfm
Copy link
Member

cfm commented Jan 19, 2024

Thanks for starting this ticket, @rocodes. I've expanded your list into a table tracing each value currently defined in config.json to each place where it's used, then categorized how.

Here's a straw proposal for how to handle each type:

  1. Removing config.json from dom0 is out of scope of this ticket. It can remain the source of the values that are injected into individual VMs via vm_config.*. But (a) no VM should have a config.json, and (b) it should be possible to replace config.json as the source in dom0 without making any changes to or within VMs.

  2. Within a VM, either:

    1. vm_config.* should be expanded into environment variables automatically; or
    2. we should provide a wrapper like with-vm-config $PROG that runs $PROG with vm_config.* expanded into environment variables.
  3. Once we have (2), we can use echo and envsubst to write individual values from vm_config.* to the arbitrary files required by Tor, GPG, etc.

But we don't have to settle this now. Next week I'll start working on the Salt states to get these values into vm_config.* in the first place.

@cfm cfm self-assigned this Jan 19, 2024
@cfm cfm moved this from Ready to go to In Progress in SecureDrop dev cycle Jan 19, 2024
@legoktm
Copy link
Member

legoktm commented Feb 5, 2024

I think config.json:.environment + /etc/apt/sources.list.d/securedrop_workstation.list will probably merit a different mechanism (or at least it might not make sense to consider here) because 1) it goes into the templateVM, not an appVM/dom0, and 2) it's only used by developers, not for prod installs (which might be a reason to not put it in config.json at all).

@legoktm
Copy link
Member

legoktm commented Feb 6, 2024

vm_config.* should be expanded into environment variables automatically; or

I'm not really sure how to do this, but if it's possible and straightforward, it would be my preference just because it avoids us from needing to prepend with-vm-config in front of nearly everything we run.

Once we have (2), we can use echo and envsubst to write individual values from vm_config.* to the arbitrary files required by Tor, GPG, etc.

In principle I agree with this as step 3, but I would rather write small Python/Rust scripts that do this interpolation instead of relying on bash scripts and retrofitting error handling into them.

@legoktm
Copy link
Member

legoktm commented Feb 6, 2024

@cfm and I discussed this more just now, starting with focusing on the proxy's needs and then looking at everything else.

We started looking at point 2 of the straw proposal, and noticed that whether we set everything in the environment (likely via a shell), or use a with-vm-config wrapper, there is some intermediary between the process.

Then we moved to, if we have to write/implement this intermediary, why not just stick it in the application itself? In the proxy v2 we read from an environment variable, which is great for non-Qubes testing, but there's no reason it couldn't also check with qubesdb first or after. The only other user we control is the client, which could have the same logic too (just in Python).

The cases we don't control are Tor and GPG, which require us to write to a file, so we will already need some script to do that, which can read from QubesDB.

@zenmonkeykstop
Copy link
Contributor

zenmonkeykstop commented Feb 6, 2024

Just as a general note, I'm a little skittish of managing the contents of sd-journalist.sec through qubesdb - the impact of a compromised vm getting hold of the fingerprint is zero (it's public) and of the JI address is mitigated by the login+2FA requirement, but the key has no protection beyond qubes-rpc policies and general VM isolation. It might be worth considering continuing to manage that one thing via the initial provisioning and not having it available in dom0 (ideally at all after the fact).

(I'm thinking of a bug or config error that makes vm-specific qubesdb entries available to unspecified vms)

@cfm
Copy link
Member

cfm commented Feb 6, 2024

It's a fair point, @zenmonkeykstop. The complications I see are that:

  1. we have decided we don't want provisioning to reach inside individual VMs once templates are configured; and

  2. if the key material is available in a VM, it is available in dom0, by definition.

Nothing we do will change (2). But I'll think more about how we can reconcile (1) and your concern, which as you note is relevant just in the case of a bug, without requiring dom0 compromise per se.

@zenmonkeykstop
Copy link
Contributor

(1) is more of a guideline than an actual rule. This is a valid exception IMO.

@cfm
Copy link
Member

cfm commented Feb 7, 2024

I agree, @zenmonkeykstop, except that to my eye it's a significant-enough exception that it may mean that the guideline itself is wrong (or at least incomplete). :-)

Today I started looking into how the admin.vm.feature.Get API is implemented to understand what its isolation properties actually are (versus, say, Salt's). I'll have more details here tomorrow.

@rocodes
Copy link
Contributor Author

rocodes commented Feb 7, 2024

I think @zenmonkeykstop's point about sd-journalist.sec is fair, since we're not trying to protect the key from dom0, but from untrusted vms, and since only relying on one form of isolation does kind of break with our longstanding efforts of defense in depth.

I also don't think it has to be all or nothing; if we start with the non sensitive (or less sensitive) credentials we will still have made significant headway in terms of making the configuration flexible and easier to reason about.

I haven't thought it all the way through, but one idea could be, rather than putting the private key itself in qubesdb, to put one (or more) flag entries representing valid keys that are accepted for decrypting submissions. (For example, if we write pubkey fingerprint to qubesdb but still require the addition of the private key to the vm itself). This doesn't move us towards a disposable vault vm yet, but sets us on a path to using qubesdb to reason about the system's provisioning. Just trying to brainstorm a little here.

@cfm
Copy link
Member

cfm commented Feb 7, 2024

Oh, the more I consider @zenmonkeykstop's concern in #936 (comment), the more convinced I am by it. I'm just trying to spell out the implications. Bear with me.

We're trying to triangulate (read: test against reality :-) three of our work-in-progress principles, namely:

  1. Prioritize statelessness/disposability.
  2. Public values can be global.
  3. One true way to enforce system configuration.

What I think we've just discovered is that as written there is a tension between (1) and (3) versus (2). That is, in assuming (1) and (3) for the purposes of this ticket, we've run up against the edge of our own comfort with (2). I think we're just missing half of it:

  1. (a) Public values can be global.
    (b) Secret values must be as tightly scoped as possible and persist in as few places as possible.

@zenmonkeykstop, I hear you prioritizing (2)(b) as an exception to (1) and (3). @rocodes, I hear you prioritizing (2)(b) as the thing we must preserve, versus (2) and (3) as things we should work towards incrementally. Acknowledged! My hypothesis is that we can still find a way to preserve (2)(b) with (1) and (3), especially given that (1) is a security goal in itself.

Here's one version of what this could (eventually) look like:

  • sd-journalist.sec never enters QubesDB.1 → (2)(b)
  • sd-journalist.sec is also not configured with Salt. → (2)(b)
  • Instead, the RPM package that (eventually) manages sd-gpg includes a scriptlet that (i) reads sd-journalist.sec, (ii) qvm-move-to-vms (not copies) it into sd-gpg's DVM template, and then (iii) ingests it in sd-gpg's DVM template via qvm-run. → (1), (2)(b), (3)
    • This has a nice side effect: changing the private key is logically equivalent to uninstalling and reinstalling sd-gpg's package.

Now, this approach would be out of the scope of this ticket, which is now all about (2)(a), excluding sd-journalist.sec. But my point is that this is an opportunity to refine our security and architectural properties so that they can apply in more situations, rather than carving out exceptions that over time require us to make case-by-case decisions.

Footnotes

  1. For what it's worth, I did some spelunking yesterday and learned that each domain's qubesdb-read talks to a dedicated (per-VM) QubesDB daemon. That QubesDB daemon then queries qubesd, which is the only process that has unrestricted access across all domains' QubesDB stores, including features.

@rocodes
Copy link
Contributor Author

rocodes commented Feb 9, 2024

I'm sorry, @cfm, I had a bit of a hard time following your last, specifically this part:

@zenmonkeykstop, I hear you prioritizing (2)(b) as an exception to (1) and (3). @rocodes, I hear you prioritizing (2)(b) as the thing we must preserve, versus (2) and (3) as things we should work towards incrementally. Acknowledged! My hypothesis is that we can still find a way to preserve (2)(b) with (1) and (3), especially given that (1) is a security goal in itself.

I don't think I need it re- explained, I'll touch base with you oob if I need. (I was just +1'ing kev about sd-journalist.sec sensitivity, not necessarily stating a design principle/constraint).

I don't think we have to all or nothing our disposability efforts (note: I don't think disposable == stateless; secrets are still somewhere!). I think we should make a plan to start with other components and then revisit the vault vm, which seems like what you and others are maybe saying too. I will add some thoughts on vault key stuff in a followup comment.

@rocodes
Copy link
Contributor Author

rocodes commented Feb 9, 2024

Followup thoughts about vaults and keys

  • I'm in favour of storing sd-journalist.sec in a (tightly controlled) vm, not in dom0. Storing fewer things in dom0 means a safer and happier backup/restore and uninstall story, among other things. I'm open to being convinced otherwise but this is my initial impression.
  • re key provisioning: There's a manual step for users no matter what, because they have to import their airgapped key Somewhere (whether it's to dom0 or a vm). Right now we have users import temporarily to the vault, then copy to dom0, then provision to sd-gpg once it's ready. It seems like there are less convoluted ways we could do it, including provisioning the system with a user-specified pubkey fingerprint, going through provisioning on that assumption, and checking at runtime to make sure the matching key is present.
  • If we have to write the key to the vault's dvm template anyway, would it not be better to only write it there (vs there and dom0)?
  • I agree with the conceptual idea of "changing the key is like uninstalling/reinstalling the package in the dvm template." For example, we could have a specific rpc policy allowing only the dvm template to request a key refresh command to dom0 via qrexexc. If the user updates the key, we'd make a qrexec request to dom0 to prompt the user and if we get user confirmation, update the fingerprint anywhere else updates are needed. But once we're solving for this we've solved a bunch of other stuff first :)

@cfm
Copy link
Member

cfm commented Feb 22, 2024

Thanks, @rocodes! I think the principles you've expressed in #936 (comment) are consistent with the approach I outlined at the end of #936 (comment).

@cfm
Copy link
Member

cfm commented Feb 29, 2024

@legoktm in #936 (comment):

[I]f we have to write/implement this intermediary, why not just stick it in the application itself? In the proxy v2 we read from an environment variable, which is great for non-Qubes testing, but there's no reason it couldn't also check with qubesdb first or after. The only other user we control is the client, which could have the same logic too (just in Python).

The cases we don't control are Tor and GPG, which require us to write to a file, so we will already need some script to do that, which can read from QubesDB.

In Python, it turns out that we can do this trivially via QubesDB.read() in the Python QubesDB bindings. The caveat is how we import them from within our virtualenv, since they're available from the Qubes Apt repository as python3-qubesdb, not as a Python package from PyPI.

In Rust: @legoktm, do you see a reason not to call qdb_read() by linking against libqubesdb?

@legoktm
Copy link
Member

legoktm commented Feb 29, 2024

In Python, it turns out that we can do this trivially via QubesDB.read() in the Python QubesDB bindings. The caveat is how we import them from within our virtualenv, since they're available from the Qubes Apt repository as python3-qubesdb, not as a Python package from PyPI.

Nice! I think we can do the same thing that we do for the PyQt packages in the client, which is create the virtualenv with --system-site-packages, so then it becomes:

try:
    import QubesDB
except ImportError:
    # Inside dev environment
    QubesDB = False

In Rust: @legoktm, do you see a reason not to call qdb_read() by linking against libqubesdb?

Seems reasonable to me, as long as the Qubes team is providing a stable ABI (aka they're fine with us doing so). We should probably be aware of QubesOS/qubes-issues#8406 and do whatever necessary CString handling in Rust. (I do wonder if they'd eventually have their own Rust wrapper given QubesOS/qubes-issues#6614).

I think we'd do something at compile time, so something like:

#[cfg(feature = "qubesdb")]
fn read(name: &str) -> Option<String> {
    todo!("FFI wrapper around libqubesdb")
}

#[cfg(not(feature = "qubesdb"))]
fn read(name: &str) -> Option<String> {
    env::var(name).ok()
}

And then cargo build --features qubesdb --release

@cfm
Copy link
Member

cfm commented Mar 1, 2024

Thanks for the reality-check, @legoktm. I've drafted the Python version in freedomofpress/securedrop-client#1883 (with the caveat in freedomofpress/securedrop-client#1882) and started work on the Rust version.

@legoktm
Copy link
Member

legoktm commented Mar 4, 2024

Here's an alternative way to install the Python QubesDB library that doesn't rely on --system-site-packages:

diff --git a/client/pyproject.toml b/client/pyproject.toml
index bc317cf3..740f45bf 100644
--- a/client/pyproject.toml
+++ b/client/pyproject.toml
@@ -42,6 +42,7 @@ pytest-qt = "^4.2.0"
 pytest-random-order = "*"
 pytest-vcr = "*"
 pytest-xdist = "^3.0.2"
+qubesdb = { git = "https://github.com/QubesOS/qubes-core-qubesdb", branch = "release4.1", subdirectory = "python"}
 semgrep = "*"
 translate-toolkit = "*"
 types-polib = "*"

Then after poetry install:

user@dev ~/g/f/s/client (main)> poetry run python
Python 3.9.18 (main, Dec 18 2023, 00:00:00) 
>>> import qubesdb
>>> qubesdb
<module 'qubesdb' from '/home/user/.cache/pypoetry/virtualenvs/securedrop-client-OfHWkjU1-py3.9/lib64/python3.9/site-packages/qubesdb.cpython-39-x86_64-linux-gnu.so'>

It links against the system libqubesdb.so:

$ ldd /home/user/.cache/pypoetry/virtualenvs/securedrop-client-OfHWkjU1-py3.9/lib64/python3.9/site-packages/qubesdb.cpython-39-x86_64-linux-gnu.so
	linux-vdso.so.1 (0x00007ffde7ffc000)
	libqubesdb.so => /lib64/libqubesdb.so (0x00007467aab55000)
	libc.so.6 => /lib64/libc.so.6 (0x00007467aa977000)
	/lib64/ld-linux-x86-64.so.2 (0x00007467aab76000)

Now if you run poetry install in a non-Qubes system, e.g. a plain Debian container, you'll get an error like:

    × Building wheel for QubesDB (pyproject.toml) did not run successfully.
    │ exit code: 1
    ╰─> [12 lines of output]
        running bdist_wheel
        running build
        running build_ext
        building 'qubesdb' extension
        creating build
        creating build/temp.linux-x86_64-cpython-311
        x86_64-linux-gnu-gcc -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -fstack-protector-strong -Wformat -Werror=format-security -g -fwrapv -O2 -fPIC -I../include -I/root/.cache/pypoetry/virtualenvs/securedrop-client-VsnhxLU2-py3.11/include -I/usr/include/python3.11 -c qubesdb.c -o build/temp.linux-x86_64-cpython-311/qubesdb.o -fno-strict-aliasing -Werror
        creating build/lib.linux-x86_64-cpython-311
        x86_64-linux-gnu-gcc -shared -Wl,-O1 -Wl,-Bsymbolic-functions -g -fwrapv -O2 build/temp.linux-x86_64-cpython-311/qubesdb.o -L../client -L/usr/lib/x86_64-linux-gnu -lqubesdb -o build/lib.linux-x86_64-cpython-311/qubesdb.cpython-311-x86_64-linux-gnu.so
        /usr/bin/ld: cannot find -lqubesdb: No such file or directory
        collect2: error: ld returned 1 exit status

So we still have to install libqubesdb somehow, which (still) means pulling down the qubes apt repo.

@cfm
Copy link
Member

cfm commented Mar 7, 2024

I've drafted the Rust implementation against libqubesdb in freedomofpress/securedrop-client#1895.

@deeplow
Copy link
Contributor

deeplow commented May 9, 2024

Found another one which could benefit from being tracked / listed here #1029

legoktm referenced this issue in freedomofpress/securedrop-client May 17, 2024
We are setting the proxy's origin in QubesDB (via dom0 salt), and as
discussed/agreed upon in
<freedomofpress/securedrop-workstation#1013>,
we're reading it directly in the proxy.

We link directly to libqubesdb, generating the Rust wrapper with bindgen
so it stays in sync (at build time at least). As a result, some unsafe
is needed, but it is clearly annotated with relevant SAFETY notes.

If we're not building with QubesDB (for development purposes), we'll
continue to read from the environment and skip the bindgen and linking
steps entirely.

Fixes <freedomofpress/securedrop-workstation#853>.

Co-authored-by: Kunal Mehta <[email protected]>
@cfm
Copy link
Member

cfm commented May 29, 2024

I believe #1051 and freedomofpress/securedrop-client#2032 now give us all the tools and options we've wanted here. Is there anything left on anyone's wishlist?

@cfm
Copy link
Member

cfm commented Jun 3, 2024

I've taken the liberty of marking that #1051 Closes: this. Feel free to change it there if you disagree!

@github-project-automation github-project-automation bot moved this from In Progress to Done in SecureDrop dev cycle Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
5 participants