Re: [HACKERS] pgcrypto: PGP armor headers
On 10/01/2014 02:47 PM, Marko Tiikkaja wrote: On 10/1/14 1:01 PM, Heikki Linnakangas wrote: I think this is now ready for commit, but since I've changed it quite significantly from what you originally submitted, please take a moment to review this. 1) I see this compiler warning: pgp-pgsql.c: In function ‘pg_armor’: pgp-pgsql.c:960:18: warning: ‘values’ may be used uninitialized in this function [-Wmaybe-uninitialized] pgp_armor_encode((uint8 *) VARDATA(data), data_len, &buf, It's bogus, but worth silencing anyway. 2) There's what looks like an extra whitespace change in pgp_armor_encode(), but maybe that's intentional? 3) Also, I think the attached two corner cases deserve their own tests. Other than the above, the patch looks good to me. Huge thanks for your work on this one! Ok, committed with those fixes. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 10/1/14 1:01 PM, Heikki Linnakangas wrote:
On 10/01/2014 11:58 AM, Marko Tiikkaja wrote:
On 10/1/14, 9:11 AM, Heikki Linnakangas wrote:
We have two options:
1. Throw an error if there are any non-ASCII characters in the key/value
arrays.
2. Don't convert them to UTF-8, but use the current database encoding.
Both seem sane to me. If we use the current database encoding, then we
have to also decide what to do with the input, in pgp_armor_headers().
If armor() uses the database encoding, but pgp_armor_headers() treats
the input as UTF-8, then a round-trip with pgp_armor_headers(armor(?))
won't work.
Yeah. Both options seem fine to me. Throwing an error perhaps slightly
more so.
I went with 1, throw an error. I also added checks that the key or value
doesn't contain any embedded newlines, and that the key doesn't contain
an embedded ": ". Those would cause the armor to be invalid.
Great.
I think this is now ready for commit, but since I've changed it quite
significantly from what you originally submitted, please take a moment
to review this.
1) I see this compiler warning:
pgp-pgsql.c: In function ‘pg_armor’:
pgp-pgsql.c:960:18: warning: ‘values’ may be used uninitialized in
this function [-Wmaybe-uninitialized]
pgp_armor_encode((uint8 *) VARDATA(data), data_len, &buf,
It's bogus, but worth silencing anyway.
2) There's what looks like an extra whitespace change in
pgp_armor_encode(), but maybe that's intentional?
3) Also, I think the attached two corner cases deserve their own tests.
Other than the above, the patch looks good to me. Huge thanks for your
work on this one!
.marko
*** a/contrib/pgcrypto/expected/pgp-armor.out
--- b/contrib/pgcrypto/expected/pgp-armor.out
***
*** 112,117 em9va2E=
--- 112,137
-END PGP MESSAGE-
');
ERROR: Corrupt ascii-armor
+ -- corrupt (no empty line)
+ select * from pgp_armor_headers('
+ -BEGIN PGP MESSAGE-
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ');
+ ERROR: Corrupt ascii-armor
+ -- no headers
+ select * from pgp_armor_headers('
+ -BEGIN PGP MESSAGE-
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ');
+ key | value
+ -+---
+ (0 rows)
+
-- header with empty value
select * from pgp_armor_headers('
-BEGIN PGP MESSAGE-
*** a/contrib/pgcrypto/sql/pgp-armor.sql
--- b/contrib/pgcrypto/sql/pgp-armor.sql
***
*** 67,72 em9va2E=
--- 67,89
-END PGP MESSAGE-
');
+ -- corrupt (no empty line)
+ select * from pgp_armor_headers('
+ -BEGIN PGP MESSAGE-
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ');
+
+ -- no headers
+ select * from pgp_armor_headers('
+ -BEGIN PGP MESSAGE-
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ');
+
-- header with empty value
select * from pgp_armor_headers('
-BEGIN PGP MESSAGE-
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 10/01/2014 11:58 AM, Marko Tiikkaja wrote:
On 10/1/14, 9:11 AM, Heikki Linnakangas wrote:
We have two options:
1. Throw an error if there are any non-ASCII characters in the key/value
arrays.
2. Don't convert them to UTF-8, but use the current database encoding.
Both seem sane to me. If we use the current database encoding, then we
have to also decide what to do with the input, in pgp_armor_headers().
If armor() uses the database encoding, but pgp_armor_headers() treats
the input as UTF-8, then a round-trip with pgp_armor_headers(armor(?))
won't work.
Yeah. Both options seem fine to me. Throwing an error perhaps slightly
more so.
I went with 1, throw an error. I also added checks that the key or value
doesn't contain any embedded newlines, and that the key doesn't contain
an embedded ": ". Those would cause the armor to be invalid.
I think this is now ready for commit, but since I've changed it quite
significantly from what you originally submitted, please take a moment
to review this.
- Heikki
diff --git a/.gitattributes b/.gitattributes
index ff96567..9466800 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -14,6 +14,7 @@ README.* conflict-marker-size=32
# Certain data files that contain special whitespace, and other special cases
*.data -whitespace
contrib/tsearch2/sql/tsearch2.sql whitespace=space-before-tab,blank-at-eof,-blank-at-eol
+contrib/pgcrypto/sql/pgp-armor.sql whitespace=-blank-at-eol
doc/bug.templatewhitespace=space-before-tab,-blank-at-eof,blank-at-eol
src/backend/catalog/sql_features.txt whitespace=space-before-tab,blank-at-eof,-blank-at-eol
src/backend/tsearch/hunspell_sample.affix whitespace=-blank-at-eof
diff --git a/contrib/pgcrypto/Makefile b/contrib/pgcrypto/Makefile
index b6c9484..d1d75ca 100644
--- a/contrib/pgcrypto/Makefile
+++ b/contrib/pgcrypto/Makefile
@@ -26,7 +26,7 @@ MODULE_big = pgcrypto
OBJS = $(SRCS:.c=.o) $(WIN32RES)
EXTENSION = pgcrypto
-DATA = pgcrypto--1.1.sql pgcrypto--1.0--1.1.sql pgcrypto--unpackaged--1.0.sql
+DATA = pgcrypto--1.2.sql pgcrypto--1.1--1.2.sql pgcrypto--1.0--1.1.sql pgcrypto--unpackaged--1.0.sql
PGFILEDESC = "pgcrypto - cryptographic functions"
REGRESS = init md5 sha1 hmac-md5 hmac-sha1 blowfish rijndael \
diff --git a/contrib/pgcrypto/expected/pgp-armor.out b/contrib/pgcrypto/expected/pgp-armor.out
index c955494..a8f73ac 100644
--- a/contrib/pgcrypto/expected/pgp-armor.out
+++ b/contrib/pgcrypto/expected/pgp-armor.out
@@ -102,3 +102,251 @@ em9va2E=
-END PGP MESSAGE-
');
ERROR: Corrupt ascii-armor
+-- corrupt (no space after the colon)
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+foo:
+
+em9va2E=
+=
+-END PGP MESSAGE-
+');
+ERROR: Corrupt ascii-armor
+-- header with empty value
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+foo:
+
+em9va2E=
+=
+-END PGP MESSAGE-
+');
+ key | value
+-+---
+ foo |
+(1 row)
+
+-- simple
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+fookey: foovalue
+barkey: barvalue
+
+em9va2E=
+=
+-END PGP MESSAGE-
+');
+ key | value
++--
+ fookey | foovalue
+ barkey | barvalue
+(2 rows)
+
+-- insane keys, part 1
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+insane:key :
+
+em9va2E=
+=
+-END PGP MESSAGE-
+');
+ key | value
+-+---
+ insane:key |
+(1 row)
+
+-- insane keys, part 2
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+insane:key : text value here
+
+em9va2E=
+=
+-END PGP MESSAGE-
+');
+ key | value
+-+-
+ insane:key | text value here
+(1 row)
+
+-- long value
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+long: this value is more than 76 characters long, but it should still parse correctly as that''s permitted by RFC 4880
+
+em9va2E=
+=
+-END PGP MESSAGE-
+');
+ key | value
+--+-
+ long | this value is more than 76 characters long, but it should still parse correctly as that's permitted by RFC 4880
+(1 row)
+
+-- long value, split up
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+long: this value is more than 76 characters long, but it should still
+long: parse correctly as that''s permitted by RFC 4880
+
+em9va2E=
+=
+-END PGP MESSAGE-
+');
+ key | value
+--+--
+ long | this value is more than 76 characters long, but it should still
+ long | parse correctly as that's permitted by RFC 4880
+(2 rows)
+
+-- long value, split up, part 2
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+
Re: [HACKERS] pgcrypto: PGP armor headers
On 10/1/14, 9:11 AM, Heikki Linnakangas wrote: I spent a little time cleaning up the regression tests and docs, and ended up with the attached. But then I realized that there's a problem with UTF-8 conversion in the armor() function. It returns the armored blob as text, but forcibly converts the keys and values to UTF-8. That's not cool, because you will get invalidly encoded strings into the database, if you use the function while connected to a database that uses some other encoding than UTF-8. RFC4880 says that the headers are in UTF-8, but armor() cannot safely return UTF-8 encoded text unless the database's encoding is also UTF-8. It also rightly says that using anything else than plain ASCII, even though nominally it's UTF-8, is a bad idea, because the whole point of ASCII-armoring is to make the format safe from encoding conversions. Ugh. Right. We have two options: 1. Throw an error if there are any non-ASCII characters in the key/value arrays. 2. Don't convert them to UTF-8, but use the current database encoding. Both seem sane to me. If we use the current database encoding, then we have to also decide what to do with the input, in pgp_armor_headers(). If armor() uses the database encoding, but pgp_armor_headers() treats the input as UTF-8, then a round-trip with pgp_armor_headers(armor(?)) won't work. Yeah. Both options seem fine to me. Throwing an error perhaps slightly more so. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 09/30/2014 06:39 PM, Marko Tiikkaja wrote:
On 9/30/14 5:17 PM, Heikki Linnakangas wrote:
I'm actually now leaning towards providing just a single function,
pgp_armor_headers(text, key OUT text, value OUT text), which returns all
the keys and values. That gives maximum flexibility, and leaves it up to
the user to decide what to do with duplicate keys. It's pretty easy to
use that to extract just a single header, too:
What do you think? Attached patch implements that, but the docs and
regression tests now need adjustment.
(You forgot the patch, but I can imagine what it would have been.)
Oops.
I'm not exactly sure to be honest. I would personally like to see a
simple function for extracting a single header value in a scalar context
without having to deal with all the pain of SRFs, multiple matches and
no matches. Sure, that got a lot better in 9.3 with LATERAL but it's
still way inferior to pgp_armor_header().
I can also see why someone would argue that I should just create the
function myself and that it doesn't have to be shipped with postgres.
But on the other hand, this is already an extension one has to
explicitly go and CREATE, and that considered one more function probably
wouldn't hurt too much.
Yeah, building the function to extract a single value is pretty simple
once you have the set-returning function:
create function pgp_armor_header(armor text, key text) returns text
language sql as $$
select string_agg(value, ' ') from pgp_armor_headers($1) where key = $2
$$;
I spent a little time cleaning up the regression tests and docs, and
ended up with the attached. But then I realized that there's a problem
with UTF-8 conversion in the armor() function. It returns the armored
blob as text, but forcibly converts the keys and values to UTF-8. That's
not cool, because you will get invalidly encoded strings into the
database, if you use the function while connected to a database that
uses some other encoding than UTF-8.
RFC4880 says that the headers are in UTF-8, but armor() cannot safely
return UTF-8 encoded text unless the database's encoding is also UTF-8.
It also rightly says that using anything else than plain ASCII, even
though nominally it's UTF-8, is a bad idea, because the whole point of
ASCII-armoring is to make the format safe from encoding conversions.
We have two options:
1. Throw an error if there are any non-ASCII characters in the key/value
arrays.
2. Don't convert them to UTF-8, but use the current database encoding.
Both seem sane to me. If we use the current database encoding, then we
have to also decide what to do with the input, in pgp_armor_headers().
If armor() uses the database encoding, but pgp_armor_headers() treats
the input as UTF-8, then a round-trip with pgp_armor_headers(armor(?))
won't work.
- Heikki
diff --git a/contrib/pgcrypto/Makefile b/contrib/pgcrypto/Makefile
index b6c9484..d1d75ca 100644
--- a/contrib/pgcrypto/Makefile
+++ b/contrib/pgcrypto/Makefile
@@ -26,7 +26,7 @@ MODULE_big = pgcrypto
OBJS = $(SRCS:.c=.o) $(WIN32RES)
EXTENSION = pgcrypto
-DATA = pgcrypto--1.1.sql pgcrypto--1.0--1.1.sql pgcrypto--unpackaged--1.0.sql
+DATA = pgcrypto--1.2.sql pgcrypto--1.1--1.2.sql pgcrypto--1.0--1.1.sql pgcrypto--unpackaged--1.0.sql
PGFILEDESC = "pgcrypto - cryptographic functions"
REGRESS = init md5 sha1 hmac-md5 hmac-sha1 blowfish rijndael \
diff --git a/contrib/pgcrypto/expected/pgp-armor.out b/contrib/pgcrypto/expected/pgp-armor.out
index c955494..bcf9590 100644
--- a/contrib/pgcrypto/expected/pgp-armor.out
+++ b/contrib/pgcrypto/expected/pgp-armor.out
@@ -102,3 +102,245 @@ em9va2E=
-END PGP MESSAGE-
');
ERROR: Corrupt ascii-armor
+-- corrupt (no space after the colon)
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+foo:
+
+em9va2E=
+=
+-END PGP MESSAGE-
+');
+ERROR: Corrupt ascii-armor
+-- header with empty value
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+foo:
+
+em9va2E=
+=
+-END PGP MESSAGE-
+');
+ key | value
+-+---
+ foo |
+(1 row)
+
+-- simple
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+fookey: foovalue
+barkey: barvalue
+
+em9va2E=
+=
+-END PGP MESSAGE-
+');
+ key | value
++--
+ fookey | foovalue
+ barkey | barvalue
+(2 rows)
+
+-- insane keys, part 1
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+insane:key :
+
+em9va2E=
+=
+-END PGP MESSAGE-
+');
+ key | value
+-+---
+ insane:key |
+(1 row)
+
+-- insane keys, part 2
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+insane:key : text value here
+
+em9va2E=
+=
+-END PGP MESSAGE-
+');
+ key | value
+-+-
+ insane:key | text value here
+(1 row)
+
+-- long value
+select * from pgp_armor_headers('
+-BEGIN PGP MESSAGE-
+long: this value is more than 76 characters long, but it should still
Re: [HACKERS] pgcrypto: PGP armor headers
On 9/30/14 5:17 PM, Heikki Linnakangas wrote: I'm actually now leaning towards providing just a single function, pgp_armor_headers(text, key OUT text, value OUT text), which returns all the keys and values. That gives maximum flexibility, and leaves it up to the user to decide what to do with duplicate keys. It's pretty easy to use that to extract just a single header, too: What do you think? Attached patch implements that, but the docs and regression tests now need adjustment. (You forgot the patch, but I can imagine what it would have been.) I'm not exactly sure to be honest. I would personally like to see a simple function for extracting a single header value in a scalar context without having to deal with all the pain of SRFs, multiple matches and no matches. Sure, that got a lot better in 9.3 with LATERAL but it's still way inferior to pgp_armor_header(). I can also see why someone would argue that I should just create the function myself and that it doesn't have to be shipped with postgres. But on the other hand, this is already an extension one has to explicitly go and CREATE, and that considered one more function probably wouldn't hurt too much. But either way I won't be unhappy, so it's up to you (and/or other members of the community) to decide this one. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 09/30/2014 05:45 PM, Marko Tiikkaja wrote:
On 9/30/14 4:37 PM, Heikki Linnakangas wrote:
On 09/29/2014 05:38 PM, Marko Tiikkaja wrote:
Maybe I just suck at $SEARCH_ENGINE, but extracting armor headers
programmatically doesn't seem to be very popular. I could only find one
example, which returned the last instance of the key. But that seemed
to be more an accident than anything else; it wasn't documented and the
source code didn't say anything about it. I also think that's the worst
behaviour. If we can't agree on concatenation, I'd rather see an error.
May I ask you why you wrote this patch? What are you doing with the headers?
We're sending arbitrary messages between systems over HTTP(S), and a
special header is used to tell the recipient system what type of message
it is. The message types are specific to the domain, but you can think
of them to be roughly equivalent to MIME types.
Ok. How quaint. :-)
If what you're trying to get a sense of is why I'd prefer to see
concatenation, I can't really help you. For our use case (and perhaps
for everyone else as well) it would actually make more sense to throw an
error if pgp_armor_header() is used on a key which appears more than
once. The concatenation behaviour was an attempt at a "one size fits
all" interface, but now that we're going to also have a
pgp_armor_headers() function for users to implement the behaviour they
want themselves, there's no real reason to try and guess what everyone
wants. I think I'd prefer to see an ERROR in this case now.
I'm actually now leaning towards providing just a single function,
pgp_armor_headers(text, key OUT text, value OUT text), which returns all
the keys and values. That gives maximum flexibility, and leaves it up to
the user to decide what to do with duplicate keys. It's pretty easy to
use that to extract just a single header, too:
postgres=# select * FROM pgp_armor_headers('
-BEGIN PGP MESSAGE-
foo: baar
foo: more foo
singlekey: fsdfsd
em9va2E=
=
-END PGP MESSAGE-
') where key = 'singlekey';
key| value
---+
singlekey | fsdfsd
(1 row)
And if you want to concatenate possible duplicates:
postgres=# select string_agg(value, ' ') FROM pgp_armor_headers('
-BEGIN PGP MESSAGE-
foo: baar
foo: more foo
singlekey: fsdfsd
em9va2E=
=
-END PGP MESSAGE-
') where key = 'foo';
string_agg
---
baar more foo
(1 row)
What do you think? Attached patch implements that, but the docs and
regression tests now need adjustment.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
Heikki Linnakangas wrote:
Happened to land on the middle of the regression test by accident and
noticed:
> +select armor('zooka', array['Version', 'Comment'], array['Created by
> pgcrypto', 'PostgreSQL, the world''s most most advanced open source
> database']);
> + armor
> +--
> + -BEGIN PGP MESSAGE- +
> + Version: Created by pgcrypto+
> + Comment: PostgreSQL, the world's most most advanced open source database+
> + +
> + em9va2E=+
> + =D5cR +
> + -END PGP MESSAGE- +
> +
> +(1 row)
"most most"?
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 9/30/14 4:37 PM, Heikki Linnakangas wrote: On 09/29/2014 05:38 PM, Marko Tiikkaja wrote: Maybe I just suck at $SEARCH_ENGINE, but extracting armor headers programmatically doesn't seem to be very popular. I could only find one example, which returned the last instance of the key. But that seemed to be more an accident than anything else; it wasn't documented and the source code didn't say anything about it. I also think that's the worst behaviour. If we can't agree on concatenation, I'd rather see an error. May I ask you why you wrote this patch? What are you doing with the headers? We're sending arbitrary messages between systems over HTTP(S), and a special header is used to tell the recipient system what type of message it is. The message types are specific to the domain, but you can think of them to be roughly equivalent to MIME types. If what you're trying to get a sense of is why I'd prefer to see concatenation, I can't really help you. For our use case (and perhaps for everyone else as well) it would actually make more sense to throw an error if pgp_armor_header() is used on a key which appears more than once. The concatenation behaviour was an attempt at a "one size fits all" interface, but now that we're going to also have a pgp_armor_headers() function for users to implement the behaviour they want themselves, there's no real reason to try and guess what everyone wants. I think I'd prefer to see an ERROR in this case now. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 09/29/2014 05:38 PM, Marko Tiikkaja wrote: On 9/29/14 3:02 PM, Heikki Linnakangas wrote: Is there any real life examples or tools out there to generate armors with headers with duplicate keys? RFC 4880 says: Note that some transport methods are sensitive to line length. While there is a limit of 76 characters for the Radix-64 data (Section 6.3), there is no limit to the length of Armor Headers. Care should be taken that the Armor Headers are short enough to survive transport. One way to do this is to repeat an Armor Header key multiple times with different values for each so that no one line is overly long. Does anyone do that in practice? Is there any precedence for concatenating the values in other tools that read armor headers? Maybe I just suck at $SEARCH_ENGINE, but extracting armor headers programmatically doesn't seem to be very popular. I could only find one example, which returned the last instance of the key. But that seemed to be more an accident than anything else; it wasn't documented and the source code didn't say anything about it. I also think that's the worst behaviour. If we can't agree on concatenation, I'd rather see an error. May I ask you why you wrote this patch? What are you doing with the headers? - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 9/29/14 3:02 PM, Heikki Linnakangas wrote: Thanks! I found the pgp_extract_armor_headers()'s signature quite weird, so I simplified that by making it always return arrays of keys and values. The callers is now responsible for returning all the keys (pgp_armor_header_keys) or finding the single key (pgp_armor_header). I also partially rewrote the implementation of pgp_extract_armor_headers(), making it more readable I hope. OK. Looks good to me. If an armor header line ends in CR+LF, pgp_armor_header() returned the CR as part of the value, with your patch. I don't think that's right, the line ending should be considered part of the armoring, so I changed that. Nice catch. You are correct. Is there any real life examples or tools out there to generate armors with headers with duplicate keys? RFC 4880 says: Note that some transport methods are sensitive to line length. While there is a limit of 76 characters for the Radix-64 data (Section 6.3), there is no limit to the length of Armor Headers. Care should be taken that the Armor Headers are short enough to survive transport. One way to do this is to repeat an Armor Header key multiple times with different values for each so that no one line is overly long. Does anyone do that in practice? Is there any precedence for concatenating the values in other tools that read armor headers? Maybe I just suck at $SEARCH_ENGINE, but extracting armor headers programmatically doesn't seem to be very popular. I could only find one example, which returned the last instance of the key. But that seemed to be more an accident than anything else; it wasn't documented and the source code didn't say anything about it. I also think that's the worst behaviour. If we can't agree on concatenation, I'd rather see an error. I wonder if it would make sense to have pgp_armor_header_keys() return both the keys and values. That would make it easier to use, and it might then make sense for it to not remove the duplicates or concatenate values, but just them as is. The caller could then deal with the duplicates any way he wants. We could keep the function for extracting the value for a single key, with the concatenating behavior, for convenience. I'd also suggest renaming it to pgp_armor_headers() in that case. But otherwise it seems like a reasonable plan to me. Want me to do that change or are you going to? .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 09/27/2014 11:50 PM, Marko Tiikkaja wrote:
Hi,
On 9/25/14, 3:56 PM, I wrote:
On 9/25/14 3:50 PM, Heikki Linnakangas wrote:
Are you planning to post the main patch rebased on top of this soon? As
in the next day or two? Otherwise I'll mark this as "Returned with
feedback" for this commitfest.
Yes. With good luck I'll get you a rebased one today, otherwise it'll
have to wait until tomorrow.
Missed that promise by a day since something unexpected came up
yesterday. Attached is v3 of the patch. The changes are:
- Rebased on top of the current master
- Added a function pgp_armor_header_keys() to list all keys present
in an armor
- Changed pgp_armor_header() to use a StringInfo instead of an mbuf
- Fixed the "error is returned" problem in the documentation pointed
out earlier
Thanks! I found the pgp_extract_armor_headers()'s signature quite weird,
so I simplified that by making it always return arrays of keys and
values. The callers is now responsible for returning all the keys
(pgp_armor_header_keys) or finding the single key (pgp_armor_header). I
also partially rewrote the implementation of
pgp_extract_armor_headers(), making it more readable I hope.
If an armor header line ends in CR+LF, pgp_armor_header() returned the
CR as part of the value, with your patch. I don't think that's right,
the line ending should be considered part of the armoring, so I changed
that.
Is there any real life examples or tools out there to generate armors
with headers with duplicate keys? RFC 4880 says:
Note that some transport methods are sensitive to line length. While
there is a limit of 76 characters for the Radix-64 data (Section
6.3), there is no limit to the length of Armor Headers. Care should
be taken that the Armor Headers are short enough to survive
transport. One way to do this is to repeat an Armor Header key
multiple times with different values for each so that no one line is
overly long.
Does anyone do that in practice? Is there any precedence for
concatenating the values in other tools that read armor headers?
I wonder if it would make sense to have pgp_armor_header_keys() return
both the keys and values. That would make it easier to use, and it might
then make sense for it to not remove the duplicates or concatenate
values, but just them as is. The caller could then deal with the
duplicates any way he wants. We could keep the function for extracting
the value for a single key, with the concatenating behavior, for
convenience.
- Heikki
diff --git a/contrib/pgcrypto/Makefile b/contrib/pgcrypto/Makefile
index b6c9484..d1d75ca 100644
--- a/contrib/pgcrypto/Makefile
+++ b/contrib/pgcrypto/Makefile
@@ -26,7 +26,7 @@ MODULE_big = pgcrypto
OBJS = $(SRCS:.c=.o) $(WIN32RES)
EXTENSION = pgcrypto
-DATA = pgcrypto--1.1.sql pgcrypto--1.0--1.1.sql pgcrypto--unpackaged--1.0.sql
+DATA = pgcrypto--1.2.sql pgcrypto--1.1--1.2.sql pgcrypto--1.0--1.1.sql pgcrypto--unpackaged--1.0.sql
PGFILEDESC = "pgcrypto - cryptographic functions"
REGRESS = init md5 sha1 hmac-md5 hmac-sha1 blowfish rijndael \
diff --git a/contrib/pgcrypto/expected/pgp-armor.out b/contrib/pgcrypto/expected/pgp-armor.out
index c955494..9aa0b4b 100644
--- a/contrib/pgcrypto/expected/pgp-armor.out
+++ b/contrib/pgcrypto/expected/pgp-armor.out
@@ -102,3 +102,348 @@ em9va2E=
-END PGP MESSAGE-
');
ERROR: Corrupt ascii-armor
+-- corrupt
+select pgp_armor_header('
+-BEGIN PGP MESSAGE-
+foo:
+
+em9va2E=
+=
+-END PGP MESSAGE-
+', 'foo');
+ERROR: Corrupt ascii-armor
+-- empty
+select pgp_armor_header('
+-BEGIN PGP MESSAGE-
+foo:
+
+em9va2E=
+=
+-END PGP MESSAGE-
+', 'foo');
+ pgp_armor_header
+--
+
+(1 row)
+
+-- simple
+select pgp_armor_header('
+-BEGIN PGP MESSAGE-
+foo: bar
+
+em9va2E=
+=
+-END PGP MESSAGE-
+', 'foo');
+ pgp_armor_header
+--
+ bar
+(1 row)
+
+-- uninteresting keys, part 1
+select pgp_armor_header('
+-BEGIN PGP MESSAGE-
+foo: found
+bar: ignored
+
+em9va2E=
+=
+-END PGP MESSAGE-
+', 'foo');
+ pgp_armor_header
+--
+ found
+(1 row)
+
+-- uninteresting keys, part 2
+select pgp_armor_header('
+-BEGIN PGP MESSAGE-
+bar: ignored
+foo: found
+
+em9va2E=
+=
+-END PGP MESSAGE-
+', 'foo');
+ pgp_armor_header
+--
+ found
+(1 row)
+
+-- uninteresting keys, part 3
+select pgp_armor_header('
+-BEGIN PGP MESSAGE-
+bar: ignored
+foo: found
+bar: ignored
+
+em9va2E=
+=
+-END PGP MESSAGE-
+', 'foo');
+ pgp_armor_header
+--
+ found
+(1 row)
+
+-- insane keys, part 1
+select pgp_armor_header('
+-BEGIN PGP MESSAGE-
+insane:key :
+
+em9va2E=
+=
+-END PGP MESSAGE-
+', 'insane:key ');
+ pgp_armor_header
+--
+
+(1 row)
+
+-- insane keys, part 2
+select pgp_armor_header('
+-BEGIN PGP MESSAGE-
+insane:key : text value here
+
+em9v
Re: [HACKERS] pgcrypto: PGP armor headers
Hi,
On 9/25/14, 3:56 PM, I wrote:
On 9/25/14 3:50 PM, Heikki Linnakangas wrote:
Are you planning to post the main patch rebased on top of this soon? As
in the next day or two? Otherwise I'll mark this as "Returned with
feedback" for this commitfest.
Yes. With good luck I'll get you a rebased one today, otherwise it'll
have to wait until tomorrow.
Missed that promise by a day since something unexpected came up
yesterday. Attached is v3 of the patch. The changes are:
- Rebased on top of the current master
- Added a function pgp_armor_header_keys() to list all keys present
in an armor
- Changed pgp_armor_header() to use a StringInfo instead of an mbuf
- Fixed the "error is returned" problem in the documentation pointed
out earlier
.marko
*** a/contrib/pgcrypto/Makefile
--- b/contrib/pgcrypto/Makefile
***
*** 26,32 MODULE_big = pgcrypto
OBJS = $(SRCS:.c=.o) $(WIN32RES)
EXTENSION = pgcrypto
! DATA = pgcrypto--1.1.sql pgcrypto--1.0--1.1.sql pgcrypto--unpackaged--1.0.sql
PGFILEDESC = "pgcrypto - cryptographic functions"
REGRESS = init md5 sha1 hmac-md5 hmac-sha1 blowfish rijndael \
--- 26,32
OBJS = $(SRCS:.c=.o) $(WIN32RES)
EXTENSION = pgcrypto
! DATA = pgcrypto--1.2.sql pgcrypto--1.1--1.2.sql pgcrypto--1.0--1.1.sql
pgcrypto--unpackaged--1.0.sql
PGFILEDESC = "pgcrypto - cryptographic functions"
REGRESS = init md5 sha1 hmac-md5 hmac-sha1 blowfish rijndael \
*** a/contrib/pgcrypto/expected/pgp-armor.out
--- b/contrib/pgcrypto/expected/pgp-armor.out
***
*** 102,104 em9va2E=
--- 102,423
-END PGP MESSAGE-
');
ERROR: Corrupt ascii-armor
+ -- corrupt
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ foo:
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ ERROR: Corrupt ascii-armor
+ -- empty
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ foo:
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+
+ (1 row)
+
+ -- simple
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ foo: bar
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+ bar
+ (1 row)
+
+ -- uninteresting keys, part 1
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ foo: found
+ bar: ignored
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+ found
+ (1 row)
+
+ -- uninteresting keys, part 2
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ bar: ignored
+ foo: found
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+ found
+ (1 row)
+
+ -- uninteresting keys, part 3
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ bar: ignored
+ foo: found
+ bar: ignored
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+ found
+ (1 row)
+
+ -- insane keys, part 1
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ insane:key :
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'insane:key ');
+ pgp_armor_header
+ --
+
+ (1 row)
+
+ -- insane keys, part 2
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ insane:key : text value here
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'insane:key ');
+ pgp_armor_header
+ --
+ text value here
+ (1 row)
+
+ -- long value
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ long: this value is more than 76 characters long, but it should still parse
correctly as that''s permitted by RFC 4880
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'long');
+ pgp_armor_header
+
-
+ this value is more than 76 characters long, but it should still parse
correctly as that's permitted by RFC 4880
+ (1 row)
+
+ -- long value, split up
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ long: this value is more than 76 characters long, but it should still
+ long: parse correctly as that''s permitted by RFC 4880
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'long');
+ pgp_armor_header
+
-
+ this value is more than 76 characters long, but it should still parse
correctly as that's permitted by RFC 4880
+ (1 row)
+
+ -- long value, split up, part 2
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ long: this value is more than
+ long: 76 characters long, but it should still
+ long: parse correctly as that''s permitted by RFC 4880
Re: [HACKERS] pgcrypto: PGP armor headers
On 9/25/14 4:08 PM, Heikki Linnakangas wrote: On 09/25/2014 04:56 PM, Marko Tiikkaja wrote: On 9/25/14 3:50 PM, Heikki Linnakangas wrote: On 09/10/2014 04:35 PM, Marko Tiikkaja wrote: It might've been a tad more efficient to return the StringInfo buffer directly from pgp_armor/dearmor, and avoid the extra palloc and memcpy, but this isn't performance critical enough for it to really matter. I couldn't see any way of doing that without breaking the VARDATA abstraction. I even went looking for similar cases in the source code, but couldn't find any. If you can come up with a way, feel free to change that -- I'd like to learn myself. You could first append VARHDRSZ zeros to the StringInfo, then append the base64-encoded data, and last replace the zeros with the real length, using SET_VARSIZE. That's assuming that VARDATA() is at exactly VARHDRSZ bytes. I couldn't find any callers making that assumption. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 09/25/2014 04:56 PM, Marko Tiikkaja wrote: On 9/25/14 3:50 PM, Heikki Linnakangas wrote: On 09/10/2014 04:35 PM, Marko Tiikkaja wrote: It might've been a tad more efficient to return the StringInfo buffer directly from pgp_armor/dearmor, and avoid the extra palloc and memcpy, but this isn't performance critical enough for it to really matter. I couldn't see any way of doing that without breaking the VARDATA abstraction. I even went looking for similar cases in the source code, but couldn't find any. If you can come up with a way, feel free to change that -- I'd like to learn myself. You could first append VARHDRSZ zeros to the StringInfo, then append the base64-encoded data, and last replace the zeros with the real length, using SET_VARSIZE. Are you planning to post the main patch rebased on top of this soon? As in the next day or two? Otherwise I'll mark this as "Returned with feedback" for this commitfest. Yes. With good luck I'll get you a rebased one today, otherwise it'll have to wait until tomorrow. Ok, I'll leave this in Waiting on Author state then. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 9/25/14 3:50 PM, Heikki Linnakangas wrote: On 09/10/2014 04:35 PM, Marko Tiikkaja wrote: OK, I've attemped to do that in the attached. I'm pretty sure I didn't get all of the overflows right, so someone should probably take a really good look at it. (I'm not too confident the original code got them right either, but whatever). Looks good, committed. Thanks! It might've been a tad more efficient to return the StringInfo buffer directly from pgp_armor/dearmor, and avoid the extra palloc and memcpy, but this isn't performance critical enough for it to really matter. I couldn't see any way of doing that without breaking the VARDATA abstraction. I even went looking for similar cases in the source code, but couldn't find any. If you can come up with a way, feel free to change that -- I'd like to learn myself. But like you, I didn't consider it too important. Are you planning to post the main patch rebased on top of this soon? As in the next day or two? Otherwise I'll mark this as "Returned with feedback" for this commitfest. Yes. With good luck I'll get you a rebased one today, otherwise it'll have to wait until tomorrow. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 09/10/2014 04:35 PM, Marko Tiikkaja wrote: On 9/10/14 1:38 PM, Heikki Linnakangas wrote: On 09/10/2014 02:26 PM, Marko Tiikkaja wrote: So I wonder if I shouldn't try and instead keep the code closer to what it is in HEAD right now; I could call enlargeStringInfo() first, then hand out a pointer to b64_encode (or b64_decode()) and finally increment StringInfoData.len by how much was actually written. That would keep the code changes a lot smaller, too. Yeah, that sounds reasonable. OK, I've attemped to do that in the attached. I'm pretty sure I didn't get all of the overflows right, so someone should probably take a really good look at it. (I'm not too confident the original code got them right either, but whatever). Looks good, committed. It might've been a tad more efficient to return the StringInfo buffer directly from pgp_armor/dearmor, and avoid the extra palloc and memcpy, but this isn't performance critical enough for it to really matter. Are you planning to post the main patch rebased on top of this soon? As in the next day or two? Otherwise I'll mark this as "Returned with feedback" for this commitfest. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 09/10/2014 04:35 PM, Marko Tiikkaja wrote: Speaking of good looks, should I add it to the next commitfest as a new patch, or should we try and get someone to review it like this? Let's handle this in this commitfest. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 9/10/14 1:38 PM, Heikki Linnakangas wrote:
On 09/10/2014 02:26 PM, Marko Tiikkaja wrote:
So I wonder if I shouldn't try and instead keep the
code closer to what it is in HEAD right now; I could call
enlargeStringInfo() first, then hand out a pointer to b64_encode (or
b64_decode()) and finally increment StringInfoData.len by how much was
actually written. That would keep the code changes a lot smaller, too.
Yeah, that sounds reasonable.
OK, I've attemped to do that in the attached. I'm pretty sure I didn't
get all of the overflows right, so someone should probably take a really
good look at it. (I'm not too confident the original code got them
right either, but whatever).
Speaking of good looks, should I add it to the next commitfest as a new
patch, or should we try and get someone to review it like this?
I'm also not sure why we need to keep a copy of the base64
encoding/decoding logic instead of exporting it in utils/adt/encode.c.
Good question...
I've left this question unanswered for now. We can fix it later,
independently of this patch.
.marko
*** a/contrib/pgcrypto/pgp-armor.c
--- b/contrib/pgcrypto/pgp-armor.c
***
*** 203,240 crc24(const uint8 *data, unsigned len)
return crc & 0xffL;
}
! int
! pgp_armor_encode(const uint8 *src, unsigned len, uint8 *dst)
{
! int n;
! uint8 *pos = dst;
unsignedcrc = crc24(src, len);
! n = strlen(armor_header);
! memcpy(pos, armor_header, n);
! pos += n;
! n = b64_encode(src, len, pos);
! pos += n;
! if (*(pos - 1) != '\n')
! *pos++ = '\n';
! *pos++ = '=';
! pos[3] = _base64[crc & 0x3f];
! crc >>= 6;
! pos[2] = _base64[crc & 0x3f];
! crc >>= 6;
! pos[1] = _base64[crc & 0x3f];
! crc >>= 6;
! pos[0] = _base64[crc & 0x3f];
! pos += 4;
! n = strlen(armor_footer);
! memcpy(pos, armor_footer, n);
! pos += n;
!
! return pos - dst;
}
static const uint8 *
--- 203,239
return crc & 0xffL;
}
! void
! pgp_armor_encode(const uint8 *src, int len, StringInfo dst)
{
! int res;
! unsignedb64len;
unsignedcrc = crc24(src, len);
! appendStringInfoString(dst, armor_header);
! /* make sure we have enough room to b64_encode() */
! b64len = b64_enc_len(len);
! if (b64len > INT_MAX)
! ereport(ERROR,
! (errcode(ERRCODE_OUT_OF_MEMORY),
!errmsg("out of memory")));
! enlargeStringInfo(dst, (int) b64len);
! res = b64_encode(src, len, (uint8 *) dst->data + dst->len);
! if (res > b64len)
! elog(FATAL, "overflow - encode estimate too small");
! dst->len += res;
! if (*(dst->data + dst->len - 1) != '\n')
! appendStringInfoChar(dst, '\n');
! appendStringInfoChar(dst, '=');
! appendStringInfoChar(dst, _base64[(crc >> 18) & 0x3f]);
! appendStringInfoChar(dst, _base64[(crc >> 12) & 0x3f]);
! appendStringInfoChar(dst, _base64[(crc >> 6) & 0x3f]);
! appendStringInfoChar(dst, _base64[crc & 0x3f]);
! appendStringInfoString(dst, armor_footer);
}
static const uint8 *
***
*** 309,315 find_header(const uint8 *data, const uint8 *datend,
}
int
! pgp_armor_decode(const uint8 *src, unsigned len, uint8 *dst)
{
const uint8 *p = src;
const uint8 *data_end = src + len;
--- 308,314
}
int
! pgp_armor_decode(const uint8 *src, int len, StringInfo dst)
{
const uint8 *p = src;
const uint8 *data_end = src + len;
***
*** 319,324 pgp_armor_decode(const uint8 *src, unsigned len, uint8 *dst)
--- 318,324
const uint8 *base64_end = NULL;
uint8 buf[4];
int hlen;
+ int blen;
int res = PXE_PGP_CORRUPT_ARMOR;
/* armor start */
***
*** 360,382 pgp_armor_decode(const uint8 *src, unsigned len, uint8 *dst)
crc = (((long) buf[0]) << 16) + (((long) buf[1]) << 8) + (long) buf[2];
/* decode data */
! res = b64_decode(base64_start, base64_end - base64_start, dst);
!
! /* check crc */
! if (res >= 0 && crc24(dst, res) != crc)
! res = PXE_PGP_CORRUPT_ARMOR;
out:
return res;
}
-
- unsigned
- pgp_armor_enc_len(unsigned len)
- {
- return b64_enc_len(len) + strlen(armor_header) + strlen(armor_footer) +
16;
- }
-
- unsigned
- pgp_armor_dec_len(unsigned len)
- {
- return b64_dec_len(len);
- }
--- 360,377
crc = (((long) buf[0]) << 16) + (((long) buf[1]) << 8) + (long) buf[2];
/* decode data */
! blen = (int) b64_dec_len(len);
! enlargeStringInfo(dst
Re: [HACKERS] pgcrypto: PGP armor headers
On 09/10/2014 02:26 PM, Marko Tiikkaja wrote: On 9/9/14 11:01 AM, I wrote: On 9/9/14 10:54 AM, Heikki Linnakangas wrote: So I think this (and the corresponding dearmor code too) should be refactored to use a StringInfo that gets enlarged as needed, instead of hoping to guess the size correctly beforehand. To ease review, might make sense to do that as a separate patch over current sources, and the main patch on top of that. Yeah, I thought the same thing while writing that code. Thanks, I'll do it that way. Attached is what I have right now. I started working on the decoding part, but it has this piece of code: /* decode crc */ if (b64_decode(p + 1, 4, buf) != 3) goto out; which makes the approach a bit uglier. If I did this the same way, I would have to create and destroy a StringInfo just for this operation, which seems ugly. So I wonder if I shouldn't try and instead keep the code closer to what it is in HEAD right now; I could call enlargeStringInfo() first, then hand out a pointer to b64_encode (or b64_decode()) and finally increment StringInfoData.len by how much was actually written. That would keep the code changes a lot smaller, too. Yeah, that sounds reasonable. I'm also not sure why we need to keep a copy of the base64 encoding/decoding logic instead of exporting it in utils/adt/encode.c. Good question... - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 9/9/14 11:01 AM, I wrote:
On 9/9/14 10:54 AM, Heikki Linnakangas wrote:
So I think this (and the corresponding dearmor code too) should be
refactored to use a StringInfo that gets enlarged as needed, instead of
hoping to guess the size correctly beforehand. To ease review, might
make sense to do that as a separate patch over current sources, and the
main patch on top of that.
Yeah, I thought the same thing while writing that code. Thanks, I'll do
it that way.
Attached is what I have right now. I started working on the decoding
part, but it has this piece of code:
/* decode crc */
if (b64_decode(p + 1, 4, buf) != 3)
goto out;
which makes the approach a bit uglier. If I did this the same way, I
would have to create and destroy a StringInfo just for this operation,
which seems ugly. So I wonder if I shouldn't try and instead keep the
code closer to what it is in HEAD right now; I could call
enlargeStringInfo() first, then hand out a pointer to b64_encode (or
b64_decode()) and finally increment StringInfoData.len by how much was
actually written. That would keep the code changes a lot smaller, too.
Is either of these approaches anywhere near what you had in mind?
I'm also not sure why we need to keep a copy of the base64
encoding/decoding logic instead of exporting it in utils/adt/encode.c.
.marko
*** a/contrib/pgcrypto/pgp-armor.c
--- b/contrib/pgcrypto/pgp-armor.c
***
*** 31,36
--- 31,38
#include "postgres.h"
+ #include "lib/stringinfo.h"
+
#include "px.h"
#include "pgp.h"
***
*** 38,58
* BASE64 - duplicated :(
*/
! static const unsigned char _base64[] =
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
! static int
! b64_encode(const uint8 *src, unsigned len, uint8 *dst)
{
- uint8 *p,
- *lend = dst + 76;
const uint8 *s,
*end = src + len;
int pos = 2;
unsigned long buf = 0;
s = src;
- p = dst;
while (s < end)
{
--- 40,58
* BASE64 - duplicated :(
*/
! static const char _base64[] =
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
! static void
! b64_encode(const uint8 *src, unsigned len, StringInfo dst)
{
const uint8 *s,
*end = src + len;
int pos = 2;
unsigned long buf = 0;
+ int linepos = 0;
s = src;
while (s < end)
{
***
*** 65,93 b64_encode(const uint8 *src, unsigned len, uint8 *dst)
*/
if (pos < 0)
{
! *p++ = _base64[(buf >> 18) & 0x3f];
! *p++ = _base64[(buf >> 12) & 0x3f];
! *p++ = _base64[(buf >> 6) & 0x3f];
! *p++ = _base64[buf & 0x3f];
pos = 2;
buf = 0;
}
! if (p >= lend)
{
! *p++ = '\n';
! lend = p + 76;
}
}
if (pos != 2)
{
! *p++ = _base64[(buf >> 18) & 0x3f];
! *p++ = _base64[(buf >> 12) & 0x3f];
! *p++ = (pos == 0) ? _base64[(buf >> 6) & 0x3f] : '=';
! *p++ = '=';
}
-
- return p - dst;
}
/* probably should use lookup table */
--- 65,92
*/
if (pos < 0)
{
! appendStringInfoCharMacro(dst, _base64[(buf >> 18) &
0x3f]);
! appendStringInfoCharMacro(dst, _base64[(buf >> 12) &
0x3f]);
! appendStringInfoCharMacro(dst, _base64[(buf >> 6) &
0x3f]);
! appendStringInfoCharMacro(dst, _base64[buf & 0x3f]);
+ linepos += 4;
pos = 2;
buf = 0;
}
! if (linepos >= 76)
{
! appendStringInfoCharMacro(dst, '\n');
! linepos = 0;
}
}
if (pos != 2)
{
! appendStringInfoCharMacro(dst, _base64[(buf >> 18) & 0x3f]);
! appendStringInfoCharMacro(dst, _base64[(buf >> 12) & 0x3f]);
! appendStringInfoCharMacro(dst, (pos == 0) ? _base64[(buf >> 6)
& 0x3f] : '=');
! appendStringInfoCharMacro(dst, '=');
}
}
/* probably should use lookup table */
***
*** 160,174 b64_decode(const uint8 *src, unsigned len, uint8 *dst)
}
static unsigned
- b64_enc_len(unsigned srclen)
- {
- /*
-* 3 bytes will be converted to 4, linefeed after 76 chars
-*/
- return (srclen + 2) * 4 / 3 + srclen / (76 * 3 / 4);
- }
-
- static unsigned
b64_dec_len(unsigne
Re: [HACKERS] pgcrypto: PGP armor headers
On 9/9/14 10:54 AM, Heikki Linnakangas wrote: So I think this (and the corresponding dearmor code too) should be refactored to use a StringInfo that gets enlarged as needed, instead of hoping to guess the size correctly beforehand. To ease review, might make sense to do that as a separate patch over current sources, and the main patch on top of that. Yeah, I thought the same thing while writing that code. Thanks, I'll do it that way. BTW, I'm surprised that there is no function to get all the armor headers. You can only probe for a particular one with pgp_armor_headder, but there is no way to list them all, if you don't know what you're looking for. Right. The assumption was that due to the nature of how the headers are used, the user would always be looking for a certain, predetermined set of values. But I don't oppose to adding a pgp_armor_header_keys() function to get the set of all keys, for example. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On 08/15/2014 11:55 AM, Marko Tiikkaja wrote:
Hi,
On 8/8/14 3:18 PM, I wrote:
Currently there's no way to generate or extract armor headers from the
PGP armored format in pgcrypto. I've written a patch to add the
support.
Latest version of the patch here, having fixed some small coding issues.
This coding style gives me the willies:
guess_len = pgp_armor_enc_len(data_len, num_headers, keys, values);
res = palloc(VARHDRSZ + guess_len);
res_len = pgp_armor_encode((uint8 *) VARDATA(data), data_len,
(uint8 *)
VARDATA(res),
num_headers, keys,
values);
if (res_len > guess_len)
ereport(ERROR,
(errcode(ERRCODE_EXTERNAL_ROUTINE_INVOCATION_EXCEPTION),
errmsg("Overflow - encode estimate too
small")));
That was OK before this patch, as the length calculation was simple
enough to verify (although if I were writing it from scratch, I would've
written it differently). But with this patch, it gets a lot more
complicated, and I can't easily convince myself that it's correct.
pgp_armor_enc_len might be vulnerable to integer overflow. Consider 1GB
worth of keys, 1GB worth of values, and 1GB worth of data. I'm not sure
if you can quite make it overflow a 32-bit unsigned integer, but at
least you can get nervously close, e.g if you use use max-sized
key/value arrays, with a single byte in each key and value. Oh, and if
you use a single-byte server encoding and characters that get expanded
to multiple bytes in UTF-8, you can go higher.
So I think this (and the corresponding dearmor code too) should be
refactored to use a StringInfo that gets enlarged as needed, instead of
hoping to guess the size correctly beforehand. To ease review, might
make sense to do that as a separate patch over current sources, and the
main patch on top of that.
BTW, I'm surprised that there is no function to get all the armor
headers. You can only probe for a particular one with pgp_armor_headder,
but there is no way to list them all, if you don't know what you're
looking for.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
Marko, et al, This is a review of the pgcrypto PGP Armor Headers patch: http://www.postgresql.org/message-id/53edcae8.20...@joh.to Contents & Purpose == This patch add functions to create and extract OpenPGP Armor Headers. from OpenPGP messages. Included in the patch are updated regression test cases and documentation. Initial Run === The patch applies cleanly to HEAD. 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. On Fri, Aug 15, 2014 at 10:55 AM, Marko Tiikkaja wrote: > Hi, > > > On 8/8/14 3:18 PM, I wrote: >> >> Currently there's no way to generate or extract armor headers from the >> PGP armored format in pgcrypto. I've written a patch to add the >> support. > > > Latest version of the patch here, having fixed some small coding issues. > > > .marko > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP armor headers
On Fri, Aug 15, 2014 at 1:55 AM, Marko Tiikkaja wrote: > Hi, > > > On 8/8/14 3:18 PM, I wrote: > >> Currently there's no way to generate or extract armor headers from the >> PGP armored format in pgcrypto. I've written a patch to add the >> support. >> > > Latest version of the patch here, having fixed some small coding issues. I've built this and tested the installation of the extension, the upgrade from earlier versions, and the basic functions, with and without --enable-cassert I did occasionally get some failures with 'make check -C contrib/pgcrypto', but I can't reproduce it. I might have accidentally had some mixture of binaries some with cassert and some without. No other problems to report. I didn't do a detailed code review, and I am not a security expert, so I will leave it to the signed-up reviewer to change the status once he takes a look. One quibble in the documentation, "an error is returned". Errors get raised, not returned. This patch will conflict with the pgp signature patch due to the pgcrypto--1.2.sql and kin. Cheers, Jeff
Re: [HACKERS] pgcrypto: PGP armor headers
Hi,
On 8/8/14 3:18 PM, I wrote:
Currently there's no way to generate or extract armor headers from the
PGP armored format in pgcrypto. I've written a patch to add the
support.
Latest version of the patch here, having fixed some small coding issues.
.marko
*** a/contrib/pgcrypto/Makefile
--- b/contrib/pgcrypto/Makefile
***
*** 26,32 MODULE_big = pgcrypto
OBJS = $(SRCS:.c=.o) $(WIN32RES)
EXTENSION = pgcrypto
! DATA = pgcrypto--1.1.sql pgcrypto--1.0--1.1.sql pgcrypto--unpackaged--1.0.sql
PGFILEDESC = "pgcrypto - cryptographic functions"
REGRESS = init md5 sha1 hmac-md5 hmac-sha1 blowfish rijndael \
--- 26,32
OBJS = $(SRCS:.c=.o) $(WIN32RES)
EXTENSION = pgcrypto
! DATA = pgcrypto--1.2.sql pgcrypto--1.1--1.2.sql pgcrypto--1.0--1.1.sql
pgcrypto--unpackaged--1.0.sql
PGFILEDESC = "pgcrypto - cryptographic functions"
REGRESS = init md5 sha1 hmac-md5 hmac-sha1 blowfish rijndael \
*** a/contrib/pgcrypto/expected/pgp-armor.out
--- b/contrib/pgcrypto/expected/pgp-armor.out
***
*** 102,104 em9va2E=
--- 102,362
-END PGP MESSAGE-
');
ERROR: Corrupt ascii-armor
+ -- corrupt
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ foo:
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ ERROR: Corrupt ascii-armor
+ -- empty
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ foo:
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+
+ (1 row)
+
+ -- simple
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ foo: bar
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+ bar
+ (1 row)
+
+ -- uninteresting keys, part 1
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ foo: found
+ bar: ignored
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+ found
+ (1 row)
+
+ -- uninteresting keys, part 2
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ bar: ignored
+ foo: found
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+ found
+ (1 row)
+
+ -- uninteresting keys, part 3
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ bar: ignored
+ foo: found
+ bar: ignored
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+ found
+ (1 row)
+
+ -- insane keys, part 1
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ insane:key :
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'insane:key ');
+ pgp_armor_header
+ --
+
+ (1 row)
+
+ -- insane keys, part 2
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ insane:key : text value here
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'insane:key ');
+ pgp_armor_header
+ --
+ text value here
+ (1 row)
+
+ -- long value
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ long: this value is more than 76 characters long, but it should still parse
correctly as that''s permitted by RFC 4880
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'long');
+ pgp_armor_header
+
-
+ this value is more than 76 characters long, but it should still parse
correctly as that's permitted by RFC 4880
+ (1 row)
+
+ -- long value, split up
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ long: this value is more than 76 characters long, but it should still
+ long: parse correctly as that''s permitted by RFC 4880
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'long');
+ pgp_armor_header
+
-
+ this value is more than 76 characters long, but it should still parse
correctly as that's permitted by RFC 4880
+ (1 row)
+
+ -- long value, split up, part 2
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ long: this value is more than
+ long: 76 characters long, but it should still
+ long: parse correctly as that''s permitted by RFC 4880
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'long');
+ pgp_armor_header
+
-
+ this value is more than 76 characters long, but it should still parse
correctly as that's permitted by RFC 4880
+ (1 row)
+
+ -- long value, split up, part 3
+ select pgp_armor_header('
+ -BEGIN PGP MES
[HACKERS] pgcrypto: PGP armor headers
Hi,
Currently there's no way to generate or extract armor headers from the
PGP armored format in pgcrypto. I've written a patch to add the
support. For example:
local:marko=#* select armor('zooka', array['Version', 'Comment'],
array['Created by pgcrypto', 'PostgreSQL, the database']);
armor
---
-BEGIN PGP MESSAGE- +
Version: Created by pgcrypto +
Comment: PostgreSQL, the database+
+
em9va2E= +
=D5cR+
-END PGP MESSAGE-+
local:marko=#* select pgp_armor_header(armor('zooka', array['Version',
'Comment'], array['Created by pgcrypto', 'PostgreSQL, the database']),
'Comment');
pgp_armor_header
--
PostgreSQL, the database
(1 row)
.marko
*** a/contrib/pgcrypto/Makefile
--- b/contrib/pgcrypto/Makefile
***
*** 26,32 MODULE_big = pgcrypto
OBJS = $(SRCS:.c=.o) $(WIN32RES)
EXTENSION = pgcrypto
! DATA = pgcrypto--1.1.sql pgcrypto--1.0--1.1.sql pgcrypto--unpackaged--1.0.sql
PGFILEDESC = "pgcrypto - cryptographic functions"
REGRESS = init md5 sha1 hmac-md5 hmac-sha1 blowfish rijndael \
--- 26,32
OBJS = $(SRCS:.c=.o) $(WIN32RES)
EXTENSION = pgcrypto
! DATA = pgcrypto--1.2.sql pgcrypto--1.0--1.1.sql pgcrypto--unpackaged--1.0.sql
PGFILEDESC = "pgcrypto - cryptographic functions"
REGRESS = init md5 sha1 hmac-md5 hmac-sha1 blowfish rijndael \
*** a/contrib/pgcrypto/expected/pgp-armor.out
--- b/contrib/pgcrypto/expected/pgp-armor.out
***
*** 102,104 em9va2E=
--- 102,362
-END PGP MESSAGE-
');
ERROR: Corrupt ascii-armor
+ -- corrupt
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ foo:
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ ERROR: Corrupt ascii-armor
+ -- empty
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ foo:
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+
+ (1 row)
+
+ -- simple
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ foo: bar
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+ bar
+ (1 row)
+
+ -- uninteresting keys, part 1
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ foo: found
+ bar: ignored
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+ found
+ (1 row)
+
+ -- uninteresting keys, part 2
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ bar: ignored
+ foo: found
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+ found
+ (1 row)
+
+ -- uninteresting keys, part 3
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ bar: ignored
+ foo: found
+ bar: ignored
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'foo');
+ pgp_armor_header
+ --
+ found
+ (1 row)
+
+ -- insane keys, part 1
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ insane:key :
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'insane:key ');
+ pgp_armor_header
+ --
+
+ (1 row)
+
+ -- insane keys, part 2
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ insane:key : text value here
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'insane:key ');
+ pgp_armor_header
+ --
+ text value here
+ (1 row)
+
+ -- long value
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ long: this value is more than 76 characters long, but it should still parse
correctly as that''s permitted by RFC 4880
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'long');
+ pgp_armor_header
+
-
+ this value is more than 76 characters long, but it should still parse
correctly as that's permitted by RFC 4880
+ (1 row)
+
+ -- long value, split up
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ long: this value is more than 76 characters long, but it should still
+ long: parse correctly as that''s permitted by RFC 4880
+
+ em9va2E=
+ =
+ -END PGP MESSAGE-
+ ', 'long');
+ pgp_armor_header
+
-
+ this value is more than 76 characters long, but it should still parse
correctly as that's permitted by RFC 4880
+ (1 row)
+
+ -- long value, split up, part 2
+ select pgp_armor_header('
+ -BEGIN PGP MESSAGE-
+ long: this value is more than
+ long: 76 characters long, but it should still
