Merge lp:~dawgfoto/duplicity/fixup1252 into lp:~duplicity-team/duplicity/0.8-series

hey Martin,

while the patch looks sound, i'd really like to know why the manifest is retrieved/decrypted from backend in the first place, before it get's applied.

@Ken?

..ede/duply.net

Seems like it tries to catch corruptions see https://bazaar.launchpad.net/~duplicity-team/duplicity/0.8-series/view/1301/bin/duplicity#L1333 and https://bazaar.launchpad.net/~duplicity-team/duplicity/0.8-series/view/1301/duplicity/collections.py#L218.
Given that this is not possible with an encrypted backup (without the key and/or password) and that the check hasn't been done despite the "non-fatal" error message, it seems fine to me to just skip it.

Changed the patch to still check the local manifest.

hey Martin,

are you aware of the "no private key issue/double key approach" as eg. described here
  https://lists.gnu.org/archive/html/duplicity-talk/2017-10/msg00015.html
?

On 23.04.2018 15:50, Martin Nowak wrote:
> Seems like it tries to catch corruptions see https://bazaar.launchpad.net/~duplicity-team/duplicity/0.8-series/view/1301/bin/duplicity#L1333 and https://bazaar.launchpad.net/~duplicity-team/duplicity/0.8-series/view/1301/duplicity/collections.py#L218.

ok, isee the manifests are compared, which sounds reasonable.

> Given that this is not possible with an encrypted backup (without the key and/or password)

it is ;) when a private key and passphrase (might be empty) for one of the used public keys is available.
as i don't see a way to easily detect a private key available (duply has some shell gpg trickery for this though) i don't see a problem logging the gpg error as a warning and simply continue.

>
and that the check hasn't been done despite the "non-fatal" error message, it seems fine to me to just skip it.
>

i'd rather log a warning that the comparison was skipped because of the decryption impossibility, so the user may be informed that a /surplus/ security measure was not applied.

finally: the current mode of allowing incrementals w/o validating the backend is error prone[1] and should be replaced in a future version of duplicity. but as it stands, nobody took care of it until today.

food for thought ..ede/duply.net

[1] https://bugs.launchpad.net/duplicity/+bug/687295

What's the corruption scenario that manifest comparison should protect against?

The manifest contains volume checksums, but those aren't checked against the volumes.
Eventually the remote manifest is just a copy of the local one. Multiple duplicity instances writing to the same remote should be ruled out as well.

The only thing that seems possible is an incomplete backup, but FWIW duplicitly only renames a local temp manifest and uploads it to the remote once the full backup is done.

On 24.04.2018 18:20, Martin Nowak wrote:
> What's the corruption scenario that manifest comparison should protect against?
>
> The manifest contains volume checksums, but those aren't checked against the volumes.

they should, at least during verify/restore

> Eventually the remote manifest is just a copy of the local one. Multiple duplicity instances writing to the same remote should be ruled out as well.

you never know. better safe than sorry.

>
> The only thing that seems possible is an incomplete backup, but FWIW duplicitly only renames a local temp manifest and uploads it to the remote once the full backup is done.
>

or some corruption in the backend or local file system or..

what stays is the question how a corrupt manifest would affect an incremental or resumed backup?
@Ken: do you want to chime in, as this smells like your area of expertise?

additionally of course, the question still remains if this check makes sense there at all. obviously someone at some point thought so in the past, but no harm in revalidating.

..ede/duply.net

The manifest check was in the original code I inherited. It's the only
file verified physically against the remote, the rest are hash based. That
verification has not caused many problems, and to my knowledge, has
detected very few comparison errors, so maybe it should just go away now.

As to hash checking on the remote, it's really not possible with the dumb
remotes we use. We only use list/del/read/write. Kinda minimal by design.

To answer @ede's question: The corruption would most likely be a
truncation, not a bit flip. As such, we would not have the hashes of the
diffdir volumes to compare against. I've never tried to run an incremental
or a restore with the check disabled and a known corrupted manifest. Could
be interesting to try.

On Wed, Apr 25, 2018 at 8:51 AM, edso <email address hidden> wrote:

> On 24.04.2018 18:20, Martin Nowak wrote:
> > What's the corruption scenario that manifest comparison should protect
> against?
> >
> > The manifest contains volume checksums, but those aren't checked against
> the volumes.
>
> they should, at least during verify/restore
>
> > Eventually the remote manifest is just a copy of the local one. Multiple
> duplicity instances writing to the same remote should be ruled out as well.
>
> you never know. better safe than sorry.
>
> >
> > The only thing that seems possible is an incomplete backup, but FWIW
> duplicitly only renames a local temp manifest and uploads it to the remote
> once the full backup is done.
> >
>
> or some corruption in the backend or local file system or..
>
> what stays is the question how a corrupt manifest would affect an
> incremental or resumed backup?
> @Ken: do you want to chime in, as this smells like your area of
> expertise?
>
> additionally of course, the question still remains if this check makes
> sense there at all. obviously someone at some point thought so in the past,
> but no harm in revalidating.
>
> ..ede/duply.net
>
> --
> https://code.launchpad.net/~dawgfoto/duplicity/fixup1252/+merge/343816
> You are subscribed to branch lp:duplicity.
>