[pve-devel] [PATCH-SERIES manager] backup permission improvements

[pve-devel] [PATCH-SERIES manager] backup permission improvements

[Fiona Ebner]

Currently, suffenciently privileged users may edit a backup job, but
cannot run the very same job manually (via the vzdump API call). The
first patch addresses this by removing the root-only restriction from
retention and performance settings. Retention will require
Datastore.Allocate on the target storage, because it's essentially
removal of certain backups, while performance settings will require
Sys.Modify on / which is the permission required to edit backup jobs.

The next three patches are for deletion of parameters when updating a
backup job. Allowing to only delete a setting (previously, update
would fail if no parameter was set) and adding a check for the delete
options.

Patch 5/6 restricts backup editing by requiring that the user has
appropriate permissions on the job's storage (and eventual newly set
storage) as well as on the default 'local' storage when removing the
storage. Jobs with a dumpdir can only be edited by root. This is a
breaking API change, but requiring permission on the storage should
be sensible and allows for more flexible permission configurations.

The last patch introduces a helper to have the "what's the storage"
logic in one place.


Fiona Ebner (6):
  api: vzdump: soften parameter permission checks
  api: backup: update: turn delete into a hash
  api: backup: update: allow only deleting
  api: backup: update: check permissions of delete params too
  api: backup: require Datastore.Allocate on storage
  api: backup/vzdump: add get_storage_param helper

 PVE/API2/Backup.pm | 62 ++++++++++++++++++++++++++++++++++++++--------
 PVE/API2/VZDump.pm | 32 ++++++++++++++++--------
 PVE/VZDump.pm      | 11 ++++++--
 3 files changed, 82 insertions(+), 23 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH manager 1/6] api: vzdump: soften parameter permission checks

[Fiona Ebner]

Allows sufficiently privileged users to pass-in retention and
performance parameters for manual backup, but keeps tmpdir, dumpdir
and script root-only. Such users could already edit the job
accordingly, so essentially not granting anything new.

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Backup.pm | 15 ++++++++++-----
 PVE/API2/VZDump.pm | 25 ++++++++++++++++++-------
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 3a079874..6aef5bb7 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -40,14 +40,19 @@ my $vzdump_job_id_prop = {
     maxLength => 50
 };
 
-my $assert_param_permission = sub {
-    my ($param, $user) = @_;
+# NOTE: also used by the vzdump API call.
+sub assert_param_permission_common {
+    my ($rpcenv, $user, $param) = @_;
     return if $user eq 'root@pam'; # always OK
 
     for my $key (qw(tmpdir dumpdir script)) {
 	raise_param_exc({ $key => "Only root may set this option."}) if exists $param->{$key};
     }
-};
+
+    if (defined($param->{bwlimit}) || defined($param->{ionice}) || defined($param->{performance})) {
+	$rpcenv->check($user, "/", [ 'Sys.Modify' ]);
+    }
+}
 
 my $convert_to_schedule = sub {
     my ($job) = @_;
@@ -207,7 +212,7 @@ __PACKAGE__->register_method({
 	my $rpcenv = PVE::RPCEnvironment::get();
 	my $user = $rpcenv->get_user();
 
-	$assert_param_permission->($param, $user);
+	assert_param_permission_common($rpcenv, $user, $param);
 
 	if (my $pool = $param->{pool}) {
 	    $rpcenv->check_pool_exist($pool);
@@ -419,7 +424,7 @@ __PACKAGE__->register_method({
 	my $rpcenv = PVE::RPCEnvironment::get();
 	my $user = $rpcenv->get_user();
 
-	$assert_param_permission->($param, $user);
+	assert_param_permission_common($rpcenv, $user, $param);
 
 	if (my $pool = $param->{pool}) {
 	    $rpcenv->check_pool_exist($pool);
diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 30d47825..8e873c05 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -14,12 +14,25 @@ use PVE::Tools qw(extract_param);
 use PVE::VZDump::Common;
 use PVE::VZDump;
 
+use PVE::API2::Backup;
 use PVE::API2Tools;
 
 use Data::Dumper; # fixme: remove
 
 use base qw(PVE::RESTHandler);
 
+my sub assert_param_permission_vzdump {
+    my ($rpcenv, $user, $param) = @_;
+    return if $user eq 'root@pam'; # always OK
+
+    PVE::API2::Backup::assert_param_permission_common($rpcenv, $user, $param);
+
+    if (!$param->{dumpdir} && (defined($param->{maxfiles}) || defined($param->{'prune-backups'}))) {
+	my $storeid = $param->{storage} || 'local';
+	$rpcenv->check($user, "/storage/$storeid", [ 'Datastore.Allocate' ]);
+    } # no else branch, because dumpdir is root-only
+}
+
 __PACKAGE__->register_method ({
     name => 'vzdump',
     path => '',
@@ -27,9 +40,10 @@ __PACKAGE__->register_method ({
     description => "Create backup.",
     permissions => {
 	description => "The user needs 'VM.Backup' permissions on any VM, and "
-	    ."'Datastore.AllocateSpace' on the backup storage. The 'maxfiles', 'prune-backups', "
-	    ."'tmpdir', 'dumpdir', 'script', 'bwlimit', 'performance' and 'ionice' parameters are "
-	    ."restricted to the 'root\@pam' user.",
+	    ."'Datastore.AllocateSpace' on the backup storage. The 'tmpdir', 'dumpdir' and "
+	    ."'script' parameters are restricted to the 'root\@pam' user. The 'maxfiles' and "
+	    ."'prune-backups' settings require 'Datastore.Allocate' on the backup storage. The "
+	    ."'bwlimit', 'performance' and 'ionice' parameters require 'Sys.Modify' on '/'.",
 	user => 'all',
     },
     protected => 1,
@@ -62,10 +76,7 @@ __PACKAGE__->register_method ({
 		if $param->{stdout};
 	}
 
-	for my $key (qw(maxfiles prune-backups tmpdir dumpdir script bwlimit performance ionice)) {
-	    raise_param_exc({ $key => "Only root may set this option."})
-		if defined($param->{$key}) && ($user ne 'root@pam');
-	}
+	assert_param_permission_vzdump($rpcenv, $user, $param);
 
 	PVE::VZDump::verify_vzdump_parameters($param, 1);
 
-- 
2.30.2

[pve-devel] applied: [PATCH manager 1/6] api: vzdump: soften parameter permission checks

[Thomas Lamprecht]

Am 16/11/2022 um 15:04 schrieb Fiona Ebner:
> Allows sufficiently privileged users to pass-in retention and
> performance parameters for manual backup, but keeps tmpdir, dumpdir
> and script root-only. Such users could already edit the job
> accordingly, so essentially not granting anything new.
> 
> Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/API2/Backup.pm | 15 ++++++++++-----
>  PVE/API2/VZDump.pm | 25 ++++++++++++++++++-------
>  2 files changed, 28 insertions(+), 12 deletions(-)
> 
>

almost forgot: as talked off-list: applied this one for now as its relatively
nonintrusive and fixes the annoying "can edit job but cannot press run-no" issue,
thanks!

[pve-devel] [PATCH manager 2/6] api: backup: update: turn delete into a hash

[Fiona Ebner]

makes it easier to check for keys in the following patches.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Backup.pm | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 6aef5bb7..1d3d6896 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -435,9 +435,7 @@ __PACKAGE__->register_method({
 
 	my $id = extract_param($param, 'id');
 	my $delete = extract_param($param, 'delete');
-	if ($delete) {
-	    $delete = [PVE::Tools::split_list($delete)];
-	}
+	$delete = { map { $_ => 1 } PVE::Tools::split_list($delete) } if $delete;
 
 	my $update_job = sub {
 	    my $data = cfs_read_file('vzdump.cron');
@@ -472,7 +470,7 @@ __PACKAGE__->register_method({
 		'repeat-missed' => 1,
 	    };
 
-	    foreach my $k (@$delete) {
+	    for my $k (keys $delete->%*) {
 		if (!PVE::VZDump::option_exists($k) && !$deletable->{$k}) {
 		    raise_param_exc({ delete => "unknown option '$k'" });
 		}
-- 
2.30.2

[pve-devel] [PATCH manager 3/6] api: backup: update: allow only deleting

[Fiona Ebner]

Previously, it was required to set something at the same time.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Backup.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 1d3d6896..c0800bac 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -443,7 +443,7 @@ __PACKAGE__->register_method({
 
 	    my $jobs = $data->{jobs} || [];
 
-	    die "no options specified\n" if !scalar(keys %$param);
+	    die "no options specified\n" if !scalar(keys $param->%*) && !scalar(keys $delete->%*);
 
 	    PVE::VZDump::verify_vzdump_parameters($param);
 	    my $opts = PVE::VZDump::JobBase->check_config($id, $param, 0, 1);
-- 
2.30.2

[pve-devel] [PATCH manager 4/6] api: backup: update: check permissions of delete params too

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Backup.pm | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index c0800bac..684a078e 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -54,6 +54,14 @@ sub assert_param_permission_common {
     }
 }
 
+my sub assert_param_permission_update {
+    my ($rpcenv, $user, $update, $delete) = @_;
+    return if $user eq 'root@pam'; # always OK
+
+    assert_param_permission_common($rpcenv, $user, $update);
+    assert_param_permission_common($rpcenv, $user, $delete);
+}
+
 my $convert_to_schedule = sub {
     my ($job) = @_;
 
@@ -424,8 +432,6 @@ __PACKAGE__->register_method({
 	my $rpcenv = PVE::RPCEnvironment::get();
 	my $user = $rpcenv->get_user();
 
-	assert_param_permission_common($rpcenv, $user, $param);
-
 	if (my $pool = $param->{pool}) {
 	    $rpcenv->check_pool_exist($pool);
 	    $rpcenv->check($user, "/pool/$pool", ['VM.Backup']);
@@ -437,6 +443,8 @@ __PACKAGE__->register_method({
 	my $delete = extract_param($param, 'delete');
 	$delete = { map { $_ => 1 } PVE::Tools::split_list($delete) } if $delete;
 
+	assert_param_permission_update($rpcenv, $user, $param, $delete);
+
 	my $update_job = sub {
 	    my $data = cfs_read_file('vzdump.cron');
 	    my $jobs_data = cfs_read_file('jobs.cfg');
-- 
2.30.2

[pve-devel] [PATCH manager 5/6] api: backup: require Datastore.Allocate on storage

[Fiona Ebner]

In particular this ensures that the user is allowed to remove data on
the storage, because configuring low retention results in removed
older backups. Of course setting the storage itself also needs to
require the same privilege then.

This is a breaking API change, but it seems sensible to require
permissions on the affected storage too.

Jobs with a dumpdir setting can be configured by root only.

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Not sure about the dumpdir jobs, should those rather require
Sys.Modify on /? The dumpdir setting itself is still root-only.

 PVE/API2/Backup.pm | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 684a078e..74bf95ca 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -54,12 +54,39 @@ sub assert_param_permission_common {
     }
 }
 
+my sub assert_param_permission_create {
+    my ($rpcenv, $user, $param) = @_;
+    return if $user eq 'root@pam'; # always OK
+
+    assert_param_permission_common($rpcenv, $user, $param);
+
+    if (!$param->{dumpdir}) {
+	my $storeid = $param->{storage} || 'local';
+	$rpcenv->check($user, "/storage/$storeid", [ 'Datastore.Allocate' ]);
+    } # no else branch, because dumpdir is root-only
+}
+
 my sub assert_param_permission_update {
-    my ($rpcenv, $user, $update, $delete) = @_;
+    my ($rpcenv, $user, $update, $delete, $current) = @_;
     return if $user eq 'root@pam'; # always OK
 
     assert_param_permission_common($rpcenv, $user, $update);
     assert_param_permission_common($rpcenv, $user, $delete);
+
+    if ($update->{storage}) {
+	$rpcenv->check($user, "/storage/$update->{storage}", [ 'Datastore.Allocate' ])
+    } elsif ($delete->{storage}) {
+	$rpcenv->check($user, "/storage/local", [ 'Datastore.Allocate' ]);
+    }
+
+    return if !$current; # early check done
+
+    if ($current->{dumpdir}) {
+	die "only root\@pam may edit jobs with a 'dumpdir' option.";
+    } else {
+	my $storeid = $current->{storage} || 'local';
+	$rpcenv->check($user, "/storage/$storeid", [ 'Datastore.Allocate' ]);
+    }
 }
 
 my $convert_to_schedule = sub {
@@ -220,7 +247,7 @@ __PACKAGE__->register_method({
 	my $rpcenv = PVE::RPCEnvironment::get();
 	my $user = $rpcenv->get_user();
 
-	assert_param_permission_common($rpcenv, $user, $param);
+	assert_param_permission_create($rpcenv, $user, $param);
 
 	if (my $pool = $param->{pool}) {
 	    $rpcenv->check_pool_exist($pool);
@@ -473,6 +500,8 @@ __PACKAGE__->register_method({
 		die "no such vzdump job\n" if !$job || $job->{type} ne 'vzdump';
 	    }
 
+	    assert_param_permission_update($rpcenv, $user, $param, $delete, $job);
+
 	    my $deletable = {
 		comment => 1,
 		'repeat-missed' => 1,
-- 
2.30.2

[pve-devel] [PATCH manager 6/6] api: backup/vzdump: add get_storage_param helper

[Fiona Ebner]

to capture the logic in a single place.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Backup.pm | 10 +++++-----
 PVE/API2/VZDump.pm | 15 ++++++++-------
 PVE/VZDump.pm      | 11 +++++++++--
 3 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 74bf95ca..25e615d1 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -60,10 +60,9 @@ my sub assert_param_permission_create {
 
     assert_param_permission_common($rpcenv, $user, $param);
 
-    if (!$param->{dumpdir}) {
-	my $storeid = $param->{storage} || 'local';
+    if (my $storeid = PVE::VZDump::get_storage_param($param)) {
 	$rpcenv->check($user, "/storage/$storeid", [ 'Datastore.Allocate' ]);
-    } # no else branch, because dumpdir is root-only
+    }
 }
 
 my sub assert_param_permission_update {
@@ -84,8 +83,9 @@ my sub assert_param_permission_update {
     if ($current->{dumpdir}) {
 	die "only root\@pam may edit jobs with a 'dumpdir' option.";
     } else {
-	my $storeid = $current->{storage} || 'local';
-	$rpcenv->check($user, "/storage/$storeid", [ 'Datastore.Allocate' ]);
+	if (my $storeid = PVE::VZDump::get_storage_param($current)) {
+	    $rpcenv->check($user, "/storage/$storeid", [ 'Datastore.Allocate' ]);
+	}
     }
 }
 
diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 8e873c05..e3dcd0bd 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -27,10 +27,11 @@ my sub assert_param_permission_vzdump {
 
     PVE::API2::Backup::assert_param_permission_common($rpcenv, $user, $param);
 
-    if (!$param->{dumpdir} && (defined($param->{maxfiles}) || defined($param->{'prune-backups'}))) {
-	my $storeid = $param->{storage} || 'local';
-	$rpcenv->check($user, "/storage/$storeid", [ 'Datastore.Allocate' ]);
-    } # no else branch, because dumpdir is root-only
+    if (defined($param->{maxfiles}) || defined($param->{'prune-backups'})) {
+	if (my $storeid = PVE::VZDump::get_storage_param($param)) {
+	    $rpcenv->check($user, "/storage/$storeid", [ 'Datastore.Allocate' ]);
+	}
+    }
 }
 
 __PACKAGE__->register_method ({
@@ -108,9 +109,9 @@ __PACKAGE__->register_method ({
 	die "you can only backup a single VM with option --stdout\n"
 	    if $param->{stdout} && scalar(@{$local_vmids}) != 1;
 
-	# If the root-only dumpdir is used rather than a storage, the check will succeed anyways.
-	my $storeid = $param->{storage} || 'local';
-	$rpcenv->check($user, "/storage/$storeid", [ 'Datastore.AllocateSpace' ]);
+	if (my $storeid = PVE::VZDump::get_storage_param($param)) {
+	    $rpcenv->check($user, "/storage/$storeid", [ 'Datastore.AllocateSpace' ]);
+	}
 
 	my $worker = sub {
 	    my $upid = shift;
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index a04837e7..bd17c986 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -55,6 +55,13 @@ foreach my $plug (@pve_vzdump_classes) {
     }
 }
 
+sub get_storage_param {
+    my ($param) = @_;
+
+    return if $param->{dumpdir};
+    return $param->{storage} || 'local';
+}
+
 # helper functions
 
 sub debugmsg {
@@ -557,8 +564,8 @@ sub new {
 	die "cannot use options 'storage' and 'dumpdir' at the same time\n";
     }
 
-    if (!$opts->{dumpdir} && !$opts->{storage}) {
-	$opts->{storage} = 'local';
+    if (my $storage = get_storage_param($opts)) {
+	$opts->{storage} = $storage;
     }
 
     # Enforced by the API too, but these options might come in via defaults. Drop them if necessary.
-- 
2.30.2

Re: [pve-devel] [PATCH-SERIES manager] backup permission improvements

[Fiona Ebner]

Am 16.11.22 um 15:04 schrieb Fiona Ebner:
> Currently, suffenciently privileged users may edit a backup job, but
> cannot run the very same job manually (via the vzdump API call). The
> first patch addresses this by removing the root-only restriction from
> retention and performance settings. Retention will require
> Datastore.Allocate on the target storage, because it's essentially
> removal of certain backups, while performance settings will require
> Sys.Modify on / which is the permission required to edit backup jobs.
> 
> The next three patches are for deletion of parameters when updating a
> backup job. Allowing to only delete a setting (previously, update
> would fail if no parameter was set) and adding a check for the delete
> options.
> 
> Patch 5/6 restricts backup editing by requiring that the user has
> appropriate permissions on the job's storage (and eventual newly set
> storage) as well as on the default 'local' storage when removing the
> storage. Jobs with a dumpdir can only be edited by root. This is a
> breaking API change, but requiring permission on the storage should
> be sensible and allows for more flexible permission configurations.
> 
> The last patch introduces a helper to have the "what's the storage"
> logic in one place.
> 

Ping for the rest of the series, should still apply.

Just ran into the issue that 3/6 fixes with:
pvesh set /cluster/backup/backup-4f2f3b87-0165 --delete script

Maybe we want to wait with 5/6 until to the next major release though.

Re: [pve-devel] [PATCH-SERIES manager] backup permission improvements

[Fiona Ebner]

Am 05.04.23 um 09:43 schrieb Fiona Ebner:
> Am 16.11.22 um 15:04 schrieb Fiona Ebner:
>> Currently, suffenciently privileged users may edit a backup job, but
>> cannot run the very same job manually (via the vzdump API call). The
>> first patch addresses this by removing the root-only restriction from
>> retention and performance settings. Retention will require
>> Datastore.Allocate on the target storage, because it's essentially
>> removal of certain backups, while performance settings will require
>> Sys.Modify on / which is the permission required to edit backup jobs.
>>
>> The next three patches are for deletion of parameters when updating a
>> backup job. Allowing to only delete a setting (previously, update
>> would fail if no parameter was set) and adding a check for the delete
>> options.
>>
>> Patch 5/6 restricts backup editing by requiring that the user has
>> appropriate permissions on the job's storage (and eventual newly set
>> storage) as well as on the default 'local' storage when removing the
>> storage. Jobs with a dumpdir can only be edited by root. This is a
>> breaking API change, but requiring permission on the storage should
>> be sensible and allows for more flexible permission configurations.
>>
>> The last patch introduces a helper to have the "what's the storage"
>> logic in one place.
>>
> 
> Ping for the rest of the series, should still apply.
> 
> Just ran into the issue that 3/6 fixes with:
> pvesh set /cluster/backup/backup-4f2f3b87-0165 --delete script
> 
> Maybe we want to wait with 5/6 until to the next major release though.

Ping again, now that we're working towards the next major release :)

[pve-devel] applied: [PATCH-SERIES manager] backup permission improvements

[Thomas Lamprecht]

Am 16/11/2022 um 15:04 schrieb Fiona Ebner:
> Currently, suffenciently privileged users may edit a backup job, but
> cannot run the very same job manually (via the vzdump API call). The
> first patch addresses this by removing the root-only restriction from
> retention and performance settings. Retention will require
> Datastore.Allocate on the target storage, because it's essentially
> removal of certain backups, while performance settings will require
> Sys.Modify on / which is the permission required to edit backup jobs.
> 
> The next three patches are for deletion of parameters when updating a
> backup job. Allowing to only delete a setting (previously, update
> would fail if no parameter was set) and adding a check for the delete
> options.
> 
> Patch 5/6 restricts backup editing by requiring that the user has
> appropriate permissions on the job's storage (and eventual newly set
> storage) as well as on the default 'local' storage when removing the
> storage. Jobs with a dumpdir can only be edited by root. This is a
> breaking API change, but requiring permission on the storage should
> be sensible and allows for more flexible permission configurations.
> 
> The last patch introduces a helper to have the "what's the storage"
> logic in one place.
> 
> 
> Fiona Ebner (6):
>   api: vzdump: soften parameter permission checks
>   api: backup: update: turn delete into a hash
>   api: backup: update: allow only deleting
>   api: backup: update: check permissions of delete params too
>   api: backup: require Datastore.Allocate on storage
>   api: backup/vzdump: add get_storage_param helper
> 
>  PVE/API2/Backup.pm | 62 ++++++++++++++++++++++++++++++++++++++--------
>  PVE/API2/VZDump.pm | 32 ++++++++++++++++--------
>  PVE/VZDump.pm      | 11 ++++++--
>  3 files changed, 82 insertions(+), 23 deletions(-)
> 


applied remaining series, thanks!

fwiw, I did some small follow-up for "api: backup: update: check permissions
of delete params too", just to get the order of checking those params
before the pool as before and as the create endpoint has, I don't think
it has a practical effect, but it just felt slightly odd..