Duplicity

Code review comment for lp:~mturquette/duplicity/duplicity

duplicity
Merge into 0.7-series

Revision history for this message

edso (ed.so) wrote on 2014-08-27:

On 26.08.2014 22:42, Mike Turquette wrote:
> On Tue, Aug 26, 2014 at 2:33 AM, edso <email address hidden> wrote:
>> On 26.08.2014 03:28, Mike Turquette wrote:
>>> Mike Turquette has proposed merging lp:~mturquette/duplicity/duplicity into lp:duplicity.
>>>
>>> Requested reviews:
>>> duplicity-team (duplicity-team)
>>>
>>> For more details, see:
>>> https://code.launchpad.net/~mturquette/duplicity/duplicity/+merge/232153
>>>
>>> I ran into a problem while using duplicity on my laptop, where I store my public gpg key, but not my private key. This is because my laptop may be stolen, lost or otherwise detained.
>>>
>>> I still want to encrypt backups, which I can do sufficiently with a public key and Duplicity does this just fine. But things fall apart when Duplicity tries to decrypt the first volume of the backup in validate_encryption_settings() and fails on my laptop due to the lack of private key.
>>>
>>> To handle this case I have introduced the --no-secret-key option, which was first proposed on the list in 2010:
>>> http://article.gmane.org/gmane.comp.sysutils.backup.duplicity.general/4299
>>
>>
>> if you read the thread more thorough you'll see that the switch was meant for a totally different point in the code. the sanity decode routine was not introduced at that time.
>>
>>>
>>> This option simply skips the encryption settings validation if set. The responsibility is on the user to periodically sanity check backups.
>>>
>>
>> it also seems to disable the "was backup (un)encrypted before and is still check" in line 330
>>
>> so i see these problems:
>>
>> - collections.py must be patched as well, as drafted in the ml thread above
>> - the manpage bin/duplicity.1 has to be updated with a proper explanation of the parameter and it's dangers
>> - only disable the decryption trial in validate_encryption_settings()
>
> Edgar,
>
> Thanks for reviewing and for providing more context and pointers.
>
>>
>> make sure that you deliver a patch for lp:duplicity , which is the new current developing branch for an upcoming duplicity 0.7
>
> My itch is scratched so I likely won't be revisiting this code any
> time soon, if ever. However if I do decide to refresh this fix (e.g.
> after my distro pulls a new, unpatched version of duplicity and I hit
> this bug agin) then I will address your points and resubmit.
>

up to you. hope you will as dev time on duplicity is rather sparse these days. ..ede/duply.net

On 26.08.2014 22:42, Mike Turquette wrote:
> On Tue, Aug 26, 2014 at 2:33 AM, edso <edgar.soldin@web.de> wrote:
>> On 26.08.2014 03:28, Mike Turquette wrote:
>>> Mike Turquette has proposed merging lp:~mturquette/duplicity/duplicity into lp:duplicity.
>>>
>>> Requested reviews:
>>>   duplicity-team (duplicity-team)
>>>
>>> For more details, see:
>>> https://code.launchpad.net/~mturquette/duplicity/duplicity/+merge/232153
>>>
>>> I ran into a problem while using duplicity on my laptop, where I store my public gpg key, but not my private key. This is because my laptop may be stolen, lost or otherwise detained.
>>>
>>> I still want to encrypt backups, which I can do sufficiently with a public key and Duplicity does this just fine. But things fall apart when Duplicity tries to decrypt the first volume of the backup in validate_encryption_settings() and fails on my laptop due to the lack of private key.
>>>
>>> To handle this case I have introduced the --no-secret-key option, which was first proposed on the list in 2010:
>>> http://article.gmane.org/gmane.comp.sysutils.backup.duplicity.general/4299
>>
>>
>> if you read the thread more thorough you'll see that the switch was meant for a totally different point in the code. the sanity decode routine was not introduced at that time.
>>
>>>
>>> This option simply skips the encryption settings validation if set. The responsibility is on the user to periodically sanity check backups.
>>>
>>
>> it also seems to disable the "was backup (un)encrypted before and is still check" in line 330
>>
>> so i see these problems:
>>
>> - collections.py must be patched as well, as drafted in the ml thread above
>> - the manpage bin/duplicity.1 has to be updated with a proper explanation of the parameter and it's dangers
>> - only disable the decryption trial in validate_encryption_settings()
> 
> Edgar,
> 
> Thanks for reviewing and for providing more context and pointers.
> 
>>
>> make sure that you deliver a patch for lp:duplicity , which is the new current developing branch for an upcoming duplicity 0.7
> 
> My itch is scratched so I likely won't be revisiting this code any
> time soon, if ever. However if I do decide to refresh this fix (e.g.
> after my distro pulls a new, unpatched version of duplicity and I hit
> this bug agin) then I will address your points and resubmit.
>

up to you. hope you will as dev time on duplicity is rather sparse these days. ..ede/duply.net

« Back to merge proposal