Re: pgcrypto: PGP signatures

On 8/22/14, 2:57 AM, Thomas Munro wrote:
> I took a quick look at your patch at
> http://www.postgresql.org/message-id/53EDBCF0.9070205@joh.to (sorry I
> didn't reply directly as I didn't have the message). It applies
> cleanly, builds, and the tests pass. I will hopefully have more to
> say after I've poked at it and understood it better, but in the
> meantime a couple of trivial things I spotted:

Thanks!

> In pgp-pgsql.c, function pgp_sym_decrypt_verify_bytea:
>
> I think the first 'arg' should be 'psw'.
>
> I think the same mistake appears in pgp_sym_decrypt_verify_text.

Yeah, these look like copypaste-os. Will fix.

> Should t be of type pg_time_t rather than time_t? Would it be better
> if PGP_Signature's member creation_time were of type uint32_t and you
> could use ntohl(sig->creation_time) instead of the bitshifting?

I tried to make the code look like the existing code in
init_litdata_packet(). I don't oppose to writing it this way, but I
think we should change both instances if so (or perhaps move the code
into a new function).

> In pgp.h:
>
> +#define PGP_MAX_KEY (256/8)
> +#define PGP_MAX_BLOCK (256/8)
> +#define PGP_MAX_DIGEST (512/8)
> +#define PGP_MAX_DIGEST_ASN1_PREFIX 20
> +#define PGP_S2K_SALT 8
>
> The PGP_MAX_DIGEST_ASN1_PREFIX line is new but the others just have
> whitespace changes, and I gather the done thing is not to reformat
> existing lines like that to distract from the changes in a patch.

Right. Sorry about that. I can revert the whitespace fixes.

> (Just curious, why do you use while (1) for an infinite loop in
> extract_signatures, but for (;;) in pullf_discard? It looks like the
> latter is much more common in the source tree.)

In the postgres source tree? Yeah. But while(1) is all over pgcrypto,
so I've tried to make the new code match that. If there are any
instances of "for (;;)" in the patch, those must be because of me typing
without thinking. I could search-and-replace those to "while (1)" to
make it consistent.

.marko