Re: scratch/package-security bcde5f8 2/2: Support expiration of metadata by package archives

Re: scratch/package-security bcde5f8 2/2: Support expiration of metadata by package archives

[Stefan Monnier]

> @@ -449,6 +458,7 @@ synchronously."
>  (define-error 'bad-size "Package size mismatch" 'package-error)
>  (define-error 'bad-signature "Failed to verify signature" 'package-error)
>  (define-error 'bad-checksum "Failed to verify checksum" 'package-error)
> +(define-error 'bad-timestamp "Failed to verify timestamp" 'package-error)

Hmm, these errors should all have a `package-` prefix.

>  \f
>  ;;; `package-desc' object definition
> @@ -1812,6 +1822,100 @@ Once it's empty, run `package--post-download-archives-hook'."
>      (message "Package refresh done")
>      (run-hooks 'package--post-download-archives-hook)))
>  
> +(defun package--parse-header-from-buffer (header name)
> +  "Find and return \"archive-contents\" HEADER for archive NAME.
> +This function assumes that the current buffer contains the
> +\"archive-contents\" file.
> +
> +A valid header looks like: \";; HEADER: <TIMESTAMP>\"
> +
> +Where <TIMESTAMP> is a valid ISO-8601 (RFC 3339) date.  If there
> +is such a line but <TIMESTAMP> is invalid, show a warning and
> +return nil.  If there is no valid header, return nil."
> +  (save-excursion
> +    (goto-char (point-min))
> +    (when (re-search-forward (concat "^;; " header ": *\\(.+?\\) *$") nil t)
> +      (condition-case-unless-debug nil
> +          (encode-time (iso8601-parse (match-string 1)))
> +        (lwarn '(package timestamp)
> +               (list (format "Malformed timestamp for archive `%s': `%s'"
> +                             name (match-string 1))))))))

Hmm... I think you forgot the `error` in this
`condition-case-unless-debug` (i.e. the way you wrote it, it will catch
all `lwarn` errors).

> +(defun package--parse-valid-until-from-buffer (name)
> +  "Find and return \"Valid-Until\" header for archive NAME."
> +  (package--parse-header-from-buffer "Valid-Until" name))

It would be easier for the ELPA archives is to use a "validity duration"
header, since it could then be constant.

Other that that, LGTM,

[ BTW, to mitigate against replay attacks in the absence of timestamps,
  we could compare the last `archive-content` with the new one and make
  sure that none of the packages's version got smaller.  ]


        Stefan

Re: scratch/package-security bcde5f8 2/2: Support expiration of metadata by package archives

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> @@ -449,6 +458,7 @@ synchronously."
>>  (define-error 'bad-size "Package size mismatch" 'package-error)
>>  (define-error 'bad-signature "Failed to verify signature" 'package-error)
>>  (define-error 'bad-checksum "Failed to verify checksum" 'package-error)
>> +(define-error 'bad-timestamp "Failed to verify timestamp" 'package-error)
>
> Hmm, these errors should all have a `package-` prefix.

Agreed.  But I was worried that changing it would break some third-party
packages.  Do we have a way to work around that?  Or do you think this
is not something we need to worry all that much about?

[...]
> Hmm... I think you forgot the `error` in this
> `condition-case-unless-debug` (i.e. the way you wrote it, it will catch
> all `lwarn` errors).

Indeed, thanks.  I'll fix it.

>> +(defun package--parse-valid-until-from-buffer (name)
>> +  "Find and return \"Valid-Until\" header for archive NAME."
>> +  (package--parse-header-from-buffer "Valid-Until" name))
>
> It would be easier for the ELPA archives is to use a "validity duration"
> header, since it could then be constant.

FWIW, I feel like the current way is more human readable: I immediately
know the exact time and date when it will expire.

Also, we don't need to have a number like "7" where it is not
immediately clear if it means hours, weeks or days, and we don't need to
write a parser for "7 days", "1 week", etc. but can just reuse existing
well-tested parsers.

But of course you can just reply that you would rather just immediately
know how long it is until these files normally expire without having to
calculate it.  So I guess we can go around in circles about this.  Hmm.

( BTW, the name and semantics of this field is based on APT:
  https://wiki.debian.org/DebianRepository/Format#Date.2C_Valid-Until )

> Other that that, LGTM,

Thanks for reviewing.

Re: scratch/package-security bcde5f8 2/2: Support expiration of metadata by package archives

[Stefan Monnier]

>>> @@ -449,6 +458,7 @@ synchronously."
>>>  (define-error 'bad-size "Package size mismatch" 'package-error)
>>>  (define-error 'bad-signature "Failed to verify signature" 'package-error)
>>>  (define-error 'bad-checksum "Failed to verify checksum" 'package-error)
>>> +(define-error 'bad-timestamp "Failed to verify timestamp" 'package-error)
>> Hmm, these errors should all have a `package-` prefix.
> Agreed.  But I was worried that changing it would break some third-party
> packages.

I thought you just preferred to "go with the flow", which would make a
lot of sense (i.e. keep the renaming for a separate patch).

> Do we have a way to work around that?

Not really, no.  You could do

    (define-error 'bad-timestamp "Failed to verify timestamp" 'package-error)
    (define-error 'package-bad-timestamp "Failed to verify timestamp" 'bad-timestamp)

so packages which catch `bad-timestamp` will also catch
`package-bad-timestamp`, but then when package.el will presumably fail
to catch the `bad-timestamp` raised by other packages.  And we don't
have a mechanism in place to mark an error like `bad-timestamp` as obsolete.

> Or do you think this is not something we need to worry all that
> much about?

I wouldn't worry about it, indeed.

>> It would be easier for the ELPA archives is to use a "validity duration"
>> header, since it could then be constant.
> FWIW, I feel like the current way is more human readable: I immediately
> know the exact time and date when it will expire.

That's fairly secondary to me.

> Also, we don't need to have a number like "7" where it is not
> immediately clear if it means hours, weeks or days, and we don't need to
> write a parser for "7 days", "1 week", etc. but can just reuse existing
> well-tested parsers.

Ah... I assumed we could reuse existing code for that.  If not, then
it's definitely not worth the trouble.


        Stefan