[PATCH proxmox{-widget-toolkit,,-backup} v2 00/43] append-only sync jobs and snapshot retention timespan

[PATCH proxmox{-widget-toolkit,,-backup} v2 00/43] append-only sync jobs and snapshot retention timespan

[Christian Ebner]

Currently sync jobs cannot be configured to be fully append-only
since namespace creation requires datastore modify privileges to do
so. Further, permissions would also allow to restore or modify owned
content.

This patch series therefore extends the current permissions
and roles to allow for append only sync jobs, by only allowing
the minimally required permissions and roles.

In particular, for push the sync jobs local user on the source
requires RemoteSyncAppendOperator as well as DatastoreReader on the
source datastore, with DatastoreAppend and DatastoreAudit (latter for
listing privs of pre-existing contents without restore) permissions
for the user on the remote instance used for connection.

For pull, the user on the target must be able to append to the
datastore via DatastoreAppend and able to read from the remote source
by the respective RemoteSyncOperator permissions on the remote and
by either DatastoreBackup or DatastoreReader permissions to access the
contents.

Further, sync jobs are extended to allow setting a retention timespan
for which synced snapshots cannot be pruned, neither by the sync job,
nor by prune jobs. Only root@pam is allowed to change the retention
period. After the retention period, snapshots behave like regular
snapshots again and can be pruned.

To protect from sync jobs or clients setting unintended retention
timespans, a maximum retention timespan must be configured on the
datastore before any retention can be set. Optionally, a default
retention timespan can be configured as fallback for new snapshots.

S3 object locking is still work in progress, so not included as part
of this patch series. This will be either a followup or included in
future versions of this patch series, if needed.

Changes since RFC version 1 (thanks to Robert and Thomas for feedback):
- New patches for proxmox-widget-toolkit to fix proxmoxDateTimeField
  behaviour in order to use it for retention time picking.
- Retention can only be set or extended, never shortened or cleared.
  The endpoint to modify it is now limited to the superuser.
- Setting a retention requires a configured max-retention-timespan on
  the datastore, enforced for backups, sync jobs, the API endpoint and
  tape restores.
- Add an optional default-retention-timespan datastore option, used
  for new snapshots if neither client nor sync job request a
  retention. It must not exceed the max. Both values are exposed in
  the datastore status, so clients can check the limits upfront.
- Add client options to set the retention on backup and a CLI command
  to extend it for existing snapshots.
- Snapshots with active retention are now treated like protected
  ones: prune keeps them without them taking up keep slots, while
  group removal and remove-vanished skip them instead of failing.
- Pull no longer replaces the whole manifest when setting the
  retention. Only the unprotected part is changed, keeping the file
  list and signature intact, and updates of the unprotected part on
  the source are still synced. Repeated pulls no longer abort the
  group for retained snapshots.
- Resync of corrupt snapshots honors the retention, retained ones are
  skipped with a warning. With append-only privileges, corrupt
  snapshots are skipped with a warning instead of the whole namespace.
- Tape restores no longer re-lock snapshots with already expired
  retention.
- Retention now also covers the second of the retention timestamp
  itself, consistently for all checks.
- Drop the bulk S3 object removal on recursive namespace deletion, as
  it did not honor group locks and bypassed per-snapshot checks.
- Do not leak the group owner in backup owner check errors.
- Do not fail the client status output for users lacking privileges
  to see the storage usage.

Design decisions:
- The retention is only set on snapshot creation by backup or sync,
  it is not renewed on resync. Retention timestamps of the sync source
  are never propagated, the target sets its own based on the sync job
  or datastore default.
- Tape restores shorten retention timestamps exceeding the datastore
  limit, or strip them if no limit is configured. s3-refresh only
  warns about retention exceeding the limit, as the bucket is intended
  to become the dominant retention once retention is enforced via S3
  object lock.
- Snapshots with a manifest that cannot be parsed can no longer be
  removed without manual intervention, as their retention cannot be
  determined.
- Retention support must be explicitley enabled on the datastore by
  setting the max-retention-timespan. This is enforced also for the
  default-retention-timespan and helps to protect against misuse.


proxmox-widget-toolkit:

Christian Ebner (5):
  form: date time field: init with now instead of epoch on empty fields
  form: date time field: set min/max value limits passed via config
  form: date time field: update to new limits when switching day/time
  form: date time field: submit combined datetime instead of item fields
  form: date time field: track value like other form fields

 src/form/DateTimeField.js | 88 ++++++++++++++++++++++++++++++++-------
 1 file changed, 72 insertions(+), 16 deletions(-)


proxmox:

Christian Ebner (6):
  pbs-api-types: add append only privilege and role
  pbs-api-types: add remote datastore append privilege and role
  pbs-api-types: extend snapshot list items by retention timestamp
  pbs-api-types: extend sync job config by retention-timespan parameter
  pbs-api-types: add max/default retention timespan property to
    datastore
  pbs-api-types: store: expose max/default retention timespans in status

 pbs-api-types/src/acl.rs       | 24 +++++++++++++
 pbs-api-types/src/datastore.rs | 31 ++++++++++++++--
 pbs-api-types/src/jobs.rs      | 64 +++++++++++++++++++++++++++++++++-
 3 files changed, 116 insertions(+), 3 deletions(-)


proxmox-backup:

Christian Ebner (32):
  config/tools: add new fields for sync and snapshot/datastore list
    items
  api: admin: improve code style for status endpoint
  client: avoid error in status if user lacks permissions
  server: allow iterating contents for Datastore.Append permissions
  api: backup: fix possible information leak in multi-tenant datastores
  datastore: s3: fix namespace level bulk remove not honoring group
    locks
  api: backup: allow backup for user/token with append permission
  api: allow namespace creation on append permissions
  api: sync: allow pull to target for user/token with append permission
  sync: pull: allow pulling for user/token with append permission
  sync: push: allow push and ns creation on Remote.DatastoreAppend
  datastore: conditionally treat missing manifest as error or benign
  api: backup: provide retain-until timestamp for extended prune
    protection
  tools: include retain-until timestamp in snapshot list items
  client: backup writer: allow to send retain-until timestamp on backup
  sync: push: allow to set retention timestamp for synced snapshots
  sync: pull: set retention timestamp from sync job for new snapshots
  datastore/config: parse and enforce maximum retention timespan
  api: config: allow to set or delete retention timespan for sync jobs
  api: admin: allow to modify snapshot retention for root user
  tape/s3-refresh: check for retentions outside datastore limits
  ui: add retention timespan form and use it for sync job edit window
  ui: allow datastore wide max retention timespan configuration
  ui: show snapshot retention in datastore contents and allow extension
  group: destroy: s3: skip snapshots with active retentions
  prune: skip snapshots with active retentions
  api/ui: expose default retention setting as datastore config option
  api: datastore: expose max/default retention timespans in status
    listing
  backup/sync: fallback to datastore default-retention-timespan
  client: add flags to set snapshot retention via timestamp/timespan
  docs: describe the privileges and roles for append only backup/sync
  docs: document snapshot retention for new backup snapshots

 docs/backup-client.rst                     |  28 ++++
 docs/maintenance.rst                       |  22 +++
 docs/managing-remotes.rst                  |  26 +++-
 docs/storage.rst                           |   7 +
 docs/user-management.rst                   |  16 +++
 examples/upload-speed.rs                   |   1 +
 pbs-client/src/backup_writer.rs            |   5 +
 pbs-datastore/src/backup_info.rs           | 110 +++++++++++++--
 pbs-datastore/src/datastore.rs             | 113 +++++++++++----
 pbs-datastore/src/manifest.rs              |  50 +++++++
 pbs-datastore/src/prune.rs                 |  54 +++++++-
 proxmox-backup-client/src/benchmark.rs     |   1 +
 proxmox-backup-client/src/main.rs          |  94 +++++++++++--
 proxmox-backup-client/src/snapshot.rs      |  98 ++++++++++++-
 src/api2/admin/datastore.rs                |  93 ++++++++++---
 src/api2/admin/namespace.rs                |   8 +-
 src/api2/backup/environment.rs             |  14 ++
 src/api2/backup/mod.rs                     |  37 ++++-
 src/api2/config/datastore.rs               |  44 ++++++
 src/api2/config/sync.rs                    |  51 ++++++-
 src/api2/pull.rs                           |  19 ++-
 src/api2/push.rs                           |  17 ++-
 src/api2/tape/restore.rs                   |  22 ++-
 src/backup/hierarchy.rs                    |  23 +++-
 src/backup/verify.rs                       |   1 +
 src/server/prune_job.rs                    |   5 +-
 src/server/pull.rs                         | 152 +++++++++++++++++++--
 src/server/push.rs                         |  46 ++++++-
 src/server/sync.rs                         |   1 +
 src/tools/mod.rs                           |  23 ++--
 tests/prune.rs                             |  49 ++++++-
 www/Makefile                               |   4 +
 www/datastore/Content.js                   | 103 +++++++++++++-
 www/datastore/OptionView.js                |  16 +++
 www/datastore/Prune.js                     |   4 +
 www/form/RetentionTimespan.js              |  22 +++
 www/window/DefaultRetentionTimespanEdit.js |  25 ++++
 www/window/MaxRetentionTimespanEdit.js     |  25 ++++
 www/window/RetentionEdit.js                |  56 ++++++++
 www/window/SyncJobEdit.js                  |   9 ++
 40 files changed, 1344 insertions(+), 150 deletions(-)
 create mode 100644 www/form/RetentionTimespan.js
 create mode 100644 www/window/DefaultRetentionTimespanEdit.js
 create mode 100644 www/window/MaxRetentionTimespanEdit.js
 create mode 100644 www/window/RetentionEdit.js


Summary over all repositories:
  44 files changed, 1532 insertions(+), 169 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH proxmox-widget-toolkit v2 01/43] form: date time field: init with now instead of epoch on empty fields

[Christian Ebner]

When no value was set yet, the date and time parts were merged into a
date with Unix epoch as base. When selecting a time, this resulted in
a date in 1970 and when picking a date, the time was based on the
current time for Unix epoch in local timezone.

For improved usability and convenience, default to midnight in local
time if a date was picked and today when picking a time.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/form/DateTimeField.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/form/DateTimeField.js b/src/form/DateTimeField.js
index e9f60e3..c871bbc 100644
--- a/src/form/DateTimeField.js
+++ b/src/form/DateTimeField.js
@@ -23,7 +23,7 @@ Ext.define('Proxmox.DateTimeField', {
                         this.set('datetime', null);
                         return;
                     }
-                    let datetime = new Date(this.get('datetime'));
+                    let datetime = new Date(this.get('datetime') ?? date);
                     datetime.setFullYear(date.getFullYear(), date.getMonth(), date.getDate());
                     this.set('datetime', datetime);
                 },
@@ -38,7 +38,7 @@ Ext.define('Proxmox.DateTimeField', {
                         this.set('datetime', null);
                         return;
                     }
-                    let datetime = new Date(this.get('datetime'));
+                    let datetime = new Date(this.get('datetime') ?? Date.now());
                     datetime.setHours(time.getHours());
                     datetime.setMinutes(time.getMinutes());
                     datetime.setSeconds(time.getSeconds());
-- 
2.47.3

[PATCH proxmox-widget-toolkit v2 02/43] form: date time field: set min/max value limits passed via config

[Christian Ebner]

Currently minValue and maxValue are ignored when set via the
components config. Allow to set them via config as well by
applying them during component init.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/form/DateTimeField.js | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/form/DateTimeField.js b/src/form/DateTimeField.js
index c871bbc..474f4d7 100644
--- a/src/form/DateTimeField.js
+++ b/src/form/DateTimeField.js
@@ -136,6 +136,14 @@ Ext.define('Proxmox.DateTimeField', {
 
         let vm = me.getViewModel();
         vm.set('datetime', me.config.value);
+
+        if (me.minValue) {
+            me.setMinValue(me.minValue);
+        }
+        if (me.maxValue) {
+            me.setMaxValue(me.maxValue);
+        }
+
         // Propagate state change to binding
         vm.bind('{datetime}', function (value) {
             me.publishState('value', value);
-- 
2.47.3

[PATCH proxmox-widget-toolkit v2 03/43] form: date time field: update to new limits when switching day/time

[Christian Ebner]

The date picker only offers days within the limits, but selecting the
day of the minimum or maximum kept the previously set time of day.
This time might however be out of the limit, so must be updated
accordingly. Fix also the minTime/maxTime not returning a date as
originally intended.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/form/DateTimeField.js | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/form/DateTimeField.js b/src/form/DateTimeField.js
index 474f4d7..096075a 100644
--- a/src/form/DateTimeField.js
+++ b/src/form/DateTimeField.js
@@ -25,6 +25,14 @@ Ext.define('Proxmox.DateTimeField', {
                     }
                     let datetime = new Date(this.get('datetime') ?? date);
                     datetime.setFullYear(date.getFullYear(), date.getMonth(), date.getDate());
+
+                    let min = this.get('minDatetime');
+                    let max = this.get('maxDatetime');
+                    if (min && datetime < min) {
+                        datetime = new Date(min);
+                    } else if (max && datetime > max) {
+                        datetime = new Date(max);
+                    }
                     this.set('datetime', datetime);
                 },
             },
@@ -66,7 +74,9 @@ Ext.define('Proxmox.DateTimeField', {
                     let current = get('datetime');
                     let min = get('minDatetime');
                     if (min && current && !this.isSameDay(current, min)) {
-                        return new Date(min).setHours('00', '00', '00', '000');
+                        let date = new Date(min);
+                        date.setHours(0, 0, 0, 0);
+                        return date;
                     }
                     return min;
                 },
@@ -77,7 +87,9 @@ Ext.define('Proxmox.DateTimeField', {
                     let current = get('datetime');
                     let max = get('maxDatetime');
                     if (max && current && !this.isSameDay(current, max)) {
-                        return new Date(max).setHours('23', '59', '59', '999');
+                        let date = new Date(max);
+                        date.setHours(23, 59, 59, 999);
+                        return date;
                     }
                     return max;
                 },
-- 
2.47.3

[PATCH proxmox-widget-toolkit v2 04/43] form: date time field: submit combined datetime instead of item fields

[Christian Ebner]

The field is a plain field container, so forms did not consider it a
form field and never submitted its combined datetime value but rather
the individual date and time item fields. This was fine for now since
the component is currently not used to submit values directly. It is
however not intended behaviour and cannot be used as datetime picker
in forms.

Fix this by using the form field mixin and exclude item fields from
being submitted. Also, set the original value after initialization,
so that the field is not considered dirty right away and make sure
the value is ignored if submitValue is false or the component is
disabled.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/form/DateTimeField.js | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/src/form/DateTimeField.js b/src/form/DateTimeField.js
index 096075a..1f1fa7f 100644
--- a/src/form/DateTimeField.js
+++ b/src/form/DateTimeField.js
@@ -4,7 +4,10 @@ Ext.define('Proxmox.DateTimeField', {
     alias: ['widget.promxoxDateTimeField'],
     xtype: 'proxmoxDateTimeField',
 
+    mixins: ['Ext.form.field.Field'],
+
     layout: 'hbox',
+    isFormField: true,
 
     viewModel: {
         data: {
@@ -126,6 +129,15 @@ Ext.define('Proxmox.DateTimeField', {
         return value ? Ext.Date.format(value, me.submitFormat) : null;
     },
 
+    getSubmitData: function () {
+        let me = this;
+        let value = me.getSubmitValue();
+        if (me.disabled || !me.submitValue || value === null) {
+            return null;
+        }
+        return { [me.getName()]: value };
+    },
+
     setMinValue: function (value) {
         this.getViewModel().set('minDatetime', value);
     },
@@ -148,6 +160,7 @@ Ext.define('Proxmox.DateTimeField', {
 
         let vm = me.getViewModel();
         vm.set('datetime', me.config.value);
+        me.resetOriginalValue();
 
         if (me.minValue) {
             me.setMinValue(me.minValue);
@@ -166,6 +179,7 @@ Ext.define('Proxmox.DateTimeField', {
     items: [
         {
             xtype: 'datefield',
+            submitValue: false,
             editable: false,
             flex: 1,
             format: 'Y-m-d',
@@ -177,6 +191,7 @@ Ext.define('Proxmox.DateTimeField', {
         },
         {
             xtype: 'timefield',
+            submitValue: false,
             format: 'H:i',
             width: 80,
             value: '00:00',
-- 
2.47.3

[PATCH proxmox-widget-toolkit v2 05/43] form: date time field: track value like other form fields

[Christian Ebner]

The value was only kept in the view model and every change of it
fired a change event with just the new value, without ever checking
the dirty state. Forms thus never got a dirtychange event and did not
notice that the field was modified, for example for enabling the
reset button of edit windows.

Keep the value via the form field mixin, so changes go through its
change detection, which fires change and dirtychange events as for
any other form field. Values are compared by their timestamp, and
values that are not dates are parsed, for example timestamps in
milliseconds, while invalid ones are treated as no value.

The change event now passes the field, new and old value, like other
form fields do, instead of only the new value.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/form/DateTimeField.js | 47 ++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/src/form/DateTimeField.js b/src/form/DateTimeField.js
index 1f1fa7f..edb984a 100644
--- a/src/form/DateTimeField.js
+++ b/src/form/DateTimeField.js
@@ -110,17 +110,34 @@ Ext.define('Proxmox.DateTimeField', {
     },
 
     config: {
-        value: null,
         submitFormat: 'U',
         disabled: false,
     },
 
+    parseDatetime: function (value) {
+        if (value === undefined || value === null || value === '') {
+            return null;
+        }
+        let date = Ext.isDate(value) ? value : new Date(value);
+        return Number.isNaN(date.getTime()) ? null : date;
+    },
+
     setValue: function (value) {
-        this.getViewModel().set('datetime', value);
+        let me = this;
+        me.value = me.parseDatetime(value);
+        me.getViewModel().set('datetime', me.value);
+        me.checkChange();
+        return me;
     },
 
     getValue: function () {
-        return this.getViewModel().get('datetime');
+        return this.value ?? null;
+    },
+
+    isEqual: function (value1, value2) {
+        let time1 = value1 ? value1.getTime() : null;
+        let time2 = value2 ? value2.getTime() : null;
+        return time1 === time2;
     },
 
     getSubmitValue: function () {
@@ -138,8 +155,17 @@ Ext.define('Proxmox.DateTimeField', {
         return { [me.getName()]: value };
     },
 
+    onChange: function (newValue) {
+        let me = this;
+        if (me.validateOnChange) {
+            me.validate();
+        }
+        me.checkDirty();
+        me.publishState('value', newValue);
+    },
+
     setMinValue: function (value) {
-        this.getViewModel().set('minDatetime', value);
+        this.getViewModel().set('minDatetime', this.parseDatetime(value));
     },
 
     getMinValue: function () {
@@ -147,7 +173,7 @@ Ext.define('Proxmox.DateTimeField', {
     },
 
     setMaxValue: function (value) {
-        this.getViewModel().set('maxDatetime', value);
+        this.getViewModel().set('maxDatetime', this.parseDatetime(value));
     },
 
     getMaxValue: function () {
@@ -158,9 +184,7 @@ Ext.define('Proxmox.DateTimeField', {
         let me = this;
         me.callParent();
 
-        let vm = me.getViewModel();
-        vm.set('datetime', me.config.value);
-        me.resetOriginalValue();
+        me.initField();
 
         if (me.minValue) {
             me.setMinValue(me.minValue);
@@ -169,11 +193,8 @@ Ext.define('Proxmox.DateTimeField', {
             me.setMaxValue(me.maxValue);
         }
 
-        // Propagate state change to binding
-        vm.bind('{datetime}', function (value) {
-            me.publishState('value', value);
-            me.fireEvent('change', value);
-        });
+        // propagate changes made via the inner date and time fields
+        me.getViewModel().bind('{datetime}', (value) => me.setValue(value));
     },
 
     items: [
-- 
2.47.3

[PATCH proxmox v2 06/43] pbs-api-types: add append only privilege and role

[Christian Ebner]

While allowing to perform backups like Datastore.Backup, the new
Datastore.Append allows further to create new namespaces, but does
not allow to list, restore or otherwise modify datastore contents,
not even if owned.

The role is intended to allow user configuration for immutable backup
and sync jobs and to be set on the user/token with target datastore
ACL path.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-api-types/src/acl.rs | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/pbs-api-types/src/acl.rs b/pbs-api-types/src/acl.rs
index 691af305..9055dfab 100644
--- a/pbs-api-types/src/acl.rs
+++ b/pbs-api-types/src/acl.rs
@@ -42,6 +42,9 @@ constnamedbitmap! {
         /// Allows verifying a datastore
         PRIV_DATASTORE_VERIFY("Datastore.Verify");
 
+        /// Datastore.Append allows to create new snapshots or namespaces,
+        /// but also requires backup ownership
+        PRIV_DATASTORE_APPEND("Datastore.Append");
         /// Datastore.Backup allows Datastore.Read|Verify and creating new snapshots,
         /// but also requires backup ownership
         PRIV_DATASTORE_BACKUP("Datastore.Backup");
@@ -126,6 +129,13 @@ pub const ROLE_DATASTORE_READER: u64 = 0
     | PRIV_DATASTORE_VERIFY
     | PRIV_DATASTORE_READ;
 
+#[rustfmt::skip]
+#[allow(clippy::identity_op)]
+/// Datastore.Append can only add backups and namespaces, but not
+/// list, restore or prune backups and cannot delete namespaces.
+pub const ROLE_DATASTORE_APPEND: u64 = 0
+    | PRIV_DATASTORE_APPEND;
+
 #[rustfmt::skip]
 #[allow(clippy::identity_op)]
 /// Datastore.Backup can do backup and restore, but no prune.
@@ -241,6 +251,8 @@ pub enum Role {
     Audit = ROLE_AUDIT,
     /// Disable Access
     NoAccess = ROLE_NO_ACCESS,
+    /// Datastore Append (append new backups and create namespaces)
+    DatastoreAppend = ROLE_DATASTORE_APPEND,
     /// Datastore Administrator
     DatastoreAdmin = ROLE_DATASTORE_ADMIN,
     /// Datastore Reader (inspect datastore content and do restores)
-- 
2.47.3

[PATCH proxmox v2 07/43] pbs-api-types: add remote datastore append privilege and role

[Christian Ebner]

While allowing to push/backup to remotes like Remote.DatastoreBackup,
Remote.DatastoreAppend also allows creation of namespaces, but never
deletion/modification as Remote.DatastoreModify would imply, not even
for owned contents.

The role is intended to allow local (source) user configuration for
immutable push sync jobs and is to be set on the user/token on the
remote's datastore ACL path.

This is intended to be used with a remote user on the push target
having Datastore.Audit and Datastore.Append on the target datastore
or sub-namespace.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-api-types/src/acl.rs | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/pbs-api-types/src/acl.rs b/pbs-api-types/src/acl.rs
index 9055dfab..f467db8d 100644
--- a/pbs-api-types/src/acl.rs
+++ b/pbs-api-types/src/acl.rs
@@ -61,6 +61,8 @@ constnamedbitmap! {
         PRIV_REMOTE_MODIFY("Remote.Modify");
         /// Remote.Read allows reading data from a configured `Remote`
         PRIV_REMOTE_READ("Remote.Read");
+        /// Remote.DatastoreAppend allows creating new snapshots and namespaces on remote datastores
+        PRIV_REMOTE_DATASTORE_APPEND("Remote.DatastoreAppend");
         /// Remote.DatastoreBackup allows creating new snapshots on remote datastores
         PRIV_REMOTE_DATASTORE_BACKUP("Remote.DatastoreBackup");
         /// Remote.DatastoreModify allows to modify remote datastores
@@ -183,6 +185,14 @@ pub const ROLE_REMOTE_SYNC_PUSH_OPERATOR: u64 = 0
     | PRIV_REMOTE_AUDIT
     | PRIV_REMOTE_DATASTORE_BACKUP;
 
+#[rustfmt::skip]
+#[allow(clippy::identity_op)]
+/// Remote.SyncAppendOperator can read remote datastores, as well as push snapshots and create
+/// namespaces on the remote.
+pub const ROLE_REMOTE_SYNC_APPEND_OPERATOR: u64 = 0
+    | PRIV_REMOTE_AUDIT
+    | PRIV_REMOTE_DATASTORE_APPEND;
+
 #[rustfmt::skip]
 #[allow(clippy::identity_op)]
 /// Remote.DatastorePowerUser can read and push snapshots to the remote, and prune owned snapshots
@@ -271,6 +281,8 @@ pub enum Role {
     RemoteSyncOperator = ROLE_REMOTE_SYNC_OPERATOR,
     /// Synchronisation Operator (push direction)
     RemoteSyncPushOperator = ROLE_REMOTE_SYNC_PUSH_OPERATOR,
+    /// Synchronisation Operator (append push direction)
+    RemoteSyncAppendOperator = ROLE_REMOTE_SYNC_APPEND_OPERATOR,
     /// Remote Datastore Prune
     RemoteDatastorePowerUser = ROLE_REMOTE_DATASTORE_POWERUSER,
     /// Remote Datastore Admin
-- 
2.47.3

[PATCH proxmox v2 08/43] pbs-api-types: extend snapshot list items by retention timestamp

[Christian Ebner]

Return the retention timestamp as part of the snapshot list items,
for it to be shown in the UI/CLI outputs just like the protected
flag.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index c545d05b..91f2ba35 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -1552,6 +1552,9 @@ pub struct SnapshotListItem {
     /// Protection from prunes
     #[serde(default)]
     pub protected: bool,
+    /// Retention timestamp (unix epoch)
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub retain_until: Option<i64>,
 }
 
 #[api(
-- 
2.47.3

[PATCH proxmox v2 09/43] pbs-api-types: extend sync job config by retention-timespan parameter

[Christian Ebner]

Allows to configure a sync job which sets a retention timestamp on
synced snapshots based on the start time of the sync.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-api-types/src/jobs.rs | 64 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
index 9a5d2b77..5327dac7 100644
--- a/pbs-api-types/src/jobs.rs
+++ b/pbs-api-types/src/jobs.rs
@@ -1,7 +1,8 @@
-use std::str::FromStr;
+use std::{fmt, str::FromStr};
 
 use anyhow::bail;
 use const_format::concatcp;
+use proxmox_time::TimeSpan;
 use regex::Regex;
 use serde::{Deserialize, Serialize};
 
@@ -94,6 +95,61 @@ pub const SYNC_WORKER_THREADS_SCHEMA: Schema = threads_schema(
     1,
 );
 
+pub const RETENTION_TIMESPAN_FORMAT: ApiStringFormat = ApiStringFormat::VerifyFn(|s| {
+    RetentionTimespan::from_str(s)?;
+    Ok(())
+});
+
+pub const RETENTION_TIMESPAN_SCHEMA: Schema = StringSchema::new("Retention timespan")
+    .format(&RETENTION_TIMESPAN_FORMAT)
+    .schema();
+
+#[derive(Clone, Debug, PartialEq, UpdaterType)]
+pub struct RetentionTimespan {
+    timespan: TimeSpan,
+}
+
+impl std::str::FromStr for RetentionTimespan {
+    type Err = anyhow::Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let timespan: TimeSpan = s.parse()?;
+        if timespan.subsec_nanos() > 0 {
+            bail!("Sub-second precision not supported by retention timespan");
+        }
+        Ok(Self { timespan })
+    }
+}
+
+impl fmt::Display for RetentionTimespan {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "{}", self.timespan)
+    }
+}
+
+impl ApiType for RetentionTimespan {
+    const API_SCHEMA: Schema = RETENTION_TIMESPAN_SCHEMA;
+}
+
+impl RetentionTimespan {
+    /// Provide the retention timespan as plain timespan
+    pub fn as_timespan(&self) -> &TimeSpan {
+        &self.timespan
+    }
+
+    /// Calculate the unix epoch for given retention timespan relative to now from system time.
+    pub fn to_timestamp_from_systemtime(&self) -> Result<i64, anyhow::Error> {
+        // no sub-second precision allowed by parser, safe to skip
+        match proxmox_time::epoch_i64().checked_add_unsigned(self.timespan.as_secs()) {
+            Some(timestamp) => Ok(timestamp),
+            None => bail!("retention timestamp calculation failed with overflow"),
+        }
+    }
+}
+
+serde_plain::derive_serialize_from_display!(RetentionTimespan);
+serde_plain::derive_deserialize_from_fromstr!(RetentionTimespan, "retention timespan");
+
 #[api(
     properties: {
         "next-run": {
@@ -690,6 +746,10 @@ pub const UNMOUNT_ON_SYNC_DONE_SCHEMA: Schema =
             },
             optional: true,
         },
+        "retention-timespan": {
+            type: RetentionTimespan,
+            optional: true,
+        },
     }
 )]
 #[derive(Serialize, Deserialize, Clone, Updater, PartialEq)]
@@ -741,6 +801,8 @@ pub struct SyncJobConfig {
     pub active_encryption_key: Option<String>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub associated_key: Option<Vec<String>>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub retention_timespan: Option<RetentionTimespan>,
 }
 
 impl SyncJobConfig {
-- 
2.47.3

[PATCH proxmox v2 10/43] pbs-api-types: add max/default retention timespan property to datastore

[Christian Ebner]

Intended to allow administrators to configure a maximum retention
timespan which can be set on snapshots for given datastore, as
otherwise users might be able to prevent snapshots from ever being
cleaned up. Default is no limit, which also blocks setting any
retention timestamps.

The default retention timespan on the other hand allows to set a
fallback retention for all snapshots not requesting a specific
retention. It must however be contained within the max retention
limits.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 91f2ba35..7307d971 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -25,8 +25,8 @@ use crate::{
     Authid, BACKUP_ID_RE, BACKUP_NS_RE, BACKUP_TIME_RE, BACKUP_TYPE_RE, CryptMode,
     DATASTORE_NOTIFY_STRING_SCHEMA, Fingerprint, GC_SCHEDULE_SCHEMA,
     GROUP_OR_SNAPSHOT_PATH_REGEX_STR, GroupFilter, MaintenanceMode, MaintenanceType,
-    PROXMOX_SAFE_ID_FORMAT, PROXMOX_SAFE_ID_REGEX_STR, PRUNE_SCHEDULE_SCHEMA, SHA256_HEX_REGEX,
-    SINGLE_LINE_COMMENT_SCHEMA, SNAPSHOT_PATH_REGEX_STR, UPID, Userid,
+    PROXMOX_SAFE_ID_FORMAT, PROXMOX_SAFE_ID_REGEX_STR, PRUNE_SCHEDULE_SCHEMA, RetentionTimespan,
+    SHA256_HEX_REGEX, SINGLE_LINE_COMMENT_SCHEMA, SNAPSHOT_PATH_REGEX_STR, UPID, Userid,
     VERIFY_JOB_READ_THREADS_SCHEMA, VERIFY_JOB_VERIFY_THREADS_SCHEMA,
 };
 
@@ -530,6 +530,14 @@ pub const MAX_SNAPSHOT_TIME_SKEW_SCHEMA: Schema =
             optional: true,
             schema: MAX_SNAPSHOT_TIME_SKEW_SCHEMA,
         },
+        "max-retention-timespan": {
+            type: RetentionTimespan,
+            optional: true,
+        },
+        "default-retention-timespan": {
+            type: RetentionTimespan,
+            optional: true,
+        },
     }
 )]
 #[derive(Serialize, Deserialize, Updater, Clone, PartialEq)]
@@ -602,6 +610,14 @@ pub struct DataStoreConfig {
     /// Maximum allowed time skew between new snapshot's timestamp and server system time.
     #[serde(skip_serializing_if = "Option::is_none")]
     pub max_snapshot_time_skew: Option<String>,
+
+    /// Maximum retention timespan which can be set on this datastore
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub max_retention_timespan: Option<RetentionTimespan>,
+
+    /// Default retention timespan used by this datastore
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub default_retention_timespan: Option<RetentionTimespan>,
 }
 
 #[api]
@@ -645,6 +661,8 @@ impl DataStoreConfig {
             notification_thresholds: None,
             counter_reset_schedule: None,
             max_snapshot_time_skew: None,
+            max_retention_timespan: None,
+            default_retention_timespan: None,
         }
     }
 
-- 
2.47.3

[PATCH proxmox v2 11/43] pbs-api-types: store: expose max/default retention timespans in status

[Christian Ebner]

Include max-retention-timespan and default-retention-timespan in
DataStoreStatus so clients can query datastore retention limits and
defaults.

This will allow clients to gather this information to generate
acceptable retention timestamps without trial and error.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 7307d971..89403c93 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -1809,6 +1809,12 @@ pub struct DataStoreStatus {
     /// S3 backend statistics (on datastores with s3 backend only).
     #[serde(skip_serializing_if = "Option::is_none")]
     pub s3_statistics: Option<S3Statistics>,
+    /// Maximum allowed retention timespan for this datastore.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub max_retention_timespan: Option<RetentionTimespan>,
+    /// Default retention timespan for this datastore.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub default_retention_timespan: Option<RetentionTimespan>,
 }
 
 #[api()]
-- 
2.47.3

[PATCH proxmox-backup v2 12/43] config/tools: add new fields for sync and snapshot/datastore list items

[Christian Ebner]

The sync job config gained an optional retention-timespan parameter
and the snapshot list items now return an optional retain-until
timestamp in the API types. For the datastore, optionally the default
and maximum retention time are now exposed.

Fix builds by adding the fields, leaving them to their empty default
until used when required in subsequent changes.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/admin/datastore.rs | 4 ++++
 src/api2/config/sync.rs     | 2 ++
 src/tools/mod.rs            | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 3ba25934f..51dae4698 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -727,6 +727,8 @@ pub async fn status(
             counts,
             backend_type,
             s3_statistics,
+            default_retention_timespan: None,
+            max_retention_timespan: None,
         }
     } else {
         DataStoreStatus {
@@ -737,6 +739,8 @@ pub async fn status(
             counts,
             backend_type,
             s3_statistics,
+            default_retention_timespan: None,
+            max_retention_timespan: None,
         }
     })
 }
diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index 2afd68898..980689781 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -876,6 +876,7 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
         worker_threads: None,
         active_encryption_key: None,
         associated_key: None,
+        retention_timespan: None,
     };
 
     // should work without ACLs
@@ -1071,6 +1072,7 @@ acl:1:/remote/remote1/remotealice1:alice@pbs:RemoteSyncOperator,RemoteSyncPushOp
         worker_threads: None,
         active_encryption_key: None,
         associated_key: None,
+        retention_timespan: None,
     };
 
     let mut update_alice = SyncJobConfigUpdater {
diff --git a/src/tools/mod.rs b/src/tools/mod.rs
index ab624d7a6..e1916cc4b 100644
--- a/src/tools/mod.rs
+++ b/src/tools/mod.rs
@@ -151,6 +151,7 @@ pub(crate) fn backup_info_to_snapshot_list_item(
                 size,
                 owner,
                 protected,
+                retain_until: None,
             };
         }
         Ok(None) => (),
@@ -178,6 +179,7 @@ pub(crate) fn backup_info_to_snapshot_list_item(
         size: None,
         owner,
         protected,
+        retain_until: None,
     }
 }
 
-- 
2.47.3

[PATCH proxmox-backup v2 13/43] api: admin: improve code style for status endpoint

[Christian Ebner]

Reduces indentation level and makes the if branches more concise.

No functional changes.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/admin/datastore.rs | 38 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 51dae4698..80a39cc8a 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -717,31 +717,23 @@ pub async fn status(
     let backend_type = datastore.backend_type();
     let s3_statistics = datastore.s3_statistics();
 
-    Ok(if store_stats {
+    let (total, used, avail) = if store_stats {
         let storage = crate::tools::fs::fs_info(datastore.base_path()).await?;
-        DataStoreStatus {
-            total: storage.total,
-            used: storage.used,
-            avail: storage.available,
-            gc_status,
-            counts,
-            backend_type,
-            s3_statistics,
-            default_retention_timespan: None,
-            max_retention_timespan: None,
-        }
+        (storage.total, storage.used, storage.available)
     } else {
-        DataStoreStatus {
-            total: 0,
-            used: 0,
-            avail: 0,
-            gc_status,
-            counts,
-            backend_type,
-            s3_statistics,
-            default_retention_timespan: None,
-            max_retention_timespan: None,
-        }
+        (0, 0, 0)
+    };
+
+    Ok(DataStoreStatus {
+        total,
+        used,
+        avail,
+        gc_status,
+        counts,
+        backend_type,
+        s3_statistics,
+        default_retention_timespan: None,
+        max_retention_timespan: None,
     })
 }
 
-- 
2.47.3

[PATCH proxmox-backup v2 14/43] client: avoid error in status if user lacks permissions

[Christian Ebner]

In case of missing permissions the total storage space will be
returned as zero, leading to the status output not being rendered
since protected from zero division.

This is however neither user friendly as the error does not reflect
there being a lack of permissions, nor is it the correct behaviour,
since the user still might have access to other information such as
snapshot count (although currently not exposed in the CLI).

Fix this by showing the values as returned by the API and skipping
the percentage display if this would lead to zero division.

While at it, rename the closure to reflect this behaviour and convert
value parsing issues to errors instead of panics.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-backup-client/src/main.rs | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 5238a75ea..1a254f071 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -2145,23 +2145,27 @@ async fn status(param: Value) -> Result<Value, Error> {
 
     record_repository(&repo);
 
-    let render_total_percentage = |v: &Value, record: &Value| -> Result<String, Error> {
-        let v = v.as_u64().unwrap();
-        let total = record["total"].as_u64().unwrap();
+    let total_with_opt_percentage = |v: &Value, record: &Value| -> Result<String, Error> {
+        let v = v
+            .as_u64()
+            .ok_or_else(|| format_err!("value is not a number"))?;
+        let total = record["total"]
+            .as_u64()
+            .ok_or_else(|| format_err!("total is not a number"))?;
         let roundup = total / 200;
         if let Some(per) = ((v + roundup) * 100).checked_div(total) {
             let info = format!(" ({per} %)");
             Ok(format!("{v} {info:>8}"))
         } else {
-            bail!("Cannot render total percentage: denominator is zero");
+            Ok(v.to_string())
         }
     };
 
     let options = default_table_format_options()
         .noheader(true)
-        .column(ColumnConfig::new("total").renderer(render_total_percentage))
-        .column(ColumnConfig::new("used").renderer(render_total_percentage))
-        .column(ColumnConfig::new("avail").renderer(render_total_percentage));
+        .column(ColumnConfig::new("total").renderer(total_with_opt_percentage))
+        .column(ColumnConfig::new("used").renderer(total_with_opt_percentage))
+        .column(ColumnConfig::new("avail").renderer(total_with_opt_percentage));
 
     let return_type = &API_METHOD_STATUS.returns;
 
-- 
2.47.3

[PATCH proxmox-backup v2 15/43] server: allow iterating contents for Datastore.Append permissions

[Christian Ebner]

Appending backups to groups in namespaces must be aware of
pre-existing contents, so it must be allowed to iterate those.

Further, sort the privs list alphabetically to improve readability.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/backup/hierarchy.rs | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/backup/hierarchy.rs b/src/backup/hierarchy.rs
index ef8efd323..d81dc3565 100644
--- a/src/backup/hierarchy.rs
+++ b/src/backup/hierarchy.rs
@@ -3,8 +3,8 @@ use std::sync::Arc;
 use anyhow::{Error, bail};
 
 use pbs_api_types::{
-    Authid, BackupNamespace, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY,
-    PRIV_DATASTORE_READ, privs_to_priv_names,
+    Authid, BackupNamespace, PRIV_DATASTORE_APPEND, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
+    PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_READ, privs_to_priv_names,
 };
 use pbs_config::CachedUserInfo;
 use pbs_datastore::{DataStore, ListGroups, ListNamespacesRecursive, backup_info::BackupGroup};
@@ -151,8 +151,11 @@ impl<'a> ListAccessibleBackupGroups<'a> {
     }
 }
 
-pub static NS_PRIVS_OK: u64 =
-    PRIV_DATASTORE_MODIFY | PRIV_DATASTORE_READ | PRIV_DATASTORE_BACKUP | PRIV_DATASTORE_AUDIT;
+pub static NS_PRIVS_OK: u64 = PRIV_DATASTORE_APPEND
+    | PRIV_DATASTORE_AUDIT
+    | PRIV_DATASTORE_BACKUP
+    | PRIV_DATASTORE_READ
+    | PRIV_DATASTORE_MODIFY;
 
 impl Iterator for ListAccessibleBackupGroups<'_> {
     type Item = Result<BackupGroup, Error>;
-- 
2.47.3

[PATCH proxmox-backup v2 16/43] api: backup: fix possible information leak in multi-tenant datastores

[Christian Ebner]

Do not leak the owner of the group in permission checks on backup, as
a user with Datastore.Backup only permissions should never be able to
see other users. This is especially of interest if users gained
unintended access to a namespace they should not be allowed to, e.g.
due to misconfiguration.

Rather include the namespace and group information.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/backup/mod.rs | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index a9b1ca5c2..ad8794f05 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -156,8 +156,13 @@ fn upgrade_to_backup_protocol(
         let correct_owner =
             owner == auth_id || (owner.is_token() && Authid::from(owner.user().clone()) == auth_id);
         if !correct_owner && worker_type != "benchmark" {
+            let namespace_and_group = if !backup_group.backup_ns().is_root() {
+                format!("[{}]:{}", backup_group.backup_ns(), backup_group.group())
+            } else {
+                backup_group.group().to_string()
+            };
             // only the owner is allowed to create additional snapshots
-            bail!("backup owner check failed ({} != {})", auth_id, owner);
+            bail!("backup owner check failed: {namespace_and_group} not owned by {auth_id}");
         }
 
         let last_backup = {
-- 
2.47.3

[PATCH proxmox-backup v2 17/43] datastore: s3: fix namespace level bulk remove not honoring group locks

[Christian Ebner]

For datastores backed by S3 recursive namespace removal was optimized
by bulk removing all object keys, excluding these which correspond to
a protected snapshot within the group. This reduced the number of
potentially more cost intensive POST calls for DeleteObjects, as
compared to the regular per-group cleanup. However, this did not take
into account the snapshot/group locking and in consequence can lead
to potential inconsistencies between the state in the local cache and
the state on the S3 backend.

To fix this, completely drop this optimization based on incomplete
assumptions and do the cleanup on snapshot/group level.

Fixes: 6ff078a5a ("datastore: prune groups/snapshots from s3 object store backend")
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index e6766a3b9..d5608d4da 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1168,29 +1168,6 @@ impl DataStore {
         // Process deepest first so a parent's rmdir sees an empty directory.
         children.sort_by_key(|b| std::cmp::Reverse(b.depth()));
 
-        // When destroying the full subtree, a bulk S3 delete under the root prefix catches
-        // any orphaned group-level metadata (owner/notes) before per-level cleanup iterates.
-        if delete_groups {
-            if let DatastoreBackend::S3(s3_client) = &self.backend()? {
-                let ns_dir = ns.path();
-                let ns_prefix = ns_dir
-                    .to_str()
-                    .ok_or_else(|| format_err!("invalid namespace path prefix"))?;
-                let prefix = format!("{S3_CONTENT_PREFIX}/{ns_prefix}");
-                let delete_objects_error = proxmox_async::runtime::block_on(
-                    s3_client.delete_objects_by_prefix_with_suffix_filter(
-                        &S3PathPrefix::Some(prefix),
-                        PROTECTED_MARKER_FILENAME,
-                        &[GROUP_OWNER_FILE_NAME, GROUP_NOTES_FILE_NAME],
-                    ),
-                )?;
-                if !delete_objects_error.is_empty() {
-                    crate::s3::log_s3_delete_objects_errors(&delete_objects_error);
-                    bail!("deleting objects failed");
-                }
-            }
-        }
-
         let mut removed_all_requested = true;
         let mut stats = BackupGroupDeleteStats::default();
         for child in &children {
-- 
2.47.3

[PATCH proxmox-backup v2 18/43] api: backup: allow backup for user/token with append permission

[Christian Ebner]

In addition to these with PRIV_DATASTORE_BACKUP, also users/tokens
with PRIV_DATASTORE_APPEND should be able to create new backup groups
and snapshots.

Since only one of them is required to be set, change the priv check to
be partial only.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/backup/mod.rs | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index ad8794f05..d658e8c17 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -23,7 +23,8 @@ use proxmox_sortable_macro::sortable;
 use pbs_api_types::{
     ArchiveType, Authid, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA,
     BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, BackupArchiveName, BackupNamespace, BackupType,
-    CHUNK_DIGEST_SCHEMA, DATASTORE_SCHEMA, Operation, PRIV_DATASTORE_BACKUP, VerifyState,
+    CHUNK_DIGEST_SCHEMA, DATASTORE_SCHEMA, Operation, PRIV_DATASTORE_APPEND, PRIV_DATASTORE_BACKUP,
+    VerifyState,
 };
 use pbs_config::CachedUserInfo;
 use pbs_datastore::fixed_index::FIXED_CHUNK_SIZE;
@@ -95,8 +96,8 @@ fn upgrade_to_backup_protocol(
             .check_privs(
                 &auth_id,
                 &backup_ns.acl_path(&store),
-                PRIV_DATASTORE_BACKUP,
-                false,
+                PRIV_DATASTORE_APPEND | PRIV_DATASTORE_BACKUP,
+                true,
             )
             .map_err(|err| http_err!(FORBIDDEN, "{err}"))?;
 
-- 
2.47.3

[PATCH proxmox-backup v2 19/43] api: allow namespace creation on append permissions

[Christian Ebner]

Only require full modify permissions for moves and namespace
destruction, allow namespace creation also with append permissions.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/admin/namespace.rs |  8 ++++----
 src/api2/tape/restore.rs    |  6 +++---
 src/backup/hierarchy.rs     | 12 +++++++++++-
 src/server/pull.rs          | 10 +++++-----
 4 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/src/api2/admin/namespace.rs b/src/api2/admin/namespace.rs
index 19e1e8cd0..f874f8129 100644
--- a/src/api2/admin/namespace.rs
+++ b/src/api2/admin/namespace.rs
@@ -56,7 +56,7 @@ pub fn create_namespace(
     let mut ns = parent.clone();
     ns.push(name.clone())?;
 
-    check_ns_modification_privs(&store, &ns, &auth_id)?;
+    check_ns_modification_privs(&store, &ns, &auth_id, false)?;
 
     let lookup = crate::tools::lookup_with(&store, Operation::Write);
     let datastore = DataStore::lookup_datastore(lookup)?;
@@ -166,7 +166,7 @@ pub fn delete_namespace(
 ) -> Result<BackupGroupDeleteStats, Error> {
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
-    check_ns_modification_privs(&store, &ns, &auth_id)?;
+    check_ns_modification_privs(&store, &ns, &auth_id, true)?;
 
     let lookup = crate::tools::lookup_with(&store, Operation::Write);
     let datastore = DataStore::lookup_datastore(lookup)?;
@@ -247,8 +247,8 @@ pub fn move_namespace(
 ) -> Result<Value, Error> {
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
-    check_ns_modification_privs(&store, &ns, &auth_id)?;
-    check_ns_modification_privs(&store, &target_ns, &auth_id)?;
+    check_ns_modification_privs(&store, &ns, &auth_id, true)?;
+    check_ns_modification_privs(&store, &target_ns, &auth_id, true)?;
 
     let datastore =
         DataStore::lookup_datastore(crate::tools::lookup_with(&store, Operation::Write))?;
diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 6544aa726..ffa63b104 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -254,9 +254,9 @@ fn check_and_create_namespaces(
         for comp in ns.components() {
             tmp_ns.push(comp.to_string())?;
             if !store.namespace_exists(&tmp_ns) {
-                check_ns_modification_privs(store.name(), &tmp_ns, auth_id).map_err(|_err| {
-                    format_err!("no permission to create namespace '{}'", tmp_ns)
-                })?;
+                check_ns_modification_privs(store.name(), &tmp_ns, auth_id, false).map_err(
+                    |_err| format_err!("no permission to create namespace '{}'", tmp_ns),
+                )?;
 
                 store.create_namespace(&tmp_ns.parent(), comp.to_string())?;
             }
diff --git a/src/backup/hierarchy.rs b/src/backup/hierarchy.rs
index d81dc3565..5cc8fd778 100644
--- a/src/backup/hierarchy.rs
+++ b/src/backup/hierarchy.rs
@@ -20,10 +20,14 @@ pub fn check_ns_privs(
 }
 
 /// Asserts that `privs` for creating/destroying namespace in datastore are fulfilled.
+///
+/// If `needs_full_modify_priv` is not set, `Datastore.Append` is sufficient which must
+/// allow namespace creation only, never deletion.
 pub fn check_ns_modification_privs(
     store: &str,
     ns: &BackupNamespace,
     auth_id: &Authid,
+    needs_full_modify_priv: bool,
 ) -> Result<(), Error> {
     // we could allow it as easy purge-whole datastore, but lets be more restrictive for now
     if ns.is_root() {
@@ -33,7 +37,13 @@ pub fn check_ns_modification_privs(
 
     let parent = ns.parent();
 
-    check_ns_privs(store, &parent, auth_id, PRIV_DATASTORE_MODIFY)
+    let required_privs = if needs_full_modify_priv {
+        PRIV_DATASTORE_MODIFY
+    } else {
+        PRIV_DATASTORE_APPEND | PRIV_DATASTORE_MODIFY
+    };
+
+    check_ns_privs(store, &parent, auth_id, required_privs)
 }
 
 /// Asserts that either either `full_access_privs` or `partial_access_privs` are fulfilled on
diff --git a/src/server/pull.rs b/src/server/pull.rs
index 501c05a1b..b7b28c3d5 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -22,8 +22,8 @@ use tokio::io::AsyncWriteExt;
 use pbs_api_types::{
     ArchiveType, Authid, BackupDir, BackupGroup, BackupNamespace, CLIENT_LOG_BLOB_NAME, CryptMode,
     Fingerprint, GroupFilter, MANIFEST_BLOB_NAME, MAX_NAMESPACE_DEPTH, Operation,
-    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, RateLimitConfig, Remote, SnapshotListItem,
-    VerifyState, print_store_and_ns,
+    PRIV_DATASTORE_APPEND, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, RateLimitConfig, Remote,
+    SnapshotListItem, VerifyState, print_store_and_ns,
 };
 use pbs_client::BackupRepository;
 use pbs_config::CachedUserInfo;
@@ -1438,7 +1438,7 @@ fn check_and_create_ns(params: &PullParameters, ns: &BackupNamespace) -> Result<
     let store_ns_str = print_store_and_ns(params.target.store.name(), ns);
 
     if !ns.is_root() && !params.target.store.namespace_path(ns).exists() {
-        check_ns_modification_privs(params.target.store.name(), ns, &params.owner)
+        check_ns_modification_privs(params.target.store.name(), ns, &params.owner, false)
             .map_err(|err| format_err!("Creating {ns} not allowed - {err}"))?;
 
         let name = match ns.components().last() {
@@ -1458,7 +1458,7 @@ fn check_and_create_ns(params: &PullParameters, ns: &BackupNamespace) -> Result<
         params.target.store.name(),
         ns,
         &params.owner,
-        PRIV_DATASTORE_BACKUP,
+        PRIV_DATASTORE_BACKUP | PRIV_DATASTORE_APPEND,
     )
     .map_err(|err| format_err!("sync into {store_ns_str} not allowed - {err}"))?;
 
@@ -1466,7 +1466,7 @@ fn check_and_create_ns(params: &PullParameters, ns: &BackupNamespace) -> Result<
 }
 
 fn check_and_remove_ns(params: &PullParameters, local_ns: &BackupNamespace) -> Result<bool, Error> {
-    check_ns_modification_privs(params.target.store.name(), local_ns, &params.owner)
+    check_ns_modification_privs(params.target.store.name(), local_ns, &params.owner, true)
         .map_err(|err| format_err!("Removing {local_ns} not allowed - {err}"))?;
 
     // The outer loop (check_and_remove_vanished_ns) iterates children first, so we only need
-- 
2.47.3

[PATCH proxmox-backup v2 20/43] api: sync: allow pull to target for user/token with append permission

[Christian Ebner]

Allows appending new backup groups/snapshots to the target datastore
on syncs in pull direction if the user/token has append permissions
and is owner of the backups.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/config/sync.rs | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index 980689781..a9a86abd5 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -7,10 +7,10 @@ use proxmox_router::{Permission, Router, RpcEnvironment, http_bail};
 use proxmox_schema::{api, param_bail};
 
 use pbs_api_types::{
-    Authid, JOB_ID_SCHEMA, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY,
-    PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, PRIV_REMOTE_AUDIT, PRIV_REMOTE_DATASTORE_BACKUP,
-    PRIV_REMOTE_DATASTORE_PRUNE, PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA, SyncJobConfig,
-    SyncJobConfigUpdater,
+    Authid, JOB_ID_SCHEMA, PRIV_DATASTORE_APPEND, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
+    PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, PRIV_REMOTE_AUDIT,
+    PRIV_REMOTE_DATASTORE_BACKUP, PRIV_REMOTE_DATASTORE_PRUNE, PRIV_REMOTE_READ,
+    PROXMOX_CONFIG_DIGEST_SCHEMA, SyncJobConfig, SyncJobConfigUpdater,
 };
 use pbs_config::sync;
 
@@ -130,7 +130,9 @@ pub fn check_sync_job_modify_access(
             }
 
             // creating backups on target check
-            if ns_anchor_privs & PRIV_DATASTORE_BACKUP == 0 {
+            if ns_anchor_privs & PRIV_DATASTORE_BACKUP == 0
+                && ns_anchor_privs & PRIV_DATASTORE_APPEND == 0
+            {
                 return false;
             }
 
-- 
2.47.3

[PATCH proxmox-backup v2 21/43] sync: pull: allow pulling for user/token with append permission

[Christian Ebner]

Extends the access checks for syncs in pull direction to allow
appending contents when user has Datastore.Append permissions.

Resync of corrupt snapshots is not allowed without Datastore.Backup
privileges, as that is a modification not append and will not work if
the lifecycle is enforced by the storage backend anyways. Instead of
a failure, skip the resync and log this as warning so it is notable
in the task state and log.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/pull.rs   | 10 +++++-----
 src/server/pull.rs | 16 ++++++++++++++++
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/src/api2/pull.rs b/src/api2/pull.rs
index 17c1e4f34..6b53c55b7 100644
--- a/src/api2/pull.rs
+++ b/src/api2/pull.rs
@@ -8,9 +8,9 @@ use proxmox_schema::api;
 
 use pbs_api_types::{
     Authid, BackupNamespace, CRYPT_KEY_ID_SCHEMA, DATASTORE_SCHEMA, GROUP_FILTER_LIST_SCHEMA,
-    GroupFilter, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_PRUNE,
-    PRIV_REMOTE_READ, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA, RESYNC_CORRUPT_SCHEMA,
-    RateLimitConfig, SYNC_ENCRYPTED_ONLY_SCHEMA, SYNC_VERIFIED_ONLY_SCHEMA,
+    GroupFilter, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_APPEND, PRIV_DATASTORE_BACKUP,
+    PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
+    RESYNC_CORRUPT_SCHEMA, RateLimitConfig, SYNC_ENCRYPTED_ONLY_SCHEMA, SYNC_VERIFIED_ONLY_SCHEMA,
     SYNC_WORKER_THREADS_SCHEMA, SyncJobConfig, TRANSFER_LAST_SCHEMA,
 };
 use pbs_config::CachedUserInfo;
@@ -36,8 +36,8 @@ pub fn check_pull_privs(
     user_info.check_privs(
         auth_id,
         &local_store_ns_acl_path,
-        PRIV_DATASTORE_BACKUP,
-        false,
+        PRIV_DATASTORE_APPEND | PRIV_DATASTORE_BACKUP,
+        true,
     )?;
 
     if let Some(remote) = remote {
diff --git a/src/server/pull.rs b/src/server/pull.rs
index b7b28c3d5..33f75c9c5 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -1090,6 +1090,22 @@ async fn pull_snapshot_from<'a>(
         .create_locked_backup_dir(snapshot.backup_ns(), snapshot.as_ref())
         .context(prefix.clone())?;
 
+    if !is_new && corrupt {
+        // only allow re-sync of corrupt snapshots with backup privileges, append only is not enough
+        let acl_path = snapshot.backup_ns().acl_path(snapshot.datastore().name());
+        let user_info = CachedUserInfo::new()?;
+        let privs = user_info.lookup_privs(&params.owner, &acl_path);
+        if privs & PRIV_DATASTORE_BACKUP == 0 {
+            log_sender
+                .log(
+                    Level::WARN,
+                    format!("{prefix}: skip re-sync of corrupt snapshot, missing 'Datastore.Backup' privileges"),
+                )
+                .await?;
+            return Ok(SyncStats::default());
+        }
+    }
+
     let result = pull_snapshot(
         params,
         reader,
-- 
2.47.3

[PATCH proxmox-backup v2 22/43] sync: push: allow push and ns creation on Remote.DatastoreAppend

[Christian Ebner]

A local sync job user with Remote.DatastoreAppend permissions is
intended to be able to append backups on the remote just like
Remote.DatastoreBackup, but also create namespaces not present on
the remote, which would otherwise require Remote.DatastoreModify.
The latter would however also allow for namespace destruction, so
cannot be used for this.

These are push job source checks only, the user connecting to the
remote as configured in the remote config must be setup on the
target PBS instance accordingly (i.e. with Datastore.Audit and
Datastore.Append on the target datastore and sub-namespace).

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/config/sync.rs |  6 +++---
 src/api2/push.rs        |  8 ++++----
 src/server/push.rs      | 13 ++++++++-----
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index a9a86abd5..7ed624c9f 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -9,8 +9,8 @@ use proxmox_schema::{api, param_bail};
 use pbs_api_types::{
     Authid, JOB_ID_SCHEMA, PRIV_DATASTORE_APPEND, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
     PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, PRIV_REMOTE_AUDIT,
-    PRIV_REMOTE_DATASTORE_BACKUP, PRIV_REMOTE_DATASTORE_PRUNE, PRIV_REMOTE_READ,
-    PROXMOX_CONFIG_DIGEST_SCHEMA, SyncJobConfig, SyncJobConfigUpdater,
+    PRIV_REMOTE_DATASTORE_APPEND, PRIV_REMOTE_DATASTORE_BACKUP, PRIV_REMOTE_DATASTORE_PRUNE,
+    PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA, SyncJobConfig, SyncJobConfigUpdater,
 };
 use pbs_config::sync;
 
@@ -171,7 +171,7 @@ pub fn check_sync_job_modify_access(
             };
 
             // check user is allowed to create backups on remote datastore
-            if target_privs & PRIV_REMOTE_DATASTORE_BACKUP == 0 {
+            if target_privs & (PRIV_REMOTE_DATASTORE_BACKUP | PRIV_REMOTE_DATASTORE_APPEND) == 0 {
                 return false;
             }
 
diff --git a/src/api2/push.rs b/src/api2/push.rs
index 16bcdf85b..8d053c00e 100644
--- a/src/api2/push.rs
+++ b/src/api2/push.rs
@@ -4,8 +4,8 @@ use futures::{future::FutureExt, select};
 use pbs_api_types::{
     Authid, BackupNamespace, CRYPT_KEY_ID_SCHEMA, DATASTORE_SCHEMA, GROUP_FILTER_LIST_SCHEMA,
     GroupFilter, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_READ,
-    PRIV_REMOTE_DATASTORE_BACKUP, PRIV_REMOTE_DATASTORE_PRUNE, REMOTE_ID_SCHEMA,
-    REMOVE_VANISHED_BACKUPS_SCHEMA, RateLimitConfig, SYNC_ENCRYPTED_ONLY_SCHEMA,
+    PRIV_REMOTE_DATASTORE_APPEND, PRIV_REMOTE_DATASTORE_BACKUP, PRIV_REMOTE_DATASTORE_PRUNE,
+    REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA, RateLimitConfig, SYNC_ENCRYPTED_ONLY_SCHEMA,
     SYNC_VERIFIED_ONLY_SCHEMA, SYNC_WORKER_THREADS_SCHEMA, TRANSFER_LAST_SCHEMA,
 };
 use proxmox_rest_server::WorkerTask;
@@ -35,8 +35,8 @@ fn check_push_privs(
     user_info.check_privs(
         auth_id,
         &target_acl_path,
-        PRIV_REMOTE_DATASTORE_BACKUP,
-        false,
+        PRIV_REMOTE_DATASTORE_BACKUP | PRIV_REMOTE_DATASTORE_APPEND,
+        true,
     )?;
 
     if delete {
diff --git a/src/server/push.rs b/src/server/push.rs
index 98f843a49..c99975a2b 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -15,8 +15,9 @@ use pbs_api_types::{
     ApiVersion, ApiVersionInfo, ArchiveType, Authid, BackupArchiveName, BackupDir, BackupGroup,
     BackupGroupDeleteStats, BackupNamespace, CLIENT_LOG_BLOB_NAME, CryptMode, GroupFilter,
     GroupListItem, MANIFEST_BLOB_NAME, NamespaceListItem, Operation, PRIV_DATASTORE_BACKUP,
-    PRIV_DATASTORE_READ, PRIV_REMOTE_DATASTORE_BACKUP, PRIV_REMOTE_DATASTORE_MODIFY,
-    PRIV_REMOTE_DATASTORE_PRUNE, RateLimitConfig, Remote, SnapshotListItem, print_store_and_ns,
+    PRIV_DATASTORE_READ, PRIV_REMOTE_DATASTORE_APPEND, PRIV_REMOTE_DATASTORE_BACKUP,
+    PRIV_REMOTE_DATASTORE_MODIFY, PRIV_REMOTE_DATASTORE_PRUNE, RateLimitConfig, Remote,
+    SnapshotListItem, print_store_and_ns,
 };
 use pbs_client::{
     BackupRepository, BackupStats, BackupWriter, BackupWriterOptions, HttpClient, IndexType,
@@ -215,7 +216,7 @@ fn check_ns_remote_datastore_privs(
     let acl_path =
         target_namespace.remote_acl_path(&params.target.remote.name, params.target.repo.store());
 
-    user_info.check_privs(&params.local_user, &acl_path, privs, false)?;
+    user_info.check_privs(&params.local_user, &acl_path, privs, true)?;
 
     Ok(())
 }
@@ -357,7 +358,8 @@ async fn check_or_create_target_namespace(
         // Namespace not present on target, create namespace.
         // Sub-namespaces have to be created by creating parent components first.
 
-        check_ns_remote_datastore_privs(params, target_namespace, PRIV_REMOTE_DATASTORE_MODIFY)
+        let privs = PRIV_REMOTE_DATASTORE_MODIFY | PRIV_REMOTE_DATASTORE_APPEND;
+        check_ns_remote_datastore_privs(params, target_namespace, privs)
             .context("Creating remote namespace not allowed")?;
 
         let mut parent = BackupNamespace::root();
@@ -554,7 +556,8 @@ pub(crate) async fn push_namespace(
 ) -> Result<(StoreProgress, SyncStats, bool), Error> {
     let target_namespace = params.map_to_target(namespace)?;
     // Check if user is allowed to perform backups on remote datastore
-    check_ns_remote_datastore_privs(&params, &target_namespace, PRIV_REMOTE_DATASTORE_BACKUP)
+    let privs = PRIV_REMOTE_DATASTORE_BACKUP | PRIV_REMOTE_DATASTORE_APPEND;
+    check_ns_remote_datastore_privs(&params, &target_namespace, privs)
         .context("Pushing to remote namespace not allowed")?;
 
     let mut list: Vec<BackupGroup> = params
-- 
2.47.3

[PATCH proxmox-backup v2 23/43] datastore: conditionally treat missing manifest as error or benign

[Christian Ebner]

A backup snapshot without a manifest is generally a transient state
during ongoing backups. In other cases it must however be treated
as error.

Therefore, refactor the manifest and blob loading logic to
conditionally allow for missing manifest blob files. By this the
caller is in control whether to treat the missing file as error or
regular operation.

No functional changes, this is in preparation for subsequent changes.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/backup_info.rs | 52 +++++++++++++++++++++++++++-----
 src/tools/mod.rs                 | 13 ++------
 2 files changed, 47 insertions(+), 18 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index be4ec8b3e..f476d4469 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -648,11 +648,26 @@ impl BackupDir {
         let mut path = self.full_path();
         path.push(filename);
 
-        proxmox_lang::try_block!({
-            let mut file = std::fs::File::open(&path)?;
-            DataBlob::load_from_reader(&mut file)
-        })
-        .map_err(|err| format_err!("unable to load blob '{:?}' - {}", path, err))
+        self.load_blob_conditionally(&path, true)
+            .map_err(|err| format_err!("unable to load blob '{path:?}' - {err}"))?
+            .ok_or(format_err!("unable to load missing blob '{path:?}'"))
+    }
+
+    /// Load the `DataBlob` from given full path, to be constructed and verified by the caller.
+    ///
+    /// If the blob file does not exist and `not_found_is_error` is `true`, propagates the error
+    /// while otherwise `Ok(None)` is returned.
+    fn load_blob_conditionally(
+        &self,
+        full_path: &Path,
+        not_found_is_error: bool,
+    ) -> Result<Option<DataBlob>, Error> {
+        let opt_data_blob = match std::fs::File::open(full_path) {
+            Ok(mut file) => Some(DataBlob::load_from_reader(&mut file)?),
+            Err(err) if err.kind() == std::io::ErrorKind::NotFound && !not_found_is_error => None,
+            Err(err) => return Err(err.into()),
+        };
+        Ok(opt_data_blob)
     }
 
     /// Generate the full archive file path with given archive name including server side type
@@ -989,11 +1004,34 @@ impl BackupDir {
     }
 
     /// Load the manifest without a lock. Must not be written back.
+    ///
+    /// A missing manifest file for the snapshot is treated as error.
     pub fn load_manifest(&self) -> Result<(BackupManifest, u64), Error> {
-        let blob = self.load_blob(MANIFEST_BLOB_NAME.as_ref())?;
+        self.load_manifest_optionally(true)?.ok_or_else(|| {
+            format_err!(
+                "unable to load missing manifest '{:?}'",
+                self.full_path().join(MANIFEST_BLOB_NAME.as_ref()),
+            )
+        })
+    }
+
+    /// Load the manifest without a lock. Must not be written back.
+    ///
+    /// If the manifest file for the snapshot is missing and `missing_manifest_is_error` is
+    /// `true`, propagates the error while otherwise `Ok(None)` is returned.
+    pub fn load_manifest_optionally(
+        &self,
+        missing_manifest_is_error: bool,
+    ) -> Result<Option<(BackupManifest, u64)>, Error> {
+        let mut full_path = self.full_path();
+        full_path.push(MANIFEST_BLOB_NAME.as_ref());
+        let Some(blob) = self.load_blob_conditionally(&full_path, missing_manifest_is_error)?
+        else {
+            return Ok(None);
+        };
         let raw_size = blob.raw_size();
         let manifest = BackupManifest::try_from(blob)?;
-        Ok((manifest, raw_size))
+        Ok(Some((manifest, raw_size)))
     }
 
     /// Update the manifest of the specified snapshot. Never write a manifest directly,
diff --git a/src/tools/mod.rs b/src/tools/mod.rs
index e1916cc4b..a69776952 100644
--- a/src/tools/mod.rs
+++ b/src/tools/mod.rs
@@ -47,17 +47,8 @@ pub fn setup_safe_path_env() {
 pub(crate) fn read_backup_index(
     backup_dir: &BackupDir,
 ) -> Result<Option<(BackupManifest, Vec<BackupContent>)>, Error> {
-    let (manifest, index_size) = match backup_dir.load_manifest() {
-        Ok((manifest, index_size)) => (manifest, index_size),
-        Err(err) => {
-            let mut manifest_path = backup_dir.full_path();
-            manifest_path.push(MANIFEST_BLOB_NAME.as_ref());
-            if !manifest_path.exists() {
-                return Ok(None);
-            } else {
-                return Err(err);
-            }
-        }
+    let Some((manifest, index_size)) = backup_dir.load_manifest_optionally(false)? else {
+        return Ok(None);
     };
 
     let mut result = Vec::new();
-- 
2.47.3

[PATCH proxmox-backup v2 24/43] api: backup: provide retain-until timestamp for extended prune protection

[Christian Ebner]

Allows to protect snapshots from being pruned by providing a retention
timestamp during backup.

Pruning must now also parse the manifest if present, which adds some
additional overhead on snapshot forget operations. This is however not
a performance critical code path as prunes run as dedicated worker
tasks or a single snapshot forget via API/CLI, for both cases the
additional parsing overhead on each snapshot prune is acceptable.

However, as a side effect manifests which cannot be parsed can no
longer be pruned without manual intervention.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/backup_info.rs |  9 ++++++
 pbs-datastore/src/manifest.rs    | 50 ++++++++++++++++++++++++++++++++
 src/api2/backup/environment.rs   | 14 +++++++++
 src/api2/backup/mod.rs           | 12 ++++++++
 4 files changed, 85 insertions(+)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index f476d4469..ac3ea8378 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -924,6 +924,15 @@ impl BackupDir {
             bail!("cannot remove protected snapshot"); // use special error type?
         }
 
+        if let Some((manifest, _size)) = self.load_manifest_optionally(false)? {
+            if let Some(retain_until) = manifest.active_retention_lock()? {
+                bail!(
+                    "snapshot protected by retention until {}",
+                    proxmox_time::epoch_to_rfc3339(retain_until)?
+                );
+            }
+        }
+
         if let DatastoreBackend::S3(s3_client) = backend {
             let path = self.relative_path();
             let snapshot_prefix = path
diff --git a/pbs-datastore/src/manifest.rs b/pbs-datastore/src/manifest.rs
index 4d482c252..548e419c6 100644
--- a/pbs-datastore/src/manifest.rs
+++ b/pbs-datastore/src/manifest.rs
@@ -82,6 +82,56 @@ impl BackupManifest {
         Ok(())
     }
 
+    /// Set or extend a retention time for given manifest. The given retention timestamp must be
+    /// greater than any already set timestamp on the manifest. Snapshot destruction is only allowed
+    /// after this time.
+    ///
+    /// The caller must assure a valid timestamp is passed, it does not exceed datastore wide
+    /// retention restrictions and required privileges to modify the timestamp are met.
+    pub fn modify_retention_timestamp(&mut self, timestamp: i64) -> Result<(), Error> {
+        if let Some(previous) = self.retention_timestamp()? {
+            if timestamp < previous {
+                bail!(
+                    "cannot shorten retention time to {}, locked until {}",
+                    proxmox_time::epoch_to_rfc3339_utc(timestamp)?,
+                    proxmox_time::epoch_to_rfc3339_utc(previous)?,
+                );
+            }
+        }
+
+        self.unprotected["retain-until"] = timestamp.into();
+
+        Ok(())
+    }
+
+    /// Parse the retention timestamp for given manifest, if set.
+    ///
+    /// Returns `Ok(None)` if not set and errors when set but parsing failed.
+    pub fn retention_timestamp(&self) -> Result<Option<i64>, Error> {
+        match &self.unprotected["retain-until"] {
+            Value::Number(timestamp) => {
+                let retain_until = timestamp.as_i64().ok_or_else(|| {
+                    format_err!("unexpected value for retain-until timestamp: {timestamp}")
+                })?;
+                Ok(Some(retain_until))
+            }
+            Value::Null => Ok(None),
+            _ => bail!("unexpected value variant for retain-until timestamp"),
+        }
+    }
+
+    /// Check if the manifest has a retention lock and if the lock is still active.
+    ///
+    /// Errors when parsing the retention timestamp fails.
+    pub fn active_retention_lock(&self) -> Result<Option<i64>, Error> {
+        if let Some(retain_until) = self.retention_timestamp()? {
+            if retain_until >= proxmox_time::epoch_i64() {
+                return Ok(Some(retain_until));
+            }
+        }
+        Ok(None)
+    }
+
     pub fn files(&self) -> &[FileInfo] {
         &self.files[..]
     }
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index be70d74f8..7b772c44f 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -154,6 +154,7 @@ pub struct BackupEnvironment {
     pub backup_dir: BackupDir,
     pub last_backup: Option<BackupInfo>,
     pub backend: DatastoreBackend,
+    pub retain_until: Option<i64>,
     state: Arc<Mutex<SharedBackupState>>,
 }
 
@@ -166,6 +167,7 @@ impl BackupEnvironment {
         backup_dir: BackupDir,
         no_cache: bool,
         backup_lock_guards: BackupLockGuards,
+        retain_until: Option<i64>,
     ) -> Result<Self, Error> {
         let state = SharedBackupState {
             finished: BackupState::Active,
@@ -195,6 +197,7 @@ impl BackupEnvironment {
             last_backup: None,
             backend,
             state: Arc::new(Mutex::new(state)),
+            retain_until,
         })
     }
 
@@ -798,6 +801,17 @@ impl BackupEnvironment {
         // check for valid manifest and store stats
         manifest.unprotected["chunk_upload_stats"] = stats;
 
+        // Strip any retention timestamp from the client side uploaded manifest,
+        // only allow the retention calculated server side.
+        // This also covers dropping any pre-existing retentions from the source
+        // during push syncs, if not stripped by the client already.
+        if !manifest.unprotected["retain-until"].take().is_null() {
+            self.log("stripped 'retain-until' from client provided manifest");
+        }
+        if let Some(retain_until) = self.retain_until {
+            manifest.modify_retention_timestamp(retain_until)?;
+        }
+
         for file_info in manifest.files() {
             let archive_name = file_info.filename.as_ref();
             match state.uploaded_archives.get(archive_name) {
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index d658e8c17..7d2236c9e 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -53,6 +53,7 @@ pub const API_METHOD_UPGRADE_BACKUP: ApiMethod = ApiMethod::new(
             ("backup-type", false, &BACKUP_TYPE_SCHEMA),
             ("backup-id", false, &BACKUP_ID_SCHEMA),
             ("backup-time", false, &BACKUP_TIME_SCHEMA),
+            ("retain-until", true, &BACKUP_TIME_SCHEMA),
             ("debug", true, &BooleanSchema::new("Enable verbose debug logging.").schema()),
             ("benchmark", true, &BooleanSchema::new("Job is a benchmark (do not keep data).").schema()),
             ("no-cache", true, &BooleanSchema::new("Disable local datastore cache for network storages").schema()),
@@ -89,6 +90,16 @@ fn upgrade_to_backup_protocol(
         let store = required_string_param(&param, "store")?.to_owned();
         let backup_ns = optional_ns_param(&param)?;
         let backup_dir_arg = pbs_api_types::BackupDir::deserialize(&param)?;
+        let retain_until = match &param["retain-until"] {
+            Value::Number(timestamp) => {
+                let timestamp = timestamp
+                    .as_i64()
+                    .ok_or_else(|| format_err!("invalid retain-until timestamp"))?;
+                Some(timestamp)
+            }
+            Value::Null => None,
+            _ => bail!("unexpected retain-until timestamp variant"),
+        };
 
         let user_info = CachedUserInfo::new()?;
 
@@ -234,6 +245,7 @@ fn upgrade_to_backup_protocol(
                     backup_dir,
                     no_cache,
                     backup_lock_guards,
+                    retain_until,
                 )?;
 
                 env.debug = debug;
-- 
2.47.3

[PATCH proxmox-backup v2 25/43] tools: include retain-until timestamp in snapshot list items

[Christian Ebner]

Include the parsed unix epoch timestamp in the listing, for it to be
shown in the UI/CLI output analogous to the protected flag.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/tools/mod.rs | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/tools/mod.rs b/src/tools/mod.rs
index a69776952..2bfe71f35 100644
--- a/src/tools/mod.rs
+++ b/src/tools/mod.rs
@@ -133,6 +133,14 @@ pub(crate) fn backup_info_to_snapshot_list_item(
 
             let size = Some(files.iter().map(|x| x.size.unwrap_or(0)).sum());
 
+            let retain_until = match manifest.retention_timestamp() {
+                Ok(opt_timestamp) => opt_timestamp,
+                Err(err) => {
+                    eprintln!("error parsing retain-until timestamp: '{err}'");
+                    None
+                }
+            };
+
             return SnapshotListItem {
                 backup,
                 comment,
@@ -142,7 +150,7 @@ pub(crate) fn backup_info_to_snapshot_list_item(
                 size,
                 owner,
                 protected,
-                retain_until: None,
+                retain_until,
             };
         }
         Ok(None) => (),
-- 
2.47.3

[PATCH proxmox-backup v2 26/43] client: backup writer: allow to send retain-until timestamp on backup

[Christian Ebner]

Extend the client options to allow configuration of a retain-until
timestamp for the to-be created backup snapshot on the PBS datastore.

If the server api supports it, the timestamp will be picked up and
processed.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 examples/upload-speed.rs               | 1 +
 pbs-client/src/backup_writer.rs        | 5 +++++
 proxmox-backup-client/src/benchmark.rs | 1 +
 proxmox-backup-client/src/main.rs      | 1 +
 src/server/push.rs                     | 1 +
 5 files changed, 9 insertions(+)

diff --git a/examples/upload-speed.rs b/examples/upload-speed.rs
index 5a63e0f09..09a5f2042 100644
--- a/examples/upload-speed.rs
+++ b/examples/upload-speed.rs
@@ -27,6 +27,7 @@ async fn upload_speed() -> Result<f64, Error> {
             debug: false,
             benchmark: true,
             no_cache: false,
+            retain_until: None,
         },
     )
     .await?;
diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
index 7f7535746..0fc487fd9 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -98,6 +98,8 @@ pub struct BackupWriterOptions<'a> {
     pub benchmark: bool,
     /// Skip datastore cache
     pub no_cache: bool,
+    /// Set retention timestamp for snapshot on PBS
+    pub retain_until: Option<i64>,
 }
 
 impl BackupWriter {
@@ -125,6 +127,9 @@ impl BackupWriter {
         if writer_options.no_cache {
             param["no-cache"] = serde_json::to_value(writer_options.no_cache)?;
         }
+        if let Some(timestamp) = writer_options.retain_until {
+            param["retain-until"] = serde_json::to_value(timestamp)?;
+        }
 
         if !writer_options.ns.is_root() {
             param["ns"] = serde_json::to_value(writer_options.ns)?;
diff --git a/proxmox-backup-client/src/benchmark.rs b/proxmox-backup-client/src/benchmark.rs
index a937bacb9..89bc2396b 100644
--- a/proxmox-backup-client/src/benchmark.rs
+++ b/proxmox-backup-client/src/benchmark.rs
@@ -246,6 +246,7 @@ async fn test_upload_speed(
             debug: false,
             benchmark: true,
             no_cache,
+            retain_until: None,
         },
     )
     .await?;
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 1a254f071..bfd26897d 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -1054,6 +1054,7 @@ async fn create_backup(
             debug: true,
             benchmark: false,
             no_cache,
+            retain_until: None,
         },
     )
     .await?;
diff --git a/src/server/push.rs b/src/server/push.rs
index c99975a2b..054ace8b4 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -1146,6 +1146,7 @@ pub(crate) async fn push_snapshot(
             debug: false,
             benchmark: false,
             no_cache: false,
+            retain_until: None,
         },
     )
     .await
-- 
2.47.3

[PATCH proxmox-backup v2 27/43] sync: push: allow to set retention timestamp for synced snapshots

[Christian Ebner]

Allow the push sync job to present the retention timestamp when
pushing the snapshot to the remote, if the remote supports it.

The timestamp is calculated based on the start time of the sync job
by push parameter construction.

Never propagate any retention timestamp present in the source
manifest, keeping them strictly separated.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/push.rs   | 11 +++++++++--
 src/server/push.rs | 36 ++++++++++++++++++++++++++++++++----
 src/server/sync.rs |  1 +
 3 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/src/api2/push.rs b/src/api2/push.rs
index 8d053c00e..714971ac9 100644
--- a/src/api2/push.rs
+++ b/src/api2/push.rs
@@ -5,8 +5,9 @@ use pbs_api_types::{
     Authid, BackupNamespace, CRYPT_KEY_ID_SCHEMA, DATASTORE_SCHEMA, GROUP_FILTER_LIST_SCHEMA,
     GroupFilter, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_READ,
     PRIV_REMOTE_DATASTORE_APPEND, PRIV_REMOTE_DATASTORE_BACKUP, PRIV_REMOTE_DATASTORE_PRUNE,
-    REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA, RateLimitConfig, SYNC_ENCRYPTED_ONLY_SCHEMA,
-    SYNC_VERIFIED_ONLY_SCHEMA, SYNC_WORKER_THREADS_SCHEMA, TRANSFER_LAST_SCHEMA,
+    REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA, RateLimitConfig, RetentionTimespan,
+    SYNC_ENCRYPTED_ONLY_SCHEMA, SYNC_VERIFIED_ONLY_SCHEMA, SYNC_WORKER_THREADS_SCHEMA,
+    TRANSFER_LAST_SCHEMA,
 };
 use proxmox_rest_server::WorkerTask;
 use proxmox_router::{Permission, Router, RpcEnvironment};
@@ -116,6 +117,10 @@ fn check_push_privs(
                 schema: CRYPT_KEY_ID_SCHEMA,
                 optional: true,
             },
+            "retention-timespan": {
+                type: RetentionTimespan,
+                optional: true,
+            },
         },
     },
     access: {
@@ -143,6 +148,7 @@ async fn push(
     transfer_last: Option<usize>,
     worker_threads: Option<usize>,
     encryption_key: Option<String>,
+    retention_timespan: Option<RetentionTimespan>,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<String, Error> {
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
@@ -176,6 +182,7 @@ async fn push(
         transfer_last,
         worker_threads,
         encryption_key,
+        retention_timespan,
     )
     .await?;
 
diff --git a/src/server/push.rs b/src/server/push.rs
index 054ace8b4..0e14fe1c8 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -17,7 +17,7 @@ use pbs_api_types::{
     GroupListItem, MANIFEST_BLOB_NAME, NamespaceListItem, Operation, PRIV_DATASTORE_BACKUP,
     PRIV_DATASTORE_READ, PRIV_REMOTE_DATASTORE_APPEND, PRIV_REMOTE_DATASTORE_BACKUP,
     PRIV_REMOTE_DATASTORE_MODIFY, PRIV_REMOTE_DATASTORE_PRUNE, RateLimitConfig, Remote,
-    SnapshotListItem, print_store_and_ns,
+    RetentionTimespan, SnapshotListItem, print_store_and_ns,
 };
 use pbs_client::{
     BackupRepository, BackupStats, BackupWriter, BackupWriterOptions, HttpClient, IndexType,
@@ -101,6 +101,8 @@ pub(crate) struct PushParameters {
     /// Encryption key to use for pushing unencrypted backup snapshots. Does not affect
     /// already encrypted snapshots.
     crypt_config: Option<(String, Arc<CryptConfig>)>,
+    /// Retention timestamp to be set for newly pushed snapshots on sync target.
+    retain_until: Option<i64>,
 }
 
 impl PushParameters {
@@ -122,6 +124,7 @@ impl PushParameters {
         transfer_last: Option<usize>,
         worker_threads: Option<usize>,
         active_encryption_key: Option<String>,
+        retention_timespan: Option<RetentionTimespan>,
     ) -> Result<Self, Error> {
         if let Some(max_depth) = max_depth {
             ns.check_max_depth(max_depth)?;
@@ -164,6 +167,17 @@ impl PushParameters {
             bail!("Unsupported remote api version, minimum v2.2 required");
         }
 
+        let retain_until = retention_timespan
+            .map(|timespan| {
+                if api_version < ApiVersion::new(4, 2, 11) {
+                    bail!(
+                        "Unsupported 'retain-until' by remote api version, minimum v4.2.11 required"
+                    );
+                }
+                timespan.to_timestamp_from_systemtime()
+            })
+            .transpose()?;
+
         let supports_prune_delete_stats = api_version >= ApiVersion::new(3, 2, 11);
 
         let target = PushTarget {
@@ -196,6 +210,7 @@ impl PushParameters {
             transfer_last,
             worker_threads,
             crypt_config,
+            retain_until,
         })
     }
 
@@ -926,12 +941,21 @@ pub(crate) async fn push_group(
             if source_snapshots.contains(&snapshot.backup.time) {
                 continue;
             }
-            if snapshot.protected {
+            if snapshot.protected
+                || snapshot
+                    .retain_until
+                    .is_some_and(|retain_until| retain_until >= proxmox_time::epoch_i64())
+            {
+                let reason = if snapshot.protected {
+                    "protected"
+                } else {
+                    "retained"
+                };
                 log_sender
                     .log(
                         Level::INFO,
                         format!(
-                            "Kept protected snapshot {name} on remote",
+                            "Kept {reason} snapshot {name} on remote",
                             name = snapshot.backup
                         ),
                     )
@@ -1146,7 +1170,7 @@ pub(crate) async fn push_snapshot(
             debug: false,
             benchmark: false,
             no_cache: false,
-            retain_until: None,
+            retain_until: params.retain_until,
         },
     )
     .await
@@ -1452,6 +1476,10 @@ pub(crate) async fn push_snapshot(
     // Rewrite manifest for pushed snapshot, recreating manifest from source on target,
     // needs to update all relevant info for new manifest.
     target_manifest.unprotected = source_manifest.unprotected.clone();
+    // Never propagate the source retention timestamp to the target, it has been re-calculated and
+    // passed via the client when starting the backup session based on the sync job's retention
+    // timespan. Keeps source and target retentions independent.
+    target_manifest.unprotected["retain-until"].take();
     if let Some((_id, crypt_config)) = &encrypt_using_key {
         let sync_source_signature = source_manifest.signature(crypt_config)?;
         target_manifest.set_sync_source_signature(&sync_source_signature)?;
diff --git a/src/server/sync.rs b/src/server/sync.rs
index 67d02a6a1..ab6792c2d 100644
--- a/src/server/sync.rs
+++ b/src/server/sync.rs
@@ -752,6 +752,7 @@ pub fn do_sync_job(
                             sync_job.transfer_last,
                             sync_job.worker_threads,
                             sync_job.active_encryption_key,
+                            sync_job.retention_timespan,
                         )
                         .await?;
                         push_store(push_params).await?
-- 
2.47.3

[PATCH proxmox-backup v2 28/43] sync: pull: set retention timestamp from sync job for new snapshots

[Christian Ebner]

The retention timestamp on the source is not relevant for the
retention on the target datastore. Therefore, drop any retention
timestamp present in the manifest when pulling from the source and
set the retention timestamp according to the target, if some.

The timestamp is calculated based on the configured retention
timespan relative to the start time of the sync job by pull
parameter construction.

During a resync, a corrupt snapshot is now as well not allowed
to be overwritten if it still is protected by a retention timespan.

Further the retention is only set when creating the snapshots, not
on any subsequent resync. By that also the last snapshot in the group
can only be protected by retention once.

When remove vanished is enabled, do not remove snapshots vanished
from the source if still protected by a retention locally.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/pull.rs   |  11 ++++-
 src/server/pull.rs | 117 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 120 insertions(+), 8 deletions(-)

diff --git a/src/api2/pull.rs b/src/api2/pull.rs
index 6b53c55b7..c197c161f 100644
--- a/src/api2/pull.rs
+++ b/src/api2/pull.rs
@@ -10,8 +10,8 @@ use pbs_api_types::{
     Authid, BackupNamespace, CRYPT_KEY_ID_SCHEMA, DATASTORE_SCHEMA, GROUP_FILTER_LIST_SCHEMA,
     GroupFilter, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_APPEND, PRIV_DATASTORE_BACKUP,
     PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
-    RESYNC_CORRUPT_SCHEMA, RateLimitConfig, SYNC_ENCRYPTED_ONLY_SCHEMA, SYNC_VERIFIED_ONLY_SCHEMA,
-    SYNC_WORKER_THREADS_SCHEMA, SyncJobConfig, TRANSFER_LAST_SCHEMA,
+    RESYNC_CORRUPT_SCHEMA, RateLimitConfig, RetentionTimespan, SYNC_ENCRYPTED_ONLY_SCHEMA,
+    SYNC_VERIFIED_ONLY_SCHEMA, SYNC_WORKER_THREADS_SCHEMA, SyncJobConfig, TRANSFER_LAST_SCHEMA,
 };
 use pbs_config::CachedUserInfo;
 use proxmox_rest_server::WorkerTask;
@@ -93,6 +93,7 @@ impl TryFrom<&SyncJobConfig> for PullParameters {
             sync_job.resync_corrupt,
             sync_job.worker_threads,
             sync_job.associated_key.clone(),
+            sync_job.retention_timespan.clone(),
         )
     }
 }
@@ -162,6 +163,10 @@ impl TryFrom<&SyncJobConfig> for PullParameters {
                 },
                 optional: true,
             },
+            "retention-timespan": {
+                type: RetentionTimespan,
+                optional: true,
+            },
         },
     },
     access: {
@@ -191,6 +196,7 @@ async fn pull(
     resync_corrupt: Option<bool>,
     worker_threads: Option<usize>,
     decryption_keys: Option<Vec<String>>,
+    retention_timespan: Option<RetentionTimespan>,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<String, Error> {
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
@@ -233,6 +239,7 @@ async fn pull(
         resync_corrupt,
         worker_threads,
         decryption_keys,
+        retention_timespan,
     )?;
 
     // fixme: set to_stdout to false?
diff --git a/src/server/pull.rs b/src/server/pull.rs
index 33f75c9c5..c1531ebda 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -23,7 +23,7 @@ use pbs_api_types::{
     ArchiveType, Authid, BackupDir, BackupGroup, BackupNamespace, CLIENT_LOG_BLOB_NAME, CryptMode,
     Fingerprint, GroupFilter, MANIFEST_BLOB_NAME, MAX_NAMESPACE_DEPTH, Operation,
     PRIV_DATASTORE_APPEND, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, RateLimitConfig, Remote,
-    SnapshotListItem, VerifyState, print_store_and_ns,
+    RetentionTimespan, SnapshotListItem, VerifyState, print_store_and_ns,
 };
 use pbs_client::BackupRepository;
 use pbs_config::CachedUserInfo;
@@ -77,6 +77,8 @@ pub(crate) struct PullParameters {
     worker_threads: Option<usize>,
     /// Decryption key ids and configs to decrypt snapshots with matching key fingerprint
     crypt_configs: Vec<(String, Arc<CryptConfig>)>,
+    // Retention timestamp to be set for newly pulled snapshots on sync target.
+    retain_until: Option<i64>,
 }
 
 impl PullParameters {
@@ -99,6 +101,7 @@ impl PullParameters {
         resync_corrupt: Option<bool>,
         worker_threads: Option<usize>,
         decryption_keys: Option<Vec<String>>,
+        retention_timespan: Option<RetentionTimespan>,
     ) -> Result<Self, Error> {
         if let Some(max_depth) = max_depth {
             ns.check_max_depth(max_depth)?;
@@ -140,6 +143,10 @@ impl PullParameters {
 
         let group_filter = group_filter.unwrap_or_default();
 
+        let retain_until = retention_timespan
+            .map(|timespan| timespan.to_timestamp_from_systemtime())
+            .transpose()?;
+
         let crypt_configs = if let Some(key_ids) = &decryption_keys {
             let mut crypt_configs = Vec::with_capacity(key_ids.len());
             for key_id in key_ids {
@@ -167,6 +174,7 @@ impl PullParameters {
             resync_corrupt,
             worker_threads,
             crypt_configs,
+            retain_until,
         })
     }
 }
@@ -744,7 +752,8 @@ async fn pull_snapshot<'a>(
     };
 
     let mut manifest_data = tmp_manifest_blob.raw_data().to_vec();
-    let manifest = BackupManifest::try_from(tmp_manifest_blob).with_context(|| prefix.clone())?;
+    let mut manifest =
+        BackupManifest::try_from(tmp_manifest_blob).with_context(|| prefix.clone())?;
 
     if ignore_not_verified_or_encrypted(
         &manifest,
@@ -762,6 +771,27 @@ async fn pull_snapshot<'a>(
         return Ok(None);
     }
 
+    let target_retain_until = match &existing_target_manifest {
+        Some(existing_target_manifest) => existing_target_manifest.retention_timestamp()?,
+        None => params.retain_until,
+    };
+
+    let source_retain_until = manifest.retention_timestamp()?;
+    if let Some(unprotected) = manifest.unprotected.as_object_mut() {
+        unprotected.remove("retain-until");
+    } else {
+        bail!("Encountered manifest without 'unprotected' section.");
+    }
+
+    if let Some(retain_until) = target_retain_until {
+        manifest.modify_retention_timestamp(retain_until)?;
+    }
+
+    let active_retention = match &existing_target_manifest {
+        Some(existing_target_manifest) => existing_target_manifest.active_retention_lock()?,
+        None => None,
+    };
+
     let (crypt_config, new_manifest) = match optionally_use_decryption_key(
         Arc::clone(&params),
         &manifest,
@@ -785,6 +815,43 @@ async fn pull_snapshot<'a>(
         }
     };
 
+    // Detect snapshots whose manifest only differs from the source by the retention timestamp
+    // set on a previous sync, so they are not re-synced on every run. At this point the source
+    // manifest already carries the retention of the existing target manifest.
+    let mut content_unchanged = false;
+    if let Some(existing) = &existing_target_manifest {
+        let dummy = CryptConfig::new([0u8; 32])?;
+        if existing.signature(&dummy)? == manifest.signature(&dummy)? {
+            if existing.unprotected == manifest.unprotected {
+                fetch_log(crypt_config).await?;
+                cleanup().await?;
+                return Ok(Some(sync_stats));
+            }
+            // only the unprotected part changed, e.g. notes, which is fine to update even if
+            // the snapshot is retained
+            content_unchanged = true;
+        }
+    }
+
+    if let Some(retain_until) = active_retention.filter(|_| !content_unchanged) {
+        let reason = if corrupt {
+            "corrupt snapshot"
+        } else {
+            "changed snapshot"
+        };
+        log_sender
+            .log(
+                if corrupt { Level::WARN } else { Level::INFO },
+                format!(
+                    "{prefix}: skipped re-sync of {reason}, retention locked until '{}'",
+                    proxmox_time::epoch_to_rfc3339(retain_until)?,
+                ),
+            )
+            .await?;
+        let _ = std::fs::remove_file(&tmp_manifest_name);
+        return Ok(None);
+    }
+
     for item in manifest.files() {
         let path = snapshot.full_archive_path(&item.filename);
         if !corrupt && path.exists() {
@@ -852,7 +919,7 @@ async fn pull_snapshot<'a>(
         sync_stats.add(stats);
     }
 
-    if let Some(new_manifest) = new_manifest {
+    let rewritten_manifest_blob = if let Some(new_manifest) = new_manifest {
         let mut new_manifest = Arc::try_unwrap(new_manifest)
             .map_err(|_arc| {
                 format_err!("failed to take ownership of still referenced new manifest")
@@ -862,6 +929,7 @@ async fn pull_snapshot<'a>(
 
         // copy over notes ecc, but drop encryption key fingerprint and verify state, to be
         // reverified independent from the sync.
+        // Retention timestamp has already been set above, so do not drop it here.
         new_manifest.unprotected = manifest.unprotected.clone();
         if let Some(unprotected) = new_manifest.unprotected.as_object_mut() {
             // legacy field replaced by sync-source-signature
@@ -878,8 +946,19 @@ async fn pull_snapshot<'a>(
             new_manifest.set_sync_source_signature(expected.bytes())?;
         }
 
+        Some(new_manifest.to_data_blob(None)?)
+    } else if source_retain_until != target_retain_until {
+        // Only the retention timestamp in the unprotected section differs between source and
+        // target. Keep everything else as is, so the file list, the signature and the key
+        // fingerprint stay intact and restores with the encryption key keep working.
+
         // keep signature
-        let manifest_blob = new_manifest.to_data_blob(None)?;
+        Some(manifest.to_data_blob(None)?)
+    } else {
+        None
+    };
+
+    if let Some(manifest_blob) = rewritten_manifest_blob {
         // update contents to be uploaded to backend
         manifest_data = manifest_blob.raw_data().to_vec();
 
@@ -1407,12 +1486,38 @@ async fn pull_group(
             if source_snapshots.contains(&snapshot.backup_time()) {
                 continue;
             }
-            if snapshot.is_protected() {
+            let is_retained_result = snapshot
+                .load_manifest()
+                .and_then(|(manifest, _size)| manifest.active_retention_lock())
+                .map(|retain_until| retain_until.is_some());
+
+            let is_retained = match is_retained_result {
+                Ok(retained) => retained,
+                Err(err) => {
+                    log_sender
+                        .log(
+                            Level::WARN,
+                            format!(
+                                "{prefix}: failed checking retention for {}: {err}",
+                                snapshot.dir(),
+                            ),
+                        )
+                        .await?;
+                    // do not delete snapshot if unable to check its retention
+                    true
+                }
+            };
+            if snapshot.is_protected() || is_retained {
+                let reason = if snapshot.is_protected() {
+                    "protected"
+                } else {
+                    "retained"
+                };
                 log_sender
                     .log(
                         Level::INFO,
                         format!(
-                            "{prefix}: don't delete vanished snapshot {} (protected)",
+                            "{prefix}: don't delete vanished snapshot {} ({reason})",
                             snapshot.dir(),
                         ),
                     )
-- 
2.47.3

[PATCH proxmox-backup v2 29/43] datastore/config: parse and enforce maximum retention timespan

[Christian Ebner]

Allows to limit the maximum retention timespan which can be set on
snapshots, so users cannot block contents for unintended extended
periods of time.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 34 +++++++++++++++++++++++++++++++++-
 src/api2/backup/mod.rs         |  4 ++++
 src/api2/config/datastore.rs   |  9 +++++++++
 src/server/pull.rs             | 13 +++++++++----
 4 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index d5608d4da..c8f14c5d0 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -34,7 +34,7 @@ use pbs_api_types::{
     ArchiveType, Authid, BackupGroupDeleteStats, BackupNamespace, BackupType, ChunkOrder,
     DEFAULT_SNAPSHOT_TIME_SKEW_SECS, DataStoreConfig, DatastoreBackendConfig, DatastoreBackendType,
     GarbageCollectionCacheStats, GarbageCollectionStatus, MAX_NAMESPACE_DEPTH, MaintenanceMode,
-    MaintenanceType, Operation, S3Statistics, UPID,
+    MaintenanceType, Operation, RetentionTimespan, S3Statistics, UPID,
 };
 use pbs_config::s3::S3_CFG_TYPE_ID;
 use pbs_config::{BackupLockGuard, ConfigVersionCache};
@@ -213,6 +213,7 @@ pub struct DataStoreImpl {
     request_counters: Option<Arc<SharedRequestCounters>>,
     thresholds_exceeded_callback: Option<ThresholdsExceededCallback>,
     max_snapshot_time_skew: i64,
+    max_retention_timespan: Option<RetentionTimespan>,
 }
 
 impl DataStoreImpl {
@@ -232,6 +233,7 @@ impl DataStoreImpl {
             request_counters: None,
             thresholds_exceeded_callback: None,
             max_snapshot_time_skew: i64::try_from(DEFAULT_SNAPSHOT_TIME_SKEW_SECS).unwrap(),
+            max_retention_timespan: None,
         })
     }
 }
@@ -814,6 +816,7 @@ impl DataStore {
             request_counters,
             thresholds_exceeded_callback,
             max_snapshot_time_skew,
+            max_retention_timespan: config.max_retention_timespan,
         })
     }
 
@@ -859,6 +862,35 @@ impl DataStore {
         Ok(())
     }
 
+    /// Return the configured maximum retention timespan
+    pub fn max_retention_timespan(&self) -> Option<&RetentionTimespan> {
+        self.inner.max_retention_timespan.as_ref()
+    }
+
+    /// Check if the given retention timestamp is contained within the maximum
+    /// retention timespan relative to current system allowed by the datastore.
+    ///
+    /// Fails if there is no maximum retention configured for this datastore.
+    pub fn check_max_retention(&self, retain_until: i64) -> Result<(), Error> {
+        if retain_until <= proxmox_time::epoch_i64() {
+            bail!("retention timestamp must be in the future");
+        }
+        if let Some(max_retention) = self.max_retention_timespan() {
+            let max_timestamp = max_retention
+                .to_timestamp_from_systemtime()?
+                .saturating_add(self.inner.max_snapshot_time_skew);
+            if retain_until > max_timestamp {
+                bail!(
+                    "retention timestamp '{}' exceeds datastore limit of '{}'",
+                    proxmox_time::epoch_to_rfc3339_utc(retain_until)?,
+                    proxmox_time::epoch_to_rfc3339_utc(max_timestamp)?,
+                );
+            }
+            return Ok(());
+        }
+        bail!("no maximum retention timespan configured for datastore");
+    }
+
     // Requires obtaining a shared chunk store lock beforehand
     pub fn create_fixed_writer<P: AsRef<Path>>(
         &self,
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 7d2236c9e..a057340ca 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -157,6 +157,10 @@ fn upgrade_to_backup_protocol(
             "backup"
         };
 
+        if let Some(retain_until) = retain_until {
+            datastore.check_max_retention(retain_until)?;
+        }
+
         // lock backup group to only allow one backup per group at a time
         let (owner, group_guard) = datastore.create_locked_backup_group(
             backup_group.backup_ns(),
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 350e48acc..03ff19fc3 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -442,6 +442,8 @@ pub enum DeletableProperty {
     CounterResetSchedule,
     /// Delete the maximum snapshot time skew property.
     MaxSnapshotTimeSkew,
+    /// Delete the max-retention-timespan
+    MaxRetentionTimespan,
 }
 
 #[api(
@@ -550,6 +552,9 @@ pub fn update_datastore(
                 DeletableProperty::MaxSnapshotTimeSkew => {
                     data.max_snapshot_time_skew = None;
                 }
+                DeletableProperty::MaxRetentionTimespan => {
+                    data.max_retention_timespan = None;
+                }
             }
         }
     }
@@ -655,6 +660,10 @@ pub fn update_datastore(
         data.max_snapshot_time_skew = update.max_snapshot_time_skew;
     }
 
+    if update.max_retention_timespan.is_some() {
+        data.max_retention_timespan = update.max_retention_timespan;
+    }
+
     config.set_data(&name, "datastore", &data)?;
 
     pbs_config::datastore::save_config(&config)?;
diff --git a/src/server/pull.rs b/src/server/pull.rs
index c1531ebda..44347c3ad 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -139,13 +139,18 @@ impl PullParameters {
         let lookup = crate::tools::lookup_with(store, Operation::Write);
         let store = DataStore::lookup_datastore(lookup)?;
         let backend = store.backend()?;
-        let target = PullTarget { store, ns, backend };
 
         let group_filter = group_filter.unwrap_or_default();
 
-        let retain_until = retention_timespan
-            .map(|timespan| timespan.to_timestamp_from_systemtime())
-            .transpose()?;
+        let retain_until = if let Some(retention) = retention_timespan {
+            let retain_until = retention.to_timestamp_from_systemtime()?;
+            (*store).check_max_retention(retain_until)?;
+            Some(retain_until)
+        } else {
+            None
+        };
+
+        let target = PullTarget { store, ns, backend };
 
         let crypt_configs = if let Some(key_ids) = &decryption_keys {
             let mut crypt_configs = Vec::with_capacity(key_ids.len());
-- 
2.47.3

[PATCH proxmox-backup v2 30/43] api: config: allow to set or delete retention timespan for sync jobs

[Christian Ebner]

Extend the sync job config API handler to allow for a retention
timespan to be configured or removed. Check for local sync targets
if the configured retention setting falls within the configured
limits of the datastore.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/config/sync.rs | 47 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 5 deletions(-)

diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index 7ed624c9f..f08285ad6 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -7,16 +7,18 @@ use proxmox_router::{Permission, Router, RpcEnvironment, http_bail};
 use proxmox_schema::{api, param_bail};
 
 use pbs_api_types::{
-    Authid, JOB_ID_SCHEMA, PRIV_DATASTORE_APPEND, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
-    PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, PRIV_REMOTE_AUDIT,
-    PRIV_REMOTE_DATASTORE_APPEND, PRIV_REMOTE_DATASTORE_BACKUP, PRIV_REMOTE_DATASTORE_PRUNE,
-    PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA, SyncJobConfig, SyncJobConfigUpdater,
+    Authid, JOB_ID_SCHEMA, Operation, PRIV_DATASTORE_APPEND, PRIV_DATASTORE_AUDIT,
+    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ,
+    PRIV_REMOTE_AUDIT, PRIV_REMOTE_DATASTORE_APPEND, PRIV_REMOTE_DATASTORE_BACKUP,
+    PRIV_REMOTE_DATASTORE_PRUNE, PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA, SyncJobConfig,
+    SyncJobConfigUpdater,
 };
 use pbs_config::sync;
 
 use crate::api2::admin::sync::ListSyncDirection;
+use crate::tools::lookup_with;
 use pbs_config::CachedUserInfo;
-use pbs_datastore::check_backup_owner;
+use pbs_datastore::{DataStore, check_backup_owner};
 
 pub fn check_sync_job_read_access(
     user_info: &CachedUserInfo,
@@ -187,6 +189,28 @@ pub fn check_sync_job_modify_access(
     }
 }
 
+// Check if the retention config for the local target datastore is within the set limits.
+// Does not check remote stores.
+fn check_retention_config(job: &SyncJobConfig) -> Result<(), Error> {
+    let Some(timespan) = &job.retention_timespan else {
+        return Ok(());
+    };
+
+    let sync_direction = job.sync_direction.unwrap_or_default();
+    if sync_direction == SyncDirection::Pull || job.remote.is_none() {
+        let target_store = if sync_direction == SyncDirection::Pull {
+            &job.store
+        } else {
+            &job.remote_store
+        };
+        let datastore = DataStore::lookup_datastore(lookup_with(target_store, Operation::Lookup))?;
+        let timestamp = timespan.to_timestamp_from_systemtime()?;
+        datastore.check_max_retention(timestamp)?;
+    }
+
+    Ok(())
+}
+
 #[api(
     input: {
         properties: {
@@ -308,6 +332,8 @@ pub fn create_sync_job(
         }
     }
 
+    check_retention_config(&config)?;
+
     let (mut section_config, _digest) = sync::config()?;
 
     if section_config.sections.contains_key(&config.id) {
@@ -423,6 +449,8 @@ pub enum DeletableProperty {
     ActiveEncryptionKey,
     /// Delete associated key property,
     AssociatedKey,
+    /// Delete retention-timespan property
+    RetentionTimespan,
 }
 
 #[api(
@@ -590,6 +618,9 @@ fn do_update_sync_job_config(
                     // Previous active encryption key might be added as associated below.
                     data.associated_key = None;
                 }
+                DeletableProperty::RetentionTimespan => {
+                    data.retention_timespan = None;
+                }
             }
         }
         keep_previous_key_as_associated(
@@ -747,6 +778,12 @@ fn do_update_sync_job_config(
         }
     }
 
+    if update.retention_timespan.is_some() {
+        data.retention_timespan = update.retention_timespan;
+    }
+
+    check_retention_config(data)?;
+
     if !check_sync_job_modify_access(user_info, auth_id, data) {
         bail!("permission check failed");
     }
-- 
2.47.3

[PATCH proxmox-backup v2 31/43] api: admin: allow to modify snapshot retention for root user

[Christian Ebner]

Only root user is able to set or extend the retention setting
previously set by the sync/backup api.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/backup_info.rs |  4 +--
 pbs-datastore/src/datastore.rs   |  1 +
 src/api2/admin/datastore.rs      | 52 ++++++++++++++++++++++++++++++++
 src/backup/verify.rs             |  1 +
 4 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index ac3ea8378..13dd52c3a 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -1048,12 +1048,12 @@ impl BackupDir {
     pub fn update_manifest(
         &self,
         backend: &DatastoreBackend,
-        update_fn: impl FnOnce(&mut BackupManifest),
+        update_fn: impl FnOnce(&mut BackupManifest) -> Result<(), Error>,
     ) -> Result<(), Error> {
         let _guard = self.lock_manifest()?;
         let (mut manifest, _) = self.load_manifest()?;
 
-        update_fn(&mut manifest);
+        update_fn(&mut manifest)?;
 
         // keep existing signature
         let blob = manifest.to_data_blob(None)?;
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index c8f14c5d0..9b0c14979 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -3462,6 +3462,7 @@ impl DataStore {
         snapshot
             .update_manifest(&backend, |manifest| {
                 manifest.unprotected["notes"] = notes.into();
+                Ok(())
             })
             .map_err(|err| format_err!("unable to update manifest blob - {err}"))?;
         Ok(())
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 80a39cc8a..097d74dc1 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -2315,6 +2315,54 @@ pub async fn set_protection(
     .await?
 }
 
+#[api(
+    input: {
+        properties: {
+            store: { schema: DATASTORE_SCHEMA },
+            ns: {
+                type: BackupNamespace,
+                optional: true,
+            },
+            backup_dir: {
+                type: pbs_api_types::BackupDir,
+                flatten: true,
+            },
+            "retain-until": {
+                type: Integer,
+                description: "Set or extend retention to given timestamp",
+            },
+        },
+    },
+    access: {
+        permission: &Permission::Superuser,
+        description: "Only root@pam is allowed to modify retention timespans",
+    },
+)]
+/// Set or extend retention timespan of given backup snapshot
+pub fn modify_retention(
+    store: String,
+    ns: Option<BackupNamespace>,
+    backup_dir: pbs_api_types::BackupDir,
+    retain_until: i64,
+    _rpcenv: &mut dyn RpcEnvironment,
+) -> Result<(), Error> {
+    let datastore = DataStore::lookup_datastore(lookup_with(&store, Operation::Write))?;
+
+    datastore.check_max_retention(retain_until)?;
+
+    let backend = datastore.backend()?;
+    let ns = ns.unwrap_or_default();
+    let backup_dir = datastore.backup_dir(ns, backup_dir)?;
+    backup_dir
+        .update_manifest(&backend, |manifest| {
+            manifest.modify_retention_timestamp(retain_until)?;
+            Ok(())
+        })
+        .map_err(|err| format_err!("failed to modify retention timestamp on manifest: {err}"))?;
+
+    Ok(())
+}
+
 #[api(
     input: {
         properties: {
@@ -2990,6 +3038,10 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
         "pxar-file-download",
         &Router::new().download(&API_METHOD_PXAR_FILE_DOWNLOAD),
     ),
+    (
+        "retention",
+        &Router::new().put(&API_METHOD_MODIFY_RETENTION),
+    ),
     ("rrd", &Router::new().get(&API_METHOD_GET_RRD_STATS)),
     ("s3-refresh", &Router::new().put(&API_METHOD_S3_REFRESH)),
     (
diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index aeb78c032..f3df70240 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -471,6 +471,7 @@ impl VerifyWorker {
             let verify_state = serde_json::to_value(verify_state)?;
             backup_dir.update_manifest(&self.datastore.backend()?, |manifest| {
                 manifest.unprotected["verify_state"] = verify_state;
+                Ok(())
             })
         } {
             info!("verify {datastore_name}:{backup_dir_name} - manifest update error: {err}");
-- 
2.47.3

[PATCH proxmox-backup v2 32/43] tape/s3-refresh: check for retentions outside datastore limits

[Christian Ebner]

The datastore might not allow for retention (no max retention
configured) or the retention timestamp of the source snapshot might
exceed the datastore limit.

Strip or shorten the retention if the limit would otherwise be
exceeded for tape jobs.

For s3-refresh this makes only sense until the backend actually
enforces this. When that is the case, the object retention must be
used for consistency, meaning the datastore cannot be re-created from
the bucket with a shorter limit. During a refresh with the
pre-existing datastore that is only an issue when they are out of
sync due to a re-configuration.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 49 ++++++++++++++++++++++++++++++----
 src/api2/tape/restore.rs       | 16 ++++++++---
 2 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 9b0c14979..8fa6959f5 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -33,8 +33,8 @@ use proxmox_worker_task::WorkerTaskContext;
 use pbs_api_types::{
     ArchiveType, Authid, BackupGroupDeleteStats, BackupNamespace, BackupType, ChunkOrder,
     DEFAULT_SNAPSHOT_TIME_SKEW_SECS, DataStoreConfig, DatastoreBackendConfig, DatastoreBackendType,
-    GarbageCollectionCacheStats, GarbageCollectionStatus, MAX_NAMESPACE_DEPTH, MaintenanceMode,
-    MaintenanceType, Operation, RetentionTimespan, S3Statistics, UPID,
+    GarbageCollectionCacheStats, GarbageCollectionStatus, MANIFEST_BLOB_NAME, MAX_NAMESPACE_DEPTH,
+    MaintenanceMode, MaintenanceType, Operation, RetentionTimespan, S3Statistics, UPID,
 };
 use pbs_config::s3::S3_CFG_TYPE_ID;
 use pbs_config::{BackupLockGuard, ConfigVersionCache};
@@ -50,7 +50,7 @@ use crate::hierarchy::{ListGroups, ListGroupsType, ListNamespaces, ListNamespace
 use crate::index::IndexFile;
 use crate::s3::{S3_CONTENT_PREFIX, digest_from_object_key};
 use crate::task_tracking::{self, update_active_operations};
-use crate::{DataBlob, LocalDatastoreLruCache};
+use crate::{BackupManifest, DataBlob, LocalDatastoreLruCache};
 
 // Cache for fully parsed datastore.cfg
 struct DatastoreConfigCache {
@@ -891,6 +891,28 @@ impl DataStore {
         bail!("no maximum retention timespan configured for datastore");
     }
 
+    /// Restricts the manifest's retention timestamp to be within the datastore
+    /// limits. Strips the timestamp if there is no max retention set.
+    ///
+    /// Already expired retention timestamps are kept as is, they no longer lock the snapshot.
+    pub fn restrict_manifest_retention(&self, manifest: &mut BackupManifest) -> Result<(), Error> {
+        let Some(retain_until) = manifest.retention_timestamp()? else {
+            return Ok(());
+        };
+
+        let Some(max_retention) = self.max_retention_timespan() else {
+            manifest.unprotected["retain-until"].take();
+            return Ok(());
+        };
+
+        let max_timestamp = max_retention.to_timestamp_from_systemtime()?;
+        if retain_until > max_timestamp {
+            manifest.unprotected["retain-until"] = max_timestamp.into();
+        }
+
+        Ok(())
+    }
+
     // Requires obtaining a shared chunk store lock beforehand
     pub fn create_fixed_writer<P: AsRef<Path>>(
         &self,
@@ -3280,11 +3302,28 @@ impl DataStore {
                         .content
                         .collect()
                         .await
-                        .context("failed to collect object contents")?;
+                        .context("failed to collect object contents")?
+                        .to_bytes();
+
+                    if object_path.ends_with(MANIFEST_BLOB_NAME.as_ref()) {
+                        let blob = DataBlob::from_raw(data.clone().to_vec())
+                            .context("failed to load manifest blob")?;
+                        let manifest = BackupManifest::try_from(blob)
+                            .context("failed to decode manifest blob")?;
+                        if let Some(retain_until) = manifest.retention_timestamp()? {
+                            if let Err(err) = self.check_max_retention(retain_until) {
+                                log::warn!(
+                                    "{object_path:?}: retention timestamp exceeds datastore limit - {err:#}"
+                                );
+                            }
+                        }
+                    }
+
                     target_file
-                        .write_all(&data.to_bytes())
+                        .write_all(&data)
                         .await
                         .context("failed to write to target file")?;
+
                     file_create_options
                         .apply_to(&mut target_file, &file_path)
                         .context("failed to set target file create options")?;
diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index ffa63b104..7c3b99fcc 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -1084,8 +1084,12 @@ fn restore_snapshots_to_tmpdir(
                 std::fs::create_dir_all(&tmp_path)?;
 
                 let chunks = chunks_list.entry(source_datastore).or_default();
-                let manifest =
-                    try_restore_snapshot_archive(worker.clone(), &mut decoder, &tmp_path)?;
+                let manifest = try_restore_snapshot_archive(
+                    worker.clone(),
+                    target_datastore,
+                    &mut decoder,
+                    &tmp_path,
+                )?;
 
                 for item in manifest.files() {
                     let mut archive_path = tmp_path.to_owned();
@@ -1405,7 +1409,7 @@ fn restore_archive<'a>(
                     if is_new {
                         info!("restore snapshot {backup_dir}");
 
-                        match restore_snapshot_archive(worker, reader, &path) {
+                        match restore_snapshot_archive(worker, datastore, reader, &path) {
                             Err(err) => {
                                 std::fs::remove_dir_all(&path)?;
                                 bail!("restore snapshot {} failed - {}", backup_dir, err);
@@ -1649,11 +1653,12 @@ fn restore_chunk_archive<'a>(
 
 fn restore_snapshot_archive<'a>(
     worker: Arc<WorkerTask>,
+    datastore: Arc<DataStore>,
     reader: Box<dyn 'a + TapeRead>,
     snapshot_path: &Path,
 ) -> Result<bool, Error> {
     let mut decoder = pxar::decoder::sync::Decoder::from_std(pxar::PxarVariant::Unified(reader))?;
-    match try_restore_snapshot_archive(worker, &mut decoder, snapshot_path) {
+    match try_restore_snapshot_archive(worker, datastore, &mut decoder, snapshot_path) {
         Ok(_) => Ok(true),
         Err(err) => {
             let reader = decoder.input();
@@ -1676,6 +1681,7 @@ fn restore_snapshot_archive<'a>(
 
 fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
     worker: Arc<WorkerTask>,
+    datastore: Arc<DataStore>,
     decoder: &mut pxar::decoder::sync::Decoder<R>,
     snapshot_path: &Path,
 ) -> Result<BackupManifest, Error> {
@@ -1734,6 +1740,8 @@ fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
                 .as_object_mut()
                 .map(|m| m.remove("verify_state"));
 
+            datastore.restrict_manifest_retention(&mut old_manifest)?;
+
             let old_manifest = serde_json::to_string_pretty(&old_manifest)?;
             let blob = DataBlob::encode(old_manifest.as_bytes(), None, true)?;
 
-- 
2.47.3

[PATCH proxmox-backup v2 33/43] ui: add retention timespan form and use it for sync job edit window

[Christian Ebner]

Based on the calender event form, the retention timespan form
provides some useful template values for snapshot retention to
be set on sync jobs.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 www/Makefile                  |  1 +
 www/form/RetentionTimespan.js | 22 ++++++++++++++++++++++
 www/window/SyncJobEdit.js     |  9 +++++++++
 3 files changed, 32 insertions(+)
 create mode 100644 www/form/RetentionTimespan.js

diff --git a/www/Makefile b/www/Makefile
index a27bf754a..fdbf75a17 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -56,6 +56,7 @@ JSSRC=							\
 	form/GroupFilter.js				\
 	form/VerifyOutdatedAfter.js			\
 	form/EncryptionKeySelector.js			\
+	form/RetentionTimespan.js			\
 	data/RunningTasksStore.js			\
 	button/TaskButton.js				\
 	panel/PrunePanel.js				\
diff --git a/www/form/RetentionTimespan.js b/www/form/RetentionTimespan.js
new file mode 100644
index 000000000..a14b68cbb
--- /dev/null
+++ b/www/form/RetentionTimespan.js
@@ -0,0 +1,22 @@
+Ext.define('PBS.data.RetentionTimespanTemplates', {
+    extend: 'Ext.data.Store',
+    alias: 'store.retentionTimespanTemplates',
+
+    field: ['value', 'text'],
+    data: [
+        { value: '1 day', text: Ext.String.format(gettext('{0} day'), 1) },
+        { value: '4 weeks', text: Ext.String.format(gettext('{0} weeks'), 4) },
+        { value: '1 month', text: Ext.String.format(gettext('{0} month'), 1) },
+        { value: '1 year', text: Ext.String.format(gettext('{0} year'), 1) },
+        { value: '', text: gettext('custom') },
+    ],
+});
+
+Ext.define('PBS.form.RetentionTimespan', {
+    extend: 'PBS.form.CalendarEvent',
+    alias: 'widget.pbsRetentionTimespan',
+
+    store: {
+        type: 'retentionTimespanTemplates',
+    },
+});
diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
index 961ce2951..df14f2f56 100644
--- a/www/window/SyncJobEdit.js
+++ b/www/window/SyncJobEdit.js
@@ -231,6 +231,15 @@ Ext.define('PBS.window.SyncJobEdit', {
                         // NOTE: handle deleteEmpty in onGetValues due to bandwidth field having a cbind too,
                         // further hide rate limit field depending on sync direction in controller init.
                     },
+                    {
+                        xtype: 'pbsRetentionTimespan',
+                        name: 'retention-timespan',
+                        fieldLabel: gettext('Retention Timespan'),
+                        emptyText: gettext('None'),
+                        cbind: {
+                            deleteEmpty: '{!isCreate}',
+                        },
+                    },
                 ],
 
                 column2: [
-- 
2.47.3

[PATCH proxmox-backup v2 34/43] ui: allow datastore wide max retention timespan configuration

[Christian Ebner]

Expose the datastore config property in the UI for ease of adaption.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 www/Makefile                           |  1 +
 www/datastore/OptionView.js            |  8 ++++++++
 www/window/MaxRetentionTimespanEdit.js | 25 +++++++++++++++++++++++++
 3 files changed, 34 insertions(+)
 create mode 100644 www/window/MaxRetentionTimespanEdit.js

diff --git a/www/Makefile b/www/Makefile
index fdbf75a17..38f9f3f37 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -91,6 +91,7 @@ JSSRC=							\
 	window/RemoteEdit.js				\
 	window/TrafficControlEdit.js			\
 	window/ThresholdResetScheduleEdit.js		\
+	window/MaxRetentionTimespanEdit.js		\
 	window/NotifyOptions.js				\
 	window/SyncJobEdit.js				\
 	window/PruneJobEdit.js				\
diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
index bf04fbb89..cf23063d4 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -392,5 +392,13 @@ Ext.define('PBS.Datastore.Options', {
                 xtype: 'pbsMaxSnapshotTimeSkewEdit',
             },
         },
+        'max-retention-timespan': {
+            required: true,
+            header: gettext('Maximum Retention'),
+            renderer: (timespan) => timespan ?? gettext('None'),
+            editor: {
+                xtype: 'pbsMaxRetentionTimespanEdit',
+            },
+        },
     },
 });
diff --git a/www/window/MaxRetentionTimespanEdit.js b/www/window/MaxRetentionTimespanEdit.js
new file mode 100644
index 000000000..c64dc86d4
--- /dev/null
+++ b/www/window/MaxRetentionTimespanEdit.js
@@ -0,0 +1,25 @@
+Ext.define('PBS.window.MaxRetentionTimespanEdit', {
+    extend: 'Proxmox.window.Edit',
+    alias: 'widget.pbsMaxRetentionTimespanEdit',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    subject: gettext('Maximum Retention Timespan'),
+
+    cbindData: function (initial) {
+        let me = this;
+
+        me.datastore = encodeURIComponent(me.datastore);
+        me.url = `/api2/extjs/config/datastore/${me.datastore}`;
+        me.method = 'PUT';
+        me.autoLoad = true;
+        return {};
+    },
+
+    items: {
+        xtype: 'pbsRetentionTimespan',
+        name: 'max-retention-timespan',
+        fieldLabel: gettext('Retention Timespan'),
+        labelWidth: 150,
+        emptyText: gettext('None'),
+    },
+});
-- 
2.47.3

[PATCH proxmox-backup v2 35/43] ui: show snapshot retention in datastore contents and allow extension

[Christian Ebner]

Follows along the line of how protection toggle and attributes are
being handled.

Extend the shown attribute next to the snapshot name to also include
the retention timestamp if set. Further, display an hourglass icon
next to the protected shield icon and show retention timestamp when
hovering. If the user is logged in as root@pam, also allow to act on
the retention by either setting or extending it. For regular users
this is not possible, the icon shown faded instead.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 www/Makefile                |   1 +
 www/datastore/Content.js    | 103 +++++++++++++++++++++++++++++++++++-
 www/window/RetentionEdit.js |  56 ++++++++++++++++++++
 3 files changed, 158 insertions(+), 2 deletions(-)
 create mode 100644 www/window/RetentionEdit.js

diff --git a/www/Makefile b/www/Makefile
index 38f9f3f37..3b2c9a27f 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -92,6 +92,7 @@ JSSRC=							\
 	window/TrafficControlEdit.js			\
 	window/ThresholdResetScheduleEdit.js		\
 	window/MaxRetentionTimespanEdit.js		\
+	window/RetentionEdit.js				\
 	window/NotifyOptions.js				\
 	window/SyncJobEdit.js				\
 	window/PruneJobEdit.js				\
diff --git a/www/datastore/Content.js b/www/datastore/Content.js
index bd989e660..dcb8ceaf6 100644
--- a/www/datastore/Content.js
+++ b/www/datastore/Content.js
@@ -46,6 +46,11 @@ Ext.define('pbs-data-store-snapshots', {
             type: 'boolean',
             defaultValue: true,
         },
+        {
+            name: 'retain-until',
+            type: 'date',
+            dateFormat: 'timestamp',
+        },
     ],
 });
 
@@ -842,6 +847,39 @@ Ext.define('PBS.DataStoreContent', {
             });
         },
 
+        onRetentionChange: function (table, rI, cI, item, e, rec) {
+            let me = this;
+            let view = me.getView();
+
+            if (!rec?.data || rec.data.ty !== 'dir' || !view.datastore) {
+                return;
+            }
+            if (Proxmox.UserName !== 'root@pam') {
+                return;
+            }
+            let data = rec.data;
+
+            let params = {
+                'backup-type': data['backup-type'],
+                'backup-id': data['backup-id'],
+                'backup-time': (data['backup-time'].getTime() / 1000).toFixed(0),
+            };
+            if (view.namespace && view.namespace !== '') {
+                params.ns = view.namespace;
+            }
+
+            Ext.create('PBS.window.RetentionEdit', {
+                subject: gettext('Retention') + ` - ${data.text}`,
+                autoShow: true,
+                datastore: view.datastore,
+                retainUntil: data['retain-until'],
+                extraRequestParams: params,
+                listeners: {
+                    destroy: () => me.reload(),
+                },
+            });
+        },
+
         onForget: function (table, rI, cI, item, e, { data }) {
             let me = this;
             let view = this.getView();
@@ -1036,6 +1074,10 @@ Ext.define('PBS.DataStoreContent', {
                     onCopy: createControllerCallback('onCopy'),
                     onVerify: createControllerCallback('onVerify'),
                     onProtectionChange: createControllerCallback('onProtectionChange'),
+                    onRetentionChange:
+                        Proxmox.UserName === 'root@pam'
+                            ? createControllerCallback('onRetentionChange')
+                            : undefined,
                     onForget: createControllerCallback('onForget'),
                 });
             }
@@ -1061,10 +1103,20 @@ Ext.define('PBS.DataStoreContent', {
             header: gettext('Backup Group'),
             dataIndex: 'text',
             renderer: (value, meta, record) => {
+                let attributes = [];
                 if (record.data.protected) {
-                    return `${value} (${gettext('protected')})`;
+                    attributes.push(gettext('protected'));
+                }
+                let until = record.data['retain-until'];
+                if (until && until > new Date()) {
+                    attributes.push(
+                        Ext.String.format(
+                            gettext('retained until {0}'),
+                            Ext.Date.format(until, 'Y-m-d H:i:s'),
+                        ),
+                    );
                 }
-                return value;
+                return attributes.length ? `${value} (${attributes.join(', ')})` : value;
             },
             flex: 1,
         },
@@ -1196,6 +1248,41 @@ Ext.define('PBS.DataStoreContent', {
                     },
                     isActionDisabled: (v, r, c, i, rec) => rec.data.ty !== 'dir',
                 },
+                {
+                    handler: 'onRetentionChange',
+                    getTip: (v, m, rec) => {
+                        if (rec.data.ty !== 'dir') {
+                            return '';
+                        }
+                        let isRoot = Proxmox.UserName === 'root@pam';
+                        let until = rec.data['retain-until'];
+                        if (until && until > new Date()) {
+                            let text = Ext.Date.format(until, 'Y-m-d H:i:s');
+                            return isRoot
+                                ? Ext.String.format(
+                                      gettext('Retained until {0}, click to extend'),
+                                      text,
+                                  )
+                                : Ext.String.format(gettext('Retained until {0}'), text);
+                        }
+                        return isRoot ? gettext('Set retention') : '';
+                    },
+                    getClass: (v, m, rec) => {
+                        if (rec.data.ty !== 'dir') {
+                            return 'pmx-hidden';
+                        }
+                        let until = rec.data['retain-until'];
+                        if (until && until > new Date()) {
+                            return 'fa fa-hourglass-half good';
+                        }
+                        // only root can set or extend retention
+                        return Proxmox.UserName === 'root@pam'
+                            ? 'fa fa-hourglass-o faded'
+                            : 'pmx-hidden';
+                    },
+                    isActionDisabled: (v, r, c, i, rec) =>
+                        rec.data.ty !== 'dir' || Proxmox.UserName !== 'root@pam',
+                },
                 {
                     handler: 'onForget',
                     getTip: (v, m, { data }) => {
@@ -1603,6 +1690,7 @@ Ext.define('PBS.datastore.SnapshotCmdMenu', {
     onCopy: undefined,
     onVerify: undefined,
     onProtectionChange: undefined,
+    onRetentionChange: undefined,
     onForget: undefined,
 
     items: [
@@ -1639,6 +1727,17 @@ Ext.define('PBS.datastore.SnapshotCmdMenu', {
                 disabled: '{!onProtectionChange}',
             },
         },
+        {
+            text: gettext('Set or Extend Retention'),
+            iconCls: 'fa fa-hourglass-half',
+            handler: function () {
+                this.up('menu').onRetentionChange();
+            },
+            cbind: {
+                hidden: '{!onRetentionChange}',
+                disabled: '{!onRetentionChange}',
+            },
+        },
         { xtype: 'menuseparator' },
         {
             text: gettext('Remove'),
diff --git a/www/window/RetentionEdit.js b/www/window/RetentionEdit.js
new file mode 100644
index 000000000..4a6bdfa84
--- /dev/null
+++ b/www/window/RetentionEdit.js
@@ -0,0 +1,56 @@
+Ext.define('PBS.window.RetentionEdit', {
+    extend: 'Proxmox.window.Edit',
+    alias: 'widget.pbsRetentionEdit',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    subject: gettext('Retention'),
+    width: 350,
+
+    method: 'PUT',
+    isCreate: false,
+    autoLoad: false,
+
+    datastore: undefined,
+    retainUntil: undefined,
+
+    cbindData: function () {
+        let me = this;
+
+        me.url = `/api2/extjs/admin/datastore/${encodeURIComponent(me.datastore)}/retention`;
+
+        let now = new Date();
+        return {
+            current: me.retainUntil,
+            minDateTime: me.retainUntil && me.retainUntil > now ? me.retainUntil : now,
+        };
+    },
+
+    items: [
+        {
+            xtype: 'displayfield',
+            fieldLabel: gettext('Retained Until'),
+            submitValue: false,
+            renderer: (val) => (val ? Ext.Date.format(val, 'Y-m-d H:i:s') : Proxmox.Utils.noneText),
+            cbind: {
+                value: '{current}',
+            },
+        },
+        {
+            xtype: 'proxmoxDateTimeField',
+            name: 'retain-until',
+            fieldLabel: gettext('New Retention'),
+            allowBlank: false,
+            cbind: {
+                minValue: '{minDateTime}',
+                value: '{current}',
+            },
+        },
+        {
+            xtype: 'displayfield',
+            userCls: 'pmx-hint',
+            value: gettext(
+                'The retention can only be extended. Snapshots cannot be removed until the retention expires.',
+            ),
+        },
+    ],
+});
-- 
2.47.3

[PATCH proxmox-backup v2 36/43] group: destroy: s3: skip snapshots with active retentions

[Christian Ebner]

Make sure that snapshots which have an active retention are not
removed prematurely from the S3 backend when doing a group cleanup.
The group cleanup on S3 backed datastores is optimized to reduce the
number of API calls, protected snapshots can be filtered out from
the to-be deleted object based on the presence of the protected
file/object marker.

This cannot be done in the same way for snapshots with retention,
as this information is stored on the manifest itself instead.

Therefore, first check if there are retained snapshots, the group
lock guarding against changes to snapshot manifests, and only if
none present, use the optimized cleanup.

If there are however retained snapshots, fallback to do a
per-snapshot cleanup instead.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/backup_info.rs | 59 ++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 10 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index 13dd52c3a..bd102a233 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -236,18 +236,48 @@ impl BackupGroup {
 
         log::info!("removing backup group {:?}", path);
         let mut delete_stats = BackupGroupDeleteStats::default();
+
+        let mut to_remove = Vec::new();
+        let mut has_retained = false;
         for snap in self.iter_snapshots()? {
             let snap = snap?;
             if snap.is_protected() {
                 delete_stats.increment_protected_snapshots();
                 continue;
             }
-            // also for S3 cleanup local only, the actual S3 objects will be removed below,
-            // reducing the number of required API calls.
-            snap.destroy(false, &DatastoreBackend::Filesystem)?;
+            if let Some(retain_until) = snap.active_retention_lock()? {
+                info!(
+                    "skip removal of snapshot {:?}, retained until {}",
+                    snap.relative_path(),
+                    proxmox_time::epoch_to_rfc3339(retain_until)?,
+                );
+                // accounted as protected, which also keeps the group itself
+                delete_stats.increment_protected_snapshots();
+                has_retained = true;
+                continue;
+            }
+            to_remove.push(snap);
+        }
+
+        // Use optimized cleanup for S3 when there are no retained snapshots.
+        // In that case, groups can be cleaned up by API calls with prefix/suffix
+        // filtering since the protected markers are present as objects.
+        let snapshot_backend = if has_retained {
+            backend
+        } else {
+            &DatastoreBackend::Filesystem
+        };
+
+        for snapshot in to_remove {
+            snapshot.destroy(false, snapshot_backend)?;
             delete_stats.increment_removed_snapshots();
         }
 
+        if has_retained {
+            // if there are retained snapshots, the group itself cannot be cleaned up.
+            return Ok(delete_stats);
+        }
+
         if let DatastoreBackend::S3(s3_client) = backend {
             let path = self.relative_group_path();
             let group_prefix = path
@@ -924,13 +954,11 @@ impl BackupDir {
             bail!("cannot remove protected snapshot"); // use special error type?
         }
 
-        if let Some((manifest, _size)) = self.load_manifest_optionally(false)? {
-            if let Some(retain_until) = manifest.active_retention_lock()? {
-                bail!(
-                    "snapshot protected by retention until {}",
-                    proxmox_time::epoch_to_rfc3339(retain_until)?
-                );
-            }
+        if let Some(retain_until) = self.active_retention_lock()? {
+            bail!(
+                "snapshot protected by retention until {}",
+                proxmox_time::epoch_to_rfc3339(retain_until)?
+            );
         }
 
         if let DatastoreBackend::S3(s3_client) = backend {
@@ -1012,6 +1040,17 @@ impl BackupDir {
         crate::SnapshotReader::new_do(self.clone())
     }
 
+    /// Returns the retention timestamp if the snapshot is still locked by it.
+    ///
+    /// Snapshots without manifest, e.g. of ongoing backups, are never locked. Errors if the
+    /// manifest or its retention timestamp cannot be parsed.
+    pub fn active_retention_lock(&self) -> Result<Option<i64>, Error> {
+        match self.load_manifest_optionally(false)? {
+            Some((manifest, _size)) => manifest.active_retention_lock(),
+            None => Ok(None),
+        }
+    }
+
     /// Load the manifest without a lock. Must not be written back.
     ///
     /// A missing manifest file for the snapshot is treated as error.
-- 
2.47.3

[PATCH proxmox-backup v2 37/43] prune: skip snapshots with active retentions

[Christian Ebner]

Make sure that snapshots which have an active retention are not
tried to be pruned but rather skipped. This is analogous to the
checks performed for protected snapshots.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/prune.rs  | 54 +++++++++++++++++++++++++++++++++----
 src/api2/admin/datastore.rs |  7 +++--
 src/server/prune_job.rs     |  5 ++--
 tests/prune.rs              | 49 +++++++++++++++++++++++++++++++--
 www/datastore/Prune.js      |  4 +++
 5 files changed, 108 insertions(+), 11 deletions(-)

diff --git a/pbs-datastore/src/prune.rs b/pbs-datastore/src/prune.rs
index 6d8a215d7..5ece66e43 100644
--- a/pbs-datastore/src/prune.rs
+++ b/pbs-datastore/src/prune.rs
@@ -2,6 +2,7 @@ use std::collections::{HashMap, HashSet};
 use std::path::PathBuf;
 
 use anyhow::Error;
+use tracing::warn;
 
 use pbs_api_types::KeepOptions;
 
@@ -10,6 +11,7 @@ use super::BackupInfo;
 #[derive(Clone, Copy, PartialEq, Eq)]
 pub enum PruneMark {
     Protected,
+    Retained,
     Keep,
     KeepPartial,
     Remove,
@@ -24,12 +26,17 @@ impl PruneMark {
     pub fn protected(self) -> bool {
         self == PruneMark::Protected
     }
+
+    pub fn retained(self) -> bool {
+        self == PruneMark::Retained
+    }
 }
 
 impl std::fmt::Display for PruneMark {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.write_str(match self {
             PruneMark::Protected => "protected",
+            PruneMark::Retained => "retained",
             PruneMark::Keep => "keep",
             PruneMark::KeepPartial => "keep-partial",
             PruneMark::Remove => "remove",
@@ -115,10 +122,39 @@ fn remove_incomplete_and_ignore_future(
     }
 }
 
+/// Check if the snapshot is still protected by its retention timestamp.
+pub fn has_active_retention(info: &BackupInfo) -> bool {
+    match info.backup_dir.active_retention_lock() {
+        Ok(retain_until) => retain_until.is_some(),
+        Err(err) => {
+            warn!(
+                "failed to check retention of {:?} - {err:#}",
+                info.backup_dir.relative_path(),
+            );
+            true
+        }
+    }
+}
+
+/// Keep snapshots marked for removal which are still protected by their retention timestamp.
+///
+/// This must run after the keep options were applied by [`compute_prune_info`].
+pub fn mark_active_retained(
+    prune_info: &mut [(BackupInfo, PruneMark)],
+    has_active_retention: impl Fn(&BackupInfo) -> bool,
+) {
+    for (info, mark) in prune_info.iter_mut() {
+        if *mark == PruneMark::Remove && has_active_retention(info) {
+            *mark = PruneMark::Retained;
+        }
+    }
+}
+
 /// This filters incomplete and kept backups.
 pub fn compute_prune_info(
     mut list: Vec<BackupInfo>,
     options: &KeepOptions,
+    has_active_retention: impl Fn(&BackupInfo) -> bool,
 ) -> Result<Vec<(BackupInfo, PruneMark)>, Error> {
     let mut mark = HashMap::new();
 
@@ -126,6 +162,18 @@ pub fn compute_prune_info(
 
     remove_incomplete_and_ignore_future(&mut mark, &list);
 
+    for info in list.iter() {
+        let backup_id = info.backup_dir.relative_path();
+        if mark.contains_key(&backup_id) {
+            continue;
+        }
+        if info.protected {
+            mark.insert(backup_id, PruneMark::Protected);
+        } else if has_active_retention(info) {
+            mark.insert(backup_id, PruneMark::Retained);
+        }
+    }
+
     if let Some(keep_last) = options.keep_last {
         mark_selections(&mut mark, &list, keep_last as usize, |info| {
             Ok(info.backup_dir.backup_time_string().to_owned())
@@ -170,11 +218,7 @@ pub fn compute_prune_info(
         .into_iter()
         .map(|info| {
             let backup_id = info.backup_dir.relative_path();
-            let mark = if info.protected {
-                PruneMark::Protected
-            } else {
-                mark.get(&backup_id).copied().unwrap_or(PruneMark::Remove)
-            };
+            let mark = mark.get(&backup_id).copied().unwrap_or(PruneMark::Remove);
 
             (info, mark)
         })
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 097d74dc1..563590430 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -58,7 +58,7 @@ use pbs_datastore::dynamic_index::{BufferedDynamicReader, DynamicIndexReader, Lo
 use pbs_datastore::fixed_index::FixedIndexReader;
 use pbs_datastore::index::IndexFile;
 use pbs_datastore::manifest::BackupManifest;
-use pbs_datastore::prune::compute_prune_info;
+use pbs_datastore::prune::{compute_prune_info, has_active_retention};
 use pbs_datastore::{
     BackupDir, DataStore, LocalChunkReader, StoreProgress, check_backup_owner,
     ensure_datastore_is_mounted, task_tracking,
@@ -1016,6 +1016,7 @@ pub fn prune(
         backup_time: i64,
         keep: bool,
         protected: bool,
+        retained: bool,
         #[serde(skip_serializing_if = "Option::is_none")]
         ns: Option<BackupNamespace>,
     }
@@ -1023,7 +1024,7 @@ pub fn prune(
 
     let list = group.list_backups()?;
 
-    let mut prune_info = compute_prune_info(list, &keep_options)?;
+    let mut prune_info = compute_prune_info(list, &keep_options, has_active_retention)?;
 
     prune_info.reverse(); // delete older snapshots first
 
@@ -1040,6 +1041,7 @@ pub fn prune(
                 backup_time: backup_dir.backup_time(),
                 keep,
                 protected: mark.protected(),
+                retained: mark.retained(),
                 ns: None,
             };
             let prune_ns = backup_dir.backup_ns();
@@ -1087,6 +1089,7 @@ pub fn prune(
                 backup_time,
                 keep,
                 protected: mark.protected(),
+                retained: mark.retained(),
                 ns: None,
             });
 
diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
index c94e34f43..79e0df804 100644
--- a/src/server/prune_job.rs
+++ b/src/server/prune_job.rs
@@ -8,7 +8,7 @@ use pbs_api_types::{
     PRIV_DATASTORE_PRUNE, PruneJobOptions, print_store_and_ns,
 };
 use pbs_datastore::DataStore;
-use pbs_datastore::prune::{PruneMark, compute_prune_info};
+use pbs_datastore::prune::{PruneMark, compute_prune_info, has_active_retention};
 use proxmox_rest_server::WorkerTask;
 
 use crate::backup::ListAccessibleBackupGroups;
@@ -56,7 +56,7 @@ pub fn prune_datastore(
         let ns = group.backup_ns();
         let list = group.list_backups()?;
 
-        let mut prune_info = compute_prune_info(list, &prune_options.keep)?;
+        let mut prune_info = compute_prune_info(list, &prune_options.keep, has_active_retention)?;
         prune_info.reverse(); // delete older snapshots first
 
         info!(
@@ -75,6 +75,7 @@ pub fn prune_datastore(
                 info.backup_dir.backup_time_string(),
                 mark = match mark {
                     PruneMark::Protected => "keep protected",
+                    PruneMark::Retained => "keep retained",
                     PruneMark::Keep => "keep",
                     PruneMark::KeepPartial => "keep partial",
                     PruneMark::Remove => "remove",
diff --git a/tests/prune.rs b/tests/prune.rs
index 612a915db..bb5184b76 100644
--- a/tests/prune.rs
+++ b/tests/prune.rs
@@ -3,7 +3,7 @@ use std::path::PathBuf;
 use anyhow::Error;
 
 use pbs_api_types::{MANIFEST_BLOB_NAME, PruneJobOptions};
-use pbs_datastore::prune::compute_prune_info;
+use pbs_datastore::prune::{PruneMark, compute_prune_info};
 use pbs_datastore::{BackupDir, BackupInfo};
 
 fn get_prune_list(
@@ -11,7 +11,7 @@ fn get_prune_list(
     return_kept: bool,
     options: &PruneJobOptions,
 ) -> Vec<PathBuf> {
-    let mut prune_info = compute_prune_info(list, &options.keep).unwrap();
+    let mut prune_info = compute_prune_info(list, &options.keep, |_| false).unwrap();
 
     prune_info.reverse();
 
@@ -74,6 +74,51 @@ fn test_prune_protected() -> Result<(), Error> {
     Ok(())
 }
 
+#[test]
+fn test_prune_retained() -> Result<(), Error> {
+    let orig_list = vec![
+        create_info("host/elsa/2019-11-15T09:39:15Z", false),
+        create_info("host/elsa/2019-11-15T10:39:15Z", false),
+        create_info("host/elsa/2019-11-15T10:49:15Z", false),
+        create_info("host/elsa/2019-11-15T10:59:15Z", false),
+    ];
+    let retained = [
+        PathBuf::from("host/elsa/2019-11-15T10:39:15Z"),
+        PathBuf::from("host/elsa/2019-11-15T10:59:15Z"),
+    ];
+
+    let mut options = PruneJobOptions::default();
+    options.keep.keep_last = Some(1);
+    let prune_info = compute_prune_info(orig_list, &options.keep, |info| {
+        retained.contains(&info.backup_dir.relative_path())
+    })?;
+
+    let marks: Vec<(PathBuf, PruneMark)> = prune_info
+        .iter()
+        .map(|(info, mark)| (info.backup_dir.relative_path(), *mark))
+        .collect();
+    let expect = vec![
+        (
+            PathBuf::from("host/elsa/2019-11-15T10:59:15Z"),
+            PruneMark::Retained,
+        ),
+        (
+            PathBuf::from("host/elsa/2019-11-15T10:49:15Z"),
+            PruneMark::Keep,
+        ),
+        (
+            PathBuf::from("host/elsa/2019-11-15T10:39:15Z"),
+            PruneMark::Retained,
+        ),
+        (
+            PathBuf::from("host/elsa/2019-11-15T09:39:15Z"),
+            PruneMark::Remove,
+        ),
+    ];
+    assert!(marks == expect);
+    Ok(())
+}
+
 #[test]
 fn test_prune_hourly() -> Result<(), Error> {
     let orig_list = vec![
diff --git a/www/datastore/Prune.js b/www/datastore/Prune.js
index 7722db2c7..abb732540 100644
--- a/www/datastore/Prune.js
+++ b/www/datastore/Prune.js
@@ -110,6 +110,10 @@ Ext.define('PBS.Datastore.PruneInputPanel', {
                             backup.keepReason = 'protected';
                             continue;
                         }
+                        if (backup.retained) {
+                            backup.keepReason = 'retained';
+                            continue;
+                        }
                         counter[rule]++;
                         if (rule !== 'keep-all') {
                             backup.keepReason = rule + ': ' + counter[rule];
-- 
2.47.3

[PATCH proxmox-backup v2 38/43] api/ui: expose default retention setting as datastore config option

[Christian Ebner]

Allows to set the default retention timespans for the datastore. Used
as fallback for new backups and syncs when no explicit retention is
requested by the client/sync job. The default is always required to
be within the limits of the max. If no max is set, then no default
can be set. This protects against potential misconfiguration.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/config/datastore.rs               | 39 ++++++++++++++++++++--
 www/Makefile                               |  1 +
 www/datastore/OptionView.js                |  8 +++++
 www/window/DefaultRetentionTimespanEdit.js | 25 ++++++++++++++
 4 files changed, 71 insertions(+), 2 deletions(-)
 create mode 100644 www/window/DefaultRetentionTimespanEdit.js

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 03ff19fc3..d3e710015 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -444,6 +444,8 @@ pub enum DeletableProperty {
     MaxSnapshotTimeSkew,
     /// Delete the max-retention-timespan
     MaxRetentionTimespan,
+    /// Delete the default-retention-timespan
+    DefaultRetentionTimespan,
 }
 
 #[api(
@@ -552,7 +554,20 @@ pub fn update_datastore(
                 DeletableProperty::MaxSnapshotTimeSkew => {
                     data.max_snapshot_time_skew = None;
                 }
+                DeletableProperty::DefaultRetentionTimespan => {
+                    data.default_retention_timespan = None;
+                }
                 DeletableProperty::MaxRetentionTimespan => {
+                    if update
+                        .default_retention_timespan
+                        .as_ref()
+                        .or(data.default_retention_timespan.as_ref())
+                        .is_some()
+                    {
+                        bail!(
+                            "Cannot remove max-retention-timespan while default-retention-timespan still set"
+                        );
+                    }
                     data.max_retention_timespan = None;
                 }
             }
@@ -660,8 +675,28 @@ pub fn update_datastore(
         data.max_snapshot_time_skew = update.max_snapshot_time_skew;
     }
 
-    if update.max_retention_timespan.is_some() {
-        data.max_retention_timespan = update.max_retention_timespan;
+    if let Some(max_retention) = update.max_retention_timespan.clone() {
+        let default_retention = update
+            .default_retention_timespan
+            .as_ref()
+            .or(data.default_retention_timespan.as_ref());
+        if let Some(default_retention) = default_retention {
+            if default_retention.as_timespan().as_secs() > max_retention.as_timespan().as_secs() {
+                bail!("New max-retention-timespan smaller than default-retention-timespan");
+            }
+        }
+        data.max_retention_timespan = Some(max_retention);
+    }
+
+    if let Some(default_retention) = update.default_retention_timespan {
+        if let Some(max_retention) = &data.max_retention_timespan {
+            if default_retention.as_timespan().as_secs() > max_retention.as_timespan().as_secs() {
+                bail!("Cannot set default-retention-timespan exceeding max-retention-timespan");
+            }
+        } else {
+            bail!("Cannot set default-retention-timespan without max-retention-timespan");
+        }
+        data.default_retention_timespan = Some(default_retention);
     }
 
     config.set_data(&name, "datastore", &data)?;
diff --git a/www/Makefile b/www/Makefile
index 3b2c9a27f..e9696949d 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -92,6 +92,7 @@ JSSRC=							\
 	window/TrafficControlEdit.js			\
 	window/ThresholdResetScheduleEdit.js		\
 	window/MaxRetentionTimespanEdit.js		\
+	window/DefaultRetentionTimespanEdit.js		\
 	window/RetentionEdit.js				\
 	window/NotifyOptions.js				\
 	window/SyncJobEdit.js				\
diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
index cf23063d4..ab15ed47b 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -400,5 +400,13 @@ Ext.define('PBS.Datastore.Options', {
                 xtype: 'pbsMaxRetentionTimespanEdit',
             },
         },
+        'default-retention-timespan': {
+            required: true,
+            header: gettext('Default Retention'),
+            renderer: (timespan) => timespan ?? gettext('None'),
+            editor: {
+                xtype: 'pbsDefaultRetentionTimespanEdit',
+            },
+        },
     },
 });
diff --git a/www/window/DefaultRetentionTimespanEdit.js b/www/window/DefaultRetentionTimespanEdit.js
new file mode 100644
index 000000000..ae88a25d7
--- /dev/null
+++ b/www/window/DefaultRetentionTimespanEdit.js
@@ -0,0 +1,25 @@
+Ext.define('PBS.window.DefaultRetentionTimespanEdit', {
+    extend: 'Proxmox.window.Edit',
+    alias: 'widget.pbsDefaultRetentionTimespanEdit',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    subject: gettext('Default Retention Timespan'),
+
+    cbindData: function (initial) {
+        let me = this;
+
+        me.datastore = encodeURIComponent(me.datastore);
+        me.url = `/api2/extjs/config/datastore/${me.datastore}`;
+        me.method = 'PUT';
+        me.autoLoad = true;
+        return {};
+    },
+
+    items: {
+        xtype: 'pbsRetentionTimespan',
+        name: 'default-retention-timespan',
+        fieldLabel: gettext('Retention Timespan'),
+        labelWidth: 150,
+        emptyText: gettext('None'),
+    },
+});
-- 
2.47.3

[PATCH proxmox-backup v2 39/43] api: datastore: expose max/default retention timespans in status listing

[Christian Ebner]

Expose max-retention-timespan and default-retention-timespan in
datastore status list items of /admin/datastore/{store}/status.
Set the default retention timespan also on the datastore instance when
parsed from the config and add the helper to access it.

This will allow client to fetch this information in order to generate
retention timestamps compatible with the datastore limits/defaults as
well as backup/sync jobs to gather the default where needed.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 8 ++++++++
 src/api2/admin/datastore.rs    | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 8fa6959f5..faf5a0410 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -214,6 +214,7 @@ pub struct DataStoreImpl {
     thresholds_exceeded_callback: Option<ThresholdsExceededCallback>,
     max_snapshot_time_skew: i64,
     max_retention_timespan: Option<RetentionTimespan>,
+    default_retention_timespan: Option<RetentionTimespan>,
 }
 
 impl DataStoreImpl {
@@ -234,6 +235,7 @@ impl DataStoreImpl {
             thresholds_exceeded_callback: None,
             max_snapshot_time_skew: i64::try_from(DEFAULT_SNAPSHOT_TIME_SKEW_SECS).unwrap(),
             max_retention_timespan: None,
+            default_retention_timespan: None,
         })
     }
 }
@@ -817,6 +819,7 @@ impl DataStore {
             thresholds_exceeded_callback,
             max_snapshot_time_skew,
             max_retention_timespan: config.max_retention_timespan,
+            default_retention_timespan: config.default_retention_timespan,
         })
     }
 
@@ -862,6 +865,11 @@ impl DataStore {
         Ok(())
     }
 
+    /// Return the configured default retention timespan
+    pub fn default_retention_timespan(&self) -> Option<&RetentionTimespan> {
+        self.inner.default_retention_timespan.as_ref()
+    }
+
     /// Return the configured maximum retention timespan
     pub fn max_retention_timespan(&self) -> Option<&RetentionTimespan> {
         self.inner.max_retention_timespan.as_ref()
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 563590430..186e43eb7 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -732,8 +732,8 @@ pub async fn status(
         counts,
         backend_type,
         s3_statistics,
-        default_retention_timespan: None,
-        max_retention_timespan: None,
+        max_retention_timespan: datastore.max_retention_timespan().cloned(),
+        default_retention_timespan: datastore.default_retention_timespan().cloned(),
     })
 }
 
-- 
2.47.3

[PATCH proxmox-backup v2 40/43] backup/sync: fallback to datastore default-retention-timespan

[Christian Ebner]

If a default retention timespan is configured on the datastore, use
that as fallback if a backup or sync job does not specify any
specific retention timestamp/timespan. Push syncs are covered by the
same path as regular backups, pulls recalculate based on local target
datastore config.

s3-refresh and tape are not covered, since for these the pre-set
retention settings are tried to be restored if compatible with
datastore limits. Especially S3 backends would otherwise run into
compatibility issues, once the S3 object locking and retention is
enforced via the backend.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/backup/mod.rs | 13 ++++++++++---
 src/server/pull.rs     | 16 ++++++++++------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index a057340ca..55ba25841 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -157,9 +157,16 @@ fn upgrade_to_backup_protocol(
             "backup"
         };
 
-        if let Some(retain_until) = retain_until {
-            datastore.check_max_retention(retain_until)?;
-        }
+        let retain_until = match retain_until {
+            Some(retain_until) => {
+                datastore.check_max_retention(retain_until)?;
+                Some(retain_until)
+            }
+            None => datastore
+                .default_retention_timespan()
+                .map(|timespan| timespan.to_timestamp_from_systemtime())
+                .transpose()?,
+        };
 
         // lock backup group to only allow one backup per group at a time
         let (owner, group_guard) = datastore.create_locked_backup_group(
diff --git a/src/server/pull.rs b/src/server/pull.rs
index 44347c3ad..c2506ba2e 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -142,12 +142,16 @@ impl PullParameters {
 
         let group_filter = group_filter.unwrap_or_default();
 
-        let retain_until = if let Some(retention) = retention_timespan {
-            let retain_until = retention.to_timestamp_from_systemtime()?;
-            (*store).check_max_retention(retain_until)?;
-            Some(retain_until)
-        } else {
-            None
+        let retain_until = match retention_timespan {
+            Some(retention) => {
+                let retain_until = retention.to_timestamp_from_systemtime()?;
+                (*store).check_max_retention(retain_until)?;
+                Some(retain_until)
+            }
+            None => (*store)
+                .default_retention_timespan()
+                .map(|timespan| timespan.to_timestamp_from_systemtime())
+                .transpose()?,
         };
 
         let target = PullTarget { store, ns, backend };
-- 
2.47.3

[PATCH proxmox-backup v2 41/43] client: add flags to set snapshot retention via timestamp/timespan

[Christian Ebner]

Add `retention-timespan` and `retain-until` options to the clients
backup command.

Since the client does not a-priori know potential limits and
defaults configured on the target datastore, fetch this information
on-demand to check already client side for incompatibility.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-backup-client/src/main.rs     | 77 +++++++++++++++++++--
 proxmox-backup-client/src/snapshot.rs | 98 ++++++++++++++++++++++++++-
 2 files changed, 169 insertions(+), 6 deletions(-)

diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index bfd26897d..ec83f0c81 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -28,9 +28,9 @@ use pxar::accessor::{MaybeReady, ReadAt, ReadAtOperation};
 use pbs_api_types::{
     ArchiveType, Authid, BACKUP_ID_SCHEMA, BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA,
     BackupArchiveName, BackupDir, BackupGroup, BackupNamespace, BackupPart, BackupType,
-    CATALOG_NAME, ClientRateLimitConfig, CryptMode, ENCRYPTED_KEY_BLOB_NAME, Fingerprint,
-    GroupListItem, MANIFEST_BLOB_NAME, PathPattern, PruneJobOptions, PruneListItem,
-    RateLimitConfig, ServerIdentity, SnapshotListItem, StorageStatus,
+    CATALOG_NAME, ClientRateLimitConfig, CryptMode, DataStoreStatus, ENCRYPTED_KEY_BLOB_NAME,
+    Fingerprint, GroupListItem, MANIFEST_BLOB_NAME, PathPattern, PruneJobOptions, PruneListItem,
+    RateLimitConfig, RetentionTimespan, ServerIdentity, SnapshotListItem, StorageStatus,
 };
 use pbs_client::catalog_shell::Shell;
 use pbs_client::pxar::{ErrorHandler as PxarErrorHandler, MetadataArchiveReader, PxarPrevRef};
@@ -794,6 +794,15 @@ fn spawn_catalog_upload(
                 optional: true,
                 default: false,
             },
+            "retain-until": {
+                type: String,
+                description: "Retention timestamp in RFC3339 UTC format.",
+                optional: true,
+            },
+            "retention-timespan": {
+                type: RetentionTimespan,
+                optional: true,
+            },
         }
     }
  )]
@@ -987,11 +996,65 @@ async fn create_backup(
 
     let backup_time = backup_time_opt.unwrap_or_else(epoch_i64);
 
+    let retain_until_param = match param["retain-until"].as_str() {
+        Some(s) => Some(proxmox_time::parse_rfc3339(s)?),
+        None => None,
+    };
+    let retention_timespan_param: Option<RetentionTimespan> =
+        match param["retention-timespan"].as_str() {
+            Some(s) => Some(s.parse()?),
+            None => None,
+        };
+
+    let retain_until = match (retain_until_param, retention_timespan_param) {
+        (Some(_), Some(_)) => {
+            bail!("cannot specify both '--retain-until' and '--retention-timespan'")
+        }
+        (Some(timestamp), None) => Some(timestamp),
+        (None, Some(timespan)) => Some(timespan.to_timestamp_from_systemtime()?),
+        (None, None) => None,
+    };
+
     let detection_mode = change_detection_mode.unwrap_or_default();
 
     let http_client = connect_rate_limited(&repo, rate_limit)?;
     record_repository(&repo);
 
+    // Check server retention limits and validate requested retention
+    let datastore_status = match http_client
+        .get(
+            &format!("api2/json/admin/datastore/{}/status", repo.store()),
+            None,
+        )
+        .await
+    {
+        Ok(mut res) => serde_json::from_value::<DataStoreStatus>(res["data"].take()).ok(),
+        Err(_) => None,
+    };
+
+    if let Some(status) = &datastore_status {
+        if let Some(target_retention) = retain_until {
+            if let Some(max_retention) = &status.max_retention_timespan {
+                let max_timestamp = max_retention.to_timestamp_from_systemtime()?;
+                if target_retention > max_timestamp {
+                    bail!(
+                        "requested retention timestamp '{}' exceeds datastore limit of '{}' (max retention: {})",
+                        proxmox_time::epoch_to_rfc3339_utc(target_retention)?,
+                        proxmox_time::epoch_to_rfc3339_utc(max_timestamp)?,
+                        max_retention,
+                    );
+                }
+            } else {
+                bail!(
+                    "datastore '{}' has no maximum retention timespan configured; retention cannot be set",
+                    repo.store()
+                );
+            }
+        } else if let Some(default_retention) = &status.default_retention_timespan {
+            log::info!("Server datastore default retention: {default_retention}");
+        }
+    }
+
     let snapshot = BackupDir::from((backup_type, backup_id.to_owned(), backup_time));
     if backup_ns.is_root() {
         log::info!("Starting backup: {snapshot}");
@@ -1007,6 +1070,12 @@ async fn create_backup(
         "Starting backup protocol: {}",
         strftime_local("%c", epoch_i64())?
     );
+    if let Some(retain_until) = retain_until {
+        log::info!(
+            "Requesting retention until: {}",
+            proxmox_time::epoch_to_rfc3339_utc(retain_until)?,
+        );
+    }
 
     let (crypt_config, rsa_encrypted_key) = match crypto.enc_key {
         None => (None, None),
@@ -1054,7 +1123,7 @@ async fn create_backup(
             debug: true,
             benchmark: false,
             no_cache,
-            retain_until: None,
+            retain_until,
         },
     )
     .await?;
diff --git a/proxmox-backup-client/src/snapshot.rs b/proxmox-backup-client/src/snapshot.rs
index 438d03b5a..4a751c5da 100644
--- a/proxmox-backup-client/src/snapshot.rs
+++ b/proxmox-backup-client/src/snapshot.rs
@@ -1,13 +1,15 @@
 use std::sync::Arc;
 
-use anyhow::Error;
+use anyhow::{Error, bail};
 use serde_json::{Value, json};
 
 use proxmox_router::cli::*;
 use proxmox_schema::api;
 use proxmox_sys::fs::file_get_contents;
 
-use pbs_api_types::{BackupGroup, BackupNamespace, CryptMode, SnapshotListItem};
+use pbs_api_types::{
+    BackupGroup, BackupNamespace, CryptMode, DataStoreStatus, RetentionTimespan, SnapshotListItem,
+};
 use pbs_client::tools::key_source::get_encryption_key_password;
 use pbs_datastore::DataBlob;
 use pbs_key_config::decrypt_key;
@@ -479,10 +481,102 @@ fn notes_cli() -> CliCommandMap {
         )
 }
 
+#[api(
+    input: {
+        properties: {
+            repo: {
+                type: BackupTargetArgs,
+                flatten: true,
+            },
+            snapshot: {
+                type: String,
+                description: "Snapshot path.",
+            },
+            "retain-until": {
+                type: String,
+                description: "Retention timestamp in RFC3339 UTC format.",
+                optional: true,
+            },
+            "retention-timespan": {
+                type: RetentionTimespan,
+                optional: true,
+            },
+        }
+    }
+)]
+/// Set or extend the retention timestamp of a snapshot
+async fn update_retention(param: Value) -> Result<(), Error> {
+    let repo = extract_repository_from_value(&param)?;
+    let path = required_string_param(&param, "snapshot")?;
+
+    let retain_until_param = match param["retain-until"].as_str() {
+        Some(s) => Some(proxmox_time::parse_rfc3339(s)?),
+        None => None,
+    };
+
+    let retention_timespan_param: Option<RetentionTimespan> =
+        match param["retention-timespan"].as_str() {
+            Some(s) => Some(s.parse()?),
+            None => None,
+        };
+
+    let retain_until = match (retain_until_param, retention_timespan_param) {
+        (Some(_), Some(_)) => {
+            bail!("cannot specify both '--retain-until' and '--retention-timespan'")
+        }
+        (Some(timestamp), None) => timestamp,
+        (None, Some(timespan)) => timespan.to_timestamp_from_systemtime()?,
+        (None, None) => bail!("must specify either '--retain-until' or '--retention-timespan'"),
+    };
+
+    let backup_ns = optional_ns_param(&param)?;
+    let snapshot: BackupDir = path.parse()?;
+    let client = connect(&repo)?;
+    let api_path = format!("api2/json/admin/datastore/{}/status", repo.store());
+
+    if let Ok(mut status_res) = client.get(&api_path, None).await {
+        if let Ok(status) = serde_json::from_value::<DataStoreStatus>(status_res["data"].take()) {
+            if let Some(max_retention) = &status.max_retention_timespan {
+                let max_timestamp = max_retention.to_timestamp_from_systemtime()?;
+                if retain_until > max_timestamp {
+                    bail!(
+                        "requested retention timestamp '{}' exceeds datastore limit of '{}' (max retention: {})",
+                        proxmox_time::epoch_to_rfc3339_utc(retain_until)?,
+                        proxmox_time::epoch_to_rfc3339_utc(max_timestamp)?,
+                        max_retention,
+                    );
+                }
+            }
+        }
+    }
+
+    let path = format!("api2/json/admin/datastore/{}/retention", repo.store());
+    let mut args = snapshot_args(&backup_ns, &snapshot)?;
+    args["retain-until"] = Value::from(retain_until);
+
+    client.put(&path, Some(args)).await?;
+
+    let date_str = proxmox_time::epoch_to_rfc3339_utc(retain_until)?;
+    log::info!("successfully set retention until {date_str}");
+
+    Ok(())
+}
+
+fn retention_cli() -> CliCommandMap {
+    CliCommandMap::new().insert(
+        "update",
+        CliCommand::new(&API_METHOD_UPDATE_RETENTION)
+            .arg_param(&["snapshot"])
+            .completion_cb("ns", complete_namespace)
+            .completion_cb("snapshot", complete_backup_snapshot),
+    )
+}
+
 pub fn snapshot_mgtm_cli() -> CliCommandMap {
     CliCommandMap::new()
         .insert("notes", notes_cli())
         .insert("protected", protected_cli())
+        .insert("retention", retention_cli())
         .insert(
             "list",
             CliCommand::new(&API_METHOD_LIST_SNAPSHOTS)
-- 
2.47.3

[PATCH proxmox-backup v2 42/43] docs: describe the privileges and roles for append only backup/sync

[Christian Ebner]

Complete the list with the newly added roles and permissions and
mention their intended usecase.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 docs/managing-remotes.rst |  6 +++++-
 docs/user-management.rst  | 16 ++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/docs/managing-remotes.rst b/docs/managing-remotes.rst
index 9f269b56b..b25e3d532 100644
--- a/docs/managing-remotes.rst
+++ b/docs/managing-remotes.rst
@@ -109,7 +109,11 @@ To set up sync jobs, the configuring user needs the following permissions:
   job, ``Datastore.Modify`` or ``Datastore.Read`` is required on the source
   datastore. Otherwise, ``Datastore.Backup`` is enough to configure a sync job
   limited to owned backup groups.
-* At least ``Datastore.Backup`` on the local target datastore (``/datastore/{store}``)
+* At least ``Datastore.Backup`` or ``Datastore.Append`` on the local target
+  datastore (``/datastore/{store}``). Using ``Datastore.Append`` enables
+  append-only sync jobs that can push or pull new snapshots and create
+  sub-namespaces, but lack permissions to read, restore, modify, or prune
+  existing backups.
 
 .. note:: A sync job can only sync backup groups that the configured remote's
   user/API token can read. If a remote is configured with a user/API token that
diff --git a/docs/user-management.rst b/docs/user-management.rst
index 066027edd..560009d3e 100644
--- a/docs/user-management.rst
+++ b/docs/user-management.rst
@@ -219,6 +219,11 @@ We currently support the following privileges:
   privileges of Datastore.Read and Datastore.Verify, but only if the backup
   group is owned by the user or one of its tokens.
 
+**Datastore.Append**
+  Datastore.Append allows a user or API token to append new backup snapshots and
+  create new namespaces, without granting privileges to read, restore, modify,
+  or prune existing backups.
+
 **Datastore.Prune**
   Datastore.Prune allows a user to delete snapshots, but additionally requires
   backup ownership.
@@ -238,6 +243,10 @@ We currently support the following privileges:
 **Remote.Read**
   Remote.Read allows a user to read data from a configured `Remote`.
 
+**Remote.DatastoreAppend**
+  Remote.DatastoreAppend allows pushing backups to a remote datastore using
+  append-only privileges.
+
 **Sys.Console**
   Sys.Console allows a user to access the system's console, note that for all
   but `root@pam` a valid system login is still required.
@@ -295,6 +304,10 @@ The following roles exist:
 **DatastoreBackup**
   Can backup and restore owned backups.
 
+**DatastoreAppend**
+  Can append new backups and create namespaces on a datastore, without reading,
+  restoring, or pruning existing data.
+
 **DatastorePowerUser**
   Can backup, restore, and prune *owned* backups.
 
@@ -307,6 +320,9 @@ The following roles exist:
 **RemoteSyncOperator**
   Is allowed to read data from a remote.
 
+**RemoteSyncAppendOperator**
+  Is allowed to push append-only backups to a remote.
+
 **TapeAdmin**
   Can do anything related to tape backup.
 
-- 
2.47.3

[PATCH proxmox-backup v2 43/43] docs: document snapshot retention for new backup snapshots

[Christian Ebner]

Basic description of what it does and limitations in connection with
datastore options, examples for how to set a new retention via the
client and for sync job configuration.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 docs/backup-client.rst    | 28 ++++++++++++++++++++++++++++
 docs/maintenance.rst      | 22 ++++++++++++++++++++++
 docs/managing-remotes.rst | 20 ++++++++++++++++++++
 docs/storage.rst          |  7 +++++++
 4 files changed, 77 insertions(+)

diff --git a/docs/backup-client.rst b/docs/backup-client.rst
index bf207452f..32841f71a 100644
--- a/docs/backup-client.rst
+++ b/docs/backup-client.rst
@@ -299,6 +299,32 @@ device images. To create a backup of a block device, run the following command:
   # proxmox-backup-client backup mydata.img:/dev/mylvm/mydata
 
 
+Setting a Retention Lock on New Backups
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+You can specify a retention timestamp when creating a backup by passing the
+``--retain-until`` parameter formatted as an RFC 3339 timestamp:
+
+.. code-block:: console
+
+  # proxmox-backup-client backup root.pxar:/ --retain-until '2026-12-31T23:59:59Z'
+
+Another convenient way to set a retention is by specifying a timespan via the
+``--retention-timespan`` parameter, which will calculate the timestamp relative
+to the current system time for you:
+
+.. code-block:: console
+
+  # proxmox-backup-client backup root.pxar:/ --retention-timespan 1h
+
+
+When set, the backup server locks the snapshot with a retention lock. The
+snapshot cannot be deleted or pruned until the retention timestamp is reached.
+The specified retention must be within the ``max-retention-timespan`` as
+configured on the backup server's datastore, otherwise the backup request is
+rejected. If the server has no configuration, setting retentions is not
+possible.
+
 Excluding Files/Directories from a Backup
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -864,6 +890,8 @@ they are newer than the last successful backup. In this case, the last failed
 backup is retained. Snapshots whose timestamps are in the future relative to
 the system time of the PBS will be ignored and always kept when pruning, only
 applying retention options to older snapshots.
+Snapshots protected by an active retention lock (``retain-until``) cannot be pruned
+or forgotten until their retention period expires.
 
 .. code-block:: console
 
diff --git a/docs/maintenance.rst b/docs/maintenance.rst
index d23a3774f..0b059c732 100644
--- a/docs/maintenance.rst
+++ b/docs/maintenance.rst
@@ -76,6 +76,28 @@ Old unfinished or incomplete backups will be removed by the prune command,
 unless they are newer than the last successful backup. In this case, the last
 failed backup is retained.
 
+
+.. _snapshot_retention:
+
+Snapshot Retention (WORM Protection)
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Individual backup snapshots can be protected by a retention lock until a specified timestamp
+(``retain-until``). This provides Write-Once-Read-Many (WORM) immutability to guard against
+premature deletion, ransomware, and administrative errors.
+
+* **Immutability**: A retained snapshot cannot be pruned, forgotten, or destroyed until its
+  retention period has expired.
+* **Prune Independence**: Retained snapshots are excluded from the keep slots (such as
+  ``keep-daily`` or ``keep-weekly``) evaluated by the prune engine. This ensures that retained
+  snapshots do not crowd out or starve regular un-retained snapshots from their intended keep slots.
+* **Strict Modification Control**: Only the superuser (``root@pam``) can modify the retention
+  timestamp of an existing snapshot. Retention can only be extended; it can never be shortened
+  or cleared before it expires.
+* **Group and Datastore Guard**: A backup group containing retained snapshots cannot be deleted
+  during group removal operations. Furthermore, a datastore containing active retentions cannot
+  be destroyed with ``--destroy-data``.
+
 Prune Simulator
 ^^^^^^^^^^^^^^^
 
diff --git a/docs/managing-remotes.rst b/docs/managing-remotes.rst
index b25e3d532..294e9c613 100644
--- a/docs/managing-remotes.rst
+++ b/docs/managing-remotes.rst
@@ -184,6 +184,26 @@ buffered briefly to keep related lines together. Note that the memory and
 connection count on both endpoints grow roughly linearly with the number of
 workers, so higher values are not always better.
 
+.. _sync_job_retention:
+
+Sync Job Retention Timespan
+^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Sync jobs can be configured with a ``retention-timespan`` (for example, ``30d``
+or ``1y``). When set, any backup snapshot newly synced by the job will have an
+active retention lock applied automatically upon creation:
+
+.. code-block:: console
+
+   # proxmox-backup-manager sync-job update pbs2-local --retention-timespan '30d'
+
+This ensures that synced snapshots cannot be deleted or pruned until their
+retention period expires, forming a reliable off-site WORM backup target. The
+sync job's retention can however never exceed the ``max-retention-timespan``
+configured for the target datastore, if it is not set, no retention is allowed.
+If no retention timespan is configured on the sync job, the target datastore's
+``default-retention-timespan`` is applied as a fallback, if set.
+
 Namespace Support
 ^^^^^^^^^^^^^^^^^
 
diff --git a/docs/storage.rst b/docs/storage.rst
index 4e2374499..3f97f625a 100644
--- a/docs/storage.rst
+++ b/docs/storage.rst
@@ -614,6 +614,13 @@ There are a few per-datastore options:
 * Maximum allowed time skew for snapshot timestamps (<= 1 day, default: 1
   minute). Does not allow for backup snapshot timestamps to be further in the
   future than the configured value relative to the server's system time.
+* **Maximum Retention Timespan** (``max-retention-timespan``): Enforces an upper
+  limit on how far into the future a snapshot's retention lock can be set on
+  this datastore. Backups or sync operations requesting retention beyond this
+  limit are rejected.
+* **Default Retention Timespan** (``default-retention-timespan``): Default
+  retention period automatically applied to new backup snapshots or sync pulls
+  if the client or sync job does not supply an explicit retention timestamp.
 
 .. _datastore_tuning_options:
 
-- 
2.47.3