User functions for building SCRAM secrets

"Jonathan S. Katz" <jkatz@postgresql.org> writes:

Attached is a (draft) patch that adds a function called
"scram_build_secret_sha256" that can take 3 arguments:

This seems like a reasonable piece of functionality, I just have one
comment on the implementation.

* password (text) - a plaintext password
* salt (text) - a base64 encoded salt

[…]

+	/*
+	 * determine if this a valid base64 encoded string
+	 * TODO: look into refactoring the SCRAM decode code in libpq/auth-scram.c
+	 */
+	salt_str_dec_len = pg_b64_dec_len(strlen(salt_str_enc));
+	salt_str_dec = palloc(salt_str_dec_len);
+	salt_str_dec_len = pg_b64_decode(salt_str_enc, strlen(salt_str_enc),
+								salt_str_dec, salt_str_dec_len);
+	if (salt_str_dec_len < 0)
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_DATA_EXCEPTION),
+				 errmsg("invalid base64 encoded string"),
+				 errhint("Use the \"encode\" function to convert to valid base64 string.")));
+	}

Instead of going through all these machinations to base64-decode the
salt and tell the user off if they encoded it wrong, why not accept the
binary salt directly as a bytea?

- ilmari

On 10/31/22 6:05 PM, Dagfinn Ilmari Mannsåker wrote:

* password (text) - a plaintext password
* salt (text) - a base64 encoded salt

[…]

+	/*
+	 * determine if this a valid base64 encoded string
+	 * TODO: look into refactoring the SCRAM decode code in libpq/auth-scram.c
+	 */
+	salt_str_dec_len = pg_b64_dec_len(strlen(salt_str_enc));
+	salt_str_dec = palloc(salt_str_dec_len);
+	salt_str_dec_len = pg_b64_decode(salt_str_enc, strlen(salt_str_enc),
+								salt_str_dec, salt_str_dec_len);
+	if (salt_str_dec_len < 0)
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_DATA_EXCEPTION),
+				 errmsg("invalid base64 encoded string"),
+				 errhint("Use the \"encode\" function to convert to valid base64 string.")));
+	}

Instead of going through all these machinations to base64-decode the
salt and tell the user off if they encoded it wrong, why not accept the
binary salt directly as a bytea?

If we did that, I think we'd have to offer both. Most users are going to
be manipulating the salt as a base64 string, both because of 1/ how we
store it within the SCRAM secret and 2/ it's convenient in many
languages to work with base64 encoded binary data.

So in that case, we'd still have to go through the "base64 machinations".

However, I'd be OK with allowing for users to specify a "bytea" salt in
addition to a base64 one if that seems reasonable. I would be -1 for
swapping the base64 salt for just a "bytea" one as I think that would
present usability challenges.

Thanks,

Jonathan

On Mon, Oct 31, 2022 at 04:27:08PM -0400, Jonathan S. Katz wrote:

1. password only -- this defers to the PG defaults for SCRAM
2. password + salt -- this is useful for the password history / dictionary
case to allow for a predictable way to check a password.

Well, one could pass a salt based on something generated by random()
to emulate what we currently do in the default case, as well. The
salt length is an extra possibility, letting it be randomly generated
by pg_strong_random().

1. Location of the functions. I put them in
src/backend/utils/adt/cryptohashfuncs.c as I wasn't sure where it would make
sense to have them (and they could easily go into their own file).

As of adt/authfuncs.c? cryptohashfuncs.c does not strike me as a good
fit.

Please let me know if you have any questions. I'll add a CF entry for this.

+{ oid => '8555', descr => 'Build a SCRAM secret',
+  proname => 'scram_build_secret_sha256', proleakproof => 't', prorettype => 'text',
+  proargtypes => 'text', prosrc => 'scram_build_secret_sha256_from_password' },
+{ oid => '8556', descr => 'Build a SCRAM secret',
+  proname => 'scram_build_secret_sha256', proleakproof => 't',
+  provolatile => 'i', prorettype => 'text',
+  proargtypes => 'text text', prosrc => 'scram_build_secret_sha256_from_password_and_salt' },
+{ oid => '8557', descr => 'Build a SCRAM secret',
+  proname => 'scram_build_secret_sha256', proleakproof => 't',
+  provolatile => 'i', prorettype => 'text',
+  proargtypes => 'text text int4', prosrc => 'scram_build_secret_sha256_from_password_and_salt_and_iterations' },

Keeping this approach as-is, I don't think that you should consume 3
OIDs, but 1 (with scram_build_secret_sha256_from_password_and_..
as prosrc) that has two defaults for the second argument (salt string,
default as NULL) and third argument (salt, default at 0), with the
defaults set up in system_functions.sql via a redefinition.

Note that you cannot pass down an expression for the password of
CREATE/ALTER USER, meaning that this would need a \gset at least if
done by a psql client for example, and passing down a password string
is not an encouraged practice, either. Another approach is to also
provide a role OID in input and store the newly-computed password in
pg_authid (as of [1]https://github.com/michaelpq/pg_plugins/blob/main/scram_utils/scram_utils.c -- Michael), so as one can store it easily.

Did you look at extending \password? Being able to extend
PQencryptPasswordConn() with custom parameters has been discussed in
the past, but this has gone nowhere. That's rather unrelated to what
you are looking for, just mentioning as we are playing with options to
have control the iteration number and the salt.

[1]: https://github.com/michaelpq/pg_plugins/blob/main/scram_utils/scram_utils.c -- Michael
--
Michael

On Mon, Oct 31, 2022 at 1:27 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:

Having a set of SCRAM secret building functions would help in a few areas:

I have mixed-to-negative feelings about this. Orthogonality with other
methods seems reasonable, except we don't really recommend that people
use those other methods today. SCRAM is supposed to be one of the
solutions where the server does not know your password at any point
and cannot impersonate you to others.

If we don't provide an easy client-side equivalent for the new
functionality, via \password or some such, then the path of least
resistance for some of these intermediate use cases (i.e. higher
iteration count) will be "just get used to sending your password in
plaintext," and that doesn't really sound all that great. Similar to
pgcrypto's current state.

2. Keeping a history file of user-stored passwords

Could you expand on this? How does being able to generate SCRAM
secrets help you keep a password history?

or checking against a common-password dictionary.

People really want to do this using SQL? Maybe my idea of the use case
is way off, but I'm skeptical that this scales (safely and/or
performantly) to a production system, *especially* if you have your
iteration counts high enough.

3. Allowing users to build SQL-functions that can precompute SCRAM
secrets on a local server before sending it to a remote server.

I guess I have fewer problems with this use case in theory, but I'm
wondering if better client-side support might also solve this one as
well, without the additional complication. Is there a reason it would
not?

Thanks,
--Jacob

On Tue, Nov 1, 2022 at 4:02 PM Jacob Champion <jchampion@timescale.com> wrote:

I guess I have fewer problems with this use case in theory, but I'm
wondering if better client-side support might also solve this one as
well, without the additional complication. Is there a reason it would
not?

To expand on this question, after giving it some more thought:

It seems to me that the use case here is extremely similar to the one
being tackled by Peter E's client-side encryption [1]/messages/by-id/89157929-c2b6-817b-6025-8e4b2d89d88f@enterprisedb.com. People want to
write SQL to perform a cryptographic operation using a secret, and
then send the resulting ciphertext (or in this case, a one-way hash)
to the server, but ideally the server should not actually have the
secret.

I don't think it's helpful for me to try to block progress on this
patchset behind the other one. But is there a way for me to help this
proposal skate in the same general direction? Could Peter's encryption
framework expand to fit this case in the future?

Thanks,
--Jacob

[1]: /messages/by-id/89157929-c2b6-817b-6025-8e4b2d89d88f@enterprisedb.com

On 04.11.22 21:39, Jacob Champion wrote:

It seems to me that the use case here is extremely similar to the one
being tackled by Peter E's client-side encryption [1]. People want to
write SQL to perform a cryptographic operation using a secret, and
then send the resulting ciphertext (or in this case, a one-way hash)
to the server, but ideally the server should not actually have the
secret.

It might be possible, but it's a bit of a reach. For instance, there
are no keys and no decryption associated with this kind of operation.

I don't think it's helpful for me to try to block progress on this
patchset behind the other one. But is there a way for me to help this
proposal skate in the same general direction? Could Peter's encryption
framework expand to fit this case in the future?

We already have support in libpq for doing this (PQencryptPasswordConn()).

On 11/8/22 12:26, Peter Eisentraut wrote:

On 04.11.22 21:39, Jacob Champion wrote:

I don't think it's helpful for me to try to block progress on this
patchset behind the other one. But is there a way for me to help this
proposal skate in the same general direction? Could Peter's encryption
framework expand to fit this case in the future?

We already have support in libpq for doing this (PQencryptPasswordConn()).

Sure, but you can't access that in SQL, right? The hand-wavy part is to
combine that existing function with your transparent encryption
proposal, as a special-case encryptor whose output could be bound to the
query.

But I guess that wouldn't really help with ALTER ROLE ... PASSWORD,
because you can't parameterize it. Hm...

--Jacob

On Tue, Nov 08, 2022 at 04:57:09PM -0800, Jacob Champion wrote:

But I guess that wouldn't really help with ALTER ROLE ... PASSWORD,
because you can't parameterize it. Hm...

Yeah, and I'd like to think that this is never something we should
allow, either, as that could be easily a footgun for users (?).
--
Michael

On Tue, Nov 8, 2022 at 9:28 PM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Nov 08, 2022 at 04:57:09PM -0800, Jacob Champion wrote:

But I guess that wouldn't really help with ALTER ROLE ... PASSWORD,
because you can't parameterize it. Hm...

Yeah, and I'd like to think that this is never something we should
allow, either, as that could be easily a footgun for users (?).

What would make it unsafe? I don't know a lot about the tradeoffs for
parameterizing queries.

--Jacob

On 10/31/22 8:56 PM, Michael Paquier wrote:

On Mon, Oct 31, 2022 at 04:27:08PM -0400, Jonathan S. Katz wrote:

1. password only -- this defers to the PG defaults for SCRAM
2. password + salt -- this is useful for the password history / dictionary
case to allow for a predictable way to check a password.

Well, one could pass a salt based on something generated by random()
to emulate what we currently do in the default case, as well. The
salt length is an extra possibility, letting it be randomly generated
by pg_strong_random().

Sure, this is a good point. From a SQL level we can get that from
pgcrypto "gen_random_bytes".

Per this and ilmari's feedback, I updated the 2nd argument to be a
bytea. See the corresponding tests that then show using decode(...,
'base64') to handle this.

When I write the docs, I'll include that in the examples.

1. Location of the functions. I put them in
src/backend/utils/adt/cryptohashfuncs.c as I wasn't sure where it would make
sense to have them (and they could easily go into their own file).

As of adt/authfuncs.c? cryptohashfuncs.c does not strike me as a good
fit.

I went with your suggested name.

Please let me know if you have any questions. I'll add a CF entry for this.

+{ oid => '8555', descr => 'Build a SCRAM secret',
+  proname => 'scram_build_secret_sha256', proleakproof => 't', prorettype => 'text',
+  proargtypes => 'text', prosrc => 'scram_build_secret_sha256_from_password' },
+{ oid => '8556', descr => 'Build a SCRAM secret',
+  proname => 'scram_build_secret_sha256', proleakproof => 't',
+  provolatile => 'i', prorettype => 'text',
+  proargtypes => 'text text', prosrc => 'scram_build_secret_sha256_from_password_and_salt' },
+{ oid => '8557', descr => 'Build a SCRAM secret',
+  proname => 'scram_build_secret_sha256', proleakproof => 't',
+  provolatile => 'i', prorettype => 'text',
+  proargtypes => 'text text int4', prosrc => 'scram_build_secret_sha256_from_password_and_salt_and_iterations' },

Keeping this approach as-is, I don't think that you should consume 3
OIDs, but 1 (with scram_build_secret_sha256_from_password_and_..
as prosrc) that has two defaults for the second argument (salt string,
default as NULL) and third argument (salt, default at 0), with the
defaults set up in system_functions.sql via a redefinition.

Thanks for the suggestion. I went with this as well.

Note that you cannot pass down an expression for the password of
CREATE/ALTER USER, meaning that this would need a \gset at least if
done by a psql client for example, and passing down a password string
is not an encouraged practice, either. Another approach is to also
provide a role OID in input and store the newly-computed password in
pg_authid (as of [1]), so as one can store it easily.

...unless you dynamically generate the CREATE/ALTER ROLE command ;) (and
yes, lots of discussion on that).

Did you look at extending \password? Being able to extend
PQencryptPasswordConn() with custom parameters has been discussed in
the past, but this has gone nowhere. That's rather unrelated to what
you are looking for, just mentioning as we are playing with options to
have control the iteration number and the salt.

Not yet, but happy to do that as a follow-up patch.

Please see version 2. If folks are generally happy with this, I'll
address any additional feedback and write up docs.

Thanks,

Jonathan

On Thu, Nov 10, 2022 at 11:14:34PM -0500, Jonathan S. Katz wrote:

On 10/31/22 8:56 PM, Michael Paquier wrote:

Well, one could pass a salt based on something generated by random()
to emulate what we currently do in the default case, as well. The
salt length is an extra possibility, letting it be randomly generated
by pg_strong_random().

Sure, this is a good point. From a SQL level we can get that from pgcrypto
"gen_random_bytes".

Could it be something we could just push into core? FWIW, I've used
that quite a bit in the last to cheaply build long random strings of
data for other things. Without pgcrypto, random() with
generate_series() has always been kind of.. fun.

+SELECT scram_build_secret_sha256(NULL);
+ERROR:  password must not be null
+SELECT scram_build_secret_sha256(NULL, NULL);
+ERROR:  password must not be null
+SELECT scram_build_secret_sha256(NULL, NULL, NULL);
+ERROR:  password must not be null

This is just testing three times the same thing as per the defaults.
I would cut the second and third cases.

git diff --check reports some whitespaces.

scram_build_secret_sha256_internal() is missing SASLprep on the
password string. Perhaps the best thing to do here is just to extend
pg_be_scram_build_secret() with more arguments so as callers can
optionally pass down a custom salt with its length, leaving the
responsibility to pg_be_scram_build_secret() to create a random salt
if nothing has been given?
--
Michael

On 11/16/22 10:09 PM, Michael Paquier wrote:

On Thu, Nov 10, 2022 at 11:14:34PM -0500, Jonathan S. Katz wrote:

On 10/31/22 8:56 PM, Michael Paquier wrote:

Well, one could pass a salt based on something generated by random()
to emulate what we currently do in the default case, as well. The
salt length is an extra possibility, letting it be randomly generated
by pg_strong_random().

Sure, this is a good point. From a SQL level we can get that from pgcrypto
"gen_random_bytes".

Could it be something we could just push into core? FWIW, I've used
that quite a bit in the last to cheaply build long random strings of
data for other things. Without pgcrypto, random() with
generate_series() has always been kind of.. fun.

I would be a +1 for moving that into core, given we did something
similar with gen_random_uuid[1]https://www.postgresql.org/docs/current/functions-uuid.html. Separate patch, of course :)

+SELECT scram_build_secret_sha256(NULL);
+ERROR:  password must not be null
+SELECT scram_build_secret_sha256(NULL, NULL);
+ERROR:  password must not be null
+SELECT scram_build_secret_sha256(NULL, NULL, NULL);
+ERROR:  password must not be null

This is just testing three times the same thing as per the defaults.
I would cut the second and third cases.

AFAICT it's not returning the defaults. Quick other example:

CREATE FUNCTION ab (a int DEFAULT 0) RETURNS int AS $$ SELECT a; $$
LANGUAGE SQL;

SELECT ab();
ab
----
0
(1 row)

SELECT ab(NULL::int);
ab
----

(1 row)

Given scram_build_secret_sha256 is not a strict function, I'd prefer to
test all of the NULL cases in case anything in the underlying code
changes in the future. It's a cheap cost to be a bit more careful.

git diff --check reports some whitespaces.

Ack. Will fix on the next pass. (I've been transitioning editors, which
could have resulted in that),

scram_build_secret_sha256_internal() is missing SASLprep on the
password string. Perhaps the best thing to do here is just to extend
pg_be_scram_build_secret() with more arguments so as callers can
optionally pass down a custom salt with its length, leaving the
responsibility to pg_be_scram_build_secret() to create a random salt
if nothing has been given?

Ah, good catch!

I think if we go with passing down the salt, we'd also have to allow for
the passing down of the iterations, too, and we're close to rebuilding
"scram_build_secret". I'll stare a bit at this on the next pass and
either 1/ just SASLprep the string in the new
"scram_build_secret_sha256_internal" func, or 2/ change the definition
of "pg_be_scram_build_secret" to accommodate more overrides.

Thanks,

Jonathan

[1]: https://www.postgresql.org/docs/current/functions-uuid.html

On 11/26/22 2:53 PM, Jonathan S. Katz wrote:

On 11/16/22 10:09 PM, Michael Paquier wrote:

git diff --check reports some whitespaces.

Ack. Will fix on the next pass. (I've been transitioning editors, which
could have resulted in that),

Fixed (and have run that check subsequently).

scram_build_secret_sha256_internal() is missing SASLprep on the
password string.  Perhaps the best thing to do here is just to extend
pg_be_scram_build_secret() with more arguments so as callers can
optionally pass down a custom salt with its length, leaving the
responsibility to pg_be_scram_build_secret() to create a random salt
if nothing has been given?

Ah, good catch!

I think if we go with passing down the salt, we'd also have to allow for
the passing down of the iterations, too, and we're close to rebuilding
"scram_build_secret". I'll stare a bit at this on the next pass and
either 1/ just SASLprep the string in the new
"scram_build_secret_sha256_internal" func, or 2/ change the definition
of "pg_be_scram_build_secret" to accommodate more overrides.

In the end I went with your suggested approach as it limited the amount
of code duplication. I did keep in all the permutations of the tests as
it did help me catch an error in my code that let to a panic.

As this seems to be closer to completion, I did include docs in this
patch. I added this function as part of the "string functions" section
of the docs as "md5" was already there. If we continue to add more
authentication helper functions, perhaps we should consider breaking
those out into their own documentation section.

Thanks,

Jonathan

On 27 Nov 2022, at 06:21, Jonathan S. Katz <jkatz@postgresql.org> wrote:
On 11/26/22 2:53 PM, Jonathan S. Katz wrote:

On 11/16/22 10:09 PM, Michael Paquier wrote:

git diff --check reports some whitespaces.

Ack. Will fix on the next pass. (I've been transitioning editors, which could have resulted in that),

Fixed (and have run that check subsequently).

The spaces-before-tabs that git is complaining about are gone but there are
still whitespace issues like scram_build_secret_sha256() which has a mix of
two-space and tab indentation. I recommend taking it for a spin with pgindent.

Sorry for not noticing the thread earlier, below are some review comments and

+ SCRAM secret equilvaent to what is stored in
s/equilvaent/equivalent/

+        <literal>SELECT scram_build_secret_sha256('secret password', '\xabba5432');</literal>
+        <returnvalue></returnvalue>
+<programlisting>
+  SCRAM-SHA-256$4096:q7pUMg==$05Nb9QHwHkMA0CRcYaEfwtgZ+3kStIefz8fLMjTEtio=:P126h1ycyP938E69yxktEfhoAILbiwL/UMsMk3Efb6o=
+</programlisting>
Shouldn't the function output be inside <returnvalue></returnvalue>?  IIRC the
use if <programlisting> with an empty <returnvalue> is for multiline output,
but I'm not 100% sure there.

+	if (iterations <= 0)
+		iterations = SCRAM_DEFAULT_ITERATIONS;
According to the RFC, the iteration-count "SHOULD be at least 4096", so we can
reduce it, but do we gain anything by allowing users to set it lower?  If so,
scram_build_secret() is already converting (iterations <= 0) to the default so
there is no need to duplicate that logic.

Personally I'd prefer if we made 4096 the minimum and only allowed higher
regardless of the fate of this patch, but thats for another thread.

+ Assert(secret != NULL);
I don't think there are any paths where this is possible to trigger, did you
see any?

On the whole I tend to agree with Jacob upthread, while this does provide
consistency it doesn't seem to move the needle for best practices. Allowing
scram_build_secret_sha256('password', 'a', 1); with the password potentially
going in cleartext over the wire and into the logs doesn't seem like a great
tradeoff for the (IMHO) niche usecases it would satisfy.

--
Daniel Gustafsson https://vmware.com/

On Tue, Nov 29, 2022 at 09:32:34PM +0100, Daniel Gustafsson wrote:

On the whole I tend to agree with Jacob upthread, while this does provide
consistency it doesn't seem to move the needle for best practices. Allowing
scram_build_secret_sha256('password', 'a', 1); with the password potentially
going in cleartext over the wire and into the logs doesn't seem like a great
tradeoff for the (IMHO) niche usecases it would satisfy.

Should we try to make \password and libpq more flexible instead? Two
things got discussed in this area since v10:
- The length of the random salt.
- The iteration number.

Or we could bump up the defaults, and come back to that in a few
years, again.. ;p
--
Michael

On 11/29/22 8:12 PM, Michael Paquier wrote:

On Tue, Nov 29, 2022 at 09:32:34PM +0100, Daniel Gustafsson wrote:

On the whole I tend to agree with Jacob upthread, while this does provide
consistency it doesn't seem to move the needle for best practices. Allowing
scram_build_secret_sha256('password', 'a', 1); with the password potentially
going in cleartext over the wire and into the logs doesn't seem like a great
tradeoff for the (IMHO) niche usecases it would satisfy.

Should we try to make \password and libpq more flexible instead? Two
things got discussed in this area since v10:
- The length of the random salt.
- The iteration number.

Or we could bump up the defaults, and come back to that in a few
years, again.. ;p

Here is another attempt at this patch that takes into account the SCRAM
code refactor. I addressed some of Daniel's previous feedback, but will
need to make another pass on the docs and the assert trace as the main
focus of this revision was bringing the code inline with the recent changes.

This patch changes the function name to "scram_build_secret" and now
accepts a new parameter of hash type. This sets it up to handle
additional hashes in the future.

I do agree we should make libpq more flexible, but as mentioned in the
original thread, this does not solve the *server side* cases where a
user needs to build a SCRAM secret. For example, being able to
precompute hashes on one server before sending them to another server,
which can require no plaintext passwords if the server is randomly
generating the data.

Another use case comes from the "pg_tle" project, specifically with the
ability to write a "check_password_hook" from an available PL[1]https://github.com/aws/pg_tle/blob/main/docs/04_hooks.md. If a
user does follow our best practices and sends a pre-built SCRAM secret
over the wire, a hook can then verify that the secret is not contained
within a common password dictionary.

Thanks,

Jonathan

[1]: https://github.com/aws/pg_tle/blob/main/docs/04_hooks.md

Hi,

On 2023-01-23 00:58:40 -0500, Jonathan S. Katz wrote:

Here is another attempt at this patch that takes into account the SCRAM code
refactor. I addressed some of Daniel's previous feedback, but will need to
make another pass on the docs and the assert trace as the main focus of this
revision was bringing the code inline with the recent changes.

This reliably fails on CI:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F42%2F3988

I think this is related to encoding issues. The 32bit debian task
intentionally uses LANG=C. Resulting in failures like:
https://api.cirrus-ci.com/v1/artifact/task/6696410851049472/testrun/build-32/testrun/regress/regress/regression.diffs

Windows fails with a similar issue:
https://api.cirrus-ci.com/v1/artifact/task/5676064060473344/testrun/build/testrun/regress/regress/regression.diffs

I've set the patch as waiting on author for now.

- Andres

On 2/14/23 3:17 PM, Andres Freund wrote:

Hi,

On 2023-01-23 00:58:40 -0500, Jonathan S. Katz wrote:

Here is another attempt at this patch that takes into account the SCRAM code
refactor. I addressed some of Daniel's previous feedback, but will need to
make another pass on the docs and the assert trace as the main focus of this
revision was bringing the code inline with the recent changes.

This reliably fails on CI:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F42%2F3988

I think this is related to encoding issues. The 32bit debian task
intentionally uses LANG=C. Resulting in failures like:
https://api.cirrus-ci.com/v1/artifact/task/6696410851049472/testrun/build-32/testrun/regress/regress/regression.diffs

Windows fails with a similar issue:
https://api.cirrus-ci.com/v1/artifact/task/5676064060473344/testrun/build/testrun/regress/regress/regression.diffs

I've set the patch as waiting on author for now.

Thanks for the explanation. I'll work on fixing that in the next go round.

Jonathan

On 2/14/23 3:19 PM, Jonathan S. Katz wrote:

On 2/14/23 3:17 PM, Andres Freund wrote:

This reliably fails on CI:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F42%2F3988

I think this is related to encoding issues. The 32bit debian task
intentionally uses LANG=C. Resulting in failures like:
https://api.cirrus-ci.com/v1/artifact/task/6696410851049472/testrun/build-32/testrun/regress/regress/regression.diffs

Windows fails with a similar issue:
https://api.cirrus-ci.com/v1/artifact/task/5676064060473344/testrun/build/testrun/regress/regress/regression.diffs

Thanks for the explanation. I'll work on fixing that in the next go round.

(First -- I really like the current status of running the tests with
Meson. I'm finding it very easy to use -- doing the locale testing was
pretty easy too!)

I stared at this for a bit to see what we do in other regression tests
using unicode strings. I looked at the regression tests for strings[1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/test/regress/sql/strings.sql
and ICU collations[2]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/test/regress/sql/collate.icu.utf8.sql.

In "strings", all the escaped Unicode strings are in the low bits so
they're convertible to ASCII.

In the ICU test, it does a check to see if we're using UTF-8: if we're
not, it bails.

For this patch, the value of the failing test is to ensure that the
SCRAM function honors SASLprep when building the secret. It makes more
sense to use the current character (U+1680), which will be converted to
a space by the algorithm, vs. moving to U+0020 or something that does
not exercise the SASLprep code.

I opted for the approach in [2]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/test/regress/sql/collate.icu.utf8.sql. v5 contains the branching logic for the
UTF8 only tests, and the corresponding output files. I tested locally on
macOS against both UTF8 + C locales.

Thanks,

Jonathan

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/test/regress/sql/strings.sql
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/test/regress/sql/strings.sql
[2]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/test/regress/sql/collate.icu.utf8.sql
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/test/regress/sql/collate.icu.utf8.sql