Skip to content

cryptsetup: if keyfile is specified as AF_UNIX socket in the fs, connect to it, and read key data from it - #17524

Merged
keszybz merged 13 commits into
systemd:masterfrom
poettering:fileio-offset
Dec 1, 2020
Merged

cryptsetup: if keyfile is specified as AF_UNIX socket in the fs, connect to it, and read key data from it#17524
keszybz merged 13 commits into
systemd:masterfrom
poettering:fileio-offset

Conversation

@poettering

Copy link
Copy Markdown
Member

No description provided.

Comment thread man/crypttab.xml Outdated

@bluca bluca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just left a minor comment about documentation phrasing. Rest of the patches all look good to me.

@poettering

Copy link
Copy Markdown
Member Author

Force pushed a new version. Only change are the two suggested doc fixes.

@keszybz keszybz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks very nice. Some minor suggestions only.

Comment thread src/cryptsetup/cryptsetup-keyfile.c Outdated
Comment thread src/cryptsetup/cryptsetup-keyfile.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's just one caller that always specifies a search path. So this code might be simplified...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

true. I wrote this code also to prepare things for the credentials logic (so that you can supply key data via LoadCredential= too), but didn't include the patch for that yet. In that case we'd not specify a search path. I'd thus like to keep this in for now. will post patch for creds hookup shortly

Comment thread man/crypttab.xml Outdated
We have easy support for this, hence use it for privileged key data.
…EADABLE when reading PEM secret key

It's secret data, hence use the appropriate flags.
Now that we can read from offsets/with size, let's port the cryptsetup
PKCS#11 key file logic over to read_full_file_full().
…ile_full()

Previously, we'd load the file with libcryptsetup's calls. Let's do that
in our own, so that we can make use of READ_FULL_FILE_CONNECT_SOCKET,
i.e. read in keys via AF_UNIX sockets, so that people can plug key
providers into our logic.

This provides functionality similar to Debian's keyscript= crypttab
option (see → systemd#3007), as it allows key scripts to be run as socket
activated services, that have stdout connected to the activated socket.
In contrast to traditional keyscript= support this logic runs stuff out
of process however, which is beneficial, since it allows sandboxing and
similar.
Let's move the 3rd way how cryptsetup acquires key files to
read_file_full() too.

Since load_key_file()'s raison d'etre now is just the search path logic,
let's rename the function to find_key_file().
The comment is pointless, ECC systematically doesn't allow
encryption/decryption directly, only RSA does that. If you want to use
ECC for asymmetric encryption/decryption you have to combine it with key
exchange scheme and symmetric scheme. This all is not a limitation of
the Yubikey, hence don't claim so. It's just how ECC is.
@poettering

Copy link
Copy Markdown
Member Author

Force pushed a new version. Only changes are the two suggested ones, for the other one see comment above.

Upgrading green label.

Thanks for the review! This is great to get merged to make #17741 a lot smaller.

@poettering poettering added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Dec 1, 2020
@keszybz keszybz removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Dec 1, 2020
@keszybz
keszybz merged commit 946f3d8 into systemd:master Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants