Re: backup manifests - Mailing list pgsql-hackers

Hi,

On 2020-03-27 15:20:27 -0400, Robert Haas wrote:
> On Fri, Mar 27, 2020 at 2:29 AM Andres Freund <andres@anarazel.de> wrote:
> > Are you planning to include a specification of the manifest file format
> > anywhere? I looked through the patches and didn't find anything.
> 
> I thought about that. I think it would be good to have. I was sort of
> hoping to leave it for a follow-on patch, but maybe that's cheating
> too much.

I don't like having a file format that's intended to be used by external
tools too that's undocumented except for code that assembles it in a
piecemeal fashion.  Do you mean in a follow-on patch this release, or
later? I don't have a problem with the former.


> > I think it'd also be good to include more information about what the
> > point of manifest files actually is.
> 
> What kind of information do you want to see included there? Basically,
> the way the documentation is written right now, it essentially says,
> well, we have this manifest thing so that you can later run
> pg_validatebackup, and pg_validatebackup says that it's there to check
> the integrity of backups using the manifest. This is all a bit
> circular, though, and maybe needs elaboration.

I do found it to be circular. I think we mostly need a paragraph or two
somewhere that explains on a higher level what the point of verifying
base backups is and what is verified.


> > Hm. Is it a great choice to include the checksum for the manifest inside
> > the manifest itself? With a cryptographic checksum it seems like it
> > could make a ton of sense to store the checksum somewhere "safe", but
> > keep the manifest itself alongside the base backup itself. While not
> > huge, they won't be tiny either.
> 
> Seems like the user could just copy the manifest checksum and store it
> somewhere, if they wish. Then they can check it against the manifest
> itself later, if they wish. Or they can take a SHA-512 of the whole
> file and store that securely. The problem is that we have no idea how
> to write that checksum to a more security storage. We could write
> backup_manifest and backup_manifest.checksum into separate files, but
> that seems like it's adding complexity without any real benefit.
> 
> To me, the security-related uses of this patch seem to be fairly
> niche. I think it's nice that they exist, but I don't think that's the
> main selling point. For me, the main selling point is that you can
> check that your disk didn't eat your data and that nobody nuked any
> files that were supposed to be there.

Oh, I agree. I wasn't really mentioning the crypto checksum because of
it being "security" stuff, but because of the quality of the guarantee
it gives. I don't know how large the manifest file will be for a setup
of with a lot of partitioned tables, but I'd expect it to not be
tiny. So not having to store it in the 'archiving sytem' is nice.

FWIW, I was thinking of backup_manifest.checksum potentially being
desirable for another reason: The need to embed the checksum inside the
document imo adds a fair bit of rigidity to the file format. See

> +static void
> +verify_manifest_checksum(JsonManifestParseState *parse, char *buffer,
> +                         size_t size)
> +{
...
> +
> +    /* Find the last two newlines in the file. */
> +    for (i = 0; i < size; ++i)
> +    {
> +        if (buffer[i] == '\n')
> +        {
> +            ++number_of_newlines;
> +            penultimate_newline = ultimate_newline;
> +            ultimate_newline = i;
> +        }
> +    }
> +
> +    /*
> +     * Make sure that the last newline is right at the end, and that there are
> +     * at least two lines total. We need this to be true in order for the
> +     * following code, which computes the manifest checksum, to work properly.
> +     */
> +    if (number_of_newlines < 2)
> +        json_manifest_parse_failure(parse->context,
> +                                    "expected at least 2 lines");
> +    if (ultimate_newline != size - 1)
> +        json_manifest_parse_failure(parse->context,
> +                                    "last line not newline-terminated");
> +
> +    /* Checksum the rest. */
> +    pg_sha256_init(&manifest_ctx);
> +    pg_sha256_update(&manifest_ctx, (uint8 *) buffer, penultimate_newline + 1);
> +    pg_sha256_final(&manifest_ctx, manifest_checksum_actual);

which certainly isn't "free form json".


> > Doesn't have to be in the first version, but could it be useful to move
> > this to common/ or such?
> 
> Yeah. At one point, this code was written in a way that was totally
> specific to pg_validatebackup, but I then realized that it would be
> better to make it more general, so I refactored it into in the form
> you see now, where pg_validatebackup.c depends on parse_manifest.c but
> not the reverse. I suspect that if someone wants to use this for
> something else they might need to change a few more things - not sure
> exactly what - but I don't think it would be too hard. I thought it
> would be best to leave that task until someone has a concrete use case
> in mind, but I did want it to to be relatively easy to do that down
> the road, and I hope that the way I've organized the code achieves
> that.

Cool.


> > > +static void
> > > +validate_backup_directory(validator_context *context, char *relpath,
> > > +                                               char *fullpath)
> > > +{
> >
> > Hm. Should this warn if the directory's permissions are set too openly
> > (world writable?)?
> 
> I don't think so, but it's pretty clear that different people have
> different ideas about what the scope of this tool ought to be, even in
> this first version.

Yea. I don't have a strong opinion on this specific issue. I was mostly
wondering because I've repeatedly seen people restore backups with world
readable properties, and with that it's obviously possible for somebody
else to change the contents after the checksum was computed.


> > Hm. I think it'd be good to verify that the checksummed size is the same
> > as the size of the file in the manifest.
> 
> That's checked in an earlier phase. Are you worried about the file
> being modified after the first pass checks the size and before we come
> through to do the checksumming?

Not really, I wondered about it for a bit, and then decided that it's
too remote an issue.

What I've seen a couple of times is that actually reading a file can
result in the file ending to be reported at a different position than
what stat() said. So by crosschecking the size while reading with the
one from stat (which was compared with the source system one) we'd make
the errors much better. It's certainly easier to know where to start
looking when validate says "error: read %llu bytes from file, expected
%llu" or something along those lines, than when it just were to report a
checksum error.

There's also some crypto hash algorithm weaknesses that are easier to
exploit when it's possible to append data to a known prefix, but that
doesn't seem an obvious threat here.


Greetings,

Andres Freund