[pve-devel] applied: [PATCH ifupdown2] patch: fix ipv6 slaac on bridge
On Wed, Aug 09, 2023 at 05:16:06PM +, DERUMIER, Alexandre wrote: > Hi, > > could it be possible to apply this patch ? applied, thanks ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] fix #474: allow transfer from container/vms
Am 09.08.23 um 16:20 schrieb Philipp Hufnagl: > On 8/9/23 13:32, Fiona Ebner wrote: > >> The permission for the original pool should be checked here?! Or is >> that already done somewhere? > > The permission of the original pool does not matter. But it should. After all, the operation is modifying the original pool, so the user better have an appropriate permission to do so. > The permission of the VM is important > (maybe the original pool granting the user permission on the VM). > Hovever I tested it with granting the > user merely audit permissions on the VM and admin permissions on the > target pool and still got the > correct permission error so I don't think the permission checks have to > be modified at all > Currently, Permissions.Modify|VM.Allocate on the VM and Pool.Allocate on the target pool would be enough to "steal" the guest, no permissions required on the original pool at all. IMHO, the user really should have a Pool.Allocate on the original pool as well. Since I noticed it in v3: we usually use "api:" and "ui:" as prefixes rather than "backend:" and "frontend:". Would be nice if you could use them too for consistency. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH access-control v2 2/3] ldap: add opt-in `check-connection` param to perform a bind check
On Tue, Aug 01, 2023 at 02:37:18PM +0200, Christoph Heiss wrote:
> Removes the dreaded DN regex, instead introducing a optional
> connect/bind check on creation/update, aligning it with the way PBS does
> it.
>
> Additionally, it has the benefit that instead of letting a sync fail on
> the first try due to e.g. bad bind credentials, it gives the user some
> direct feedback when trying to add/update a LDAP realm, if enabled.
>
> Should be rather a corner case, but it's the easiest way for us to
> accomodate and the most versatile for users needing this.
>
> This is part of the result of a previous discussion [0], and the same
> approach is already implemented for PBS [1].
>
> [0] https://lists.proxmox.com/pipermail/pve-devel/2023-May/056839.html
> [1] https://git.proxmox.com/?p=proxmox-backup.git;a=commitdiff;h=5210f3b5
>
> Signed-off-by: Christoph Heiss
> ---
> Changes v1 -> v2:
> * Do not store the 'check-connection' parameter in the realm config
> * Small fix; pass full config to check_connection() on update
>
> The decision to put this behind an optional (off-by-default) parameter
> was made during an off-list discussion with Lukas. The summary of that
> can be found as a follow-up on the previous series [2] [3].
>
> Note that the `base_dn`/`group_dn` now goes completely unvalided - but
> again, that is how it works in PBS. We /could/ use canonical_dn() for
> this (see previous series [4]), depending on whether we want this kind
> of "divergence" between PVE & PBS.
>
> [2] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058540.html
> [3] https://lists.proxmox.com/pipermail/pve-devel/2023-August/058582.html
> [4] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058392.html
>
> src/PVE/API2/Domains.pm | 10 ++
> src/PVE/Auth/LDAP.pm| 25 -
> src/PVE/Auth/Plugin.pm | 8
> 3 files changed, 34 insertions(+), 9 deletions(-)
>
> diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm
> index 9332aa7..ed2b6c4 100644
> --- a/src/PVE/API2/Domains.pm
> +++ b/src/PVE/API2/Domains.pm
> @@ -133,6 +133,7 @@ __PACKAGE__->register_method ({
>
> my $realm = extract_param($param, 'realm');
> my $type = $param->{type};
> + my $check_connection = extract_param($param,
> 'check-connection');
>
> die "domain '$realm' already exists\n"
> if $ids->{$realm};
> @@ -165,6 +166,10 @@ __PACKAGE__->register_method ({
> }
> $plugin->on_add_hook($realm, $config, password => $password);
>
> + # Only for LDAP/AD, implied through the existence of the
> 'check-connection' param
> + $plugin->check_connection($realm, $config, password =>
> $password)
> + if $check_connection;
> +
> cfs_write_file($domainconfigfile, $cfg);
> }, "add auth server failed");
>
> @@ -198,6 +203,7 @@ __PACKAGE__->register_method ({
> PVE::SectionConfig::assert_if_modified($cfg, $digest);
>
> my $realm = extract_param($param, 'realm');
> + my $check_connection = extract_param($param,
> 'check-connection');
>
> die "domain '$realm' does not exist\n"
> if !$ids->{$realm};
> @@ -237,6 +243,10 @@ __PACKAGE__->register_method ({
> $plugin->on_update_hook($realm, $config);
> }
>
> + # Only for LDAP/AD, implied through the existence of the
> 'check-connection' param
> + $plugin->check_connection($realm, $ids->{$realm}, password =>
> $password)
> + if $check_connection;
> +
> cfs_write_file($domainconfigfile, $cfg);
> }, "update auth server failed");
>
> diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
> index fc82a17..d5f4909 100755
> --- a/src/PVE/Auth/LDAP.pm
> +++ b/src/PVE/Auth/LDAP.pm
> @@ -10,9 +10,6 @@ use PVE::Tools;
>
> use base qw(PVE::Auth::Plugin);
>
> -my $dn_part_regex = qr!("[^"]+"|[^ ,+"/<>;=#][^,+"/<>;=]*[^ ,+"/<>;=]|[^
> ,+"/<>;=#])!;
> -our $dn_regex = qr!\w+=${dn_part_regex}(,\s*\w+=${dn_part_regex})*!;
> -
> sub type {
> return 'ldap';
> }
> @@ -22,7 +19,6 @@ sub properties {
> base_dn => {
> description => "LDAP base domain name",
> type => 'string',
> - pattern => $dn_regex,
> optional => 1,
> maxLength => 256,
> },
> @@ -36,7 +32,6 @@ sub properties {
> bind_dn => {
> description => "LDAP bind domain name",
> type => 'string',
> - pattern => $dn_regex,
> optional => 1,
> maxLength => 256,
> },
> @@ -94,7 +89,6 @@ sub properties {
> description => "LDAP base domain name for group sync. If not set,
> the"
> ." base_dn will be used.",
> type => 'string',
> - pattern => $dn_regex,
> optional => 1,
> maxLength => 256,
> },
> @
Re: [pve-devel] [PATCH v3 qemu-server] fix #3963: Skip TPM startup for template VMs
Am 09.08.23 um 17:24 schrieb Filip Schauer:
> Skip the software TPM startup when starting a template VM for performing
> a backup. This fixes an error that occurs when the TPM state disk is
> write-protected.
>
> Signed-off-by: Filip Schauer
Reviewed-by: Fiona Ebner
> ---
> Changes since v2:
> * Do not add the TPM to the command line arguments when VM is a template
>
> PVE/QemuServer.pm | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 484bc7f..aa98d5f 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3823,7 +3823,9 @@ sub config_to_command {
> push @$devices, @$audio_devs;
> }
>
Adding a short comment here about why would be nice for future readers.
> -add_tpm_device($vmid, $devices, $conf);
> +if (!PVE::QemuConfig->is_template($conf)) {
> + add_tpm_device($vmid, $devices, $conf);
> +}
Style nit: could be done in one line by using a post-if
Nit: This also modifies the output of qm showcmd. On the one hand, it's
correct, because it is the actual command the template is started with.
On the other hand, people might expect it to be what the command for a
non-template VM with the same settings would be. But I'd say, it's fine.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH access-control v2 2/3] ldap: add opt-in `check-connection` param to perform a bind check
On Thu, Aug 10, 2023 at 09:55:51AM +0200, Wolfgang Bumiller wrote:
> On Tue, Aug 01, 2023 at 02:37:18PM +0200, Christoph Heiss wrote:
[..]
> > @@ -137,7 +131,13 @@ sub properties {
> > type => 'boolean',
> > optional => 1,
> > default => 1,
> > - }
> > + },
> > + 'check-connection' => {
> > + description => 'Check bind connection to LDAP server.',
> > + type => 'boolean',
> > + optional => 1,
> > + default => 0,
> > + },
>
> While there's special handling for how we store the password, this
> schema here should still actually describe the stored config.
> Since this is a parameter specifically for the add/update API methods we
> should declare it in those functions as parameter.
>
> Some of our methods to get schemas have an optional hash parameter to
> include an extra set of base properties in its returned contents (see
> `get_standard_option` as an example), but `createSchema` and
> `updateSchema` do not.
Right, I was unsure anyway if this was the right way anyway to add this,
at least I did not see any other way - that explains why :^)
>
> We could either add this, or, since this is currently only required
> once, just move the `{create,update}Schema` calls over the
> `register_method()` calls and modify them right there before use...
> Since this series already touches pve-common, I have a *slight*
> preference to extending the `create/updateSchema` subs in
> `PVE::SectionConfig`,
Seems like the right thing - I'd also rather do it properly once than to
introduce a hack that sticks around ..
> although AFAICT the common patch does not strictly
> require a dependency bump inside pve-access-control as it mostly about
> how errors are presented to end-users (?), so either way is fine with
Exactly, the changes in pve-common are purely cosmectic.
> me. If we update the SectionConfig we'll definitely need a versioned
> dependency bump.
If it's OK for you I will go this route, extending
{create,update}Schema() as needed for this, in the same way
get_standard_option() works.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH access-control v2 2/3] ldap: add opt-in `check-connection` param to perform a bind check
On Thu, Aug 10, 2023 at 10:35:14AM +0200, Christoph Heiss wrote:
>
> On Thu, Aug 10, 2023 at 09:55:51AM +0200, Wolfgang Bumiller wrote:
> > On Tue, Aug 01, 2023 at 02:37:18PM +0200, Christoph Heiss wrote:
> [..]
> > > @@ -137,7 +131,13 @@ sub properties {
> > > type => 'boolean',
> > > optional => 1,
> > > default => 1,
> > > - }
> > > + },
> > > + 'check-connection' => {
> > > + description => 'Check bind connection to LDAP server.',
> > > + type => 'boolean',
> > > + optional => 1,
> > > + default => 0,
> > > + },
> >
> > While there's special handling for how we store the password, this
> > schema here should still actually describe the stored config.
> > Since this is a parameter specifically for the add/update API methods we
> > should declare it in those functions as parameter.
> >
> > Some of our methods to get schemas have an optional hash parameter to
> > include an extra set of base properties in its returned contents (see
> > `get_standard_option` as an example), but `createSchema` and
> > `updateSchema` do not.
> Right, I was unsure anyway if this was the right way anyway to add this,
> at least I did not see any other way - that explains why :^)
>
> >
> > We could either add this, or, since this is currently only required
> > once, just move the `{create,update}Schema` calls over the
> > `register_method()` calls and modify them right there before use...
> > Since this series already touches pve-common, I have a *slight*
> > preference to extending the `create/updateSchema` subs in
> > `PVE::SectionConfig`,
> Seems like the right thing - I'd also rather do it properly once than to
> introduce a hack that sticks around ..
>
> > although AFAICT the common patch does not strictly
> > require a dependency bump inside pve-access-control as it mostly about
> > how errors are presented to end-users (?), so either way is fine with
> Exactly, the changes in pve-common are purely cosmectic.
>
> > me. If we update the SectionConfig we'll definitely need a versioned
> > dependency bump.
> If it's OK for you I will go this route, extending
> {create,update}Schema() as needed for this, in the same way
> get_standard_option() works.
Sure, just be aware that `get_standard_option()` only needs 1 level,
whereas for the section config ones it works one level above, as in
it returns a full schema like:
{
type => 'object',
properties => { ... }
}
It's unlikely that we'll need to modify anything at the top level, so I
think it would be fine for the extra parameter to only affect the
contained properties hash, so no need to do any multi-level merging.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v4 qemu-server] fix #3963: Skip TPM startup for template VMs
Skip the software TPM startup when starting a template VM for performing
a backup. This fixes an error that occurs when the TPM state disk is
write-protected.
Signed-off-by: Filip Schauer
---
Changes since v3:
* Add a comment explaining why not to add a TPM to the command if the VM is a
template
* Refactor code with post-if
PVE/QemuServer.pm | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 484bc7f..bf1de17 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3823,7 +3823,9 @@ sub config_to_command {
push @$devices, @$audio_devs;
}
-add_tpm_device($vmid, $devices, $conf);
+# Add a TPM only if the VM is not a template,
+# to support backing up template VMs even if the TPM disk is
write-protected.
+add_tpm_device($vmid, $devices, $conf) if
(!PVE::QemuConfig->is_template($conf));
my $sockets = 1;
$sockets = $conf->{smp} if $conf->{smp}; # old style - no longer iused
@@ -5923,7 +5925,7 @@ sub vm_start_nolock {
PVE::Systemd::enter_systemd_scope($vmid, "Proxmox VE VM $vmid",
%systemd_properties);
my $tpmpid;
- if (my $tpm = $conf->{tpmstate0}) {
+ if ((my $tpm = $conf->{tpmstate0}) &&
!PVE::QemuConfig->is_template($conf)) {
# start the TPM emulator so QEMU can connect on start
$tpmpid = start_swtpm($storecfg, $vmid, $tpm, $migratedfrom);
}
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] fix #474: allow transfer from container/vms
On 8/10/23 09:16, Fiona Ebner wrote: But it should. After all, the operation is modifying the original pool, so the user better have an appropriate permission to do so. Currently, Permissions.Modify|VM.Allocate on the VM and Pool.Allocate on the target pool would be enough to "steal" the guest, no permissions required on the original pool at all. IMHO, the user really should have a Pool.Allocate on the original pool as well. You are right! It would be possible to "steal" a VM in a way that it was not before! Thank you for finding this! Will fix! Since I noticed it in v3: we usually use "api:" and "ui:" as prefixes rather than "backend:" and "frontend:". Would be nice if you could use them too for consistency. Ok. Good to know. I will do that. Thanks ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager v4 1/2] fix #474: api: allow transfer from container/vms
When the newly introduced optional parameter "transfer" is set, the user
add a vm/container to a pool even if it is already in one. If so it will
be removed from the old pool
Signed-off-by: Philipp Hufnagl
---
PVE/API2/Pool.pm | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/PVE/API2/Pool.pm b/PVE/API2/Pool.pm
index 007fc815..3c0ae2a0 100644
--- a/PVE/API2/Pool.pm
+++ b/PVE/API2/Pool.pm
@@ -131,6 +131,11 @@ __PACKAGE__->register_method ({
type => 'string', format => 'pve-storage-id-list',
optional => 1,
},
+ transfer => {
+ description => "Allow transferring VMs to another pool.",
+ type => 'boolean',
+ optional => 1,
+ },
delete => {
description => "Remove vms/storage (instead of adding it).",
type => 'boolean',
@@ -165,8 +170,15 @@ __PACKAGE__->register_method ({
} else {
die "VM $vmid is already a pool member\n" if
$pool_config->{vms}->{$vmid};
my $existing_pool = $usercfg->{vms}->{$vmid};
- die "VM $vmid belongs already to pool
'$existing_pool'\n" if defined($existing_pool);
-
+ if (defined($existing_pool)) {
+ if ($param->{transfer}) {
+ $rpcenv->check($authuser,
"/pool/$existing_pool", ['Pool.Allocate']);
+ my $existing_pool_config =
$usercfg->{pools}->{$existing_pool};
+ delete $existing_pool_config->{vms}->{$vmid};
+ } else {
+ die "VM $vmid belongs already to pool
'$existing_pool' and transfer is not set\n";
+ }
+ }
$pool_config->{vms}->{$vmid} = 1;
$usercfg->{vms}->{$vmid} = $pool;
}
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager v4 0/2] fix #474: allow transfer from container/vms
When a member of 2 pools wants to transfer a vm/container to an other pool they can not do that. The vm/container would have first to be removed form the current pool resulting in a loss of privileges of the pool member for this vm/contianer. This feature introduces a way to transfer a vm between pools, guarded by a checkbox from accidental transfers changes to v4: * check for allocate permissions of the originating pool changes to v3: * fix subject typo * at version log changes to v2: * split patch in front and backend Philipp Hufnagl (2): fix #474: api: allow transfer from container/vms fix #474: ui: allow transfer from container/vms PVE/API2/Pool.pm | 16 ++-- www/manager6/grid/PoolMembers.js | 17 ++--- 2 files changed, 28 insertions(+), 5 deletions(-) -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager v4 2/2] fix #474: ui: allow transfer from container/vms
A user can no see all vms/containers, even the ones that are already a
member of a pool. They can be transfered now after checking the newly
introduced "allow transfer" checkbox.
Signed-off-by: Philipp Hufnagl
---
www/manager6/grid/PoolMembers.js | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
index 6acb622d..224daca3 100644
--- a/www/manager6/grid/PoolMembers.js
+++ b/www/manager6/grid/PoolMembers.js
@@ -1,7 +1,7 @@
Ext.define('PVE.pool.AddVM', {
extend: 'Proxmox.window.Edit',
width: 600,
-height: 400,
+height: 420,
isAdd: true,
isCreate: true,
initComponent: function() {
@@ -30,7 +30,7 @@ Ext.define('PVE.pool.AddVM', {
],
filters: [
function(item) {
- return (item.data.type === 'lxc' || item.data.type ===
'qemu') && item.data.pool === '';
+ return (item.data.type === 'lxc' || item.data.type ===
'qemu') &&item.data.pool !== me.pool;
},
],
});
@@ -63,6 +63,10 @@ Ext.define('PVE.pool.AddVM', {
header: gettext('Node'),
dataIndex: 'node',
},
+ {
+ header: gettext('Pool'),
+ dataIndex: 'pool',
+ },
{
header: gettext('Status'),
dataIndex: 'uptime',
@@ -85,9 +89,16 @@ Ext.define('PVE.pool.AddVM', {
},
],
});
+
+ let transfer = Ext.create('Ext.form.field.Checkbox', {
+ name: 'transfer',
+ boxLabel: gettext('Allow Transfer'),
+ inputValue: 1,
+ value: 0,
+ });
Ext.apply(me, {
subject: gettext('Virtual Machine'),
- items: [vmsField, vmGrid],
+ items: [vmsField, vmGrid, transfer],
});
me.callParent();
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer 0/2] add stricter checks w.r.t. BIOS boot on 4Kn disks
Booting in legacy BIOS mode on 4Kn disks is generally unsupported, or rather, simply does not work. The GUI installer already checks that properly for LVM and ZFS, but is missing it for Btrfs, so extend the check appropriately. Further, the TUI installer only checked it for ZFS RAIDs, but not for LVM and Btrfs. Add proper checks there as well, such that the user gets immediate feedback early than during the install process. Seems nobody noticed yet, but apparently 4Kn disks are pretty rare these days anyway. Prerequisites - This series is based on top of [PATCH installer 0/6] some small, assorted fixes & cleanups https://lists.proxmox.com/pipermail/pve-devel/2023-August/058708.html which fixes another 4Kn disk issue. Testing --- Tested by creating a block device with 4K sectorsize using the following QEMU args: -device virtio-blk,drive=testdrive4k,logical_block_size=4096,physical_block_size=4096 -drive file=/path/to/4k-testdisk.img,if=none,id=testdrive4k The 4k-testdisk.img was created with: qemu-img create -f qcow2 /path/to/4k-testdisk.img 16G Christoph Heiss (2): raid setup: btrfs: do not allow legacy BIOS boot on 4Kn disks tui: disallow legacy BIOS boot from 4Kn disks for all filesystems Proxmox/Install.pm | 4 + proxmox-tui-installer/src/main.rs | 2 +- proxmox-tui-installer/src/setup.rs | 2 +- proxmox-tui-installer/src/views/bootdisk.rs | 171 4 files changed, 74 insertions(+), 105 deletions(-) -- 2.41.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer 1/2] raid setup: btrfs: do not allow legacy BIOS boot on 4Kn disks
This is already checked for LVM and ZFS setups, but not for Btrfs. Add
it there too, as it doesn't work anyway.
Tested by creating a block device with 4K sectorsize using
the following QEMU args:
-device
virtio-blk,drive=testdrive4k,logical_block_size=4096,physical_block_size=4096
-drive file=/path/to/4k-testdisk.img,if=none,id=testdrive4k
The 4k-testdisk.img was created with:
qemu-img create -f qcow2 /path/to/4k-testdisk.img 16G
Signed-off-by: Christoph Heiss
---
Proxmox/Install.pm | 4
1 file changed, 4 insertions(+)
diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index 1c4811d..1117fc4 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -299,6 +299,10 @@ sub get_btrfs_raid_setup {
my $diskcount = scalar(@$devlist);
die "$filesys needs at least one device\n" if $diskcount < 1;
+foreach my $hd (@$devlist) {
+ legacy_bios_4k_check(@$hd[4]);
+}
+
my $mode;
if ($diskcount == 1) {
--
2.41.0
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer 2/2] tui: disallow legacy BIOS boot from 4Kn disks for all filesystems
The GUI installer already has the same rules in place, not allowing to
boot from 4Kn disks when booting in legacy BIOS mode. The TUI installer
currently only checks that for ZFS RAIDs, so extend that check to all
filesystem configurations.
Signed-off-by: Christoph Heiss
---
proxmox-tui-installer/src/main.rs | 2 +-
proxmox-tui-installer/src/setup.rs | 2 +-
proxmox-tui-installer/src/views/bootdisk.rs | 171
3 files changed, 70 insertions(+), 105 deletions(-)
diff --git a/proxmox-tui-installer/src/main.rs
b/proxmox-tui-installer/src/main.rs
index 580cb34..23a4ead 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -431,7 +431,7 @@ fn bootdisk_dialog(siv: &mut Cursive) -> InstallerView {
InstallerView::new(
&state,
-BootdiskOptionsView::new(&state.runtime_info.disks,
&state.options.bootdisk)
+BootdiskOptionsView::new(siv, &state.runtime_info.disks,
&state.options.bootdisk)
.with_name("bootdisk-options"),
Box::new(|siv| {
let options = siv.call_on_name("bootdisk-options",
BootdiskOptionsView::get_values);
diff --git a/proxmox-tui-installer/src/setup.rs
b/proxmox-tui-installer/src/setup.rs
index dec91cb..46b47cd 100644
--- a/proxmox-tui-installer/src/setup.rs
+++ b/proxmox-tui-installer/src/setup.rs
@@ -392,7 +392,7 @@ pub struct RuntimeInfo {
pub hvm_supported: bool,
}
-#[derive(Clone, Eq, Deserialize, PartialEq)]
+#[derive(Copy, Clone, Eq, Deserialize, PartialEq)]
#[serde(rename_all = "lowercase")]
pub enum BootType {
Bios,
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs
b/proxmox-tui-installer/src/views/bootdisk.rs
index d01495e..dbd13ea 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -16,17 +16,18 @@ use crate::{
FsType, LvmBootdiskOptions, ZfsBootdiskOptions, ZfsRaidLevel, FS_TYPES,
ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
},
-setup::{BootType, RuntimeInfo},
+setup::BootType,
};
use crate::{setup::ProxmoxProduct, InstallerState};
pub struct BootdiskOptionsView {
view: LinearLayout,
advanced_options: Rc>,
+boot_type: BootType,
}
impl BootdiskOptionsView {
-pub fn new(disks: &[Disk], options: &BootdiskOptions) -> Self {
+pub fn new(siv: &mut Cursive, disks: &[Disk], options: &BootdiskOptions)
-> Self {
let bootdisk_form = FormView::new()
.child(
"Target harddisk",
@@ -53,9 +54,15 @@ impl BootdiskOptionsView {
.child(DummyView)
.child(advanced_button);
+let boot_type = siv
+.user_data::()
+.map(|state| state.runtime_info.boot_type)
+.unwrap_or(BootType::Bios);
+
Self {
view,
advanced_options,
+boot_type,
}
}
@@ -74,6 +81,7 @@ impl BootdiskOptionsView {
options.disks = vec![disk];
}
+check_disks_4kn_legacy_boot(self.boot_type, &options.disks)?;
Ok(options)
}
}
@@ -168,7 +176,7 @@ impl AdvancedBootdiskOptionsView {
);
}
-fn get_values(&mut self, runinfo: &RuntimeInfo) -> Result {
+fn get_values(&mut self) -> Result {
let fstype = self
.view
.get_child(1)
@@ -198,8 +206,7 @@ impl AdvancedBootdiskOptionsView {
.ok_or("Failed to retrieve advanced bootdisk options")?;
if let FsType::Zfs(level) = fstype {
-check_zfs_raid_config(runinfo, level, &disks)
-.map_err(|err| format!("{fstype}: {err}"))?;
+check_zfs_raid_config(level, &disks).map_err(|err|
format!("{fstype}: {err}"))?;
}
Ok(BootdiskOptions {
@@ -554,17 +561,11 @@ fn advanced_options_view(disks: &[Disk], options:
Rc>)
.button("Ok", {
let options_ref = options.clone();
move |siv| {
-let runinfo = siv
-.user_data::()
-.unwrap()
-.runtime_info
-.clone();
-
let options = siv
.call_on_name("advanced-bootdisk-options-dialog", |view: &mut
Dialog| {
view.get_content_mut()
.downcast_mut::()
-.map(|v| v.get_values(&runinfo))
+.map(AdvancedBootdiskOptionsView::get_values)
})
.flatten();
@@ -626,29 +627,33 @@ fn check_raid_min_disks(disks: &[Disk], min: usize) ->
Result<(), String> {
}
}
-/// Checks whether a user-supplied ZFS RAID setup is valid or not, such as
disk sizes, minimum
-/// number of disks and legacy BIOS compatibility.
+/// Checks all disks for legacy BIOS boot compatibility and reports an error
as appropriate. 4Kn
+/// disks are generally broken with legacy BIOS and cannot be booted from.
///
///
[pve-devel] [PATCH manager v3 4/4] ui: ldap: add 'Check connection' checkbox as advanced option
The checkbox is enabled by default, setting the new `check-connection`
parameter. See also [0] for the rationale.
[0] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058559.html
Signed-off-by: Christoph Heiss
---
N.B.: As this uses the newly introduced `check-connection` API
parameter, a bump on libpve-access-control is needed.
Changes v2 -> v3:
* No changes
Changes v1 -> v2:
* Add "Check connection" checkbox to AD edit too
www/manager6/dc/AuthEditAD.js | 15 +++
www/manager6/dc/AuthEditLDAP.js | 15 +++
2 files changed, 30 insertions(+)
diff --git a/www/manager6/dc/AuthEditAD.js b/www/manager6/dc/AuthEditAD.js
index a1999cb74..3cbb47c9f 100644
--- a/www/manager6/dc/AuthEditAD.js
+++ b/www/manager6/dc/AuthEditAD.js
@@ -79,6 +79,21 @@ Ext.define('PVE.panel.ADInputPanel', {
},
];
+ me.advancedItems = [
+ {
+ xtype: 'proxmoxcheckbox',
+ fieldLabel: gettext('Check connection'),
+ name: 'check-connection',
+ uncheckedValue: 0,
+ checked: true,
+ autoEl: {
+ tag: 'div',
+ 'data-qtip':
+ gettext('Verify connection parameters and bind
credentials on save'),
+ },
+ },
+ ];
+
me.callParent();
},
onGetValues: function(values) {
diff --git a/www/manager6/dc/AuthEditLDAP.js b/www/manager6/dc/AuthEditLDAP.js
index 2ce16e58c..9986db8a9 100644
--- a/www/manager6/dc/AuthEditLDAP.js
+++ b/www/manager6/dc/AuthEditLDAP.js
@@ -79,6 +79,21 @@ Ext.define('PVE.panel.LDAPInputPanel', {
},
];
+ me.advancedItems = [
+ {
+ xtype: 'proxmoxcheckbox',
+ fieldLabel: gettext('Check connection'),
+ name: 'check-connection',
+ uncheckedValue: 0,
+ checked: true,
+ autoEl: {
+ tag: 'div',
+ 'data-qtip':
+ gettext('Verify connection parameters and bind
credentials on save'),
+ },
+ },
+ ];
+
me.callParent();
},
onGetValues: function(values) {
--
2.41.0
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH common/access-control/manager v3 0/4] ldap: check bind connection on realm add/update
First of, remove the dreaded LDAP DN regex.
Further, upon saving a LDAP realm in the UI, it tries to connect & bind
using the provided credentials, providing the user with immediate
feedback whether they are valid or not.
The same approach is already implemented in PBS [0], and I'll plan to
implement the same for PMG too, if & when the PVE side is done.
Testing
---
Changes were tested against slapd 2.5.13+dfsg-5 (for LDAP), Samba 4.18.5
and Windows Server 2022 (for AD), using both the web UI and `pveum` to
create and update realms with different combinations of valid and
invalid parameters, mixed with using new `check-connection` parameter.
Prior art
-
v2: https://lists.proxmox.com/pipermail/pve-devel/2023-August/058586.html
v1: https://lists.proxmox.com/pipermail/pve-devel/2023-July/058551.html
Notable changes v2 -> v3:
* Move 'check-connection' param out of schema to extra param
(thanks Wolfgang!)
Notable changes v1 -> v2:
* Added patch #1 from previous series [1], missed that in v1
* Do not store the 'check-connection' parameter in the realm config
* Add "Check connection" checkbox to AD edit too
This series supersedes [1], which previously solved this using a new
schema format by validating DNs using Net::LDAP::Util::canonical_dn().
But this has the problem that it does not support AD-specific DN syntax.
After a off-list discussion with Lukas (summary [2] [3]), it was decided to
rather implement it much like PBS does it - simply drop the explicit
validation of DN parameters, instead just trying to connect & bind to
the target server.
[0] https://git.proxmox.com/?p=proxmox-backup.git;a=commitdiff;h=5210f3b5
[1] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058392.html
[2] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058540.html
[3] https://lists.proxmox.com/pipermail/pve-devel/2023-August/058582.html
pve-common:
Christoph Heiss (2):
ldap: handle errors explicitly everywhere instead of simply `die`ing
section config: allow base properties for {create,update}Schema()
src/PVE/LDAP.pm | 11 ++-
src/PVE/SectionConfig.pm | 8
2 files changed, 10 insertions(+), 9 deletions(-)
pve-access-control:
Christoph Heiss (1):
ldap: add opt-in `check-connection` param to perform a bind check
src/PVE/API2/Domains.pm | 36 +---
src/PVE/Auth/LDAP.pm| 18 +-
src/PVE/Auth/Plugin.pm | 8
3 files changed, 50 insertions(+), 12 deletions(-)
pve-manager:
Christoph Heiss (1):
ui: ldap: add 'Check connection' checkbox as advanced option
www/manager6/dc/AuthEditAD.js | 15 +++
www/manager6/dc/AuthEditLDAP.js | 15 +++
2 files changed, 30 insertions(+)
--
2.41.0
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH access-control v3 3/4] ldap: add opt-in `check-connection` param to perform a bind check
Removes the dreaded DN regex, instead introducing a optional
connect/bind check on creation/update, aligning it with the way PBS does
it.
Additionally, it has the benefit that instead of letting a sync fail on
the first try due to e.g. bad bind credentials, it gives the user some
direct feedback when trying to add/update a LDAP realm, if enabled.
Should be rather a corner case, but it's the easiest way for us to
accomodate and the most versatile for users needing this.
This is part of the result of a previous discussion [0], and the same
approach is already implemented for PBS [1].
[0] https://lists.proxmox.com/pipermail/pve-devel/2023-May/056839.html
[1] https://lists.proxmox.com/pipermail/pbs-devel/2023-June/006237.html
Signed-off-by: Christoph Heiss
---
N.B.: Needs a version bump of libpve-common-perl for the base properties
parameter on {create,update}Schema().
Changes v2 -> v3:
* Move 'check-connection' out of schema definition to extra param
Pointed out by Wolfgang, it should not be part of the (stored)
config schema if never stored. [2]
Changes v1 -> v2:
* Do not store the 'check-connection' parameter in the realm config
The decision to put this behind an optional (off-by-default) parameter
was made during an off-list discussion with Lukas. The summary of that
can be found as a follow-up on the previous series [3] [4].
Note that the `base_dn`/`group_dn` now goes completely unvalided - but
again, that is how it works in PBS.
[2] https://lists.proxmox.com/pipermail/pve-devel/2023-August/058724.html
[3] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058540.html
[4] https://lists.proxmox.com/pipermail/pve-devel/2023-August/058582.html
src/PVE/API2/Domains.pm | 36 +---
src/PVE/Auth/LDAP.pm| 18 +-
src/PVE/Auth/Plugin.pm | 8
3 files changed, 50 insertions(+), 12 deletions(-)
diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm
index 9332aa7..e7b7d39 100644
--- a/src/PVE/API2/Domains.pm
+++ b/src/PVE/API2/Domains.pm
@@ -117,7 +117,14 @@ __PACKAGE__->register_method ({
check => ['perm', '/access/realm', ['Realm.Allocate']],
},
description => "Add an authentication server.",
-parameters => PVE::Auth::Plugin->createSchema(),
+parameters => PVE::Auth::Plugin->createSchema(0, {
+ 'check-connection' => {
+ description => 'Check bind connection to the server.',
+ type => 'boolean',
+ optional => 1,
+ default => 0,
+ },
+}),
returns => { type => 'null' },
code => sub {
my ($param) = @_;
@@ -133,6 +140,7 @@ __PACKAGE__->register_method ({
my $realm = extract_param($param, 'realm');
my $type = $param->{type};
+ my $check_connection = extract_param($param,
'check-connection');
die "domain '$realm' already exists\n"
if $ids->{$realm};
@@ -143,6 +151,9 @@ __PACKAGE__->register_method ({
die "unable to create builtin type '$type'\n"
if ($type eq 'pam' || $type eq 'pve');
+ die "'check-connection' parameter can only be set for realms of
type 'ldap' or 'ad'\n"
+ if defined($check_connection) && !($type eq 'ldap' || $type
eq 'ad');
+
if ($type eq 'ad' || $type eq 'ldap') {
$map_sync_default_options->($param, 1);
}
@@ -165,6 +176,10 @@ __PACKAGE__->register_method ({
}
$plugin->on_add_hook($realm, $config, password => $password);
+ # Only for LDAP/AD, implied through the existence of the
'check-connection' param
+ $plugin->check_connection($realm, $config, password =>
$password)
+ if $check_connection;
+
cfs_write_file($domainconfigfile, $cfg);
}, "add auth server failed");
@@ -180,7 +195,14 @@ __PACKAGE__->register_method ({
},
description => "Update authentication server settings.",
protected => 1,
-parameters => PVE::Auth::Plugin->updateSchema(),
+parameters => PVE::Auth::Plugin->updateSchema(0, {
+ 'check-connection' => {
+ description => 'Check bind connection to the server.',
+ type => 'boolean',
+ optional => 1,
+ default => 0,
+ },
+}),
returns => { type => 'null' },
code => sub {
my ($param) = @_;
@@ -198,10 +220,15 @@ __PACKAGE__->register_method ({
PVE::SectionConfig::assert_if_modified($cfg, $digest);
my $realm = extract_param($param, 'realm');
+ my $type = $ids->{$realm}->{type};
+ my $check_connection = extract_param($param,
'check-connection');
die "domain '$realm' does not exist\n"
if !$ids->{$realm};
+ die "'check-connection' parameter can only be set for realms of
type 'ldap' or '
[pve-devel] [PATCH common v3 2/4] section config: allow base properties for {create, update}Schema()
This works the same way as e.g. get_standard_option does it.
Signed-off-by: Christoph Heiss
---
Changes v2 -> v3:
* New patch; as suggested by Wolfgang
src/PVE/SectionConfig.pm | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 97492db..2b75e6a 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -52,13 +52,13 @@ sub plugindata {
}
sub createSchema {
-my ($class, $skip_type) = @_;
+my ($class, $skip_type, $base) = @_;
my $pdata = $class->private();
my $propertyList = $pdata->{propertyList};
my $plugins = $pdata->{plugins};
-my $props = {};
+my $props = $base || {};
my $copy_property = sub {
my ($src) = @_;
@@ -107,13 +107,13 @@ sub createSchema {
}
sub updateSchema {
-my ($class, $single_class) = @_;
+my ($class, $single_class, $base) = @_;
my $pdata = $class->private();
my $propertyList = $pdata->{propertyList};
my $plugins = $pdata->{plugins};
-my $props = {};
+my $props = $base || {};
my $filter_type = $single_class ? $class->type() : undef;
--
2.41.0
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH common v3 1/4] ldap: handle errors explicitly everywhere instead of simply `die`ing
Most codepaths already have explicit error handling (by the means of
checking the return value), which is essential dead code due to setting
`onerror`.
As LDAP errors might get presented to users due to upcoming changes, the
error location should not be present in these error messages, thus
switch to explicit handling.
Only two calls were missing such explicit handling of errors, so these
are amended as appropriate. Further, some `die`s were missing newlines
at the end of the message, which - again - would cause the error
location to be included.
Signed-off-by: Christoph Heiss
---
Changes v2 -> v3:
* No changes
Changes v1 -> v2:
* New patch; from previous series [0], as it is also needed here
[0] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058392.html
src/PVE/LDAP.pm | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/PVE/LDAP.pm b/src/PVE/LDAP.pm
index 342c352..16a0a8e 100644
--- a/src/PVE/LDAP.pm
+++ b/src/PVE/LDAP.pm
@@ -22,7 +22,6 @@ sub ldap_connect {
scheme => $scheme,
port => $port,
timeout => 10,
- onerror => 'die',
);
my $hosts = [];
@@ -41,7 +40,8 @@ sub ldap_connect {
my $ldap = Net::LDAP->new($hosts, %ldap_opts) || die "$@\n";
if ($start_tls) {
- $ldap->start_tls(%$opts);
+ my $res = $ldap->start_tls(%$opts);
+ die $res->error . "\n" if $res->code;
}
return $ldap;
@@ -73,6 +73,7 @@ sub get_user_dn {
filter => "$attr=$name",
attrs => ['dn']
);
+die $result->error . "\n" if $result->code;
return undef if !$result->entries;
my @entries = $result->entries;
return $entries[0]->dn;
@@ -93,7 +94,7 @@ sub auth_user_dn {
if ($code) {
return undef if $noerr;
- die $err;
+ die "$err\n";
}
return 1;
@@ -184,7 +185,7 @@ sub query_users {
$err = "LDAP user query unsuccessful" if !$err;
}
-die $err if $err;
+die "$err\n" if $err;
return $users;
}
@@ -265,7 +266,7 @@ sub query_groups {
$err = "LDAP group query unsuccessful" if !$err;
}
-die $err if $err;
+die "$err\n" if $err;
return $groups;
}
--
2.41.0
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
