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

mock: bind mount qubes-pesign socket on build #77

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

fepitre
Copy link
Member

@fepitre fepitre commented Nov 16, 2023

@@ -17,6 +17,10 @@ config_opts['use_bootstrap'] = False
config_opts['plugin_conf']['bind_mount_enable'] = os.environ.get("BIND_MOUNT_ENABLE", False)
config_opts['plugin_conf']['bind_mount_opts']['dirs'].append(('@BUILDER_DIR@/plugins', '/plugins' ))

# mount the pesign socket into the chroot
config_opts['plugin_conf']['bind_mount_opts']['dirs'].append(('/var/run/qubes-pesign', '/var/run/qubes-pesign' ))
config_opts['nspawn_args'] += ['--bind=/var/run/qubes-pesign']
Copy link
Member

Choose a reason for hiding this comment

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

Aren't those two meaning the same thing? Why both are needed?

Copy link
Member Author

@fepitre fepitre Dec 2, 2023

Choose a reason for hiding this comment

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

I've copy pasted some snippet in a thread but I've tested and only the second one is needed.

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Attention: Patch coverage is 87.17949% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.25%. Comparing base (63809ee) to head (2e85589).
Report is 80 commits behind head on main.

Files with missing lines Patch % Lines
qubesbuilder/config.py 86.84% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
+ Coverage   76.62%   78.25%   +1.62%     
==========================================
  Files          47       47              
  Lines        5317     5444     +127     
==========================================
+ Hits         4074     4260     +186     
+ Misses       1243     1184      -59     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fepitre fepitre changed the title mock: bind mount pesign socket on build mock: bind mount qubes-pesign socket on build Dec 2, 2023
rpc/qubes.PESign Outdated Show resolved Hide resolved
@marmarek
Copy link
Member

marmarek commented Jan 9, 2024

@fepitre what is the recommended method for redirecting /var/run/qubes-pesign to qrexec? socat started in /rw/config/rc.local? systemd .socket unit? Can you document it somewhere (README here? or in vmm-xen-unified repo?) ?

fepitre added a commit to QubesOS/qubes-vmm-xen-unified that referenced this pull request Aug 2, 2024
@fepitre
Copy link
Member Author

fepitre commented Aug 7, 2024

@fepitre what is the recommended method for redirecting /var/run/qubes-pesign to qrexec? socat started in /rw/config/rc.local? systemd .socket unit? Can you document it somewhere (README here? or in vmm-xen-unified repo?) ?

Doc is in vmm-xen-unified repo

@marmarek
Copy link
Member

And related vmm-xen-unified changes: QubesOS/qubes-vmm-xen-unified#1

Disable 'set -x' - it may leak some unintentional data in a future
version.
Print error message to stderr instead of stdout - otherwise it would be
taken as a signed payload.

And also rename the service to qubesbuilder.PESign.

QubesOS/qubes-issues#8206
head --bytes=100MB > "$payload"

# We don't allow payload being at least 100MiB
if [ "$(stat --format=%s "$payload")" -ge $((100 * 1024 * 1024)) ]; then

Choose a reason for hiding this comment

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

Suggested change
if [ "$(stat --format=%s "$payload")" -ge $((100 * 1024 * 1024)) ]; then
size=$(stat --format=%s "$payload")
if [ "$size" -ge "$((100 * 1024 * 1024))" ]; then

Also please include a call to the PE checker I wrote.

Copy link
Member

Choose a reason for hiding this comment

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

link?

Choose a reason for hiding this comment

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

README.md Outdated
mkdir -p /rw/bind-dirs/etc/systemd/system/
mkdir -p /usr/local/etc/default
# adjust value if you used different key nickname, replace spaces with __
echo 'KEY_NAME="Qubes__OS__Unified__Kernel__Image__Signing__Key"' > /usr/local/etc/default
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
echo 'KEY_NAME="Qubes__OS__Unified__Kernel__Image__Signing__Key"' > /usr/local/etc/default
echo 'KEY_NAME="Qubes__OS__Unified__Kernel__Image__Signing__Key"' > /usr/local/etc/default/qubes-pesign

Some certificates (especially stored on a hardware token) may need extra
parameters for `pesign`. Support those by sourcing
`~/.config/qubes-pesign/$CERTIFICATE`. The filename is safe as it's
sanitized by qrexec already. This also allows more flexible certificat
name handling - the config can override CERTIFICATE variable completely,
so the qrexec argument may be a friendly nickname independent of the
actual certificate name in the database (which is also a nickname, not
necessarily full DN).

For tokens, the config may look like this:

    # dbpath with pkcs11 module configured
    PESIGN_ARGS+=( "--certdir=$HOME/pesign-token-db" )
    # token name
    PESIGN_ARGS+=( "--token=token name" )
    # pinfile path, if relevant
    PESIGN_ARGS+=( "--pinfile=$HOME/pesign-token-pin.txt" )
    # you can also override CERTIFICATE
    CERTIFICATE="certificate name as on the token"

QubesOS/qubes-issues#8206
This includes how to setup builder disposable template, socket->qrexec
proxy and all relevant parameters
This moves some of the setup steps from README.md in vmm-xen-unified
repo.

QubesOS/qubes-issues#8206
But don't build it by default, as it requires extra setup (signing keys
specifically).

QubesOS/qubes-issues#8206
Previously, a CLI options used allow_append=True to allow setting
+components etc options without removing all existing entries. This
works when adding a single option, but breaks when adding multiple - as
later option will override earlier options (even with different name).

Replace this special CLI handling, with a more generic merging of
dict-like lists. Specifically, when merging two lists and both consists
of only one-key dicts (or just strings), merge them similar to merging
dicts, but preserving the structure.
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.

3 participants