Thread: User functions for building SCRAM secrets

Hi,

We currently do not provide any SQL functions for generating SCRAM 
secrets, whereas we have this support for other passwords types 
(plaintext and md5 via `md5(password || username)`). If a user wants to 
build a SCRAM secret via SQL, they have to implement our SCRAM hashing 
funcs on their own.

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

1. Ensuring we have a SQL-equivalent of CREATE/ALTER ROLE ... PASSWORD 
where we can compute a pre-hashed password.

2. Keeping a history file of user-stored passwords or checking against a 
common-password dictionary.

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

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

* password (text) - a plaintext password
* salt (text) - a base64 encoded salt
* iterations (int) - the number of iterations to hash the plaintext 
password.

There are three variations of the function:

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.
3. password + salt + iterations -- this allows the user to modify the 
number of iterations to hash a password.

The design of the patch primarily delegates to the existing SCRAM secret 
building code and provides a few wrapper functions around it that 
evaluate user input.

There are a few open items on this patch, i.e.:

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).

2. I noticed a common set of base64 function calls that could possibly 
be refactored into one; I left a TODO comment around that.

3. More tests

4. Docs -- if it seems like we're OK with including these functions, 
I'll write these up.

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

Thanks,

Jonathan

P.S. I used this as a forcing function to get the meson build system set 
up and thus far I quite like it!

"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]), 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

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]. 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] https://www.postgresql.org/message-id/flat/89157929-c2b6-817b-6025-8e4b2d89d88f%40enterprisedb.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]. 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]. 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] 
and ICU collations[2].

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]. 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
[2] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/test/regress/sql/collate.icu.utf8.sql

On Tue, Feb 14, 2023 at 06:16:18PM -0500, Jonathan S. Katz wrote:
> I opted for the approach in [2]. 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.

I was reading this thread again, and pondered on this particular
point:
https://www.postgresql.org/message-id/CAAWbhmhjcFc4oaGA_7YLUhtj6J+rxEY+BoDryGzNdaFLGfZZMg@mail.gmail.com

We've had our share of complains over the years that Postgres logs
password data in the logs with various DDLs, so I'd tend to agree that
this is not a practice we should try to encourage more.  The
parameterization of the SCRAM verifiers through GUCs (like Daniel's
https://commitfest.postgresql.org/42/4201/ for the iteration number)
is more promising because it is possible to not have to send the
password over the wire with once we let libpq take care of the
computation, and the server would not know about that.
--
Michael

On 3/22/23 2:48 AM, Michael Paquier wrote:
> On Tue, Feb 14, 2023 at 06:16:18PM -0500, Jonathan S. Katz wrote:
>> I opted for the approach in [2]. 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.
> 
> I was reading this thread again, and pondered on this particular
> point:
> https://www.postgresql.org/message-id/CAAWbhmhjcFc4oaGA_7YLUhtj6J+rxEY+BoDryGzNdaFLGfZZMg@mail.gmail.com
> 
> We've had our share of complains over the years that Postgres logs
> password data in the logs with various DDLs, so I'd tend to agree that
> this is not a practice we should try to encourage more.  The
> parameterization of the SCRAM verifiers through GUCs (like Daniel's
> https://commitfest.postgresql.org/42/4201/ for the iteration number)
> is more promising because it is possible to not have to send the
> password over the wire with once we let libpq take care of the
> computation, and the server would not know about that

I generally agree with not allowing password data to be in logs, but in 
practice, there are a variety of tools and extensions that obfuscate or 
remove passwords from PostgreSQL logs. Additionally, this function is 
not targeted for SQL statements directly, but stored procedures.

For example, an extension like "pg_tle" exposes the ability for someone 
to write a "check_password_hook" directly from PL/pgSQL[1] (and other 
languages). As we've made it a best practice to pre-hash the password on 
the client-side, a user who wants to write a check password hook against 
a SCRAM verifier needs to be able to compare the verifier against some 
existing set of plaintext criteria, and has to write their own function 
to do it. I have heard several users who have asked to do this, and the 
only feedback I can give them is "implement your own SCRAM build secret 
function."

And, if my PostgreSQL server _is_ the client, e.g. it's making a dblink 
call to another PostgreSQL server, the only way it can modify a password 
is by sending the plaintext credential over the wire.

I don't see how the parameterization work applies here -- would we allow 
salts to be parameterized? -- and it still would not allow the server to 
build out a SCRAM secret for these cases.

Maybe I'm not conveying the problem this is solving -- I'm happy to go 
one more round trying to make it clearer -- but if this is not clear, 
it'd be good to at develop an alternative approach to this before 
withdrawing the patch.

Thanks,

Jonathan

[1] 

https://github.com/aws/pg_tle/blob/main/docs/06_plpgsql_examples.md#example-password-check-hook-against-bad-password-dictionary

> On 22 Mar 2023, at 15:06, Jonathan S. Katz <jkatz@postgresql.org> wrote:
> On 3/22/23 2:48 AM, Michael Paquier wrote:

>> I was reading this thread again, and pondered on this particular
>> point:
>> https://www.postgresql.org/message-id/CAAWbhmhjcFc4oaGA_7YLUhtj6J+rxEY+BoDryGzNdaFLGfZZMg@mail.gmail.com
>> We've had our share of complains over the years that Postgres logs
>> password data in the logs with various DDLs, so I'd tend to agree that
>> this is not a practice we should try to encourage more.  The
>> parameterization of the SCRAM verifiers through GUCs (like Daniel's
>> https://commitfest.postgresql.org/42/4201/ for the iteration number)
>> is more promising because it is possible to not have to send the
>> password over the wire with once we let libpq take care of the
>> computation, and the server would not know about that
>
> I generally agree with not allowing password data to be in logs, but in practice, there are a variety of tools and
extensionsthat obfuscate or remove passwords from PostgreSQL logs. Additionally, this function is not targeted for SQL
statementsdirectly, but stored procedures. 
>
> For example, an extension like "pg_tle" exposes the ability for someone to write a "check_password_hook" directly
fromPL/pgSQL[1] (and other languages). As we've made it a best practice to pre-hash the password on the client-side, a
userwho wants to write a check password hook against a SCRAM verifier needs to be able to compare the verifier against
someexisting set of plaintext criteria, and has to write their own function to do it. I have heard several users who
haveasked to do this, and the only feedback I can give them is "implement your own SCRAM build secret function." 

I'm not sure I follow; doesn't this - coupled with this patch - imply passing
the plaintext password from client to the server, hashing it with a known salt
and comparing this with something in plaintext hashed with the same known salt?
If so, that's admittedly not a usecase I am terribly excited about.  My
preference is to bring password checks to the plaintext password, not bring the
plaintext password to the password check.

> And, if my PostgreSQL server _is_ the client, e.g. it's making a dblink call to another PostgreSQL server, the only
wayit can modify a password is by sending the plaintext credential over the wire. 

My experience with dblink setups is too small to have much insight here, but I
(perhaps naively) assumed that dblink setups generally involved two servers
under the same control making such changes be possible out of band.

> Maybe I'm not conveying the problem this is solving -- I'm happy to go one more round trying to make it clearer --
butif this is not clear, it'd be good to at develop an alternative approach to this before withdrawing the patch. 

If this is mainly targeting extension use, is there a way in which an extension
could have all this functionality with no: a) not exposing any of it from the
server; b) not having to copy/paste lots into the extension and; c) have a
reasonable way to keep up with any changes made in the backend?

--
Daniel Gustafsson

On Fri, Mar 24, 2023 at 4:48 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 22 Mar 2023, at 15:06, Jonathan S. Katz <jkatz@postgresql.org> wrote:
> > On 3/22/23 2:48 AM, Michael Paquier wrote:
>
> >> I was reading this thread again, and pondered on this particular
> >> point:
> >> https://www.postgresql.org/message-id/CAAWbhmhjcFc4oaGA_7YLUhtj6J+rxEY+BoDryGzNdaFLGfZZMg@mail.gmail.com
> >> We've had our share of complains over the years that Postgres logs
> >> password data in the logs with various DDLs, so I'd tend to agree that
> >> this is not a practice we should try to encourage more.  The
> >> parameterization of the SCRAM verifiers through GUCs (like Daniel's
> >> https://commitfest.postgresql.org/42/4201/ for the iteration number)
> >> is more promising because it is possible to not have to send the
> >> password over the wire with once we let libpq take care of the
> >> computation, and the server would not know about that
> >
> > I generally agree with not allowing password data to be in logs, but in practice, there are a variety of tools and
extensionsthat obfuscate or remove passwords from PostgreSQL logs. Additionally, this function is not targeted for SQL
statementsdirectly, but stored procedures. 
> >
> > For example, an extension like "pg_tle" exposes the ability for someone to write a "check_password_hook" directly
fromPL/pgSQL[1] (and other languages). As we've made it a best practice to pre-hash the password on the client-side, a
userwho wants to write a check password hook against a SCRAM verifier needs to be able to compare the verifier against
someexisting set of plaintext criteria, and has to write their own function to do it. I have heard several users who
haveasked to do this, and the only feedback I can give them is "implement your own SCRAM build secret function." 
>
> I'm not sure I follow; doesn't this - coupled with this patch - imply passing
> the plaintext password from client to the server, hashing it with a known salt
> and comparing this with something in plaintext hashed with the same known salt?
> If so, that's admittedly not a usecase I am terribly excited about.  My
> preference is to bring password checks to the plaintext password, not bring the
> plaintext password to the password check.

Given how much we marketed, for good reasons, SCRAM as a way to
finally make postgres *not* do this, it seems like a really strange
path to take to go back to doing it again.

Having the function always generate a random salt seems more
reasonable though, and would perhaps be something that helps in some
of the cases? It won't help with the password policy one, as it's too
secure for that, but it would help with the postgres-is-the-client
one?


> > And, if my PostgreSQL server _is_ the client, e.g. it's making a dblink call to another PostgreSQL server, the only
wayit can modify a password is by sending the plaintext credential over the wire. 
>
> My experience with dblink setups is too small to have much insight here, but I
> (perhaps naively) assumed that dblink setups generally involved two servers
> under the same control making such changes be possible out of band.

I have seen a few, and certainly more FDW based ones these days. But
I'm not sure I've come across one yet where one wants to *change the
password* through dblink. Since it's server-to-server, most people
would just change the password on the target server and then update
the FDW/dblink configuration with the new password. (Of course, the
storage of that password on the FDW/dblink layer is a terrible thing
in the first place from a security perspective, but it's what we
have).


> > Maybe I'm not conveying the problem this is solving -- I'm happy to go one more round trying to make it clearer --
butif this is not clear, it'd be good to at develop an alternative approach to this before withdrawing the patch. 
>
> If this is mainly targeting extension use, is there a way in which an extension
> could have all this functionality with no: a) not exposing any of it from the
> server; b) not having to copy/paste lots into the extension and; c) have a
> reasonable way to keep up with any changes made in the backend?

I realize I forgot to write this reply back when the thread was alive,
so here's a zombie-awakener!

One way to accomplish this would be to create a new predefined role,
say pg_change_own_password, by default granted to public. When a user
is a member of this role they can, well, change their own password.
And it will be done using the full security of scram, without cutting
anything. For those that want to enforce a password policy or anything
else that requires the server to see the cleartext or
cleartext-equivalent of the password can then revoke this role from
public, and force password changes to go through a security definer
funciton, like SELECT pg_change_password_with_policy('joe',
'mysupersecretpasswrod').

This function can then be under the control of an extension or
whatever you want. If one had the client under control one could for
example do the policy validation on the client and pass it to the
backend with some internal hash as well -- this would be entirely
under the control of the application though, as a generic libpq
connection could and should not be considered "client under control"
in this case.

--
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On Tue, Apr 11, 2023 at 11:27:17AM +0200, Magnus Hagander wrote:
> Having the function always generate a random salt seems more
> reasonable though, and would perhaps be something that helps in some
> of the cases? It won't help with the password policy one, as it's too
> secure for that, but it would help with the postgres-is-the-client
> one?

While this is still hot..  Would it make sense to have a
scram_salt_length GUC to control the length of the salt used when
generating the SCRAM secret?
--
Michael

> On 14 Apr 2023, at 01:14, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Apr 11, 2023 at 11:27:17AM +0200, Magnus Hagander wrote:
>> Having the function always generate a random salt seems more
>> reasonable though, and would perhaps be something that helps in some
>> of the cases? It won't help with the password policy one, as it's too
>> secure for that, but it would help with the postgres-is-the-client
>> one?
>
> While this is still hot..  Would it make sense to have a
> scram_salt_length GUC to control the length of the salt used when
> generating the SCRAM secret?

What would be the intended usecase? I don’t have the RFC handy, does it say anything about salt length?

./daniel

On Fri, Apr 14, 2023 at 01:27:46AM +0200, Daniel Gustafsson wrote:
> What would be the intended usecase? I don’t have the RFC handy, does
> it say anything about salt length?

Hmm.  I thought it did, but RFC 5802 has only these two paragraphs:

   If the authentication information is stolen from the authentication
   database, then an offline dictionary or brute-force attack can be
   used to recover the user's password.  The use of salt mitigates this
   attack somewhat by requiring a separate attack on each password.
   Authentication mechanisms that protect against this attack are
   available (e.g., the EKE class of mechanisms).  RFC 2945 [RFC2945] is
   an example of such technology.  The WG elected not to use EKE like
   mechanisms as a basis for SCRAM.

   If an attacker obtains the authentication information from the
   authentication repository and either eavesdrops on one authentication
   exchange or impersonates a server, the attacker gains the ability to
   impersonate that user to all servers providing SCRAM access using the
   same hash function, password, iteration count, and salt.  For this
   reason, it is important to use randomly generated salt values.
--
Michael

> On 14 Apr 2023, at 05:50, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Apr 14, 2023 at 01:27:46AM +0200, Daniel Gustafsson wrote:
>> What would be the intended usecase? I don’t have the RFC handy, does
>> it say anything about salt length?
>
> Hmm.  I thought it did, but RFC 5802 has only these two paragraphs:
>
>   If the authentication information is stolen from the authentication
>   database, then an offline dictionary or brute-force attack can be
>   used to recover the user's password.  The use of salt mitigates this
>   attack somewhat by requiring a separate attack on each password.
>   Authentication mechanisms that protect against this attack are
>   available (e.g., the EKE class of mechanisms).  RFC 2945 [RFC2945] is
>   an example of such technology.  The WG elected not to use EKE like
>   mechanisms as a basis for SCRAM.
>
>   If an attacker obtains the authentication information from the
>   authentication repository and either eavesdrops on one authentication
>   exchange or impersonates a server, the attacker gains the ability to
>   impersonate that user to all servers providing SCRAM access using the
>   same hash function, password, iteration count, and salt.  For this
>   reason, it is important to use randomly generated salt values.

The salt needs to be unique, unpredictable and shall not repeat across password
generation.  The current 16 byte salted with pg_strong_random should provide
that and I'm not sure I see a usecase for allowing users to configure that.
The iteration count has a direct effect with the security/speed tradeoff but
changing the salt can basically only lead to lowering the security while not
gaining efficiency, or am I missing something?

--
Daniel Gustafsson

On Wed, Mar 22, 2023 at 9:06 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>
> Maybe I'm not conveying the problem this is solving -- I'm happy to go
> one more round trying to make it clearer -- but if this is not clear,
> it'd be good to at develop an alternative approach to this before
> withdrawing the patch.

This thread had some lively discussion, but it doesn't seem to have
converged towards consensus, and hasn't had activity since April. That
being the case, maybe it's time to withdraw and reconsider the
approach later?

On Sat, 2 Dec 2023 at 12:22, John Naylor <johncnaylorls@gmail.com> wrote:
>
> On Wed, Mar 22, 2023 at 9:06 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
> >
> > Maybe I'm not conveying the problem this is solving -- I'm happy to go
> > one more round trying to make it clearer -- but if this is not clear,
> > it'd be good to at develop an alternative approach to this before
> > withdrawing the patch.
>
> This thread had some lively discussion, but it doesn't seem to have
> converged towards consensus, and hasn't had activity since April. That
> being the case, maybe it's time to withdraw and reconsider the
> approach later?

I have changed the status of this commitfest entry to "Returned with
Feedback" as currently nobody pursued the discussion to get a
conclusion. Feel free to discuss more on this and once it reaches a
better shape, add a new entry for this to take it forward.

Regards,
Vignesh