[PATCH manager] ceph: osd: remove the lockbox key when destroying an OSD

[PATCH manager] ceph: osd: remove the lockbox key when destroying an OSD

[Kefu Chai]

The destroy worker runs 'osd crush remove', 'auth del' and 'osd rm'. It
never asks the monitors for the cleanup that is keyed by the OSD's UUID.
So every destroyed OSD leaves behind its lockbox entity
'client.osd-lockbox.<uuid>', the dm-crypt key of an encrypted OSD in
'dm-crypt/osd/<uuid>/luks' and its daemon-private config keys.

The old lockbox entities are then reported as insecure keys when a
cluster is checked for the aes256k migration [0]. And the LUKS
passphrase of a disk that is long gone stays readable in the mon store.

Ask the monitors to destroy the OSD before removing it. They look the
UUID up in the OSD map, so the call has to come before 'osd rm'. The
monitors also refuse it while the OSD is up, but the endpoint already
stops earlier in that case.

Move the monitor commands into their own sub, so a test can check them.

[0]: https://forum.proxmox.com/threads/186359/

Signed-off-by: Kefu Chai <k.chai@proxmox.com>
---
 PVE/API2/Ceph/OSD.pm | 86 +++++++++++++++++++++++++++-----------------
 test/OSD_test.pl     | 42 +++++++++++++++++++++-
 2 files changed, 94 insertions(+), 34 deletions(-)

diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
index 5df529aa..24b8e9ff 100644
--- a/PVE/API2/Ceph/OSD.pm
+++ b/PVE/API2/Ceph/OSD.pm
@@ -958,6 +958,58 @@ sub osd_belongs_to_node {
     return grep($_ == $osdid, @$osds);
 }
 
+# Drop everything the monitors still hold for $osdid.
+#
+# Note: 'osd destroy' does the cleanup that is keyed by the OSD's UUID, namely the lockbox
+# entity and the dm-crypt key of an encrypted OSD, and the daemon-private config keys. It
+# reads that UUID from the OSD map, so it has to run before 'osd rm'.
+sub remove_osd_from_monitors {
+    my ($rados, $osdid) = @_;
+
+    my $osdsection = "osd.$osdid";
+
+    print "Remove $osdsection from the CRUSH map\n";
+    $rados->mon_command({ prefix => "osd crush remove", name => $osdsection, format => 'plain' });
+
+    print "Remove the $osdsection authentication key.\n";
+    $rados->mon_command({
+        prefix => "auth del",
+        entity => $osdsection,
+        format => 'plain',
+    });
+
+    print "Remove the $osdsection lockbox key and private config keys\n";
+    $rados->mon_command({
+        prefix => "osd destroy-actual",
+        id => $osdid,
+        yes_i_really_mean_it => JSON::true,
+        format => 'plain',
+    });
+
+    print "Remove OSD $osdsection\n";
+    $rados->mon_command({
+        prefix => "osd rm",
+        ids => [$osdsection],
+        format => 'plain',
+    });
+
+    print "Remove $osdsection mclock max capacity iops settings from config\n";
+    $rados->mon_command(
+        {
+            prefix => "config rm",
+            who => $osdsection,
+            name => 'osd_mclock_max_capacity_iops_ssd',
+        },
+    );
+    $rados->mon_command(
+        {
+            prefix => "config rm",
+            who => $osdsection,
+            name => 'osd_mclock_max_capacity_iops_hdd',
+        },
+    );
+}
+
 __PACKAGE__->register_method({
     name => 'destroyosd',
     path => '{osdid}',
@@ -1038,39 +1090,7 @@ __PACKAGE__->register_method({
             };
             warn $@ if $@;
 
-            print "Remove $osdsection from the CRUSH map\n";
-            $rados->mon_command(
-                { prefix => "osd crush remove", name => $osdsection, format => 'plain' });
-
-            print "Remove the $osdsection authentication key.\n";
-            $rados->mon_command({
-                prefix => "auth del",
-                entity => $osdsection,
-                format => 'plain',
-            });
-
-            print "Remove OSD $osdsection\n";
-            $rados->mon_command({
-                prefix => "osd rm",
-                ids => [$osdsection],
-                format => 'plain',
-            });
-
-            print "Remove $osdsection mclock max capacity iops settings from config\n";
-            $rados->mon_command(
-                {
-                    prefix => "config rm",
-                    who => $osdsection,
-                    name => 'osd_mclock_max_capacity_iops_ssd',
-                },
-            );
-            $rados->mon_command(
-                {
-                    prefix => "config rm",
-                    who => $osdsection,
-                    name => 'osd_mclock_max_capacity_iops_hdd',
-                },
-            );
+            remove_osd_from_monitors($rados, $osdid);
 
             # try to unmount from standard mount point
             my $mountpoint = "/var/lib/ceph/osd/ceph-$osdid";
diff --git a/test/OSD_test.pl b/test/OSD_test.pl
index f7bab3a8..83b28ea1 100755
--- a/test/OSD_test.pl
+++ b/test/OSD_test.pl
@@ -74,4 +74,44 @@ is(
     "Early-return false if there's no/empty node tree",
 );
 
-done_testing(@belong_to_B + @not_belong_to_B + 2);
+# Destroying an OSD has to leave nothing of it behind on the monitors.
+
+{
+
+    package FakeRados;
+
+    sub new {
+        my ($class) = @_;
+        return bless { commands => [] }, $class;
+    }
+
+    sub mon_command {
+        my ($self, $cmd) = @_;
+        push $self->{commands}->@*, $cmd;
+        return {};
+    }
+}
+
+my $rados = FakeRados->new();
+{
+    my $out = '';
+    open(my $stdout, '>', \$out) or die $!;
+    local *STDOUT = $stdout;
+    PVE::API2::Ceph::OSD::remove_osd_from_monitors($rados, 7);
+}
+
+my $commands = $rados->{commands};
+
+# The monitors look up the OSD's UUID in the OSD map to find its lockbox entity and its
+# dm-crypt key, so the destroy has to come before the 'osd rm'.
+is_deeply(
+    [map { $_->{prefix} } @$commands],
+    ['osd crush remove', 'auth del', 'osd destroy-actual', 'osd rm', 'config rm', 'config rm'],
+    'the monitors drop the CRUSH entry, the keys and the OSD itself, in that order',
+);
+
+my ($destroy) = grep { $_->{prefix} eq 'osd destroy-actual' } @$commands;
+is($destroy->{id}, 7, "'osd destroy-actual' names the OSD");
+ok($destroy->{yes_i_really_mean_it}, 'and confirms, as the monitors refuse it otherwise');
+
+done_testing();
-- 
2.47.3