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

libzfs: add keylocation=https://, backed by fetch(3) or libcurl #11956

Merged
merged 2 commits into from
May 13, 2021

Conversation

nabijaczleweli
Copy link
Contributor

@nabijaczleweli nabijaczleweli commented Apr 27, 2021

Motivation and Context

#9947

Description

zfs create -o encryption=on -o keylocation=https://nabijaczleweli.xyz/pgp.txt -o keyformat=passphrase zest/test__
zfs load-key -nL https://nabijaczleweli.xyz/pgp.txt zest/test__

Also needs ZTS entries, but not today.
Might also be best to install libcurl-dev on the buildds. The GitHub ones have it, see openzfs/zfs-buildbot#224 for buildbot

The key difference from #9543 is that it only allows https:// and not whatever's on tap. This is better for security (no plaintext passwords), and, once you extend this thought to libcurl, you could, theoretically, download keys off IMAP, telnet, or CIFS. This cannot be backed by fetch(3), nor should it. Anything more complex and/or not required for feature parity with Solaris is really better served by something better adjusted for it (like #11731 or facsimiles thereof).

How Has This Been Tested?

Ran'er through libcurl-dynamic, libcurl-static (well, it's still the .so, but linked directly in), and none.
Also fetch(3) ex situ, rest should be covered by buildds.

I haven't tried this on FreeBSD, but it builds, though I'm not perfectly sure of the error path there. Should be easy enough to test for a FreeBSD user. Works.

I'm also not sure of the availability of the libfetch.so soname, but that is also a question for a FreeBSD enjoyer. Went with libfetch.so.6, available since 8.0-RELEASE.

No back-end: http://build.zfsonlinux.org/builders/Debian%2010%20x86_64%20%28TEST%29/builds/7659/steps/shell_4/logs/log
404: http://build.zfsonlinux.org/builders/FreeBSD%20stable%2F13%20amd64%20%28TEST%29/builds/608/steps/shell_4/logs/log

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv) – only to the internal opaque struct libzfs_handle
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the OpenZFS code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes.
  • I have run the ZFS Test Suite with this change applied. – CI take my hand
  • All commit messages are properly formatted and contain Signed-off-by.

@nabijaczleweli nabijaczleweli force-pushed the htytpsko branch 3 times, most recently from b9cb6a5 to 1b2241e Compare April 27, 2021 20:54
@nabijaczleweli
Copy link
Contributor Author

abigail has decided that make storeabi is not a valid way to generate an ABI again, dunno what I'd do about this

@gmelikov
Copy link
Member

What's about backward compatibility? Look's like we need a feature flag here. Hope that I've missed something and we don't.

@nabijaczleweli
Copy link
Contributor Author

nabijaczleweli commented Apr 28, 2021

When a raw replication stream is zfs-received, you get "cannot set property for 'filling/test__': invalid keylocation", and it falls back to keylocation=prompt, but everything else works as desired.

Importing a pool with a keylocation=https:// dataset works as expected, but doing zfs load-key pool/dset returns "Key load error: URI scheme is not supported".

So this behaves as expected and shouldn't need special a feature gate. Plus, AFAIUI, features are only for on-disk format changes, and this remains compatible and rectangularly in userspace.

@behlendorf
Copy link
Contributor

@jasonbking @kithrup since this builds on the changes from #10218 would you mind reviewing it.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Apr 28, 2021
@nabijaczleweli nabijaczleweli marked this pull request as ready for review April 28, 2021 19:56
behlendorf pushed a commit to openzfs/zfs-buildbot that referenced this pull request Apr 28, 2021
@nabijaczleweli
Copy link
Contributor Author

From an administrative PoV: what should the https:// URLs in the ZTS point at? I have them point at raw files on the master branch (diverged in the second commit to a previous SHA from here), but it may be preferable to host them elsewhere maybe?

@behlendorf
Copy link
Contributor

what should the https:// URLs in the ZTS point at?

One thing we could do is split the change which adds the PASSPHRASE, HEXKEY, and RAWKEY in to its own commit. Then merge that independently to the master branch and reference that commit form the test cases. That way even in the unlikely case we change these files on the master branch it won't break the test cases in previous releases.

One other minor concern is that the test suite may be run in an environment without network access. In which case it'd be nice if these tests didn't just fail. We can skip the tests by calling log_unsupported if for some reason the URI is unreachable, then add these tests to the maybe section of tests/test-runner/bin/zts-report.py as 'SKIP' (instead of 'FAIL') to indicate if they're skipped don't consider the ZTS run a failure.

@nabijaczleweli nabijaczleweli force-pushed the htytpsko branch 3 times, most recently from 6faa3bb to 12d0625 Compare April 29, 2021 10:08
@nabijaczleweli
Copy link
Contributor Author

nabijaczleweli commented Apr 29, 2021

Right, gated the bits that use the network on ping github.com and added the https://-specific test as maybe SKIP with a bail-out at the top. It's not perfect, of course, since some otherwise-funxional networks block ICMP, but it's the best I can test for with commands.cfg I think.

Regarding the URL bit – I was mostly thinking about using some URL under OpenZFS' namespace and/or control, like openzfs.org/tests/key/{PASSPHRASE,HEXKEY,RAWKEY} or zfsonlinux.org/… (the whois of which suggests they're both owned by the laboratory).
If you'd rather stick with GitHub URLs, then using a separate commit and FFing this branch will work just as well, yeah.

@nabijaczleweli
Copy link
Contributor Author

nabijaczleweli commented Apr 29, 2021

These seem to get skipped on GHA, so that works as expected.

checkstyle fails due to an abigail classic:

  [C]'const pool_config_ops_t libzfs_config_ops' was changed to 'pool_config_ops_t libzfs_config_ops' at libzutil.h:59:1:
    type of variable changed:
     entity changed from 'const pool_config_ops_t' to compatible type 'typedef pool_config_ops_t' at libzutil.h:54:1
       'const const pool_config_ops' changed to 'const pool_config_ops'

nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request May 14, 2021
This also removes empty Wants= and After=s from they key-load services,
with no ill effect, since we already set DefaultDependencies=no

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Ref: openzfs#11956
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request May 20, 2021
This also removes empty Wants= and After=s from they key-load services,
with no ill effect, since we already set DefaultDependencies=no

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Ref: openzfs#11956
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request May 25, 2021
This also removes empty Wants= and After=s from they key-load services,
with no ill effect, since we already set DefaultDependencies=no

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Ref: openzfs#11956
ghost pushed a commit to truenas/zfs that referenced this pull request May 25, 2021
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Issue: openzfs#11956
Closes openzfs#11976
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request May 26, 2021
This also removes empty Wants= and After=s from they key-load services,
with no ill effect, since we already set DefaultDependencies=no

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Ref: openzfs#11956
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request May 27, 2021
This also removes empty Wants= and After=s from they key-load services,
with no ill effect, since we already set DefaultDependencies=no

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Ref: openzfs#11956
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request May 27, 2021
This also removes empty Wants= and After=s from they key-load services,
with no ill effect, since we already set DefaultDependencies=no

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Ref: openzfs#11956
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request May 27, 2021
This also removes empty Wants= and After=s from they key-load services,
with no ill effect, since we already set DefaultDependencies=no

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Ref: openzfs#11956
ghost pushed a commit to truenas/zfs that referenced this pull request May 27, 2021
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Issue: openzfs#11956
Closes openzfs#11976
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Afterward, git grep ZoL matches:
  * README.md:  * [ZoL Site](https://zfsonlinux.org)
  - Correct
  * etc/default/zfs.in:# ZoL userland configuration.
  - Changing this would induce a needless upgrade-check,
    if the user has modified the configuration;
    this can be updated the next time the defaults change
  * module/zfs/dmu_send.c:   * ZoL < 0.7 does not handle [...]
  - Before 0.7 is ZoL, so fair enough

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Issue openzfs#11956
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 8, 2021
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Issue openzfs#11956
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 8, 2021
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Issue openzfs#11956
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Jun 9, 2021
This also removes empty Wants= and After=s from they key-load services,
with no ill effect, since we already set DefaultDependencies=no

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Ref: openzfs#11956
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Jun 9, 2021
This also removes empty Wants= and After=s from they key-load services,
with no ill effect, since we already set DefaultDependencies=no

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Ref: openzfs#11956
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Nov 11, 2021
This also removes empty Wants= and After=s from they key-load services,
with no ill effect, since we already set DefaultDependencies=no

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Ref: openzfs#11956
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Nov 12, 2021
This also removes empty Wants= and After=s from they key-load services,
with no ill effect, since we already set DefaultDependencies=no

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Ref: openzfs#11956
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Nov 12, 2021
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Nov 30, 2021
Also simplify RequiresMountsFor= handling

Ref: openzfs#11956
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
tonynguien pushed a commit that referenced this pull request Nov 30, 2021
…]://

* etc/systemd/zfs-mount-generator: serialise

The wins for a relatively normal workload are rather slim:
	real	0.02119s/0.00985s=2.15029x
	user	0.02130s/0.00346s=6.15560x
	sys	0.03858s/0.00643s=6.00062x

	wall-total	0.014518s/0.005925s=2.45009x
	wall-init	0.014518s/0.002457s=5.90684x
	wall-real	0.014518s/0.003467s=4.18668x

But this is a big win on machines with a lot of datasets and expensive
forks.

For example, the gain on a VM on my work laptop with 900+ legacy-mount
Docker datasets, the original gains from the C rewrite were
only five-fold:
	real    0.516s/0.102s=5.05882x
	user    0.237s/0.143s=1.65734x
	sys     0.287s/0.100s=2.87x

And this serial variant gains this back there as well:
	real    0.102s/0.008s=12.75x
	user    0.143s/0.007s=20.42857
	sys     0.100s/0.001s=100x

	wall-total	0.09717s/0.00319s=30.40255x
	wall-init	0.00203s/0.00200s=1.015941x
	wall-real	0.09513s/0.00118s=80.02043x

For a total of
	real    0.516s/0.008s=64.5x
	user    0.237s/0.007s=33.85714x
	sys     0.287s/0.001s=287x

Suggested-by: Richard Laager <[email protected]>

* etc/systemd/zfs-mount-generator: pull in network for keylocation=https

Also simplify RequiresMountsFor= handling
Ref: #11956

Reviewed-by: Richard Laager <[email protected]>
Reviewed-by: Tony Nguyen <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes #12138
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Feb 10, 2022
Add support for http and https to the keylocation properly to
allow encryption keys to be fetched from the specified URL.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Issue openzfs#9543
Closes openzfs#9947
Closes openzfs#11956
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Feb 14, 2022
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Issue: openzfs#11956
Closes openzfs#11976
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Feb 14, 2022
Add support for http and https to the keylocation properly to
allow encryption keys to be fetched from the specified URL.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Issue openzfs#9543
Closes openzfs#9947
Closes openzfs#11956
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Feb 16, 2022
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Issue: openzfs#11956
Closes openzfs#11976
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Feb 16, 2022
Add support for http and https to the keylocation properly to
allow encryption keys to be fetched from the specified URL.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Issue openzfs#9543
Closes openzfs#9947
Closes openzfs#11956
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Feb 17, 2022
Add support for http and https to the keylocation properly to
allow encryption keys to be fetched from the specified URL.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Issue openzfs#9543
Closes openzfs#9947
Closes openzfs#11956
nabijaczleweli pushed a commit to nabijaczleweli/zfs that referenced this pull request Mar 12, 2022
The get_key_material_https() function error code path had a bogus
free() call, either resulting in double-free or free() of undefined
pointer.

Fixes: openzfs#11956
Signed-off-by: Harry Sintonen <[email protected]>
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Mar 29, 2022
…]://

* etc/systemd/zfs-mount-generator: serialise

The wins for a relatively normal workload are rather slim:
	real	0.02119s/0.00985s=2.15029x
	user	0.02130s/0.00346s=6.15560x
	sys	0.03858s/0.00643s=6.00062x

	wall-total	0.014518s/0.005925s=2.45009x
	wall-init	0.014518s/0.002457s=5.90684x
	wall-real	0.014518s/0.003467s=4.18668x

But this is a big win on machines with a lot of datasets and expensive
forks.

For example, the gain on a VM on my work laptop with 900+ legacy-mount
Docker datasets, the original gains from the C rewrite were
only five-fold:
	real    0.516s/0.102s=5.05882x
	user    0.237s/0.143s=1.65734x
	sys     0.287s/0.100s=2.87x

And this serial variant gains this back there as well:
	real    0.102s/0.008s=12.75x
	user    0.143s/0.007s=20.42857
	sys     0.100s/0.001s=100x

	wall-total	0.09717s/0.00319s=30.40255x
	wall-init	0.00203s/0.00200s=1.015941x
	wall-real	0.09513s/0.00118s=80.02043x

For a total of
	real    0.516s/0.008s=64.5x
	user    0.237s/0.007s=33.85714x
	sys     0.287s/0.001s=287x

Suggested-by: Richard Laager <[email protected]>

* etc/systemd/zfs-mount-generator: pull in network for keylocation=https

Also simplify RequiresMountsFor= handling
Ref: openzfs#11956

Reviewed-by: Richard Laager <[email protected]>
Reviewed-by: Tony Nguyen <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Upstream-commit: 4325de0
Closes openzfs#12138
behlendorf pushed a commit that referenced this pull request Apr 1, 2022
…]://

* etc/systemd/zfs-mount-generator: serialise

The wins for a relatively normal workload are rather slim:
	real	0.02119s/0.00985s=2.15029x
	user	0.02130s/0.00346s=6.15560x
	sys	0.03858s/0.00643s=6.00062x

	wall-total	0.014518s/0.005925s=2.45009x
	wall-init	0.014518s/0.002457s=5.90684x
	wall-real	0.014518s/0.003467s=4.18668x

But this is a big win on machines with a lot of datasets and expensive
forks.

For example, the gain on a VM on my work laptop with 900+ legacy-mount
Docker datasets, the original gains from the C rewrite were
only five-fold:
	real    0.516s/0.102s=5.05882x
	user    0.237s/0.143s=1.65734x
	sys     0.287s/0.100s=2.87x

And this serial variant gains this back there as well:
	real    0.102s/0.008s=12.75x
	user    0.143s/0.007s=20.42857
	sys     0.100s/0.001s=100x

	wall-total	0.09717s/0.00319s=30.40255x
	wall-init	0.00203s/0.00200s=1.015941x
	wall-real	0.09513s/0.00118s=80.02043x

For a total of
	real    0.516s/0.008s=64.5x
	user    0.237s/0.007s=33.85714x
	sys     0.287s/0.001s=287x

Suggested-by: Richard Laager <[email protected]>

* etc/systemd/zfs-mount-generator: pull in network for keylocation=https

Also simplify RequiresMountsFor= handling
Ref: #11956

Reviewed-by: Richard Laager <[email protected]>
Reviewed-by: Tony Nguyen <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Upstream-commit: 4325de0
Closes #12138
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
…]://

* etc/systemd/zfs-mount-generator: serialise

The wins for a relatively normal workload are rather slim:
	real	0.02119s/0.00985s=2.15029x
	user	0.02130s/0.00346s=6.15560x
	sys	0.03858s/0.00643s=6.00062x

	wall-total	0.014518s/0.005925s=2.45009x
	wall-init	0.014518s/0.002457s=5.90684x
	wall-real	0.014518s/0.003467s=4.18668x

But this is a big win on machines with a lot of datasets and expensive
forks.

For example, the gain on a VM on my work laptop with 900+ legacy-mount
Docker datasets, the original gains from the C rewrite were
only five-fold:
	real    0.516s/0.102s=5.05882x
	user    0.237s/0.143s=1.65734x
	sys     0.287s/0.100s=2.87x

And this serial variant gains this back there as well:
	real    0.102s/0.008s=12.75x
	user    0.143s/0.007s=20.42857
	sys     0.100s/0.001s=100x

	wall-total	0.09717s/0.00319s=30.40255x
	wall-init	0.00203s/0.00200s=1.015941x
	wall-real	0.09513s/0.00118s=80.02043x

For a total of
	real    0.516s/0.008s=64.5x
	user    0.237s/0.007s=33.85714x
	sys     0.287s/0.001s=287x

Suggested-by: Richard Laager <[email protected]>

* etc/systemd/zfs-mount-generator: pull in network for keylocation=https

Also simplify RequiresMountsFor= handling
Ref: openzfs#11956

Reviewed-by: Richard Laager <[email protected]>
Reviewed-by: Tony Nguyen <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12138
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
…]://

* etc/systemd/zfs-mount-generator: serialise

The wins for a relatively normal workload are rather slim:
	real	0.02119s/0.00985s=2.15029x
	user	0.02130s/0.00346s=6.15560x
	sys	0.03858s/0.00643s=6.00062x

	wall-total	0.014518s/0.005925s=2.45009x
	wall-init	0.014518s/0.002457s=5.90684x
	wall-real	0.014518s/0.003467s=4.18668x

But this is a big win on machines with a lot of datasets and expensive
forks.

For example, the gain on a VM on my work laptop with 900+ legacy-mount
Docker datasets, the original gains from the C rewrite were
only five-fold:
	real    0.516s/0.102s=5.05882x
	user    0.237s/0.143s=1.65734x
	sys     0.287s/0.100s=2.87x

And this serial variant gains this back there as well:
	real    0.102s/0.008s=12.75x
	user    0.143s/0.007s=20.42857
	sys     0.100s/0.001s=100x

	wall-total	0.09717s/0.00319s=30.40255x
	wall-init	0.00203s/0.00200s=1.015941x
	wall-real	0.09513s/0.00118s=80.02043x

For a total of
	real    0.516s/0.008s=64.5x
	user    0.237s/0.007s=33.85714x
	sys     0.287s/0.001s=287x

Suggested-by: Richard Laager <[email protected]>

* etc/systemd/zfs-mount-generator: pull in network for keylocation=https

Also simplify RequiresMountsFor= handling
Ref: openzfs#11956

Reviewed-by: Richard Laager <[email protected]>
Reviewed-by: Tony Nguyen <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12138
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants