[pbs-devel] [PATCH v2 proxmox-backup 1/2] docs: add security implications of prune and change detection mode

[Christian Ebner]

On 11/14/24 10:25, Gabriel Goller wrote:
> On 13.11.2024 16:55, Christian Ebner wrote:
>> diff --git a/docs/maintenance.rst b/docs/maintenance.rst
>> index 4bb135e4e..e8a26d69c 100644
>> --- a/docs/maintenance.rst
>> +++ b/docs/maintenance.rst
>> @@ -6,8 +6,34 @@ Maintenance Tasks
>> Pruning
>> -------
>>
>> -Prune lets you specify which backup snapshots you want to keep.
>> -The following retention options are available:
>> +Prune lets you specify which backup snapshots you want to keep, 
>> removing others.
>> +When pruning a snapshot, only the snapshot metadata (manifest, 
>> indices, blobs,
>> +log and notes) is removed. The chunks containing the actual backup 
>> data and
>> +previously referenced by the pruned snapshot, have to be removed by a 
>> garbage
>> +collection run.
>> +
>> +.. Caution:: Take into consideration that sensitive information 
>> stored in a
>> +   given data chunk will outlive pruned snapshots and remain present 
>> in the
>> +   datastore as long as referenced by at least one backup snapshot. 
>> Further,
>> +   *even* if no snapshot references a given chunk, it will remain 
>> present until
>> +   removed by the garbage collection.
>> +
>> +   Further, file-level backups created using the change detection mode
> 
> Second sentence that begins with 'Further' – maybe substitute this one
> with 'Moreover' or 'Additionally' so it reads better.

True, `Moreover` sounds better to me...

> 
>> +   `metadata` can reference backup chunks containing files which have 
>> vanished
> 
> use double backticks here to highlight correctly, so: ``metadata``.

Acked, thx!

> 
>> +   since the previous backup, but might still be accessible when 
>> reading the
>> +   chunks raw data is possible (client or server side).
> 
> This sentence is a bit messy and long, maybe we could rewrite it as:
> 
>      Moreover, file-level backups created using the change detection mode
>      ``metadata`` can reference backup chunks containing files which have
>      vanished since the previous backup. These might still be accessible

Yes, I do agree that splitting this into two sentences makes it easier 
to read. In that case I would even suggest to explicitly mention that 
this refers to the files, non just the chunks, e.g. by:
...
vanished since the previous backup. These files might still be accessible.
...

>      by reading the raw data (client or server side).
> 
>> +   To remove chunks containing sensitive data, prune any snapshot 
>> made while the
>> +   data was part of the backup input and run a garbage collection. 
>> Further, if
>> +   using file-based backups with change detection mode `metadata`, 
>> additionally
> 
> s/`metadata`/``metadata``/

Acked, thx!

> 
>> +   prune all snapshots since the sensitive data was no longer part of 
>> the backup
>> +   input and run a garbage collection.
>> +
>> +   The no longer referenced chunks will then be marked for deletion 
>> on the next
>> +   garbage collection run and removed by a subsequent run after the 
>> grace
>> +   period.
>> +
>> +The following retention options are available for pruning:
>>
>> ``keep-last <N>``
>>   Keep the last ``<N>`` backup snapshots.
> 
> Everything else is fine!

OK, will send a new version, thx!