[pve-devel] [PATCH v2 container] lxc start: warn in case of conflicting lxc.idmap entries
Wolfgang Bumiller
w.bumiller at proxmox.com
Fri May 12 11:19:51 CEST 2023
On Tue, May 02, 2023 at 05:05:36PM +0200, Friedrich Weber wrote:
> Users can customize the mapping between host and container uids/gids
> by providing `lxc.idmap` entries in the container config. The syntax
> is described in lxc.container.conf(5). One source of errors are
> conflicting entries for one or more uid/gids. An example:
>
> ...
> lxc.idmap: u 0 100000 65536
> lxc.idmap: u 1000 1000 10
> ...
>
> Assuming `root:1000:10` is correctly added to /etc/subuid, starting
> the container fails with an error that is hard to interpret:
>
> lxc_map_ids: 3701 newuidmap failed to write mapping
> "newuidmap: write to uid_map failed: Invalid argument":
> newuidmap 67993 0 100000 65536 1000 1000 10
>
> In order to simplify troubleshooting, validate the mapping before
> starting the container and print a warning if a conflict is detected.
> For the above mapping:
>
> lxc.idmap: invalid map entry 'u 1000 1000 10':
> container uid 1000 is also mapped by entry 'u 0 100000 65536'
>
> The warning appears in the task log and in the output of `pct start`.
>
> The validation subroutine considers uid and gid mappings separately.
> For each of the two types, it makes one pass to detect container id
> conflicts and one pass to detect host id conflicts. The subroutine
> dies with the first detected conflict.
>
> A failed validation only prints a warning instead of erroring out, to
> make sure buggy (or outdated) validation logic does not prevent
> containers from starting.
>
> Note that validation does not take /etc/sub{uid,gid} into account,
> which, if misconfigured, could still prevent the container from
> starting with an error like
>
> "newuidmap: uid range [1000-1010) -> [1000-1010) not allowed"
>
> If needed, validating /etc/sub{uid,gid} could be added in the future.
>
> Signed-off-by: Friedrich Weber <f.weber at proxmox.com>
> ---
>
> Notes:
> Changes from v1:
> - implemented simpler sorting algorithm suggested by Wolfgang
> - removed "< 100 entries" safeguard. Due to the more efficient
> algorithm it is not needed anymore.
>
> The validation subroutine is now quite straightforward, but only
> reports the first detected conflict (which is also not necessarily the
> first lxc.idmap line with a conflict).
>
> I originally planned to let the validation subroutine print *all*
> detected conflicts, but realized that would require a more
> sophisticated algorithm. For example, if the mapping is (ignoring
> container gids and host uid/gids):
>
> lxc.idmap: u 5 ... 6 # 5..10
> lxc.idmap: u 0 ... 11 # 0..10
> lxc.idmap: u 3 ... 2 # 3..4
>
> we would sort this and get
>
> lxc.idmap: u 0 ... 11 # 0..10
> lxc.idmap: u 3 ... 2 # 3..4
> lxc.idmap: u 5 ... 6 # 5..10
>
> then iterate over the array, detect that 0 + 11 > 3, and report that
> 0..10 and 3..4 are in conflict. Since we want to report all conflicts,
> we would keep iterating. As 3 + 2 <= 5, there is no overlap and we
> wouldn't report anything. But now we missed that 0..10 and 5..10 are
> in conflict, because we only compared every entry with the previous
> one and forgot about the "currently active" interval 0..10.
>
> So to report *all* conflicts, we'd need an algorithm that keeps track
> of all currently active intervals while iterating. I'm open for
^ Only the end really.
> implementing this, but in my opinion, the algorithm in this patch is
> good enough to be helpful, and since it's nicer to keep things simple,
> I'd keep that one. What do you think?
Yeah, given that we don't know the user's actual intention anyway, this
really doesn't matter and the current implementation is fine.
Apart from some style issues, see below...
>
> src/PVE/LXC.pm | 40 +++++++++
> src/test/Makefile | 5 +-
> src/test/idmap-test.pm | 174 ++++++++++++++++++++++++++++++++++++
> src/test/run_idmap_tests.pl | 10 +++
> 4 files changed, 228 insertions(+), 1 deletion(-)
> create mode 100644 src/test/idmap-test.pm
> create mode 100755 src/test/run_idmap_tests.pl
>
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index d138161..9afb7f6 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -2324,6 +2324,40 @@ sub parse_id_maps {
> return ($id_map, $rootuid, $rootgid);
> }
>
> +sub validate_id_maps {
> + my ($id_map) = @_;
> +
> + # $mappings->{$type}{$side} = [ { line => $line, start => $start, count => $count }, ... ]
^ we always use arrows there: {$type}->{$side}
Basically, we always strictly separate with arrows. Old code might have
some mixed prefix-derefs, but there shouldn't be any code using
consecutive `{x}{y}` style dereferencing.
> + # $type: either "u" or "g"
> + # $side: either "container" or "host"
> + # $line: index of this mapping in @$id_map
> + # $start, $count: interval of this mapping
> + my $mappings = {};
> + for (my $i = 0; $i < scalar(@$id_map); $i++) {
> + my ($type, $ct_start, $host_start, $count) = $id_map->[$i]->@*;
> + push $mappings->{$type}{host}->@*, { line => $i, start => $host_start, count => $count};
> + push $mappings->{$type}{container}->@*, { line => $i, start => $ct_start, count => $count};
^ same
> + }
> +
> + # find the first conflict between two consecutive mappings when sorted by their start id
> + for my $type (qw(u g)) {
> + for my $side (qw(container host)) {
> + my @entries = sort { $a->{start} <=> $b->{start} } $mappings->{$type}{$side}->@*;
^ same
> + for my $idx (1..scalar(@entries) - 1) {
> + my $previous = $entries[$idx - 1];
> + my $current = $entries[$idx];
> + if ($previous->{start} + $previous->{count} > $current->{start}) {
> + my $conflict = $current->{start};
> + my @previous_line = $id_map->[$previous->{line}]->@*;
> + my @current_line = $id_map->[$current->{line}]->@*;
> + die "invalid map entry '@current_line': $side ${type}id $conflict "
> + ."is also mapped by entry '@previous_line'\n";
> + }
> + }
> + }
> + }
> +}
> +
> sub userns_command {
> my ($id_map) = @_;
> if (@$id_map) {
> @@ -2406,6 +2440,12 @@ sub vm_start {
>
> update_lxc_config($vmid, $conf);
>
> + eval {
> + my ($id_map, undef, undef) = PVE::LXC::parse_id_maps($conf);
> + PVE::LXC::validate_id_maps($id_map);
> + };
> + warn "lxc.idmap: $@" if $@;
> +
> my $skiplock_flag_fn = "/run/lxc/skiplock-$vmid";
>
> if ($skiplock) {
> diff --git a/src/test/Makefile b/src/test/Makefile
> index 8734879..82e8967 100644
> --- a/src/test/Makefile
> +++ b/src/test/Makefile
> @@ -2,7 +2,7 @@ RUN_USERNS := lxc-usernsexec -m "u:0:`id -u`:1" -m "g:0:`id -g`:1" --
>
> all: test
>
> -test: test_setup test_snapshot test_bindmount
> +test: test_setup test_snapshot test_bindmount test_idmap
>
> test_setup: run_setup_tests.pl
> $(RUN_USERNS) ./run_setup_tests.pl
> @@ -13,5 +13,8 @@ test_snapshot: run_snapshot_tests.pl
> test_bindmount: bindmount_test.pl
> $(RUN_USERNS) ./bindmount_test.pl
>
> +test_idmap: run_idmap_tests.pl
> + ./run_idmap_tests.pl
> +
> clean:
> rm -rf tmprootfs
> diff --git a/src/test/idmap-test.pm b/src/test/idmap-test.pm
> new file mode 100644
> index 0000000..db12aa5
> --- /dev/null
> +++ b/src/test/idmap-test.pm
> @@ -0,0 +1,174 @@
> +package PVE::LXC::Test;
> +
> +use strict;
> +use warnings;
> +
> +use lib qw(..);
> +
> +use Test::More;
> +use Time::HiRes qw (gettimeofday tv_interval);
> +
> +use PVE::LXC;
> +
> +subtest 'valid: default config (unprivileged)' => sub {
> + plan tests => 1;
> +
> + my ($id_maps, undef, undef) = PVE::LXC::parse_id_maps({
> + unprivileged => 1,
> + lxc => [ ['rootfs', 'xyz'] ],
> + });
> +
> + $@ = undef;
> + eval {
> + PVE::LXC::validate_id_maps($id_maps);
> + };
> + is($@, "", "no error");
> +};
> +
> +subtest 'valid: mapping one user/group to host' => sub {
> + plan tests => 1;
> +
> + my ($id_maps, undef, undef) = PVE::LXC::parse_id_maps({
> + lxc => [
> + ['lxc.idmap', 'u 0 100000 1005'],
> + ['lxc.idmap', 'g 0 100000 1007'],
> + ['lxc.idmap', 'u 1005 1005 1'],
> + ['lxc.idmap', 'g 1007 1007 1'],
> + ['lxc.idmap', 'u 1006 101006 64530'],
> + ['lxc.idmap', 'g 1008 101008 64528'],
> + ],
> + });
> +
> + $@ = undef;
> + eval {
> + PVE::LXC::validate_id_maps($id_maps);
> + };
> + is($@, "", "no error");
> +};
> +
> +subtest 'valid: mapping user/group ranges to host' => sub {
> + plan tests => 1;
> +
> + my ($id_maps, undef, undef) = PVE::LXC::parse_id_maps({
> + lxc => [
> + ['lxc.idmap', 'u 3000 103000 60000'],
> + ['lxc.idmap', 'u 2000 1000 1000'],
> + ['lxc.idmap', 'u 0 100000 2000'],
> + ['lxc.idmap', 'g 2000 102000 63534'],
> + ['lxc.idmap', 'g 1000 2000 1000'],
> + ['lxc.idmap', 'g 0 100000 1000'],
> + ['lxc.idmap', 'u 63000 263000 2536'],
> + ['lxc.idmap', 'g 65534 365534 2'],
> + ],
> + });
> +
> + $@ = undef;
> + eval {
> + PVE::LXC::validate_id_maps($id_maps);
> + };
> + is($@, "", "no error");
> +};
> +
> +subtest 'invalid: ambiguous mappings' => sub {
> + plan tests => 10;
> +
> + $@ = undef;
> + eval {
> + my ($id_maps, undef, undef) = PVE::LXC::parse_id_maps({
> + lxc => [
> + ['lxc.idmap', 'u 0 100000 1005'],
> + ['lxc.idmap', 'u 1005 1005 2'], # maps host uid 1005
> + ['lxc.idmap', 'u 1007 101007 992'],
> + ['lxc.idmap', 'u 11000 1000 10'], # maps host uid 1005 again
> + ],
> + });
> + PVE::LXC::validate_id_maps($id_maps);
> + };
> + like($@, qr/invalid map entry 'u 1005 1005 2'/, '$@ correct');
> + like($@, qr/host uid 1005 is also mapped by entry 'u 11000 1000 10'/, '$@ correct');
> +
> + $@ = undef;
> + eval {
> + my ($id_maps, undef, undef) = PVE::LXC::parse_id_maps({
> + lxc => [
> + ['lxc.idmap', 'u 0 100000 65536'], # maps container uid 1005
> + ['lxc.idmap', 'u 1005 1005 1'], # maps container uid 1005 again
> + ['lxc.idmap', 'u 1006 201006 64530'],
> + ],
> + });
> + PVE::LXC::validate_id_maps($id_maps);
> + };
> +
> + like($@, qr/invalid map entry 'u 1005 1005 1'/, '$@ correct');
> + like($@, qr/container uid 1005 is also mapped by entry 'u 0 100000 65536'/, '$@ correct');
> +
> + $@ = undef;
> + eval {
> + my ($id_maps, undef, undef) = PVE::LXC::parse_id_maps({
> + lxc => [
> + ['lxc.idmap', 'u 5 100000 6'], # 5..10
> + ['lxc.idmap', 'u 0 200000 11'], # 0..10
> + ['lxc.idmap', 'u 3 300000 2'], # 3..4
> + ],
> + });
> + PVE::LXC::validate_id_maps($id_maps);
> + };
> +
> + # this flags line 2 and 3. the input is [ 0..10, 3..4, 5..10 ], and the
> + # algorithm misses the conflict between 0..10 and 5..10.
> + like($@, qr/invalid map entry 'u 3 300000 2'/, '$@ correct');
> + like($@, qr/container uid 3 is also mapped by entry 'u 0 200000 11'/, '$@ correct');
> +
> + $@ = undef;
> + eval {
> + my ($id_maps, undef, undef) = PVE::LXC::parse_id_maps({
> + lxc => [
> + ['lxc.idmap', 'g 0 100000 1001'], # maps container gid 1000
> + ['lxc.idmap', 'g 1000 1000 10'], # maps container gid 1000 again
> + ],
> + });
> + PVE::LXC::validate_id_maps($id_maps);
> + };
> + like($@, qr/invalid map entry 'g 1000 1000 10'/, '$@ correct');
> + like($@, qr/container gid 1000 is also mapped by entry 'g 0 100000 1001'/, '$@ correct');
> +
> + $@ = undef;
> + eval {
> + my ($id_maps, undef, undef) = PVE::LXC::parse_id_maps({
> + lxc => [
> + ['lxc.idmap', 'g 0 100000 1000'], # maps host gid 100000
> + ['lxc.idmap', 'g 2000 102000 1000'],
> + ['lxc.idmap', 'g 3500 99500 5000'], # maps host gid 100000 again
> + ],
> + });
> + PVE::LXC::validate_id_maps($id_maps);
> + };
> + like($@, qr/invalid map entry 'g 0 100000 1000'/, '$@ correct');
> + like($@, qr/host gid 100000 is also mapped by entry 'g 3500 99500 5000'/, '$@ correct');
> +};
> +
> +subtest 'check performance' => sub {
> + plan tests => 1;
> +
> + # generate mapping with 1000 entries
> + my $entries = [];
> + foreach my $i (0..999) {
> + my $first_container_uid = $i * 10;
> + my $first_host_uid = 100000 + $first_container_uid;
> + push @$entries, ['lxc.idmap', "u $first_container_uid $first_host_uid 10"]
> + }
> +
> + my ($id_maps, undef, undef) = PVE::LXC::parse_id_maps({ lxc => $entries });
> +
> + my $start_time = [ gettimeofday() ];
> + $@ = undef;
> + eval {
> + PVE::LXC::validate_id_maps($id_maps);
> + };
> + my $elapsed = tv_interval($start_time);
> +
> + is($@, "", "no error");
> + diag("validation took $elapsed seconds");
> +};
> +
> +done_testing();
> diff --git a/src/test/run_idmap_tests.pl b/src/test/run_idmap_tests.pl
> new file mode 100755
> index 0000000..38bb494
> --- /dev/null
> +++ b/src/test/run_idmap_tests.pl
> @@ -0,0 +1,10 @@
> +#!/usr/bin/perl
> +
> +use strict;
> +use warnings;
> +
> +use TAP::Harness;
> +
> +my $harness = TAP::Harness->new( { "verbosity" => -2 });
> +my $res = $harness->runtests( "idmap-test.pm");
> +exit -1 if $res->{failed};
> --
> 2.30.2
More information about the pve-devel
mailing list