subject:"\[HACKERS\] Password identifiers, protocol aging and SCRAM protocol"

On 14 December 2016 20:12:05 EET, Bruce Momjian  wrote:
>On Wed, Dec 14, 2016 at 11:27:15AM +0100, Magnus Hagander wrote:
>> I would so like to just drop support for plain passwords completely
>:) But
>> there's a backwards compatibility issue to think about of course.
>> 
>> But -- is there any actual usecase for them anymore?
>
>I thought we recommended 'password' for SSL connections because if you
>use MD5 passwords the password text layout is known and that simplifies
>cryptanalysis.

No, that makes no sense. And whether you use 'password' or 'md5' authentication 
is a different question than whether you store passwords in plaintext or as md5 
hashes. Magnus was asking whether it ever makes sense to *store* passwords in 
plaintext.

Since you brought it up, there is a legitimate argument to be made that 
'password' authentication is more secure than 'md5', when SSL is used. Namely, 
if an attacker can acquire contents of pg_authid e.g. by stealing a backup 
tape, with 'md5' authentication he can log in as any user, using just the 
stolen hashes. But with 'password', he needs to reverse the hash first. It's 
not a great difference, but it's something.

 - 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: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

2016-12-14 Thread Bruce Momjian

On Wed, Dec 14, 2016 at 11:27:15AM +0100, Magnus Hagander wrote:
> I would so like to just drop support for plain passwords completely :) But
> there's a backwards compatibility issue to think about of course.
> 
> But -- is there any actual usecase for them anymore?

I thought we recommended 'password' for SSL connections because if you
use MD5 passwords the password text layout is known and that simplifies
cryptanalysis.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

2016-12-14 Thread Stephen Frost

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 12/14/16 5:15 AM, Michael Paquier wrote:
> > I would be tempted to suggest adding the verifier type as a new column
> > of pg_authid
> 
> Yes please.

This discussion seems to continue to come up and I don't entirely
understand why we keep trying to shove more things into pg_authid, or
worse, into rolpassword.

We should have an independent table for the verifiers, which has a
different column for the verifier type, and either starts off supporting
multiple verifiers per role or at least gives us the ability to add that
easily later.  We should also move rolvaliduntil to that new table.

No, I am specifically *not* concerned with "backwards compatibility" of
that table- we continually add to it and change it and applications
which are so closely tied to PG that they look at pg_authid need to be
updated with nearly every release anyway.  What we *do* need to make
sure we get correct is what pg_dump/pg_upgrade do, but that's entirely
within our control to manage and shouldn't be that much of an issue to
implement.

Thanks!

Stephen

signature.asc
Description: Digital signature

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

2016-12-14 Thread Peter Eisentraut

On 12/14/16 5:15 AM, Michael Paquier wrote:
> I would be tempted to suggest adding the verifier type as a new column
> of pg_authid

Yes please.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)


On 12/14/2016 12:27 PM, Magnus Hagander wrote:

I would so like to just drop support for plain passwords completely :) But
there's a backwards compatibility issue to think about of course.

But -- is there any actual usecase for them anymore?


Hmm. At the moment, I don't think there is.

But, a password stored in plaintext works with either MD5 or SCRAM, or 
any future authentication mechanism. So as soon as we have SCRAM 
authentication, it becomes somewhat useful again.


In a nutshell:

auth / stored   MD5 SCRAM   plaintext
-
passwordY   Y   Y
md5 Y   N   Y
scram   N   Y   Y

If a password is stored in plaintext, it can be used with any 
authentication mechanism. And the plaintext 'password' authentication 
mechanism works with any kind of a stored password. But an MD5 hash 
cannot be used with SCRAM authentication, or vice versa.



I just noticed that the manual for CREATE ROLE says:


Note that older clients might lack support for the MD5 authentication
mechanism that is needed to work with passwords that are stored
encrypted.


That's is incorrect. The alternative to MD5 authentication is plain 
'password' authentication, and that works just fine with MD5-hashed 
passwords. I think that sentence is a leftover from when we still 
supported "crypt" authentication (so I actually get to blame you for 
that ;-), commit 53a5026b). Back then, it was true that if an MD5 hash 
was stored in pg_authid, you couldn't do "crypt" authentication. That 
might have left old clients out in the cold.


Now that we're getting SCRAM authentication, we'll need a similar notice 
there again, for the incompatibility of a SCRAM verifier with MDD5 
authentication and vice versa.




If not, another option could be to just specifically check that it's *not*
"md5" or "scram-:". That would invalidate
plaintext passwords that have those texts in them of course, but what's the
likelyhood of that in reality?


Hmm, we have dismissed that risk for the MD5 hashes (and we also have a 
length check for them), but as we get new hash formats, the risk 
increases. Someone might well want to use "plain:of:jars" as password. 
Perhaps we should use a more complicated pattern.


I googled around for how others store SCRAM and other password hashes. 
Many other systems seem to have similar naming schemes. The closest 
thing to a standard I could find was:


https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md

Perhaps we should also use something like "$plain$" or 
"$scram-sha-256"?


There's also https://tools.ietf.org/html/rfc5803, which specifies how to 
store SCRAM verifiers in LDAP. I don't understand enough of LDAP to 
understand what those actually look like, though, and there were no 
examples in the RFC.


I wonder if we should also worry about storing multiple verifiers in 
rolpassword? We don't support that now, but we might in the future. It 
might come handy, if you could easily store multiple hashes in a single 
string, separated by commas for example.


- 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: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)


On 12/14/2016 12:15 PM, Michael Paquier wrote:

This work is definitely something that should be done before anything
else. Need a patch or are you on it?


I'm on it..

- 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: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

2016-12-14 Thread Magnus Hagander

On Wed, Dec 14, 2016 at 9:51 AM, Heikki Linnakangas  wrote:

> On 12/09/2016 10:19 AM, Michael Paquier wrote:
>
>> On Fri, Dec 9, 2016 at 5:11 PM, Heikki Linnakangas 
>> wrote:
>>
>>> Couple of things I should write down before I forget:
>>>
>>> 1. It's a bit cumbersome that the scram verifiers stored in
>>> pg_authid.rolpassword don't have any clear indication that they're scram
>>> verifiers. MD5 hashes are readily identifiable by the "md5" prefix. I
>>> think
>>> we should use a "scram-sha-256:" for scram verifiers.
>>>
>>
>> scram-sha-256 would make the most sense to me.
>>
>> Actually, I think it'd be awfully nice to also prefix plaintext passwords
>>> with "plain:", but I'm not sure it's worth breaking the compatibility, if
>>> there are tools out there that peek into rolpassword. Thoughts?
>>>
>>
>> pgbouncer is the only thing coming up in mind. It looks at pg_shadow
>> for password values. pg_dump'ing data from pre-10 instances will also
>> need to adapt. I see tricky the compatibility with the exiting CREATE
>> USER PASSWORD command though, so I am wondering if that's worth the
>> complication.
>>
>> 2. It's currently not possible to use the plaintext "password"
>>> authentication method, for a user that has a SCRAM verifier in
>>> rolpassword.
>>> That seems like an oversight. We can't do MD5 authentication with a SCRAM
>>> verifier, but "password" we could.
>>>
>>
>> Yeah, that should be possible...
>>
>
> The tip of the work branch can now do SCRAM authentication, when a user
> has a plaintext password in pg_authid.rolpassword. The reverse doesn't
> work, however: you cannot do plain "password" authentication, when the user
> has a SCRAM verifier in pg_authid.rolpassword. It gets worse: plain
> "password" authentication doesn't check if the string stored in
> pg_authid.rolpassword is a SCRAM authenticator, and treats it as a
> plaintext password, so you can do this:
>
> PGPASSWORD="scram-sha-256:mDBuqO1mEekieg==:4096:17dc259499c1
> a184c26ee5b19715173d9354195f510b4d3af8be585acb39ae33:d3d7131
> 49c6becbbe56bae259aafe4e95b79ab7e3b50f2fbd850ea7d7b7c114f"  psql
> postgres  -h localhost -U scram_user
>
> I think we're going to have a more bugs like this, if we don't start to
> explicitly label plaintext passwords as such.
>
> So, let's add "plain:" prefix to plaintext passwords, in
> pg_authid.rolpassword. With that, these would be valid values in
> pg_authid.rolpassword:
>
> plain:foo
> md55a962ce7a24371a10e85627a484cac28
> scram-sha-256:mDBuqO1mEekieg==:4096:17dc259499c1a184c26ee5b1
> 9715173d9354195f510b4d3af8be585acb39ae33:d3d713149c6becbbe56
> bae259aafe4e95b79ab7e3b50f2fbd850ea7d7b7c114f
>

I would so like to just drop support for plain passwords completely :) But
there's a backwards compatibility issue to think about of course.

But -- is there any actual usecase for them anymore?

If not, another option could be to just specifically check that it's *not*
"md5" or "scram-:". That would invalidate
plaintext passwords that have those texts in them of course, but what's the
likelyhood of that in reality?

Though I guess that might at least in theory be more bug-prone, so going
with a "plain:" prefix seems like a good idea as well.



> But anything that doesn't begin with "plain:", "md5", or "scram-sha-256:"
> would be invalid. You shouldn't have invalid values in the column, but if
> you do, all the authentication mechanisms would reject it.
>
> It would be nice to also change the format of MD5 passwords to have a
> colon, as in "md5:", but that's probably not worth breaking
> compatibility for. Almost no-one stores passwords in plaintext, so changing
> the format of that wouldn't affect many people, but there might well be
> tools out there that peek into MD5 hashes.


There are definitely tools that do that, so +1 on leaving that alone.

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

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

2016-12-14 Thread Michael Paquier

On Wed, Dec 14, 2016 at 5:51 PM, Heikki Linnakangas  wrote:
> The tip of the work branch can now do SCRAM authentication, when a user has
> a plaintext password in pg_authid.rolpassword. The reverse doesn't work,
> however: you cannot do plain "password" authentication, when the user has a
> SCRAM verifier in pg_authid.rolpassword. It gets worse: plain "password"
> authentication doesn't check if the string stored in pg_authid.rolpassword
> is a SCRAM authenticator, and treats it as a plaintext password, so you can
> do this:
>
> PGPASSWORD="scram-sha-256:mDBuqO1mEekieg==:4096:17dc259499c1a184c26ee5b19715173d9354195f510b4d3af8be585acb39ae33:d3d713149c6becbbe56bae259aafe4e95b79ab7e3b50f2fbd850ea7d7b7c114f"
> psql postgres  -h localhost -U scram_user

This one's fun.

> I think we're going to have a more bugs like this, if we don't start to
> explicitly label plaintext passwords as such.
>
> So, let's add "plain:" prefix to plaintext passwords, in
> pg_authid.rolpassword. With that, these would be valid values in
> pg_authid.rolpassword:
>
> [...]
>
> But anything that doesn't begin with "plain:", "md5", or "scram-sha-256:"
> would be invalid. You shouldn't have invalid values in the column, but if
> you do, all the authentication mechanisms would reject it.

I would be tempted to suggest adding the verifier type as a new column
of pg_authid, but as CREATE USER PASSWORD accepts strings with md5
prefix as-is for ages using the "plain:" prefix is definitely a better
plan. My opinion on the matter has changed compared to a couple of
months back.

> It would be nice to also change the format of MD5 passwords to have a colon,
> as in "md5:", but that's probably not worth breaking compatibility
> for. Almost no-one stores passwords in plaintext, so changing the format of
> that wouldn't affect many people, but there might well be tools out there
> that peek into MD5 hashes.

Yes, let's not take this road.

This work is definitely something that should be done before anything
else. Need a patch or are you on it?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

On 12/09/2016 10:19 AM, Michael Paquier wrote:

On Fri, Dec 9, 2016 at 5:11 PM, Heikki Linnakangas wrote:

Couple of things I should write down before I forget:

1. It's a bit cumbersome that the scram verifiers stored in
pg_authid.rolpassword don't have any clear indication that they're scram
verifiers. MD5 hashes are readily identifiable by the "md5" prefix. I think
we should use a "scram-sha-256:" for scram verifiers.

scram-sha-256 would make the most sense to me.

Actually, I think it'd be awfully nice to also prefix plaintext passwords
with "plain:", but I'm not sure it's worth breaking the compatibility, if
there are tools out there that peek into rolpassword. Thoughts?

pgbouncer is the only thing coming up in mind. It looks at pg_shadow
for password values. pg_dump'ing data from pre-10 instances will also
need to adapt. I see tricky the compatibility with the exiting CREATE
USER PASSWORD command though, so I am wondering if that's worth the
complication.

2. It's currently not possible to use the plaintext "password"
authentication method, for a user that has a SCRAM verifier in rolpassword.
That seems like an oversight. We can't do MD5 authentication with a SCRAM
verifier, but "password" we could.

Yeah, that should be possible...

The tip of the work branch can now do SCRAM authentication, when a user
has a plaintext password in pg_authid.rolpassword. The reverse doesn't
work, however: you cannot do plain "password" authentication, when the
user has a SCRAM verifier in pg_authid.rolpassword. It gets worse: plain
"password" authentication doesn't check if the string stored in
pg_authid.rolpassword is a SCRAM authenticator, and treats it as a
plaintext password, so you can do this:

PGPASSWORD="scram-sha-256:mDBuqO1mEekieg==:4096:17dc259499c1a184c26ee5b19715173d9354195f510b4d3af8be585acb39ae33:d3d713149c6becbbe56bae259aafe4e95b79ab7e3b50f2fbd850ea7d7b7c114f"
psql postgres -h localhost -U scram_user

I think we're going to have a more bugs like this, if we don't start to
explicitly label plaintext passwords as such.

So, let's add "plain:" prefix to plaintext passwords, in
pg_authid.rolpassword. With that, these would be valid values in
pg_authid.rolpassword:

plain:foo
md55a962ce7a24371a10e85627a484cac28
scram-sha-256:mDBuqO1mEekieg==:4096:17dc259499c1a184c26ee5b19715173d9354195f510b4d3af8be585acb39ae33:d3d713149c6becbbe56bae259aafe4e95b79ab7e3b50f2fbd850ea7d7b7c114f

But anything that doesn't begin with "plain:", "md5", or
"scram-sha-256:" would be invalid. You shouldn't have invalid values in
the column, but if you do, all the authentication mechanisms would
reject it.

It would be nice to also change the format of MD5 passwords to have a
colon, as in "md5:", but that's probably not worth breaking
compatibility for. Almost no-one stores passwords in plaintext, so
changing the format of that wouldn't affect many people, but there might
well be tools out there that peek into MD5 hashes.

- 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] Password identifiers, protocol aging and SCRAM protocol

2016-12-12 Thread Michael Paquier

On Tue, Dec 13, 2016 at 10:43 AM, Michael Paquier
 wrote:
> On Mon, Dec 12, 2016 at 11:39 PM, Heikki Linnakangas  wrote:
>> A few couple more things that caught my eye while hacking on this:

Looking at what we have now, in the branch...

>> * Use SASLPrep for passwords.

SASLPrep is defined here:
https://tools.ietf.org/html/rfc4013
And stringprep is here:
https://tools.ietf.org/html/rfc3454
So that's roughly applying a conversion from the mapping table, taking
into account prohibited, bi-directional, mapping characters, etc. The
spec says that the password should be in unicode. But we cannot be
sure of that, right? Those mapping tables should be likely a separated
thing.. (perl has Unicode::Stringprep::Mapping for example).

>> * Check nonces, etc. to not contain invalid characters.

Fixed this one.

>> * Derive mock SCRAM verifier for non-existent users deterministically from
>> username.

You have put in place the facility to allow that. The only thing that
comes in mind to generate something per-cluster is to have
BootStrapXLOG() generate an "authentication secret identifier" with a
uint64 and add that in the control file. Using pg_backend_random()
would be a good idea here.

>> * Allow plain 'password' authentication for users with a SCRAM verifier in
>> rolpassword.

Done.

>> * Throw an error if an "authorization identity" is given. ATM, we just
>> ignore it, but seems better to reject the attempt than do something that
>> might not be what the client expects.

Done.

>> * Add "scram-sha-256" prefix to SCRAM verifiers stored in
>> pg_authid.rolpassword.

You did it.
-- 
Michael

-- 
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] Password identifiers, protocol aging and SCRAM protocol

2016-12-12 Thread Michael Paquier

On Mon, Dec 12, 2016 at 11:39 PM, Heikki Linnakangas  wrote:
> A few couple more things that caught my eye while hacking on this:
>
> 1. We don't use SASLPrep to scrub username's and passwords. That's by
> choice, for usernames, because historically in PostgreSQL usernames can be
> stored in any encoding, but SASLPrep assumes UTF-8. We dodge that by passing
> an empty username in the authentication exchange anyway, because we always
> use the username we got from the startup packet. But for passwords, I think
> we need to fix that. The spec is very clear on that:
>
>> Note that implementations MUST either implement SASLprep or disallow
>> use of non US-ASCII Unicode codepoints in "str".
>
> 2. I think we should check nonces, etc. more carefully, to not contain
> invalid characters. For example, in the server, we use the read_attr_value()
> function to read the client's nonce. Per the spec, the nonce should consist
> of ASCII printable characters, but we will accept anything except the comma.
> That's no trouble to the server, but let's be strict.
>
> To summarize, here's the overall TODO list so far:
>
> * Use SASLPrep for passwords.
>
> * Check nonces, etc. to not contain invalid characters.
>
> * Derive mock SCRAM verifier for non-existent users deterministically from
> username.
>
> * Allow plain 'password' authentication for users with a SCRAM verifier in
> rolpassword.
>
> * Throw an error if an "authorization identity" is given. ATM, we just
> ignore it, but seems better to reject the attempt than do something that
> might not be what the client expects.
>
> * Add "scram-sha-256" prefix to SCRAM verifiers stored in
> pg_authid.rolpassword.
>
> Anything else I'm missing?
>
> I've created a wiki page, mostly to host that TODO list, while we hack this
> to completion: https://wiki.postgresql.org/wiki/SCRAM_authentication. Feel
> free to add stuff that comes to mind, and remove stuff as you push patches
> to the branch on github.

Based on the current code, I think you have the whole list. I'll try
to look once again at the code to see I have anything else in mind.
Improving the TAP regression tests is also an item, with SCRAM
authentication support when a plain password is stored.
-- 
Michael


-- 
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] Password identifiers, protocol aging and SCRAM protocol

2016-12-12 Thread Craig Ringer

On 12 December 2016 at 22:39, Heikki Linnakangas  wrote:

> * Throw an error if an "authorization identity" is given. ATM, we just
> ignore it, but seems better to reject the attempt than do something that
> might not be what the client expects.

Yeah. That might be an opportunity to make admins' and connection
poolers' lives much happier down the track, but first we'd need a way
of specifying a mapping for the other users a given user is permitted
to masquerade as (like we have for roles and role membership). We have
SET SESSION AUTHORIZATION already, which has all the same benefits and
security problems as allowing connect-time selection of authorization
identity without such a framework. And we have SET ROLE.

ERRORing is the right thing to do here, so we can safely use this
protocol functionality later if we want to allow user masquerading.

-- 
 Craig Ringer   http://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] Password identifiers, protocol aging and SCRAM protocol

2016-12-12 Thread Heikki Linnakangas


A few couple more things that caught my eye while hacking on this:

1. We don't use SASLPrep to scrub username's and passwords. That's by 
choice, for usernames, because historically in PostgreSQL usernames can 
be stored in any encoding, but SASLPrep assumes UTF-8. We dodge that by 
passing an empty username in the authentication exchange anyway, because 
we always use the username we got from the startup packet. But for 
passwords, I think we need to fix that. The spec is very clear on that:



Note that implementations MUST either implement SASLprep or disallow
use of non US-ASCII Unicode codepoints in "str".



2. I think we should check nonces, etc. more carefully, to not contain 
invalid characters. For example, in the server, we use the 
read_attr_value() function to read the client's nonce. Per the spec, the 
nonce should consist of ASCII printable characters, but we will accept 
anything except the comma. That's no trouble to the server, but let's be 
strict.



To summarize, here's the overall TODO list so far:

* Use SASLPrep for passwords.

* Check nonces, etc. to not contain invalid characters.

* Derive mock SCRAM verifier for non-existent users deterministically 
from username.


* Allow plain 'password' authentication for users with a SCRAM verifier 
in rolpassword.


* Throw an error if an "authorization identity" is given. ATM, we just 
ignore it, but seems better to reject the attempt than do something that 
might not be what the client expects.


* Add "scram-sha-256" prefix to SCRAM verifiers stored in 
pg_authid.rolpassword.


Anything else I'm missing?

I've created a wiki page, mostly to host that TODO list, while we hack 
this to completion: 
https://wiki.postgresql.org/wiki/SCRAM_authentication. Feel free to add 
stuff that comes to mind, and remove stuff as you push patches to the 
branch on github.


- 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] Password identifiers, protocol aging and SCRAM protocol

2016-12-12 Thread Heikki Linnakangas


On 12/09/2016 01:10 PM, Michael Paquier wrote:

On Fri, Dec 09, 2016 at 11:51:45AM +0200, Heikki Linnakangas wrote:

On 12/09/2016 05:58 AM, Michael Paquier wrote:


One thing is: when do we look up at pg_authid? After receiving the
first message from client or before beginning the exchange? As the
first message from client has the user name, it would make sense to do
the lookup after receiving it, but from PG prospective it would just
make sense to use the data already present in the startup packet. The
current patch does the latter. What do you think?


While hacking on this, I came up with the attached refactoring, against
current master. I think it makes the current code more readable, anyway, and
it provides a get_role_password() function that SCRAM can use, to look up
the stored password. (This is essentially the same refactoring that was
included in the SCRAM patch set, that introduced the get_role_details()
function.)

Barring objections, I'll go ahead and commit this first.


Ok, committed.


-   shadow_pass = TextDatumGetCString(datum);
+   *shadow_pass = TextDatumGetCString(datum);

datum = SysCacheGetAttr(AUTHNAME, roleTup,

Anum_pg_authid_rolvaliduntil, );
@@ -83,100 +83,146 @@ md5_crypt_verify(const char *role, char *client_pass,
{
*logdetail = psprintf(_("User \"%s\" has an empty password."),
  role);
+   *shadow_pass = NULL;
return STATUS_ERROR;/* empty password */
}


Here the password is allocated by text_to_cstring(), that's only 1 byte
but it should be free()'d.


Fixed. Thanks, good catch! It doesn't matter in practice as we'll 
disconnect shortly afterwards anyway, but given that the callers pfree() 
other things on error, let's be tidy.


- 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] Password identifiers, protocol aging and SCRAM protocol

2016-12-09 Thread Michael Paquier

On Fri, Dec 09, 2016 at 11:51:45AM +0200, Heikki Linnakangas wrote:
> On 12/09/2016 05:58 AM, Michael Paquier wrote:
> > 
> > One thing is: when do we look up at pg_authid? After receiving the
> > first message from client or before beginning the exchange? As the
> > first message from client has the user name, it would make sense to do
> > the lookup after receiving it, but from PG prospective it would just
> > make sense to use the data already present in the startup packet. The
> > current patch does the latter. What do you think?
> 
> While hacking on this, I came up with the attached refactoring, against
> current master. I think it makes the current code more readable, anyway, and
> it provides a get_role_password() function that SCRAM can use, to look up
> the stored password. (This is essentially the same refactoring that was
> included in the SCRAM patch set, that introduced the get_role_details()
> function.)
> 
> Barring objections, I'll go ahead and commit this first.

Here are some comments.

> @@ -720,12 +721,16 @@ CheckMD5Auth(Port *port, char **logdetail)
>   sendAuthRequest(port, AUTH_REQ_MD5, md5Salt, 4);
>  
>   passwd = recv_password_packet(port);
> -
>   if (passwd == NULL)
>   return STATUS_EOF;  /* client wouldn't send 
> password */

This looks like useless noise.

> - shadow_pass = TextDatumGetCString(datum);
> + *shadow_pass = TextDatumGetCString(datum);
>  
>   datum = SysCacheGetAttr(AUTHNAME, roleTup,
>   
> Anum_pg_authid_rolvaliduntil, );
> @@ -83,100 +83,146 @@ md5_crypt_verify(const char *role, char *client_pass,
>   {
>   *logdetail = psprintf(_("User \"%s\" has an empty password."),
> role);
> + *shadow_pass = NULL;
>   return STATUS_ERROR;/* empty password */
>   }

Here the password is allocated by text_to_cstring(), that's only 1 byte
but it should be free()'d.
-- 
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

2016-12-09 Thread Heikki Linnakangas


On 12/09/2016 05:58 AM, Michael Paquier wrote:


One thing is: when do we look up at pg_authid? After receiving the
first message from client or before beginning the exchange? As the
first message from client has the user name, it would make sense to do
the lookup after receiving it, but from PG prospective it would just
make sense to use the data already present in the startup packet. The
current patch does the latter. What do you think?


While hacking on this, I came up with the attached refactoring, against 
current master. I think it makes the current code more readable, anyway, 
and it provides a get_role_password() function that SCRAM can use, to 
look up the stored password. (This is essentially the same refactoring 
that was included in the SCRAM patch set, that introduced the 
get_role_details() function.)


Barring objections, I'll go ahead and commit this first.

- Heikki

>From 30be98cf09e8807d477827257a1e55c979dbe877 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas 
Date: Fri, 9 Dec 2016 11:49:36 +0200
Subject: [PATCH 1/1] Refactor the code for verifying user's password.

Split md5_crypt_verify() into three functions:
* get_role_password() to fetch user's password from pg_authid, and check
  its expiration.
* md5_crypt_verify() to check an MD5 authentication challenge
* plain_crypt_verify() to check a plaintext password.

get_role_password() will be needed as a separate function by the upcoming
SCRAM authentication patch set. Most of the remaining functionality in
md5_crypt_verify() was different for MD5 and plaintext authentication, so
split that for readability.

While we're at it, simplify the *_crypt_verify functions by using
stack-allocated buffers to hold the temporary MD5 hashes, instead of
pallocing.
---
 src/backend/libpq/auth.c  |  18 +++-
 src/backend/libpq/crypt.c | 214 --
 src/include/libpq/crypt.h |   9 +-
 3 files changed, 151 insertions(+), 90 deletions(-)

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index f8bffe3..5c9ee06 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -707,6 +707,7 @@ CheckMD5Auth(Port *port, char **logdetail)
 {
 	char		md5Salt[4];		/* Password salt */
 	char	   *passwd;
+	char	   *shadow_pass;
 	int			result;
 
 	if (Db_user_namespace)
@@ -720,12 +721,16 @@ CheckMD5Auth(Port *port, char **logdetail)
 	sendAuthRequest(port, AUTH_REQ_MD5, md5Salt, 4);
 
 	passwd = recv_password_packet(port);
-
 	if (passwd == NULL)
 		return STATUS_EOF;		/* client wouldn't send password */
 
-	result = md5_crypt_verify(port->user_name, passwd, md5Salt, 4, logdetail);
+	result = get_role_password(port->user_name, _pass, logdetail);
+	if (result == STATUS_OK)
+		result = md5_crypt_verify(port->user_name, shadow_pass, passwd,
+  md5Salt, 4, logdetail);
 
+	if (shadow_pass)
+		pfree(shadow_pass);
 	pfree(passwd);
 
 	return result;
@@ -741,16 +746,21 @@ CheckPasswordAuth(Port *port, char **logdetail)
 {
 	char	   *passwd;
 	int			result;
+	char	   *shadow_pass;
 
 	sendAuthRequest(port, AUTH_REQ_PASSWORD, NULL, 0);
 
 	passwd = recv_password_packet(port);
-
 	if (passwd == NULL)
 		return STATUS_EOF;		/* client wouldn't send password */
 
-	result = md5_crypt_verify(port->user_name, passwd, NULL, 0, logdetail);
+	result = get_role_password(port->user_name, _pass, logdetail);
+	if (result == STATUS_OK)
+		result = plain_crypt_verify(port->user_name, shadow_pass, passwd,
+	logdetail);
 
+	if (shadow_pass)
+		pfree(shadow_pass);
 	pfree(passwd);
 
 	return result;
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index b4ca174..fb6d1af 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -30,28 +30,28 @@
 
 
 /*
- * Check given password for given user, and return STATUS_OK or STATUS_ERROR.
+ * Fetch stored password for a user, for authentication.
  *
- * 'client_pass' is the password response given by the remote user.  If
- * 'md5_salt' is not NULL, it is a response to an MD5 authentication
- * challenge, with the given salt.  Otherwise, it is a plaintext password.
+ * Returns STATUS_OK on success.  On error, returns STATUS_ERROR, and stores
+ * a palloc'd string describing the reason, for the postmaster log, in
+ * *logdetail.  The error reason should *not* be sent to the client, to avoid
+ * giving away user information!
  *
- * In the error case, optionally store a palloc'd string at *logdetail
- * that will be sent to the postmaster log (but not the client).
+ * If the password is expired, it is still returned in *shadow_pass, but the
+ * return code is STATUS_ERROR.  On other errors, *shadow_pass is set to
+ * NULL.
  */
 int
-md5_crypt_verify(const char *role, char *client_pass,
- char *md5_salt, int md5_salt_len, char **logdetail)
+get_role_password(const char *role, char **shadow_pass, char **logdetail)
 {
 	int			retval = STATUS_ERROR;
-	char	   *shadow_pass,
-			   *crypt_pwd;
 	TimestampTz vuntil

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

2016-12-09 Thread Michael Paquier

On Fri, Dec 9, 2016 at 5:11 PM, Heikki Linnakangas  wrote:
> Couple of things I should write down before I forget:
>
> 1. It's a bit cumbersome that the scram verifiers stored in
> pg_authid.rolpassword don't have any clear indication that they're scram
> verifiers. MD5 hashes are readily identifiable by the "md5" prefix. I think
> we should use a "scram-sha-256:" for scram verifiers.

scram-sha-256 would make the most sense to me.

> Actually, I think it'd be awfully nice to also prefix plaintext passwords
> with "plain:", but I'm not sure it's worth breaking the compatibility, if
> there are tools out there that peek into rolpassword. Thoughts?

pgbouncer is the only thing coming up in mind. It looks at pg_shadow
for password values. pg_dump'ing data from pre-10 instances will also
need to adapt. I see tricky the compatibility with the exiting CREATE
USER PASSWORD command though, so I am wondering if that's worth the
complication.

> 2. It's currently not possible to use the plaintext "password"
> authentication method, for a user that has a SCRAM verifier in rolpassword.
> That seems like an oversight. We can't do MD5 authentication with a SCRAM
> verifier, but "password" we could.

Yeah, that should be possible...
-- 
Michael


-- 
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] Password identifiers, protocol aging and SCRAM protocol

2016-12-09 Thread Heikki Linnakangas


Couple of things I should write down before I forget:

1. It's a bit cumbersome that the scram verifiers stored in 
pg_authid.rolpassword don't have any clear indication that they're scram 
verifiers. MD5 hashes are readily identifiable by the "md5" prefix. I 
think we should use a "scram-sha-256:" for scram verifiers.


Actually, I think it'd be awfully nice to also prefix plaintext 
passwords with "plain:", but I'm not sure it's worth breaking the 
compatibility, if there are tools out there that peek into rolpassword. 
Thoughts?


2. It's currently not possible to use the plaintext "password" 
authentication method, for a user that has a SCRAM verifier in 
rolpassword. That seems like an oversight. We can't do MD5 
authentication with a SCRAM verifier, but "password" we could.


- 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] Password identifiers, protocol aging and SCRAM protocol

2016-12-09 Thread Heikki Linnakangas


On 12/09/2016 05:58 AM, Michael Paquier wrote:

On Thu, Dec 8, 2016 at 10:05 PM, Michael Paquier
 wrote:

On Thu, Dec 8, 2016 at 5:55 PM, Heikki Linnakangas  wrote:

Actually, we don't give away that information currently. If you try to log
in with password or MD5 authentication, and the user doesn't exist, you get
the same error as with an incorrect password. So, I think we do need to give
the client a made-up salt and iteration count in that case, to hide the fact
that the user doesn't exist. Furthermore, you can't just generate random
salt and iteration count, because then you could simply try connecting
twice, and see if you get the same salt and iteration count. We need to
deterministically derive the salt from the username, so that you get the
same salt/iteration count every time you try connecting with that username.
But it needs indistinguishable from a random salt, to the client. Perhaps a
SHA hash of the username and some per-cluster secret value, created by
initdb. There must be research papers out there on how to do this..


A simple idea would be to use the system ID when generating this fake
salt? That's generated by initdb, once per cluster. I am wondering if
it would be risky to use it for the salt. For the number of iterations
the default number could be used.


I think I'd feel better with a completely separate randomly-generated 
value for this. System ID is not too difficult to guess, and there's no 
need to skimp on this. Yes, default number of iterations makes sense.


We cannot completely avoid leaking information through this, 
unfortunately. For example, if you have a user with a non-default number 
of iterations, and an attacker probes that, he'll know that the username 
was valid, because he got back a non-default number of iterations. But 
let's do our best.



I have been thinking more about this part quite a bit, and here is the
most simple thing that we could do while respecting the protocol.
That's more or less what I think you have in mind by re-reading
upthread, but it does not hurt to rewrite the whole flow to be clear:
1) Server gets the startup packet, maps pg_hba.conf and moves on to
the scram authentication code path.
2) Server sends back sendAuthRequest() to request user to provide a
password. This maps to the plain/md5 behavior as no errors would be
issued to user until he has provided a password.
3) Client sends back the password, and the first message with the user name.
4) Server receives it, and checks the data. If a failure happens at
this stage, just ERROR on PG-side without sending back a e= message.
This includes the username-mismatch, empty password and end of
password validity. So we would never use e=unknown-user. This sticks
with what you quoted upthread that the server may end the exchange
before sending the final message.


If we want to mimic the current behavior with MD5 authentication, I 
think we need to follow through with the challenge, and only fail in the 
last step, even if we know the password was empty or expired. MD5 
authentication doesn't currently give away that information to the user.


But it's OK to bail out early on OOM, or if the client sends an outright 
broken message. Those don't give away any information on the user account.



5) Server sends back the challenge, and client answers back with its
reply to it.



Then enters the final stage of the exchange, at which point the server
would issue its final message that would be e= in case of errors. If
something like an OOM happens, no message would be sent so failing on
an OOM ERROR on PG side would be fine as well.



6) Read final message from client and validate.
7) issue final message of server.

On failure at steps 6) or 7), an e= message is returned instead of the
final message. Does that look right?


Yep.


One thing is: when do we look up at pg_authid? After receiving the
first message from client or before beginning the exchange? As the
first message from client has the user name, it would make sense to do
the lookup after receiving it, but from PG prospective it would just
make sense to use the data already present in the startup packet. The
current patch does the latter. What do you think?


Let's see what fits the program flow best. Probably best to do it before 
beginning the exchange. I'm hacking on this right now...



By the way, I have pushed the extra patches you sent into this branch:
https://github.com/michaelpq/postgres/tree/scram


Thanks! We had a quick chat with Michael, and agreed that we'd hack 
together on that github repository, to avoid stepping on each other's 
toes, and cut rebased patch sets from there to pgsql-hackers every now 
and 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] Password identifiers, protocol aging and SCRAM protocol

2016-12-08 Thread Michael Paquier

On Thu, Dec 8, 2016 at 10:05 PM, Michael Paquier
 wrote:
> On Thu, Dec 8, 2016 at 5:55 PM, Heikki Linnakangas  wrote:
>> On 12/08/2016 10:18 AM, Michael Paquier wrote:
>>> Hmmm. How do we handle the case where the user name does not match
>>> then? The spec gives an error message e= specifically for this case.
>>
>> Hmm, interesting. I wonder how/when they imagine that error message to be
>> used. I suppose you could send a dummy server-first message, with a made-up
>> salt and iteration count, if the user is not found, so that you can report
>> that in the server-final message. But that seems unnecessarily complicated,
>> compared to just sending the error immediately. I could imagine using a
>> dummy server-first message to hide whether the user exists, but that
>> argument doesn't hold water if you're going to report an "unknown-user"
>> error, anyway.
>
> Using directly an error message would map with MD5 and plain, but
> that's definitely a new protocol piece so I'd rather think that using
> e= once the client has sent its first message in the exchange should
> be answered with an appropriate SASL error...
>
>> Actually, we don't give away that information currently. If you try to log
>> in with password or MD5 authentication, and the user doesn't exist, you get
>> the same error as with an incorrect password. So, I think we do need to give
>> the client a made-up salt and iteration count in that case, to hide the fact
>> that the user doesn't exist. Furthermore, you can't just generate random
>> salt and iteration count, because then you could simply try connecting
>> twice, and see if you get the same salt and iteration count. We need to
>> deterministically derive the salt from the username, so that you get the
>> same salt/iteration count every time you try connecting with that username.
>> But it needs indistinguishable from a random salt, to the client. Perhaps a
>> SHA hash of the username and some per-cluster secret value, created by
>> initdb. There must be research papers out there on how to do this..
>
> A simple idea would be to use the system ID when generating this fake
> salt? That's generated by initdb, once per cluster. I am wondering if
> it would be risky to use it for the salt. For the number of iterations
> the default number could be used.

I have been thinking more about this part quite a bit, and here is the
most simple thing that we could do while respecting the protocol.
That's more or less what I think you have in mind by re-reading
upthread, but it does not hurt to rewrite the whole flow to be clear:
1) Server gets the startup packet, maps pg_hba.conf and moves on to
the scram authentication code path.
2) Server sends back sendAuthRequest() to request user to provide a
password. This maps to the plain/md5 behavior as no errors would be
issued to user until he has provided a password.
3) Client sends back the password, and the first message with the user name.
4) Server receives it, and checks the data. If a failure happens at
this stage, just ERROR on PG-side without sending back a e= message.
This includes the username-mismatch, empty password and end of
password validity. So we would never use e=unknown-user. This sticks
with what you quoted upthread that the server may end the exchange
before sending the final message.
5) Server sends back the challenge, and client answers back with its
reply to it.

Then enters the final stage of the exchange, at which point the server
would issue its final message that would be e= in case of errors. If
something like an OOM happens, no message would be sent so failing on
an OOM ERROR on PG side would be fine as well.

6) Read final message from client and validate.
7) issue final message of server.

On failure at steps 6) or 7), an e= message is returned instead of the
final message. Does that look right?

One thing is: when do we look up at pg_authid? After receiving the
first message from client or before beginning the exchange? As the
first message from client has the user name, it would make sense to do
the lookup after receiving it, but from PG prospective it would just
make sense to use the data already present in the startup packet. The
current patch does the latter. What do you think?

By the way, I have pushed the extra patches you sent into this branch:
https://github.com/michaelpq/postgres/tree/scram
-- 
Michael

-- 
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] Password identifiers, protocol aging and SCRAM protocol

2016-12-08 Thread Michael Paquier

On Thu, Dec 8, 2016 at 5:55 PM, Heikki Linnakangas  wrote:
> On 12/08/2016 10:18 AM, Michael Paquier wrote:
>> Hmmm. How do we handle the case where the user name does not match
>> then? The spec gives an error message e= specifically for this case.
>
> Hmm, interesting. I wonder how/when they imagine that error message to be
> used. I suppose you could send a dummy server-first message, with a made-up
> salt and iteration count, if the user is not found, so that you can report
> that in the server-final message. But that seems unnecessarily complicated,
> compared to just sending the error immediately. I could imagine using a
> dummy server-first messaage to hide whether the user exists, but that
> argument doesn't hold water if you're going to report an "unknown-user"
> error, anyway.

Using directly an error message would map with MD5 and plain, but
that's definitely a new protocol piece so I'd rather think that using
e= once the client has sent its first message in the exchange should
be answered with an appropriate SASL error...

> Actually, we don't give away that information currently. If you try to log
> in with password or MD5 authentication, and the user doesn't exist, you get
> the same error as with an incorrect password. So, I think we do need to give
> the client a made-up salt and iteration count in that case, to hide the fact
> that the user doesn't exist. Furthermore, you can't just generate random
> salt and iteration count, because then you could simply try connecting
> twice, and see if you get the same salt and iteration count. We need to
> deterministically derive the salt from the username, so that you get the
> same salt/iteration count every time you try connecting with that username.
> But it needs indistinguishable from a random salt, to the client. Perhaps a
> SHA hash of the username and some per-cluster secret value, created by
> initdb. There must be research papers out there on how to do this..

A simple idea would be to use the system ID when generating this fake
salt? That's generated by initdb, once per cluster. I am wondering if
it would be risky to use it for the salt. For the number of iterations
the default number could be used.

> To be really pedantic about that, we should also ward off timing attacks, by
> making sure that the dummy authentication is no faster/slower than a real
> one..

There is one catalog lookup when extracting the verifier from
pg_authid, I'd guess that if we generate a fake verifier things should
get pretty close.

>> If this is taken into account we need to perform sanity checks at
>> initialization phase I am afraid as the number of iterations and the
>> salt are part of the verifier. So you mean that just sending out a
>> normal ERROR message is fine at an earlier step (with *logdetails
>> filled for the backend)? I just want to be sure I understand what you
>> mean here.
>
> That's right, we can send a normal ERROR message. (But not for the
> "user-not-found" case, as discussed above.)

I'd think that the cases where the password is empty and the password
has passed valid duration should be returned with e=other-error. If
the caller sends a SCRAM request that would be impolite (?) to just
throw up an error once the exchange has begun.

> Although, currently, the whole pg_hba.conf file in that example is a valid
> file that someone might have on a real server. With the above addition, it
> would not be. You would never have the two lines with the same
> host/database/user combination in pg_hba.conf.

Okay.
-- 
Michael


-- 
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] Password identifiers, protocol aging and SCRAM protocol

2016-12-08 Thread Heikki Linnakangas


On 12/08/2016 10:18 AM, Michael Paquier wrote:

On Thu, Dec 8, 2016 at 5:54 AM, Heikki Linnakangas  wrote:

Attached those here, as add-on patches to your latest patch set.


Thanks for looking at it!


I'll continue reviewing, but a couple of things caught my eye that you may want
to jump on, in the meanwhile:

On error messages, the spec says:


o  e: This attribute specifies an error that occurred during
  authentication exchange.  It is sent by the server in its final
  message and can help diagnose the reason for the authentication
  exchange failure.  On failed authentication, the entire server-
  final-message is OPTIONAL; specifically, a server implementation
  MAY conclude the SASL exchange with a failure without sending the
  server-final-message.  This results in an application-level error
  response without an extra round-trip.  If the server-final-message
  is sent on authentication failure, then the "e" attribute MUST be
  included.



Note that it says that the server can send the error message with the e=
attribute, in the *final message*. It's not a valid response in the earlier
state, before sending server-first-message. I think we need to change the
INIT state handling in pg_be_scram_exchange() to not send e= messages to the
client. On an error at that state, it needs to just bail out without a
message. The spec allows that. We can always log the detailed reason in the
server log, anyway.


Hmmm. How do we handle the case where the user name does not match
then? The spec gives an error message e= specifically for this case.


Hmm, interesting. I wonder how/when they imagine that error message to 
be used. I suppose you could send a dummy server-first message, with a 
made-up salt and iteration count, if the user is not found, so that you 
can report that in the server-final message. But that seems 
unnecessarily complicated, compared to just sending the error 
immediately. I could imagine using a dummy server-first messaage to hide 
whether the user exists, but that argument doesn't hold water if you're 
going to report an "unknown-user" error, anyway.


Actually, we don't give away that information currently. If you try to 
log in with password or MD5 authentication, and the user doesn't exist, 
you get the same error as with an incorrect password. So, I think we do 
need to give the client a made-up salt and iteration count in that case, 
to hide the fact that the user doesn't exist. Furthermore, you can't 
just generate random salt and iteration count, because then you could 
simply try connecting twice, and see if you get the same salt and 
iteration count. We need to deterministically derive the salt from the 
username, so that you get the same salt/iteration count every time you 
try connecting with that username. But it needs indistinguishable from a 
random salt, to the client. Perhaps a SHA hash of the username and some 
per-cluster secret value, created by initdb. There must be research 
papers out there on how to do this..


To be really pedantic about that, we should also ward off timing 
attacks, by making sure that the dummy authentication is no 
faster/slower than a real one..



If this is taken into account we need to perform sanity checks at
initialization phase I am afraid as the number of iterations and the
salt are part of the verifier. So you mean that just sending out a
normal ERROR message is fine at an earlier step (with *logdetails
filled for the backend)? I just want to be sure I understand what you
mean here.


That's right, we can send a normal ERROR message. (But not for the 
"user-not-found" case, as discussed above.)



As Peter E pointed out earlier, the documentation is lacking, on how to
configure MD5 and/or SCRAM. If you put "scram" as the authentication method
in pg_hba.conf, what does it mean? If you have a line for both "scram" and
"md5" in pg_hba.conf, with the same database/user/hostname combo, what does
that mean? Answer: The first one takes effect, the second one has no effect.
Yet the example in the docs now has that, which is nonsense :-). Hopefully
we'll have some kind of a "both" option, before the release, but in the
meanwhile, we need describe how this works now in the docs.


OK, it would be better to add a paragraph in client-auth.sgml
regarding the mapping of the two settings. For the example of file in
postgresql.conf, I would have really thought that adding directly a
line with "scram" listed was enough though. Perhaps a comment to say
that if md5 and scram are specified the first one wins where a user
and database name map?


So, I think this makes no sense:


 # Allow any user from host 192.168.12.10 to connect to database
-# "postgres" if the user's password is correctly supplied.
+# "postgres" if the user's password is correctly supplied and is
+# using the correct password method.
 #
 # TYPE  DATABASEUSERADDRESS METHOD
 hostpostgres

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

2016-12-08 Thread Michael Paquier

On Thu, Dec 8, 2016 at 5:54 AM, Heikki Linnakangas  wrote:
> Attached those here, as add-on patches to your latest patch set.

Thanks for looking at it!

> I'll continue reviewing, but a couple of things caught my eye that you may 
> want
> to jump on, in the meanwhile:
>
> On error messages, the spec says:
>
>> o  e: This attribute specifies an error that occurred during
>>   authentication exchange.  It is sent by the server in its final
>>   message and can help diagnose the reason for the authentication
>>   exchange failure.  On failed authentication, the entire server-
>>   final-message is OPTIONAL; specifically, a server implementation
>>   MAY conclude the SASL exchange with a failure without sending the
>>   server-final-message.  This results in an application-level error
>>   response without an extra round-trip.  If the server-final-message
>>   is sent on authentication failure, then the "e" attribute MUST be
>>   included.
>
>
> Note that it says that the server can send the error message with the e=
> attribute, in the *final message*. It's not a valid response in the earlier
> state, before sending server-first-message. I think we need to change the
> INIT state handling in pg_be_scram_exchange() to not send e= messages to the
> client. On an error at that state, it needs to just bail out without a
> message. The spec allows that. We can always log the detailed reason in the
> server log, anyway.

Hmmm. How do we handle the case where the user name does not match
then? The spec gives an error message e= specifically for this case.
If this is taken into account we need to perform sanity checks at
initialization phase I am afraid as the number of iterations and the
salt are part of the verifier. So you mean that just sending out a
normal ERROR message is fine at an earlier step (with *logdetails
filled for the backend)? I just want to be sure I understand what you
mean here.

> As Peter E pointed out earlier, the documentation is lacking, on how to
> configure MD5 and/or SCRAM. If you put "scram" as the authentication method
> in pg_hba.conf, what does it mean? If you have a line for both "scram" and
> "md5" in pg_hba.conf, with the same database/user/hostname combo, what does
> that mean? Answer: The first one takes effect, the second one has no effect.
> Yet the example in the docs now has that, which is nonsense :-). Hopefully
> we'll have some kind of a "both" option, before the release, but in the
> meanwhile, we need describe how this works now in the docs.

OK, it would be better to add a paragraph in client-auth.sgml
regarding the mapping of the two settings. For the example of file in
postgresql.conf, I would have really thought that adding directly a
line with "scram" listed was enough though. Perhaps a comment to say
that if md5 and scram are specified the first one wins where a user
and database name map?
-- 
Michael


-- 
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] Password identifiers, protocol aging and SCRAM protocol

2016-12-07 Thread Heikki Linnakangas


On 12/07/2016 08:39 AM, Michael Paquier wrote:

On Tue, Nov 29, 2016 at 1:36 PM, Michael Paquier
 wrote:

Nothing more will likely happen in this CF, so I have moved it to
2017-01 with the same status of "Needs Review".


Attached is a new set of patches using the new routines
pg_backend_random() and pg_strong_random() to handle the randomness in
SCRAM:
- 0001 refactors the SHA2 routines. pgcrypto uses raw files from
src/common when compiling with this patch. That works on any platform,
and this is the simplified version of upthread.
- 0002 adds base64 routines to src/common.
- 0003 does some refactoring regarding the password encryption in
ALTER/CREATE USER queries.
- 0004 adds the clause PASSWORD (val USING method) in CREATE/ALTER USER.
- 0005 is the code patch for SCRAM. Note that this switches pgcrypto
to link to libpgcommon as SHA2 routines are used by the backend.
- 0006 adds some regression tests for passwords.
- 0007 adds some TAP tests for authentication.
This is added to the upcoming CF.


I spent a little time reading through this once again. Steady progress, 
did some small fixes:


* Rewrote the nonce generation. In the server-side, it first generated a 
string of ascii-printable characters, then base64-encoded them, which is 
superfluous. Also, avoid calling pg_strong_random() one byte at a time, 
for performance reasons.


* Added a more sophisticated fallback implementation in libpq, for the 
--disable-strong-random cases, similar to pg_backend_random().


* No need to disallow SCRAM with db_user_namespace. It doesn't include 
the username in the salt like MD5 does.


Attached those here, as add-on patches to your latest patch set. I'll 
continue reviewing, but a couple of things caught my eye that you may 
want to jump on, in the meanwhile:


On error messages, the spec says:


o  e: This attribute specifies an error that occurred during
  authentication exchange.  It is sent by the server in its final
  message and can help diagnose the reason for the authentication
  exchange failure.  On failed authentication, the entire server-
  final-message is OPTIONAL; specifically, a server implementation
  MAY conclude the SASL exchange with a failure without sending the
  server-final-message.  This results in an application-level error
  response without an extra round-trip.  If the server-final-message
  is sent on authentication failure, then the "e" attribute MUST be
  included.


Note that it says that the server can send the error message with the e= 
attribute, in the *final message*. It's not a valid response in the 
earlier state, before sending server-first-message. I think we need to 
change the INIT state handling in pg_be_scram_exchange() to not send e= 
messages to the client. On an error at that state, it needs to just bail 
out without a message. The spec allows that. We can always log the 
detailed reason in the server log, anyway.


As Peter E pointed out earlier, the documentation is lacking, on how to 
configure MD5 and/or SCRAM. If you put "scram" as the authentication 
method in pg_hba.conf, what does it mean? If you have a line for both 
"scram" and "md5" in pg_hba.conf, with the same database/user/hostname 
combo, what does that mean? Answer: The first one takes effect, the 
second one has no effect. Yet the example in the docs now has that, 
which is nonsense :-). Hopefully we'll have some kind of a "both" 
option, before the release, but in the meanwhile, we need describe how 
this works now in the docs.


- Heikki

>From 4d3a59ae1cb5742499c71b0c1e048d30dcef6836 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas 
Date: Wed, 7 Dec 2016 15:24:55 +0200
Subject: [PATCH 08/11] Rewrite nonce generation.

In the server, the nonce was generated using only ASCII-printable
characters, and the result was base64-encoded. The base64 encoding is
pointless, if we use only ASCII-printable chars to begin with.

Calling pg_strong_random() can be somewhat expensive, as with the
/dev/urandom implementation, it has to open the device, read the bytes,
and close, on every call. So avoid calling it in a loop, generating only
one byte in each call.

I went back to using base64-encoding method of turning the raw bytes into
the final nonce. That was more convenient than writing something that
encodes to the whole ASCII-printable range. That means that we're not using
the whole range of chars allowed in the nonce, but I believe that doesn't
make any difference. (Both the frontend and backend will still accept the
full range from the other side of the connection).
---
 src/backend/libpq/auth-scram.c   | 52 ---
 src/include/common/scram-common.h|  6 +++-
 src/include/libpq/libpq-be.h |  2 --
 src/interfaces/libpq/fe-auth-scram.c | 60 ++--
 4 files changed, 34 insertions(+), 86 deletions(-)

diff --git a/src/backend/libpq/auth-scram.c

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

2016-11-28 Thread Michael Paquier

On Fri, Nov 18, 2016 at 2:51 AM, Michael Paquier
 wrote:
> On Thu, Nov 17, 2016 at 8:12 AM, Robert Haas  wrote:
>> So, the problem isn't Darwin-specific.  I experimented with this on
>> Linux and found Linux does the same thing with libpgcommon_srv.a that
>> macOS does: a file in the archive that is totally unused is omitted
>> from the postgres binary.  In Linux, however, that doesn't prevent
>> pgcrypto from compiling anyway.  It does, however, prevent it from
>> working.  Instead of failing at compile time with a complaint about
>> missing symbols, it fails at load time.  I think that's because macOS
>> has -bundle-loader and we use it; without that, I think we'd get the
>> same behavior on macOS that we get on Windows.
>
> Yes, right. I recall seeing the regression tests failing with pgcrypto
> when doing that. Though I did not recall if this was specific to macos
> or Linux when I looked again at this patch yesterday. When testing
> again yesterday I was able to make the tests of pgcrypto to pass, but
> perhaps my build was not in a clean state...
>
>> 1. Rejigger things so that we don't build libpgcommon_srv.a in the
>> first place, and instead add $(top_builddir)/src/common to
>> src/backend/Makefile's value of SUBDIRS.  With appropriate adjustments
>> to src/common/Makefile, this should allow us to include all of the
>> object files on the linker command line individually instead of
>> building an archive library that is then used only for the postgres
>> binary itself anyway.  Then, things wouldn't get dropped.
>>
>> 2. Just postpone committing this patch until we're ready to use the
>> new code in the backend someplace (or add a dummy reference to it
>> someplace).
>
> At the end this refactoring makes sense because it will be used in the
> backend with the SCRAM engine, so we could just wait for 2 instead of
> having some workarounds. This is dropping the ball for later and there
> will be already a lot of work for the SCRAM core part, though I don't
> think that the SHA2 refactoring will change much going forward.
>
> Option 3 would be to do things the patch does it, aka just compiling
> pgcrypto using the source files directly and put a comment to revert
> that once the APIs are used in the backend. I can guess that you don't
> like that.

Nothing more will likely happen in this CF, so I have moved it to
2017-01 with the same status of "Needs Review".
-- 
Michael


-- 
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] Password identifiers, protocol aging and SCRAM protocol

2016-11-17 Thread Michael Paquier

On Thu, Nov 17, 2016 at 8:12 AM, Robert Haas  wrote:
> So, the problem isn't Darwin-specific.  I experimented with this on
> Linux and found Linux does the same thing with libpgcommon_srv.a that
> macOS does: a file in the archive that is totally unused is omitted
> from the postgres binary.  In Linux, however, that doesn't prevent
> pgcrypto from compiling anyway.  It does, however, prevent it from
> working.  Instead of failing at compile time with a complaint about
> missing symbols, it fails at load time.  I think that's because macOS
> has -bundle-loader and we use it; without that, I think we'd get the
> same behavior on macOS that we get on Windows.

Yes, right. I recall seeing the regression tests failing with pgcrypto
when doing that. Though I did not recall if this was specific to macos
or Linux when I looked again at this patch yesterday. When testing
again yesterday I was able to make the tests of pgcrypto to pass, but
perhaps my build was not in a clean state...

> 1. Rejigger things so that we don't build libpgcommon_srv.a in the
> first place, and instead add $(top_builddir)/src/common to
> src/backend/Makefile's value of SUBDIRS.  With appropriate adjustments
> to src/common/Makefile, this should allow us to include all of the
> object files on the linker command line individually instead of
> building an archive library that is then used only for the postgres
> binary itself anyway.  Then, things wouldn't get dropped.
>
> 2. Just postpone committing this patch until we're ready to use the
> new code in the backend someplace (or add a dummy reference to it
> someplace).

At the end this refactoring makes sense because it will be used in the
backend with the SCRAM engine, so we could just wait for 2 instead of
having some workarounds. This is dropping the ball for later and there
will be already a lot of work for the SCRAM core part, though I don't
think that the SHA2 refactoring will change much going forward.

Option 3 would be to do things the patch does it, aka just compiling
pgcrypto using the source files directly and put a comment to revert
that once the APIs are used in the backend. I can guess that you don't
like that.
-- 
Michael


-- 
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] Password identifiers, protocol aging and SCRAM protocol

2016-11-17 Thread Robert Haas

On Wed, Nov 16, 2016 at 11:28 PM, Michael Paquier
 wrote:
> On Wed, Nov 16, 2016 at 8:04 PM, Michael Paquier
>  wrote:
>> In the current set of patches, the sha2 functions would not get used
>> until the main patch for SCRAM gets committed so that's a couple of
>> steps and many months ahead.. And --as-needed/--no-as-needed are not
>> supported in macos. So I would believe that the best route is just to
>> use this patch with the way it does things, and once SCRAM gets in we
>> could switch the build into more appropriate linking. At least that's
>> far less ugly than having fake objects in the backend code. Of course
>> a comment in pgcrypo's Makefile would be appropriate.
>
> Or a comment with a "ifeq ($(PORTNAME), darwin)" containing the
> additional objects to make clear that this is proper to only OSX.
> Other ideas are welcome.

So, the problem isn't Darwin-specific.  I experimented with this on
Linux and found Linux does the same thing with libpgcommon_srv.a that
macOS does: a file in the archive that is totally unused is omitted
from the postgres binary.  In Linux, however, that doesn't prevent
pgcrypto from compiling anyway.  It does, however, prevent it from
working.  Instead of failing at compile time with a complaint about
missing symbols, it fails at load time.  I think that's because macOS
has -bundle-loader and we use it; without that, I think we'd get the
same behavior on macOS that we get on Windows.

The fundamental problem here is that the archive-member-dropping
behavior that we're getting here is not really what we want, and I
think that's going to happen on most or all architectures.  For GNU
ld, we could add -Wl,--whole-archive, and macOS has -all_load, but I
that this is just a nest of portability problems waiting to happen.  I
think there are two things we can do here that are far simpler:

1. Rejigger things so that we don't build libpgcommon_srv.a in the
first place, and instead add $(top_builddir)/src/common to
src/backend/Makefile's value of SUBDIRS.  With appropriate adjustments
to src/common/Makefile, this should allow us to include all of the
object files on the linker command line individually instead of
building an archive library that is then used only for the postgres
binary itself anyway.  Then, things wouldn't get dropped.

2. Just postpone committing this patch until we're ready to use the
new code in the backend someplace (or add a dummy reference to it
someplace).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] Password identifiers, protocol aging and SCRAM protocol

On Wed, Nov 16, 2016 at 8:04 PM, Michael Paquier
 wrote:
> In the current set of patches, the sha2 functions would not get used
> until the main patch for SCRAM gets committed so that's a couple of
> steps and many months ahead.. And --as-needed/--no-as-needed are not
> supported in macos. So I would believe that the best route is just to
> use this patch with the way it does things, and once SCRAM gets in we
> could switch the build into more appropriate linking. At least that's
> far less ugly than having fake objects in the backend code. Of course
> a comment in pgcrypo's Makefile would be appropriate.

Or a comment with a "ifeq ($(PORTNAME), darwin)" containing the
additional objects to make clear that this is proper to only OSX.
Other ideas are welcome.
-- 
Michael


-- 
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] Password identifiers, protocol aging and SCRAM protocol

On Wed, Nov 16, 2016 at 6:51 PM, Robert Haas  wrote:
> So, it seems that the linker is willing to drop archive members if the
> entire .o file is used, but not individual symbols.  That explains why
> Michael thinks we need to do something special here, because with his
> 0001 patch, nothing in the new sha2(_openssl).c file would immediately
> be used in the backend.  And indeed I see now that my earlier testing
> was done incorrectly, and pgcrypto does in fact fail to build under my
> proposal.  Oops.

Ah, thanks! I did not notice that before in configure.in:
if test "$PORTNAME" = "darwin"; then
  PGAC_PROG_CC_LDFLAGS_OPT([-Wl,-dead_strip_dylibs], $link_test_func)
elif test "$PORTNAME" = "openbsd"; then
  PGAC_PROG_CC_LDFLAGS_OPT([-Wl,-Bdynamic], $link_test_func)
else
  PGAC_PROG_CC_LDFLAGS_OPT([-Wl,--as-needed], $link_test_func)
fi

In the current set of patches, the sha2 functions would not get used
until the main patch for SCRAM gets committed so that's a couple of
steps and many months ahead.. And --as-needed/--no-as-needed are not
supported in macos. So I would believe that the best route is just to
use this patch with the way it does things, and once SCRAM gets in we
could switch the build into more appropriate linking. At least that's
far less ugly than having fake objects in the backend code. Of course
a comment in pgcrypo's Makefile would be appropriate.
-- 
Michael


-- 
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] Password identifiers, protocol aging and SCRAM protocol

On Wed, Nov 16, 2016 at 7:36 PM, Andres Freund  wrote:
> With -Wl,--as-neeeded the linker will dismiss unused symbols found in a
> static library. Maybe that's the difference?

The man page --as-needed says that --as-needed modifies the behavior
of dynamic libraries, not static ones.  If there is any such effect,
it is undocumented.  Here is the text:

LD> This option affects ELF DT_NEEDED tags for dynamic libraries mentioned
LD> on the command line after the --as-needed option. Normally the linker will
LD> add a DT_NEEDED tag for each dynamic library mentioned on the
LD> command line, regardless of whether the library is actually needed or not.
LD> --as-needed causes a DT_NEEDED tag to only be emitted for a library
LD> that at that point in the link satisfies a non-weak undefined
symbol reference
LD> from a regular object file or, if the library is not found in the DT_NEEDED
LD> lists of other needed libraries, a non-weak undefined symbol reference
LD> from another needed dynamic library. Object files or libraries appearing
LD> on the command line after the library in question do not affect whether the
LD> library is seen as needed. This is similar to the rules for
extraction of object
LD> files from archives. --no-as-needed restores the default behaviour.

Some experimentation on my Mac reveals that my previous statement
about how this works was incorrect.  See attached patch for what I
tried.  What I find is:

1. If I create an additional source file in src/common containing a
completely unused symbol (wunk) it appears in the nm output for
libpgcommon_srv.a but not in the nm output for the postgres binary.

2. If I add an additional function to an existing source file in
src/common containing a completely unused symbol (quux) it appears in
the nm output for both libpgcommon_srv.a and also in the nm output for
the postgres binary.

3. If I create an additional source file in src/backend containing a
completely unused symbol (blarfle) it appears in the nm output for the
postgres binary.

So, it seems that the linker is willing to drop archive members if the
entire .o file is used, but not individual symbols.  That explains why
Michael thinks we need to do something special here, because with his
0001 patch, nothing in the new sha2(_openssl).c file would immediately
be used in the backend.  And indeed I see now that my earlier testing
was done incorrectly, and pgcrypto does in fact fail to build under my
proposal.  Oops.

But I think that's a temporary thing.  As soon as the backend is using
the sha2 routines for anything (which is the point, right?) the build
changes become unnecessary.  For example, if I apply this patch:

--- a/src/backend/lib/binaryheap.c
+++ b/src/backend/lib/binaryheap.c
@@ -305,3 +305,7 @@ sift_down(binaryheap *heap, int node_off)
node_off = swap_off;
}
 }
+
+#include "common/sha2.h"
+extern void ugh(void);
+void ugh(void) { pg_sha224_init(NULL); }

...then the backend ends up sucking in everything in sha2.c and the
pgcrypto build works again.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/common/Makefile b/src/common/Makefile
index 03dfaa1..f84264a 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -46,7 +46,7 @@ OBJS_COMMON = config_info.o controldata_utils.o exec.o ip.o keywords.o \

 OBJS_FRONTEND = $(OBJS_COMMON) fe_memutils.o file_utils.o restricted_token.o

-OBJS_SRV = $(OBJS_COMMON:%.o=%_srv.o)
+OBJS_SRV = $(OBJS_COMMON:%.o=%_srv.o) wunk.o

 all: libpgcommon.a libpgcommon_srv.a

diff --git a/src/common/ip.c b/src/common/ip.c
index 797d910..d517802 100644
--- a/src/common/ip.c
+++ b/src/common/ip.c
@@ -258,3 +258,11 @@ getnameinfo_unix(const struct sockaddr_un * sa, int salen,
 	return 0;
 }
 #endif   /* HAVE_UNIX_SOCKETS */
+
+extern void quux(void);
+
+void
+quux(void)
+{
+	/* quux */
+}
diff --git a/src/common/wunk.c b/src/common/wunk.c
new file mode 100644
index 000..2db667c
--- /dev/null
+++ b/src/common/wunk.c
@@ -0,0 +1,7 @@
+extern void wunk(void);
+
+void
+wunk(void)
+{
+	/* wunk */
+}

-- 
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] Password identifiers, protocol aging and SCRAM protocol

2016-11-16 Thread Andres Freund

Hi,

On 2016-11-16 19:29:41 -0500, Robert Haas wrote:
> On Wed, Nov 16, 2016 at 6:56 PM, Michael Paquier
>  wrote:
> > On Wed, Nov 16, 2016 at 11:24 AM, Robert Haas  wrote:
> >> diff --git a/contrib/pgcrypto/Makefile b/contrib/pgcrypto/Makefile
> >> index 805db76..ddb0183 100644
> >> --- a/contrib/pgcrypto/Makefile
> >> +++ b/contrib/pgcrypto/Makefile
> >> @@ -1,6 +1,6 @@
> >>  # contrib/pgcrypto/Makefile
> >>
> >> -INT_SRCS = md5.c sha1.c sha2.c internal.c internal-sha2.c blf.c 
> >> rijndael.c \
> >> +INT_SRCS = md5.c sha1.c internal.c internal-sha2.c blf.c rijndael.c \
> >>  fortuna.c random.c pgp-mpi-internal.c imath.c
> >>  INT_TESTS = sha2
> >
> > I would like to do so. And while Linux is happy with that, macOS is
> > not, this results in linking resolution errors when compiling the
> > library.
>
> Well, I'm running macOS and it worked for me.  TBH, I don't even quite
> understand how it could NOT work.  What makes the symbols provided by
> libpgcommon any different from any other symbols that are part of the
> binary?  How could one set work and the other set fail?  I can
> understand how there might be some problem if the backend were
> dynamically linked libpgcommon, but it's not.  It's doing this:

With -Wl,--as-neeeded the linker will dismiss unused symbols found in a
static library. Maybe that's the difference?

Andres


-- 
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] Password identifiers, protocol aging and SCRAM protocol

On Wed, Nov 16, 2016 at 6:56 PM, Michael Paquier
 wrote:
> On Wed, Nov 16, 2016 at 11:24 AM, Robert Haas  wrote:
>> diff --git a/contrib/pgcrypto/Makefile b/contrib/pgcrypto/Makefile
>> index 805db76..ddb0183 100644
>> --- a/contrib/pgcrypto/Makefile
>> +++ b/contrib/pgcrypto/Makefile
>> @@ -1,6 +1,6 @@
>>  # contrib/pgcrypto/Makefile
>>
>> -INT_SRCS = md5.c sha1.c sha2.c internal.c internal-sha2.c blf.c rijndael.c \
>> +INT_SRCS = md5.c sha1.c internal.c internal-sha2.c blf.c rijndael.c \
>>  fortuna.c random.c pgp-mpi-internal.c imath.c
>>  INT_TESTS = sha2
>
> I would like to do so. And while Linux is happy with that, macOS is
> not, this results in linking resolution errors when compiling the
> library.

Well, I'm running macOS and it worked for me.  TBH, I don't even quite
understand how it could NOT work.  What makes the symbols provided by
libpgcommon any different from any other symbols that are part of the
binary?  How could one set work and the other set fail?  I can
understand how there might be some problem if the backend were
dynamically linked libpgcommon, but it's not.  It's doing this:

gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -O2 -Wall -Werror -L../../src/port -L../../src/common
-Wl,-dead_strip_dylibs  -Wall -Werror   access/brin/brin.o [many more
.o files omitted for brevity] utils/fmgrtab.o
../../src/timezone/localtime.o ../../src/timezone/strftime.o
../../src/timezone/pgtz.o ../../src/port/libpgport_srv.a
../../src/common/libpgcommon_srv.a -lm -o postgres

As I understand it, listing the .a file on the linker command line
like that is exactly equivalent to listing out each individual .o file
that is part of that static library.  There shouldn't be any
difference in how a symbol that's provided by one of the .o files
looks vs. how a symbol that's provided by one of the .a files looks.
Let's test it.

[rhaas pgsql]$ nm src/backend/postgres | grep -E 'GetUserIdAndContext|psprintf'
0001003d71d0 T _GetUserIdAndContext
00010040f160 T _psprintf

So... how would the dynamic loader know that it was supposed to find
the first one and fail to find the second one?  More to the point,
it's clear that it DOES find the second one on every platform in the
buildfarm, because adminpack, dblink, pageinspect, and pgstattuple all
use psprintf without the push-ups you are proposing to undertake here.
pg_md5_encrypt is used by passwordcheck, and forkname_to_number is
used by pageinspect and pg_prewarm.  It all just works.  No special
magic required.

> Yes we could do that for consistency with the other nix platforms. But
> is that really necessary as libpgcommon already has those objects?

The point is that *postgres* already has those objects.  You don't
need to include them twice.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] Password identifiers, protocol aging and SCRAM protocol

On Wed, Nov 16, 2016 at 11:24 AM, Robert Haas  wrote:
> diff --git a/contrib/pgcrypto/Makefile b/contrib/pgcrypto/Makefile
> index 805db76..ddb0183 100644
> --- a/contrib/pgcrypto/Makefile
> +++ b/contrib/pgcrypto/Makefile
> @@ -1,6 +1,6 @@
>  # contrib/pgcrypto/Makefile
>
> -INT_SRCS = md5.c sha1.c sha2.c internal.c internal-sha2.c blf.c rijndael.c \
> +INT_SRCS = md5.c sha1.c internal.c internal-sha2.c blf.c rijndael.c \
>  fortuna.c random.c pgp-mpi-internal.c imath.c
>  INT_TESTS = sha2

I would like to do so. And while Linux is happy with that, macOS is
not, this results in linking resolution errors when compiling the
library.

> And for Mkvcbuild.pm I think you could just do this:
>
> diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
> index de764dd..1993764 100644
> --- a/src/tools/msvc/Mkvcbuild.pm
> +++ b/src/tools/msvc/Mkvcbuild.pm
> @@ -114,6 +114,15 @@ sub mkvcbuild
>md5.c pg_lzcompress.c pgfnames.c psprintf.c relpath.c rmtree.c
>string.c username.c wait_error.c);
>
> +if ($solution->{options}->{openssl})
> +{
> +push(@pgcommonallfiles, 'sha2_openssl.c');
> +}
> +else
> +{
> +push(@pgcommonallfiles, 'sha2.c');
> +}
> +
>  our @pgcommonfrontendfiles = (
>  @pgcommonallfiles, qw(fe_memutils.c file_utils.c
>restricted_token.c));
> @@ -422,7 +431,7 @@ sub mkvcbuild
>  {
>  $pgcrypto->AddFiles(
>  'contrib/pgcrypto',   'md5.c',
> -'sha1.c', 'sha2.c',
> +'sha1.c',
>  'internal.c', 'internal-sha2.c',
>  'blf.c',  'rijndael.c',
>  'fortuna.c',  'random.c',
>
> Is there some reason that won't work?

Yes we could do that for consistency with the other nix platforms. But
is that really necessary as libpgcommon already has those objects?
-- 
Michael


-- 
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] Password identifiers, protocol aging and SCRAM protocol

On Wed, Nov 16, 2016 at 1:53 PM, Michael Paquier
 wrote:
>> Yeah, I don't see a point to that.
>
> OK, by doing so here is what I have. The patch generated by
> format-patch, as well as diffs generated by git diff -M are reduced
> and the patch gets half in size. They could be reduced more by adding
> at the top of sha2.c a couple of defined to map the old SHAXXX_YYY
> variables with their PG_ equivalents, but that does not seem worth it
> to me, and diffs are listed line by line.

All right, this version is much easier to review.  I am a bit puzzled,
though.  It looks like src/common will include sha2.o if built without
OpenSSL and sha2_openssl.o if built with OpenSSL.  So far, so good.
One would think, then, that pgcrypto would not need to worry about
these functions any more because libpgcommon_srv.a is linked into the
server, so any references to those symbols would presumably just work.
However, that's not what you did.  On Windows, you added a dependency
on libpgcommon which I think is unnecessary because that stuff is
already linked into the server.  On non-Windows systems, however, you
have instead taught pgcrypto to copy the source file it needs from
src/common and recompile it.  I don't understand why you need to do
any of that, or why it should be different on Windows vs. non-Windows.
So I think that the changes for the pgcrypto Makefile could just look
like this:

diff --git a/contrib/pgcrypto/Makefile b/contrib/pgcrypto/Makefile
index 805db76..ddb0183 100644
--- a/contrib/pgcrypto/Makefile
+++ b/contrib/pgcrypto/Makefile
@@ -1,6 +1,6 @@
 # contrib/pgcrypto/Makefile

-INT_SRCS = md5.c sha1.c sha2.c internal.c internal-sha2.c blf.c rijndael.c \
+INT_SRCS = md5.c sha1.c internal.c internal-sha2.c blf.c rijndael.c \
 fortuna.c random.c pgp-mpi-internal.c imath.c
 INT_TESTS = sha2

And for Mkvcbuild.pm I think you could just do this:

diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index de764dd..1993764 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -114,6 +114,15 @@ sub mkvcbuild
   md5.c pg_lzcompress.c pgfnames.c psprintf.c relpath.c rmtree.c
   string.c username.c wait_error.c);

+if ($solution->{options}->{openssl})
+{
+push(@pgcommonallfiles, 'sha2_openssl.c');
+}
+else
+{
+push(@pgcommonallfiles, 'sha2.c');
+}
+
 our @pgcommonfrontendfiles = (
 @pgcommonallfiles, qw(fe_memutils.c file_utils.c
   restricted_token.c));
@@ -422,7 +431,7 @@ sub mkvcbuild
 {
 $pgcrypto->AddFiles(
 'contrib/pgcrypto',   'md5.c',
-'sha1.c', 'sha2.c',
+'sha1.c',
 'internal.c', 'internal-sha2.c',
 'blf.c',  'rijndael.c',
 'fortuna.c',  'random.c',

Is there some reason that won't work?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] Password identifiers, protocol aging and SCRAM protocol

On Wed, Nov 16, 2016 at 4:46 AM, Robert Haas  wrote:
> On Tue, Nov 15, 2016 at 5:12 PM, Michael Paquier
>  wrote:
>> On Tue, Nov 15, 2016 at 12:40 PM, Robert Haas  wrote:
>>> On Tue, Nov 15, 2016 at 2:24 PM, Michael Paquier
>>>  wrote:
 How do you plug in that with OpenSSL? Are you suggesting to use a set
 of undef definitions in the new header in the same way as pgcrypto is
 doing, which is rather ugly? Because that's what the deal is about in
 this patch.
>>>
>>> Perhaps that justifies renaming them -- although I would think the
>>> fact that they are static would prevent conflicts -- but why reorder
>>> them and change variable names?
>>
>> Yeah... Perhaps I should not have done that, which was just for
>> consistency's sake, and even if the new reordering makes more sense
>> actually...
>
> Yeah, I don't see a point to that.

OK, by doing so here is what I have. The patch generated by
format-patch, as well as diffs generated by git diff -M are reduced
and the patch gets half in size. They could be reduced more by adding
at the top of sha2.c a couple of defined to map the old SHAXXX_YYY
variables with their PG_ equivalents, but that does not seem worth it
to me, and diffs are listed line by line.
-- 
Michael
From 3171c40390703e9b12f97e25914f31accf480a52 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Wed, 16 Nov 2016 10:48:42 -0800
Subject: [PATCH] Refactor SHA2 functions and move them to src/common/

This way both frontend and backends can refer to them if needed. Those
functions are taken from pgcrypto, which now fetches directly the source
files it needs from src/common/ when compiling its library.

A new interface, which is more PG-like is designed for those SHA2 functions,
allowing to link to either OpenSSL or the in-core stuff taken from KAME
as need be, which is the most flexible solution.
---
 contrib/pgcrypto/.gitignore |   4 +
 contrib/pgcrypto/Makefile   |   5 +-
 contrib/pgcrypto/fortuna.c  |  12 +--
 contrib/pgcrypto/internal-sha2.c|  82 +++
 contrib/pgcrypto/sha2.h | 100 --
 src/common/Makefile |   6 ++
 {contrib/pgcrypto => src/common}/sha2.c | 174 +---
 src/common/sha2_openssl.c   | 102 +++
 src/include/common/sha2.h   | 115 +
 src/tools/msvc/Mkvcbuild.pm |  22 ++--
 10 files changed, 388 insertions(+), 234 deletions(-)
 delete mode 100644 contrib/pgcrypto/sha2.h
 rename {contrib/pgcrypto => src/common}/sha2.c (82%)
 create mode 100644 src/common/sha2_openssl.c
 create mode 100644 src/include/common/sha2.h

diff --git a/contrib/pgcrypto/.gitignore b/contrib/pgcrypto/.gitignore
index 5dcb3ff..30619bf 100644
--- a/contrib/pgcrypto/.gitignore
+++ b/contrib/pgcrypto/.gitignore
@@ -1,3 +1,7 @@
+# Source file copied from src/common
+/sha2.c
+/sha2_openssl.c
+
 # Generated subdirectories
 /log/
 /results/
diff --git a/contrib/pgcrypto/Makefile b/contrib/pgcrypto/Makefile
index 805db76..4085abb 100644
--- a/contrib/pgcrypto/Makefile
+++ b/contrib/pgcrypto/Makefile
@@ -4,7 +4,7 @@ INT_SRCS = md5.c sha1.c sha2.c internal.c internal-sha2.c blf.c rijndael.c \
 		fortuna.c random.c pgp-mpi-internal.c imath.c
 INT_TESTS = sha2
 
-OSSL_SRCS = openssl.c pgp-mpi-openssl.c
+OSSL_SRCS = openssl.c pgp-mpi-openssl.c sha2_openssl.c
 OSSL_TESTS = sha2 des 3des cast5
 
 ZLIB_TST = pgp-compression
@@ -59,6 +59,9 @@ SHLIB_LINK += $(filter -leay32, $(LIBS))
 SHLIB_LINK += -lws2_32
 endif
 
+sha2.c sha2_openssl.c: % : $(top_srcdir)/src/common/%
+	rm -f $@ && $(LN_S) $< .
+
 rijndael.o: rijndael.tbl
 
 rijndael.tbl:
diff --git a/contrib/pgcrypto/fortuna.c b/contrib/pgcrypto/fortuna.c
index 5028203..ba74db6 100644
--- a/contrib/pgcrypto/fortuna.c
+++ b/contrib/pgcrypto/fortuna.c
@@ -34,9 +34,9 @@
 #include 
 #include 
 
+#include "common/sha2.h"
 #include "px.h"
 #include "rijndael.h"
-#include "sha2.h"
 #include "fortuna.h"
 
 
@@ -112,7 +112,7 @@
 #define CIPH_BLOCK		16
 
 /* for internal wrappers */
-#define MD_CTX			SHA256_CTX
+#define MD_CTX			pg_sha256_ctx
 #define CIPH_CTX		rijndael_ctx
 
 struct fortuna_state
@@ -154,22 +154,22 @@ ciph_encrypt(CIPH_CTX * ctx, const uint8 *in, uint8 *out)
 static void
 md_init(MD_CTX * ctx)
 {
-	SHA256_Init(ctx);
+	pg_sha256_init(ctx);
 }
 
 static void
 md_update(MD_CTX * ctx, const uint8 *data, int len)
 {
-	SHA256_Update(ctx, data, len);
+	pg_sha256_update(ctx, data, len);
 }
 
 static void
 md_result(MD_CTX * ctx, uint8 *dst)
 {
-	SHA256_CTX	tmp;
+	pg_sha256_ctx	tmp;
 
 	memcpy(, ctx, sizeof(*ctx));
-	SHA256_Final(dst, );
+	pg_sha256_final(, dst);
 	px_memset(, 0, sizeof(tmp));
 }
 
diff --git a/contrib/pgcrypto/internal-sha2.c b/contrib/pgcrypto/internal-sha2.c
index 55ec7e1..e06f554 100644
---

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol