Re: backup manifests

Hi,

On 2020-03-23 12:15:54 -0400, Robert Haas wrote:
> +       <varlistentry>
> +        <term><literal>MANIFEST</literal></term>
> +        <listitem>
> +         <para>
> +          When this option is specified with a value of <literal>ye'</literal>

s/ye'/yes/

> +          or <literal>force-escape</literal>, a backup manifest is created
> +          and sent along with the backup. The latter value forces all filenames
> +          to be hex-encoded; otherwise, this type of encoding is performed only
> +          for files whose names are non-UTF8 octet sequences.
> +          <literal>force-escape</literal> is intended primarily for testing
> +          purposes, to be sure that clients which read the backup manifest
> +          can handle this case. For compatibility with previous releases,
> +          the default is <literal>MANIFEST 'no'</literal>.
> +         </para>
> +        </listitem>
> +       </varlistentry>

Are you planning to include a specification of the manifest file format
anywhere? I looked through the patches and didn't find anything.

I think it'd also be good to include more information about what the
point of manifest files actually is.


> +  <para>
> +   <application>pg_validatebackup</application> reads the manifest file of a
> +   backup, verifies the manifest against its own internal checksum, and then
> +   verifies that the same files are present in the target directory as in the
> +   manifest itself. It then verifies that each file has the expected checksum,
> +   unless the backup was taken the checksum algorithm set to
> +   <literal>none</literal>, in which case checksum verification is not
> +   performed. The presence or absence of directories is not checked, except
> +   indirectly: if a directory is missing, any files it should have contained
> +   will necessarily also be missing. Certain files and directories are
> +   excluded from verification:
> +  </para>

Depending on what you want to use the manifest for, we'd also need to
check that there are no additional files. That seems to actually be
implemented, which imo should be mentioned here.




> +/*
> + * Finalize the backup manifest, and send it to the client.
> + */
> +static void
> +SendBackupManifest(manifest_info *manifest)
> +{
> +	StringInfoData protobuf;
> +	uint8		checksumbuf[PG_SHA256_DIGEST_LENGTH];
> +	char		checksumstringbuf[PG_SHA256_DIGEST_STRING_LENGTH];
> +	size_t		manifest_bytes_done = 0;
> +
> +	/*
> +	 * If there is no buffile, then the user doesn't want a manifest, so
> +	 * don't waste any time generating one.
> +	 */
> +	if (manifest->buffile == NULL)
> +		return;
> +
> +	/* Terminate the list of files. */
> +	AppendStringToManifest(manifest, "],\n");
> +
> +	/*
> +	 * Append manifest checksum, so that the problems with the manifest itself
> +	 * can be detected.
> +	 *
> +	 * We always use SHA-256 for this, regardless of what algorithm is chosen
> +	 * for checksumming the files.  If we ever want to make the checksum
> +	 * algorithm used for the manifest file variable, the client will need a
> +	 * way to figure out which algorithm to use as close to the beginning of
> +	 * the manifest file as possible, to avoid having to read the whole thing
> +	 * twice.
> +	 */
> +	manifest->still_checksumming = false;
> +	pg_sha256_final(&manifest->manifest_ctx, checksumbuf);
> +	AppendStringToManifest(manifest, "\"Manifest-Checksum\": \"");
> +	hex_encode((char *) checksumbuf, sizeof checksumbuf, checksumstringbuf);
> +	checksumstringbuf[PG_SHA256_DIGEST_STRING_LENGTH - 1] = '\0';
> +	AppendStringToManifest(manifest, checksumstringbuf);
> +	AppendStringToManifest(manifest, "\"}\n");

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.



> diff --git a/src/bin/pg_validatebackup/parse_manifest.c b/src/bin/pg_validatebackup/parse_manifest.c
> new file mode 100644
> index 0000000000..e6b42adfda
> --- /dev/null
> +++ b/src/bin/pg_validatebackup/parse_manifest.c
> @@ -0,0 +1,576 @@
> +/*-------------------------------------------------------------------------
> + *
> + * parse_manifest.c
> + *	  Parse a backup manifest in JSON format.
> + *
> + * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
> + * Portions Copyright (c) 1994, Regents of the University of California
> + *
> + * src/bin/pg_validatebackup/parse_manifest.c
> + *
> + *-------------------------------------------------------------------------
> + */

Doesn't have to be in the first version, but could it be useful to move
this to common/ or such?



> +/*
> + * Validate one directory.
> + *
> + * 'relpath' is NULL if we are to validate the top-level backup directory,
> + * and otherwise the relative path to the directory that is to be validated.
> + *
> + * 'fullpath' is the backup directory with 'relpath' appended; i.e. the actual
> + * filesystem path at which it can be found.
> + */
> +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?)?


> +/*
> + * Validate the checksum of a single file.
> + */
> +static void
> +validate_file_checksum(validator_context *context, manifestfile *tabent,
> +					   char *fullpath)
> +{
> +	pg_checksum_context checksum_ctx;
> +	char	   *relpath = tabent->pathname;
> +	int			fd;
> +	int			rc;
> +	uint8		buffer[READ_CHUNK_SIZE];
> +	uint8		checksumbuf[PG_CHECKSUM_MAX_LENGTH];
> +	int			checksumlen;
> +
> +	/* Open the target file. */
> +	if ((fd = open(fullpath, O_RDONLY | PG_BINARY, 0)) < 0)
> +	{
> +		report_backup_error(context, "could not open file \"%s\": %m",
> +						   relpath);
> +		return;
> +	}
> +
> +	/* Initialize checksum context. */
> +	pg_checksum_init(&checksum_ctx, tabent->checksum_type);
> +
> +	/* Read the file chunk by chunk, updating the checksum as we go. */
> +	while ((rc = read(fd, buffer, READ_CHUNK_SIZE)) > 0)
> +		pg_checksum_update(&checksum_ctx, buffer, rc);
> +	if (rc < 0)
> +		report_backup_error(context, "could not read file \"%s\": %m",
> +						   relpath);
> +

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.



Greetings,

Andres Freund