pgcrypto: PGP signatures

[Marko Tiikkaja]

Hi hackers,

Attached is a patch to add support for PGP signatures in encrypted
messages into pgcrypto.

Currently, the list of limitations is the following:

- It only knows how to generate one signature per message. I don't
see that as a problem.
- If a message has been signed with multiple keys which have the
same keyid as the one specified to verify the message, an error is
returned. Naively, it seems that we should try all of them and return
"OK" if even one of them matches, but that seems icky.
- Only RSA signatures are supported. It wouldn't be too hard for
someone familiar with DSA to add it in, but I'm not volunteering to do
it. Personally I think supporting RSA is better than no support at all.

As per usual, I'll also add this to the upcoming commitfest. Any
feedback appreciated before that, of course.


.marko

[Marko Tiikkaja]

Post by Marko Tiikkaja
Attached is a patch to add support for PGP signatures in encrypted
messages into pgcrypto.

Here's v2 of the patch. I've changed the info-extracting code to not
look for signatures beyond the data, which also meant that it had to
parse one-pass signatures (which it didn't do before). This matches the
behaviour of the main decryption code.


.marko

[Marko Tiikkaja]

Hi,

Post by Marko Tiikkaja
Here's v2 of the patch. I've changed the info-extracting code to not
look for signatures beyond the data, which also meant that it had to
parse one-pass signatures (which it didn't do before). This matches the
behaviour of the main decryption code.

Here's the latest version where I've added the option to extract the
creation time from the signatures.



.marko

[Jeff Janes]

Post by Marko Tiikkaja
Hi,

Post by Marko Tiikkaja
Here's v2 of the patch. I've changed the info-extracting code to not
look for signatures beyond the data, which also meant that it had to
parse one-pass signatures (which it didn't do before). This matches the
behaviour of the main decryption code.

Here's the latest version where I've added the option to extract the
creation time from the signatures.

There is trivial sgml patch application conflict due to a grammar
correction in 05258761bf12a64befc9caec1947b254cdeb74c5

I wanted to start simple so I have a file which is signed, but not
encrypted. I can't figure out what to do with it. All of the functions
seem to require that it also be encrypted. I tried providing an empty
password for pgp_sym_signatures but it didn't work.

Is there a way to deal with this situation?

Thanks

Jeff

[Marko Tiikkaja]

Post by Jeff Janes
I wanted to start simple so I have a file which is signed, but not
encrypted. I can't figure out what to do with it. All of the functions
seem to require that it also be encrypted. I tried providing an empty
password for pgp_sym_signatures but it didn't work.

Right. This patch only adds support for signing data when encrypting it
at the same time. There's no support for detached signatures, nor is
there support for anything other than signatures of encrypted data. I
should have been more clear on that in my initial email. :-(


.marko

[Jeff Janes]

Post by Marko Tiikkaja

Post by Jeff Janes
I wanted to start simple so I have a file which is signed, but not
encrypted. I can't figure out what to do with it. All of the functions
seem to require that it also be encrypted. I tried providing an empty
password for pgp_sym_signatures but it didn't work.

Right. This patch only adds support for signing data when encrypting it
at the same time. There's no support for detached signatures, nor is there
support for anything other than signatures of encrypted data. I should
have been more clear on that in my initial email. :-(

OK, thanks. How hard do you think it would to allow NULL (or empty
string?) passwords to gpg_sym_signatures and gpg_sym_decrypt_verify to
accommodate this?

I think docs section F.25.3 needs to be re-titled and expanded to reflect
signatures as well as encryption, and an explanation added about signatures
only being processed on encrypted data if that restriction can't be removed.

I've switched to using a signed plus symmetrically encrypted message for
testing.

One surprising thing so far is that the 3rd argument to
gpg_sym_decrypt_verify must be dearmored. I thought it would detect and
dearmor automatically.

Once I wrap it in dearmor, I get the ERROR: No signature matching the key
id present in the message

The public key block I am giving it is for the keyid that is reported
by pgp_sym_signatures, so I don't know what the problem might be.

When I get more time, I'll look at your examples from the regression tests
to see if I can figure it out.

Thanks,

Jeff

[Marko Tiikkaja]

Post by Jeff Janes

Post by Marko Tiikkaja
Right. This patch only adds support for signing data when encrypting it
at the same time. There's no support for detached signatures, nor is there
support for anything other than signatures of encrypted data. I should
have been more clear on that in my initial email. :-(

OK, thanks. How hard do you think it would to allow NULL (or empty
string?) passwords to gpg_sym_signatures and gpg_sym_decrypt_verify to
accommodate this?

To sign without encrypting? I think those should really be a different
set of functions altogether. But this patch is already humongous (on my
standards, at least), so I think that should be done separately.

Post by Jeff Janes
I think docs section F.25.3 needs to be re-titled and expanded to reflect
signatures as well as encryption, and an explanation added about signatures
only being processed on encrypted data if that restriction can't be removed.

I don't have an objection to that. I fully acknowledge that the
documentation doesn't state the limitations of signing should this patch
be applied.

Post by Jeff Janes
I've switched to using a signed plus symmetrically encrypted message for
testing.
One surprising thing so far is that the 3rd argument to
gpg_sym_decrypt_verify must be dearmored. I thought it would detect and
dearmor automatically.

I can't see that as an improvement to be honest.

Post by Jeff Janes
Once I wrap it in dearmor, I get the ERROR: No signature matching the key
id present in the message
The public key block I am giving it is for the keyid that is reported
by pgp_sym_signatures, so I don't know what the problem might be.

Have you tried with the debug=1 option? (It's undocumented, but it was
like that before this patch and I didn't touch it).

Post by Jeff Janes
When I get more time, I'll look at your examples from the regression tests
to see if I can figure it out.

Thanks! I'm happy to help if you run into any trouble!


.marko

[Jeff Janes]

Post by Marko Tiikkaja

Post by Jeff Janes

Post by Marko Tiikkaja
Right. This patch only adds support for signing data when encrypting it
at the same time. There's no support for detached signatures, nor is there
support for anything other than signatures of encrypted data. I should
have been more clear on that in my initial email. :-(
OK, thanks. How hard do you think it would to allow NULL (or empty

string?) passwords to gpg_sym_signatures and gpg_sym_decrypt_verify to
accommodate this?

To sign without encrypting?

To verify signatures of things that are not encrypted. I'm not really
interested in storing private keys in PostgreSQL, just things that can be
done with public keys. (But I will make a dummy private key for testing if
I get that far.)

...

Post by Marko Tiikkaja
Once I wrap it in dearmor, I get the ERROR: No signature matching the key

Post by Jeff Janes
id present in the message
The public key block I am giving it is for the keyid that is reported
by pgp_sym_signatures, so I don't know what the problem might be.

Have you tried with the debug=1 option? (It's undocumented, but it was
like that before this patch and I didn't touch it).

I have now, but it didn't produce any output for this situation. I have
two theories for the problem. My test signed message was signed with a
keyring that had a signing subkey, so it was signed with that, not with the
master. Maybe it doesn't like that. Also, I created the signed message in
gpg, then imported it to PostgreSQL, and maybe it doesn't like that.

I've never used the pgp functions of pgcrypto before, so I decided to take
a step back and try some of the functions that predate the proposed patch.
And I can't get them to work well, either.

If I use pgp_sym_encrypt to encrypt a message with AES, then
pgp_sym_decrypt will decrypt, and so will gpg command line tool. But if I
use gpg to encrypt a message, pgp_sym_decrypt will not decrypt it.


select pgp_sym_decrypt(dearmor('-----BEGIN PGP MESSAGE-----
Version: GnuPG v2.0.14 (GNU/Linux)
Password: foobar

jA0EBwMCqywsAv/hXJ7D0j8BWsD+9H7DY4KhrIIw2oV/6tBueVQ28+VDjBw9rGiy
3JRPmyXNN4wRTZXIyTVzK3LylWLomD9pQkao4hrQwSs=
=02RI
-----END PGP MESSAGE-----
'),'foobar','debug=1');
NOTICE: dbg: parse_literal_data: data type=b
ERROR: Not text data


So I don't know if I am doing something wrong, or if the PostgreSQL
implementation of pgp is just not interoperable with other implementations.
That makes it hard to test the new features if I can't make the old ones
work.


The two messages I am working with are:


Created: echo -n 'a message'|gpg -c --armor --cipher-algo AES -
-----BEGIN PGP MESSAGE-----
Version: GnuPG v2.0.14 (GNU/Linux)
Password: foobar

jA0EBwMCqywsAv/hXJ7D0j8BWsD+9H7DY4KhrIIw2oV/6tBueVQ28+VDjBw9rGiy
3JRPmyXNN4wRTZXIyTVzK3LylWLomD9pQkao4hrQwSs=
=02RI
-----END PGP MESSAGE-----



and


Created: select armor(pgp_sym_encrypt('a message','foobar'));
-----BEGIN PGP MESSAGE-----

ww0EBwMCYzgp4dU3zCJ30joBViH28prwc9jIHhzUyXt31omiHao7NeOuLhCR0/uhAB6GRfYAXWVa
x+FTsW27F46/W7dlRjxCuzcu
=jQGZ
-----END PGP MESSAGE-----


Cheers,

Jeff

[Marko Tiikkaja]

Post by Jeff Janes

Post by Marko Tiikkaja
To sign without encrypting?

To verify signatures of things that are not encrypted. I'm not really
interested in storing private keys in PostgreSQL, just things that can be
done with public keys. (But I will make a dummy private key for testing if
I get that far.)

Right. That functionality might be useful, but I think it should be a
separate patch completely. (And I doubt I have any interest in
implementing it).

Post by Jeff Janes

Post by Marko Tiikkaja
Once I wrap it in dearmor, I get the ERROR: No signature matching the key

Post by Jeff Janes
id present in the message
The public key block I am giving it is for the keyid that is reported
by pgp_sym_signatures, so I don't know what the problem might be.

Have you tried with the debug=1 option? (It's undocumented, but it was
like that before this patch and I didn't touch it).

I have now, but it didn't produce any output for this situation. I have
two theories for the problem. My test signed message was signed with a
keyring that had a signing subkey, so it was signed with that, not with the
master. Maybe it doesn't like that.

Yeah, this patch only supports signing and verifying signatures with
main keys.

Post by Jeff Janes
Also, I created the signed message in
gpg, then imported it to PostgreSQL, and maybe it doesn't like that.

That should not be a problem. I used gpg extensively when testing the
patch.

Post by Jeff Janes
I've never used the pgp functions of pgcrypto before, so I decided to take
a step back and try some of the functions that predate the proposed patch.
And I can't get them to work well, either.
If I use pgp_sym_encrypt to encrypt a message with AES, then
pgp_sym_decrypt will decrypt, and so will gpg command line tool. But if I
use gpg to encrypt a message, pgp_sym_decrypt will not decrypt it.
select pgp_sym_decrypt(dearmor('-----BEGIN PGP MESSAGE-----
Version: GnuPG v2.0.14 (GNU/Linux)
Password: foobar
jA0EBwMCqywsAv/hXJ7D0j8BWsD+9H7DY4KhrIIw2oV/6tBueVQ28+VDjBw9rGiy
3JRPmyXNN4wRTZXIyTVzK3LylWLomD9pQkao4hrQwSs=
=02RI
-----END PGP MESSAGE-----
'),'foobar','debug=1');
NOTICE: dbg: parse_literal_data: data type=b
ERROR: Not text data
So I don't know if I am doing something wrong, or if the PostgreSQL
implementation of pgp is just not interoperable with other implementations.
That makes it hard to test the new features if I can't make the old ones
work.

The NOTICE here says what's wrong: the message has been marked to
contain binary data, not text. You should be able to decrypt it with
pgp_sym_decrypt_bytea() (and you can use convert_from() to get a text
value out).



.marko

[Jeff Janes]

Post by Jeff Janes
select pgp_sym_decrypt(dearmor('-----BEGIN PGP MESSAGE-----
Version: GnuPG v2.0.14 (GNU/Linux)
Password: foobar
jA0EBwMCqywsAv/hXJ7D0j8BWsD+9H7DY4KhrIIw2oV/6tBueVQ28+VDjBw9rGiy
3JRPmyXNN4wRTZXIyTVzK3LylWLomD9pQkao4hrQwSs=
=02RI
-----END PGP MESSAGE-----
'),'foobar','debug=1');
NOTICE: dbg: parse_literal_data: data type=b
ERROR: Not text data
So I don't know if I am doing something wrong, or if the PostgreSQL
implementation of pgp is just not interoperable with other
implementations.
That makes it hard to test the new features if I can't make the old ones
work.

The NOTICE here says what's wrong: the message has been marked to contain
binary data, not text. You should be able to decrypt it with
pgp_sym_decrypt_bytea() (and you can use convert_from() to get a text value
out).

OK, thanks. That is obvious in retrospect. I'll put it on my todo list to
try to clean up some of documentation and error messages to make it more
obvious to the naive user, but that is not part of this patch.

One problem I've run into now is that if I try to sign a message
with pgp_pub_encrypt_sign but give it the public, not private, key as the
3rd argument, it generates this message:

ERROR: Cannot decrypt with public key

Should be 'sign', not 'decrypt'.

Similarly for verification:

ERROR: Refusing to encrypt with secret key

'encrypt' should be 'verify signature'.

Cheers,

Jeff

[Joel Jacobson]

Marko, et al,

This is a review of the pgcrypto PGP signatures patch:
http://www.postgresql.org/message-id/***@joh.to

There hasn't been any discussion, at least that I've been able to find.

Contents & Purpose
==================
This patch add functions to create, verify and extract infromation
from OpenPGP signatures. Previously pgcrypto only peformed
PGP encrypt/decrypt, not sign/verify. This is a painful limitation
since a very common use-case for OpenPGP is the signature-part,
where two parties want to verify messages originate from each other,
and not only encrypt the messages.

Included in the patch are updated regression test cases and documentation.

Initial Run
===========
The patch applies cleanly to HEAD after changing a single line in the patch:
< ! Giving this function a secret key will produce an error.
---

! Giving this function a secret key will produce a error.

This grammar fix was already fixed in 05258761bf12a64befc9caec1947b254cdeb74c5,
and therefore caused the conflict.

The 144 regression tests all pass successfully against the new patch.

Conclusion
==========
Since I'm using these functions in the BankAPI project,
https://github.com/trustly/bankapi, I have tested them
by actually using them in production, in addition to the provided
regression tests, which is a good sign they are working not just
in theory.

+1 for committer review after the changes suggested by Jeff Janes and
Thomas Munro.

Hi,

Post by Marko Tiikkaja
Here's v2 of the patch. I've changed the info-extracting code to not
look for signatures beyond the data, which also meant that it had to
parse one-pass signatures (which it didn't do before). This matches the
behaviour of the main decryption code.

Here's the latest version where I've added the option to extract the
creation time from the signatures.
.marko
--
http://www.postgresql.org/mailpref/pgsql-hackers

[Marko Tiikkaja]

Hi all,

I've updated the patch with a number of changes:
1) I've documented the current limitations of signatures
2) I've expanded section F.25.3 to add information about signatures
(though I'm not sure why this part is in the user-facing documentation
in the first place).
3) I've changed the code to use ntohl() and pg_time_t as per Thomas'
comments.
4) I've changed the code to consistently use "while (1)" instead of
"for (;;)" (except for the math library, but I didn't touch that at all)

I've also changed the behaviour when passing a message with a signature
to the decrypt functions which don't verify signatures. They now report
"ERROR: Wrong key or corrupt data" instead of decrypting and silently
ignoring the signature. The behaviour is now backwards compatible, but
I see two ways we could possibly possibly improve this:
1) Produce a better error message (I'm sure most people don't know
about the hidden debug=1 setting)
2) Provide an option to ignore the signature if decrypting the data
is desirable even if the signature can't be verified

Any thoughts, comments appreciated.


.marko

[Marko Tiikkaja]

Post by Marko Tiikkaja
3) I've changed the code to use ntohl() and pg_time_t as per Thomas'
comments.
sig->creation_time = ntohl(*((uint32_t *) creation_time));

This is probably a horrible idea due to strict aliasing rules and
alignment, though. I think I'll just hide the bit shifts behind a
function instead.



.marko

[Jeff Janes]

Post by Marko Tiikkaja
Hi all,
1) I've documented the current limitations of signatures
2) I've expanded section F.25.3 to add information about signatures
(though I'm not sure why this part is in the user-facing documentation in
the first place).
3) I've changed the code to use ntohl() and pg_time_t as per Thomas'
comments.
4) I've changed the code to consistently use "while (1)" instead of "for
(;;)" (except for the math library, but I didn't touch that at all)
I've also changed the behaviour when passing a message with a signature to
the decrypt functions which don't verify signatures. They now report
"ERROR: Wrong key or corrupt data" instead of decrypting and silently
ignoring the signature. The behaviour is now backwards compatible, but I
1) Produce a better error message (I'm sure most people don't know about
the hidden debug=1 setting)
2) Provide an option to ignore the signature if decrypting the data is
desirable even if the signature can't be verified

If i understand the sequence here: The current git HEAD is that
pgp_pub_decrypt would throw an error if given a signed and encrypted
message, and earlier version of your patch changed that to decrypt the
message and ignore the signature, and the current version went back to
throwing an error.

I think I prefer the middle of those behaviors. The original behavior
seems like a bug to me, and I don't think we need to be backwards
compatible with bugs. Why should a function called "decrypt" care if the
message is also signed? That is not its job.

If we decide to throw the error, a better error message certainly wouldn't
hurt. And the output of 'debug=1' is generally not comprehensible unless
you are familiar with the source code, so that is not a substitute.

(By the way, there are now 2 patches in this series named
pgcrypto_sigs.v3.patch--so be careful which one you look it.)

There seems to be a memory leak in pgp_sym_decrypt_verify that does not
exist in pgp_sym_decrypt. It is about 58 bytes per decryption.

Perl test script:


my $dbh=connect(...);
my $pub=`cat public.asc`;
my $pri=`cat private.asc`;


my $enc= $dbh->prepare("select
armor(pgp_sym_encrypt_sign('asdlkfjsldkfjsadf',?,dearmor(?),'debug=1'))");
my $dec= $dbh->prepare("select
pgp_sym_decrypt_verify(dearmor(?),?,dearmor(?),'debug=1')");
my $i=1;

$enc->execute("foobar$i",$pri);
my ($message)=$enc->fetchrow();

foreach my $ii (1..1000000) {
## my $i=$ii;
$dec->execute($message,"foobar$i",$pub);
my ($message2)=$dec->fetchrow();
die unless $message2 eq "asdlkfjsldkfjsadf";
warn "$i\t", time() if $i%1000 ==0;
};



Cheers,

Jeff

[Marko Tiikkaja]

Post by Jeff Janes

Post by Marko Tiikkaja
I've also changed the behaviour when passing a message with a signature to
the decrypt functions which don't verify signatures. They now report
"ERROR: Wrong key or corrupt data" instead of decrypting and silently
ignoring the signature. The behaviour is now backwards compatible, but I
1) Produce a better error message (I'm sure most people don't know about
the hidden debug=1 setting)
2) Provide an option to ignore the signature if decrypting the data is
desirable even if the signature can't be verified

If i understand the sequence here: The current git HEAD is that
pgp_pub_decrypt would throw an error if given a signed and encrypted
message, and earlier version of your patch changed that to decrypt the
message and ignore the signature, and the current version went back to
throwing an error.

You got that right, yes.

Post by Jeff Janes
I think I prefer the middle of those behaviors. The original behavior
seems like a bug to me, and I don't think we need to be backwards
compatible with bugs. Why should a function called "decrypt" care if the
message is also signed? That is not its job.

Yeah, that seems reasonable, I guess. I'm kind of torn between the two
behaviours to be honest. But perhaps it would make sense to change the
previous behaviour (i.e. go back to way this patch was earlier) and
document that somewhere.

Post by Jeff Janes
There seems to be a memory leak in pgp_sym_decrypt_verify that does not
exist in pgp_sym_decrypt. It is about 58 bytes per decryption.

Interesting. Thanks! I'll have a look.


.marko

[Marko Tiikkaja]

Hi Jeff,

Post by Jeff Janes
There seems to be a memory leak in pgp_sym_decrypt_verify that does not
exist in pgp_sym_decrypt. It is about 58 bytes per decryption.

Thanks. There seemed to have been a small confusion about the ownership
of ctx->sig_digest_ctx. I've fixed that now and the test case you
provided doesn't appear to be leaking memory anymore. I also added some
other missing free calls to pgp_free().

I've attached a patch with this problem fixed, in case you still want to
keep testing (all your work so far very much appreciated, btw!) The
attached also fixes the ntohl() problem I pointed out in my previous
patch, and now AFAIK there aren't any outstanding technical issues.

However..

Post by Jeff Janes
If i understand the sequence here: The current git HEAD is that
pgp_pub_decrypt would throw an error if given a signed and encrypted
message, and earlier version of your patch changed that to decrypt the
message and ignore the signature, and the current version went back to
throwing an error.
I think I prefer the middle of those behaviors. The original behavior
seems like a bug to me, and I don't think we need to be backwards
compatible with bugs. Why should a function called "decrypt" care if the
message is also signed? That is not its job.

I haven't updated the patch yet because I don't want to waste my time
going back and forth until we have a consensus, but I think I prefer
Jeff's suggestion here to make the _decrypt() functions ignore
signatures. Does anyone else want to voice their opinion?


.marko

[Alvaro Herrera]

Post by Marko Tiikkaja

Post by Jeff Janes
If i understand the sequence here: The current git HEAD is that
pgp_pub_decrypt would throw an error if given a signed and encrypted
message, and earlier version of your patch changed that to decrypt the
message and ignore the signature, and the current version went back to
throwing an error.
I think I prefer the middle of those behaviors. The original behavior
seems like a bug to me, and I don't think we need to be backwards
compatible with bugs. Why should a function called "decrypt" care if the
message is also signed? That is not its job.

I haven't updated the patch yet because I don't want to waste my
time going back and forth until we have a consensus, but I think I
prefer Jeff's suggestion here to make the _decrypt() functions
ignore signatures. Does anyone else want to voice their opinion?

+1 for ignoring sigs. If somebody want to check sigs, that's a
separate step. Maybe we could have an optional boolean flag, default
false, to enable checking sigs, but that seems material for a future
patch.

That said, I do wonder if it's a behavior change with security
implications: if somebody is relying on the current behavior of throwing
an error when sigs don't match, they wouldn't be thrilled to hear that
their security checks now fail to detect a problem because we don't
verify signatures when decrypting. In other words, is this established
practice already? If not, it's okay; otherwise, hmm.

[Abhijit Menon-Sen]

(I have't read the patch, or even earlier correspondence in this
thread, so I apologise for just jumping in.)

Post by Alvaro Herrera
+1 for ignoring sigs. If somebody want to check sigs, that's a
separate step.

For what it's worth, although it seems logical to split up cryptographic
primitives like this, I think it's widely recognised these days to have
contributed to plenty of bad crypto implementations. These seems to be
general trend of moving towards higher-level interfaces that require
fewer decisions and can be relied upon do the Right Thing.

I don't like the idea of ignoring signature verification errors any more
than I would like "if somebody wants to check the HMAC before decypting,
that's a separate step".

Of course, all that is an aside. If the function ever threw an error on
signature verification failures, I would strongly object to changing it
to ignore such errors for exactly the reasons you mention already.

-- Abhijit

[Marko Tiikkaja]

Post by Abhijit Menon-Sen
(I have't read the patch, or even earlier correspondence in this
thread, so I apologise for just jumping in.)

Post by Alvaro Herrera
+1 for ignoring sigs. If somebody want to check sigs, that's a
separate step.

For what it's worth, although it seems logical to split up cryptographic
primitives like this, I think it's widely recognised these days to have
contributed to plenty of bad crypto implementations. These seems to be
general trend of moving towards higher-level interfaces that require
fewer decisions and can be relied upon do the Right Thing.
I don't like the idea of ignoring signature verification errors any more
than I would like "if somebody wants to check the HMAC before decypting,
that's a separate step".
Of course, all that is an aside. If the function ever threw an error on
signature verification failures, I would strongly object to changing it
to ignore such errors for exactly the reasons you mention already.

I'm not sure we're talking about the same thing. Currently, we throw an
error if *any* signature was present, valid or otherwise. The "decrypt
only" functions don't have enough information to verify the validity of
the signature, so we must either ignore the signatures or throw an error
in their presence.

The only downside of ignoring signatures here as far as I can tell is a
scenario where you're sending messages to someone, and they accept your
signed messages. You might get the impression that the receiving party
is actually validating the signature, but I guess that's trivial to
test, and relying on such unwritten contracts is a bit suspicious anyway
when it comes to cryptography.

I've changed the patch back to ignore signatures when not using the
decrypt_verify() functions in the attached.


.marko

[Abhijit Menon-Sen]

Post by Marko Tiikkaja
I'm not sure we're talking about the same thing.

No, we weren't. I was under the impression that the signatures
could be validated. Sorry for the noise.

-- Abhijit

[Heikki Linnakangas]

I looked at this briefly, and was surprised that there is no support for
signing a message without encrypting it. Is that intentional? Instead of
adding a function to encrypt and sign a message, I would have expected
this to just add a new function for signing, and you could then pass it
an already-encrypted blob, or plaintext.

- Heikki

[Marko Tiikkaja]

Post by Heikki Linnakangas
I looked at this briefly, and was surprised that there is no support for
signing a message without encrypting it. Is that intentional? Instead of
adding a function to encrypt and sign a message, I would have expected
this to just add a new function for signing, and you could then pass it
an already-encrypted blob, or plaintext.

Yes, that's intentional. The signatures are part of the encrypted data
here, so you can't look at a message and determine who sent it.

There was brief discussion about this upthread (though no one probably
added any links to those discussions into the commit fest app), and I
still think that both types of signing would probably be valuable. But
this patch is already quite big, and I really have no desire to work on
this "sign anything" functionality. The pieces are there, though, so if
someone wants to do it, I don't see why they couldn't.


.marko

[Jeff Janes]

Post by Marko Tiikkaja
I've changed the patch back to ignore signatures when not using the
decrypt_verify() functions in the attached.

Hi Marko,

This patch needs a rebase now that the armor header patch has been
committed.

Thanks,

Jeff

[Marko Tiikkaja]

Hi,

Post by Jeff Janes
This patch needs a rebase now that the armor header patch has been
committed.

Thanks. Will fix that shortly.

I'm guessing there's no need to bump the pgcrypto version to 1.3, since
there hasn't been a release with the 1.2 version?


.marko

[Michael Paquier]

Post by Marko Tiikkaja
I'm guessing there's no need to bump the pgcrypto version to 1.3, since
there hasn't been a release with the 1.2 version?

Yep. One version bump by major release is fine for a contrib module.

[Marko Tiikkaja]

Hi,

Here's the rebased patch -- as promised -- in a v7.


.marko

[Jeff Janes]

Post by Alvaro Herrera

Post by Marko Tiikkaja

Post by Jeff Janes
If i understand the sequence here: The current git HEAD is that
pgp_pub_decrypt would throw an error if given a signed and encrypted
message, and earlier version of your patch changed that to decrypt the
message and ignore the signature, and the current version went back to
throwing an error.
I think I prefer the middle of those behaviors. The original behavior
seems like a bug to me, and I don't think we need to be backwards
compatible with bugs. Why should a function called "decrypt" care if

the

Post by Marko Tiikkaja

Post by Jeff Janes
message is also signed? That is not its job.

I haven't updated the patch yet because I don't want to waste my
time going back and forth until we have a consensus, but I think I
prefer Jeff's suggestion here to make the _decrypt() functions
ignore signatures. Does anyone else want to voice their opinion?

+1 for ignoring sigs. If somebody want to check sigs, that's a
separate step. Maybe we could have an optional boolean flag, default
false, to enable checking sigs, but that seems material for a future
patch.
That said, I do wonder if it's a behavior change with security
implications: if somebody is relying on the current behavior of throwing
an error when sigs don't match, they wouldn't be thrilled to hear that
their security checks now fail to detect a problem because we don't
verify signatures when decrypting. In other words, is this established
practice already? If not, it's okay; otherwise, hmm.

If it is established practise, I think the only security model that could
be used to justify it is "The bad guys will be nice enough to always sign
their attacks, while the good guys will refrain from signing them." It is
not clear why the bad guys would cooperate with that.

Anyone who can produce an encrypted and signed attack message could also
produce an encrypted and unsigned one, couldn't they?

Cheers,

Jeff

[Peter Eisentraut]

Post by Marko Tiikkaja

Post by Marko Tiikkaja
Attached is a patch to add support for PGP signatures in encrypted
messages into pgcrypto.

Here's v2 of the patch. I've changed the info-extracting code to not
look for signatures beyond the data, which also meant that it had to
parse one-pass signatures (which it didn't do before). This matches the
behaviour of the main decryption code.

There is a compiler warning:

pgp-sig.c: In function ‘pgp_parse_onepass_signature’:
pgp-sig.c:715:8: error: variable ‘last’ set but not used [-Werror=unused-but-set-variable]
uint8 last;
^

[Joel Jacobson]

Post by Marko Tiikkaja
Hi hackers,
Attached is a patch to add support for PGP signatures in encrypted messages
into pgcrypto.

I noticed Heikki wanted to check if there is any interested for the
patches in the current commitfest.

Yes, our company Trustly are very interested in the two PGP additions
to pgcrypto.

We currently use these patches in production in a separate database,
but if they would be part of standard postgres, we wouldn't need to
run the application using the functionality in a separate database
server, which would simplify things a lot.

Without these patches, there is no way to deal with PGP signatures.
Since signatures is a crucial component of OpenPGP, the existing
encryption/decryption features are useful, but not nearly as useful as
if you also have the capabilities to generate and verify PGP
signatures.

We use the PGP functionality in a system called BankAPI, which is open
source and available here: https://github.com/trustly/bankapi

Also, in the documentation, it has already been acknowledged the lack
of signing is a current limitation:
"F.25.3.9. Limitations of PGP Code
No support for signing. That also means that it is not checked whether
the encryption subkey belongs to the master key."

[Heikki Linnakangas]

Post by Joel Jacobson

Post by Marko Tiikkaja
Hi hackers,
Attached is a patch to add support for PGP signatures in encrypted messages
into pgcrypto.

I noticed Heikki wanted to check if there is any interested for the
patches in the current commitfest.
Yes, our company Trustly are very interested in the two PGP additions
to pgcrypto.

Cool. Please sign up as a reviewer then, so that we can get these
patches reviewed and committed.

- Heikki