WIP: SCRAM authentication

* Heikki Linnakangas (hlinnaka@iki.fi) wrote:

There have been numerous threads on replacing our MD5 authentication
method, so I started hacking on that to see what it might look like.
Just to be clear, this is 9.6 material. Attached is a WIP patch
series that adds support for SCRAM. There's no need to look at the
details yet, but it demonstrates what the protocol changes and the
code structure would be like.

Great! Very glad that you're working on this.

I'm not wedded to SCRAM - SRP or JPAKE or something else might be
better. But replacing the algorithm, or adding more of them, should
be straightforward with this.

Excellent.

3. Allow storing multiple verifiers in pg_authid
------------------------------------------------

Replace the pg_authid.rolpassword text field with an array, and
rename it to 'rolverifiers'. This allows storing multiple password
hashes: an MD5 hash for MD5 authentication, and a SCRAM salt and
stored key for SCRAM authentication, etc. Each element in the array
is a string that begins with the method's name. For example
"md5:<MD5 hash>", or "password:<plaintext>".

For dump/reload, and for clients that wish to create the hashes in
the client-side, there is a new option to CREATE/ALTER USER
commands: PASSWORD VERIFIERS '{ ... }', that allows replacing the
array.

The old "ENCRYPTED/UNENCRYPTED PASSWORD 'foo'" options are still
supported for backwards-compatibility, but it's not clear what it
should mean now.

TODO:

* Password-checking hook needs to be redesigned, to allow for more
kinds of hashes.

* With "CREATE USER PASSWORD 'foo'", which hashes/verifiers should
be generated by default? We currently have a boolean
password_encryption setting for that. Needs to be a list.

This generally sounds good to me but we definitely need to have that
list of hashes to be used. The MIT KDC for Kerberos (and I believe all
the other Kerberos implementations) have a similar setting for what will
be stored and what will be allowed for hashing and encryption options.
It's very important that we allow users to tweak this list, as we will
want to encourage users to migrate off of the existing md5 storage
mechanism and on to the SCRAM based one eventually.

Unfortunately, the first major release with this will certainly need to
default to including md5 as we can't have a password update or change
break clients right off the bat. What I think would be fantastic would
be a warning, perhaps in the first release or maybe the second, which
deprecates md5 as an auth method and is thrown when a password is set
which includes storing an md5-based password. I'm sure there will be
plenty of discussion about that in the future.

One additional item is that we need to have a way to prefer SCRAM-based
auth while allowing a fall-back to md5 if the client doesn't support it.
This might have to be driven by the client side explicitly saying "I
support SCRAM" from the start to avoid breaking existing clients.

4. Implement SCRAM
------------------

The protocol and the code is structured so that it would be fairly
easy to add more built-in SASL mechanisms, or to use a SASL library
to provide more. But for now I'm focusing on adding exactly one new
built-in mechanism, to replace MD5 in the long term.

In the protocol, there is a new AuthenticationSASL message,
alongside the existing AuthenticationMD5, AuthenticationSSPI etc.
The AuthenticationSASL message contains the name of the SASL
mechanism used ("SCRAM-SHA-1"). Just like in the GSSAPI/SSPI
authentication, a number of PasswordMessage and
AuthenticationSASLContinue messages are exchanged after that,
carrying the data specified by the SCRAM spec, until the
authentication succeeds (or not).

TODO:

* Per the SCRAM specification, the client sends the username in the
handshake. But in the FE/BE protocol, we've already sent it in the
startup packet. In the patch, libpq always sends an empty username
in the SCRAM exchange, and the username from the startup packet is
what matters. We could also require it to be the same, but in SCRAM
the username to be UTF-8 encoded, while in PostgreSQL the username
can be in any encoding. That is a source of annoyance in itself, as
it's not well-defined in PostgreSQL which encoding to use when
sending a username to the server. But I don't want to try fixing
that in this patch, so it seems easiest to just require the username
to be empty.

I don't like having it be empty.. I'm not looking at the spec right at
the moment, but have you confirmed that the username being empty during
the SCRAM discussion doesn't reduce the effectiveness of the
authentication method overall in some way? Is it ever used in
generation of the authentication verifier, etc? One way to address the
risk which you bring up about the different encodings might be to simply
discourage using non-UTF8-compliant encodings by throwing a warning or
refusing to support SCRAM in cases where the role wouldn't be allowed by
SCRAM (eg: in CREATE ROLE or ALTER ROLE when the SCRAM auth verifier
storage is being handled). Another option might be to define a way to
convert from "whatever" to "UTF8 something" for the purposes of the
SCRAM auth method.

* Need a source of randomness in client, to generate random nonces
used in the handshake. The SCRAM specification is not explicit about
it, but I believe it doesn't need to be unpredictable, as long as a
different nonce is used for each authentication.

I'd *very* much prefer a well defined and understood way (ideally
implemented in some well known and maintained library) rather than
trying to work out something ourselves. Further, it'd be good to review
what others have done in this space with SCRAM as there may be lessons
learned or at least well reviewed approaches to consider.

* The client does not authenticate the server, even though the SCRAM
protocol allows that. The client does verify the proof the server
sends, but nothing stops a malicious server that's impersonating the
real server from not requesting SCRAM authentication in the first
place. It could just send AuthenticationOK without any
authentication at all. To take advantage of the server
authentication, we'll need to add something similar to the
"sslmode=verify-ca" option in the client. In that mode, the client
should refuse the connection if the server doesn't request SCRAM
authentication (or some other future authentication mechanism that
authenticates the server to the client).

Agreed, we should have a way for the client to require SCRAM.
Presumably we would do this for libpq-based clients and expect other
implementations to look at the options we build into libpq for their own
versions (eg: JDBC). There's nothing protocol-level to be done here
that I can think of off-hand.

* Channel binding is not implemented. Not essential, but would be
nice to have. Together with the new client option mentioned in the
previous point, it would allow the client to know that there is no
man-in-the-middle, without having to verify the server's SSL
certificate.

Agreed, this is definitely one of the good features of SCRAM and should
be included.

Haven't looked at the code at all.

Thanks!

Stephen

On 03/30/2015 06:46 PM, Stephen Frost wrote:

* Heikki Linnakangas (hlinnaka@iki.fi) wrote:

* With "CREATE USER PASSWORD 'foo'", which hashes/verifiers should
be generated by default? We currently have a boolean
password_encryption setting for that. Needs to be a list.

This generally sounds good to me but we definitely need to have that
list of hashes to be used. The MIT KDC for Kerberos (and I believe all
the other Kerberos implementations) have a similar setting for what will
be stored and what will be allowed for hashing and encryption options.
It's very important that we allow users to tweak this list, as we will
want to encourage users to migrate off of the existing md5 storage
mechanism and on to the SCRAM based one eventually.

Unfortunately, the first major release with this will certainly need to
default to including md5 as we can't have a password update or change
break clients right off the bat. What I think would be fantastic would
be a warning, perhaps in the first release or maybe the second, which
deprecates md5 as an auth method and is thrown when a password is set
which includes storing an md5-based password. I'm sure there will be
plenty of discussion about that in the future.

Yeah. And even if client are updated, and the server is upgraded, you
still cannot use SCRAM until all the passwords have been changed and the
SCRAM verifiers for them generated. Unless we go with the scheme I
mentioned earlier, and use the MD5 hash of the password as the
"plaintext" password to SCRAM.

One additional item is that we need to have a way to prefer SCRAM-based
auth while allowing a fall-back to md5 if the client doesn't support it.
This might have to be driven by the client side explicitly saying "I
support SCRAM" from the start to avoid breaking existing clients.

I'll start a separate thread on this. It's an interesting feature on its
own. As well as an option in libpq to refuse plaintext authentication
even if the server asks for it.

* Per the SCRAM specification, the client sends the username in the
handshake. But in the FE/BE protocol, we've already sent it in the
startup packet. In the patch, libpq always sends an empty username
in the SCRAM exchange, and the username from the startup packet is
what matters. We could also require it to be the same, but in SCRAM
the username to be UTF-8 encoded, while in PostgreSQL the username
can be in any encoding. That is a source of annoyance in itself, as
it's not well-defined in PostgreSQL which encoding to use when
sending a username to the server. But I don't want to try fixing
that in this patch, so it seems easiest to just require the username
to be empty.

I don't like having it be empty.. I'm not looking at the spec right at
the moment, but have you confirmed that the username being empty during
the SCRAM discussion doesn't reduce the effectiveness of the
authentication method overall in some way?

Yes.

Is it ever used in
generation of the authentication verifier, etc? One way to address the
risk which you bring up about the different encodings might be to simply
discourage using non-UTF8-compliant encodings by throwing a warning or
refusing to support SCRAM in cases where the role wouldn't be allowed by
SCRAM (eg: in CREATE ROLE or ALTER ROLE when the SCRAM auth verifier
storage is being handled). Another option might be to define a way to
convert from "whatever" to "UTF8 something" for the purposes of the
SCRAM auth method.

Presumably the username used in the SCRAM exchange would have to match
the username sent in the startup packet. Otherwise things get weird. If
an empty string is a problem (there actually seems to be some language
in the spec to forbid or at least discourage using an empty string as
username), we could also specify some other constant that must be used,
to mean "same as in startup packet".

- Heikki

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

Heikki,

* Heikki Linnakangas (hlinnaka@iki.fi) wrote:

On 03/30/2015 06:46 PM, Stephen Frost wrote:

Unfortunately, the first major release with this will certainly need to
default to including md5 as we can't have a password update or change
break clients right off the bat. What I think would be fantastic would
be a warning, perhaps in the first release or maybe the second, which
deprecates md5 as an auth method and is thrown when a password is set
which includes storing an md5-based password. I'm sure there will be
plenty of discussion about that in the future.

Yeah. And even if client are updated, and the server is upgraded,
you still cannot use SCRAM until all the passwords have been changed
and the SCRAM verifiers for them generated. Unless we go with the
scheme I mentioned earlier, and use the MD5 hash of the password as
the "plaintext" password to SCRAM.

I don't like using the MD5 hash of the password for the token or to
generate the verifier. I'm no cryptographer and can't speak with any
certainty on the topic, but I understand there can be cases where doing
such would actively reduce the complexity required to find a usable
token. Further, to be frank and slightly paranoid, it's possible that
individuals have built up caches of known-to-be-valid PG-encoded md5
strings from backups or other stolen data and we would not be doing
people who run those systems any favors with that approach.

At the end of the day, I'd much prefer to see a clean break with new
passwords rolled out by administrators working with their users. That,
of course, would be much easier if we provided the basics that every
other auth system out there does today, which are the capabilities
available through pam_unix, pam_pwquality, pam_cracklib, etc. In
particular, things like password history, password validity, password
complexity, account inactivity, account login history (successful vs.
failed attempts), are required in many environments, specifically called
out by NIST 800-53 which is required for US Government deployments, and
probably included in other standards also (eg: PCI, HIPPA, etc).

These are all things I was fighting for 10-or-so years ago. I sincerely
hope that we're ready to consider these capabilities as being desirable.
I'm certainly hoping to work on them for 9.6 and will be happy to
support them going forward.

One additional item is that we need to have a way to prefer SCRAM-based
auth while allowing a fall-back to md5 if the client doesn't support it.
This might have to be driven by the client side explicitly saying "I
support SCRAM" from the start to avoid breaking existing clients.

I'll start a separate thread on this. It's an interesting feature on
its own. As well as an option in libpq to refuse plaintext
authentication even if the server asks for it.

Agreed on both counts.

Is it ever used in
generation of the authentication verifier, etc? One way to address the
risk which you bring up about the different encodings might be to simply
discourage using non-UTF8-compliant encodings by throwing a warning or
refusing to support SCRAM in cases where the role wouldn't be allowed by
SCRAM (eg: in CREATE ROLE or ALTER ROLE when the SCRAM auth verifier
storage is being handled). Another option might be to define a way to
convert from "whatever" to "UTF8 something" for the purposes of the
SCRAM auth method.

Presumably the username used in the SCRAM exchange would have to
match the username sent in the startup packet. Otherwise things get
weird. If an empty string is a problem (there actually seems to be
some language in the spec to forbid or at least discourage using an
empty string as username), we could also specify some other constant
that must be used, to mean "same as in startup packet".

Ok.. Having it be a constant which means "same as in startup packet"
may be workable. My suggestion above was intended to be "let's figure
out a way to encode whatever is in the startup packet to work in UTF8,
and then we will decode it ourselves" kind of idea. That would make it
byte-wise different (at least in some cases- we might be able to
minimize the number of cases that happens), but semantically identical.
What would be *really* nice would be to say "if your client only speaks
UTF8, then you don't have to worry about this and everything just works
like normal." That might be too much of a stretch though.

Thanks!

Stephen

On Mon, Mar 30, 2015 at 7:52 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

There have been numerous threads on replacing our MD5 authentication
method, so I started hacking on that to see what it might look like. Just
to be clear, this is 9.6 material. Attached is a WIP patch series that adds
support for SCRAM. There's no need to look at the details yet, but it
demonstrates what the protocol changes and the code structure would be like.

I'm not wedded to SCRAM - SRP or JPAKE or something else might be better.
But replacing the algorithm, or adding more of them, should be
straightforward with this.

Agreed. We need such a facility.

There is no negotiation of the authentication mechanism. SCRAM is just
added as a new one, alongside all the existing ones. If the server requests
SCRAM authentication, but the client doesn't support it, the attempt will
fail. We might want to do something about that, to make the transition
easier, but it's an orthogonal feature and not absolutely required.

There are four patches in the series. The first two are just refactoring:
moving the SHA-1 implementation from pgcrypto to src/common, and some
refactoring in src/backend/auth.c that IMHO would make sense anyway.

The two first patches of the series look good to me.

Patches three and four are the interesting ones:

I have not looked in details yet at number implementing SCRAM.

3. Allow storing multiple verifiers in pg_authid
------------------------------------------------

Replace the pg_authid.rolpassword text field with an array, and rename it
to 'rolverifiers'. This allows storing multiple password hashes: an MD5
hash for MD5 authentication, and a SCRAM salt and stored key for SCRAM
authentication, etc. Each element in the array is a string that begins with
the method's name. For example "md5:<MD5 hash>", or "password:<plaintext>".

For dump/reload, and for clients that wish to create the hashes in the
client-side, there is a new option to CREATE/ALTER USER commands: PASSWORD
VERIFIERS '{ ... }', that allows replacing the array.

The old "ENCRYPTED/UNENCRYPTED PASSWORD 'foo'" options are still supported
for backwards-compatibility, but it's not clear what it should mean now.

TODO:

* Password-checking hook needs to be redesigned, to allow for more kinds
of hashes.

* With "CREATE USER PASSWORD 'foo'", which hashes/verifiers should be
generated by default? We currently have a boolean password_encryption
setting for that. Needs to be a list.

I have been looking more in depths at this one, which adds essential
infrastructure to support multiple authentication hashes for more
protocols. Here are some comments:
- Docs are missing (not a big issue for a WIP)
- Instead of an array that has an identified embedded, let's add a new
catalog pg_authid_hashes that stores all the hashes for a user (idea by
Heikki):
-- hashrol, role Oid associated with the hash
-- hashmet, hash method
-- hashval, value of the hash
- New password-checking hook (contrib/passwordcheck will need a refresh).
As of now, we have that:
void (*check_password_hook_type)
(const char *username,
const char *password,
int password_type,
Datum validuntil_time,
bool validuntil_null);
We need to switch to something that checks a list of hashes:
void (*check_password_hook_type)
(const char *username,
list *passwd,
Datum validuntil_time,
bool validuntil_null);
passwd is a structure containing the password type and the hash value.
Password type can then be "plain" (or password to match pg_hba.conf) or
"md5" for now.
- When password_encryption is switched to a list, true means md5, and false
means plain. At the addition of SCRAM, we could think harder the default
value, "true" may be worth meaning "md5,scram".
- For CREATE ROLE/ALTER ROLE, it is necessary to be able to define the list
of hashes that need to be generated, with something like that for example:
[ ENCRYPTED [(md5[, scram])] | UNENCRYPTED ] PASSWORD 'password'
When UNENCRYPTED is used, we could simply store the password as plain. When
only ENCRYPTED is used, we store it for all the methods available, except
"plain". ENCRYPTED and plain are not allowed combinations.
- Also, do we really want an option at SQL level to allow storing custom
hashes generated on client side as a first step? We could have something
like WITH (md5 = 'blah', scram = 'blah2') appended after PASSWORD for
example.
- rolpassword is removed from pg_authid.

I am willing to write a patch for the next CF following more or less those
lines, depending of course on the outcome of the discussion we can have
here, so feel free to comment.

I'll have a look more in-depth at the scram patch as well.
Regards,
--
Michael

On Tue, Aug 4, 2015 at 4:20 PM, Michael Paquier wrote:

I have been looking more in depths at this one, which adds essential
infrastructure to support multiple authentication hashes for more protocols.
Here are some comments:
[spec lines]
I am willing to write a patch for the next CF following more or less those
lines, depending of course on the outcome of the discussion we can have
here, so feel free to comment.

OK, attached as 0001 is the patch that respects those lines for the
support of multiple password verifiers in system catalogs. I have
added a new catalog called pg_auth_verifiers that is used at
authentication to fetch a password value depending on the protocol
used. With only this patch attached there are two types of verifiers:
plain and md5. This new catalog is REVOKE'd like pg_authid (pg_authid
could be made readable be this seems sensitive to me so I am not
changing it).

I have as well done the following things:
- Added PASSWORD VERIFIER (md5 = 'hoge', plain = 'hoge') which is used
as well by pg_dump all to be able to specify password verifiers one by
one.
- password check hook has been reworked as mentioned to be able to
manage a list of password verifiers instead of a single entry.
contrib/passwordcheck has been updated as well.
- Added regression tests testing UNENCRYPTED/ENCRYPTED, PASSWORD
VERIFIER, PASSWORD, etc.
- The patch does not break backward compatibility regarding CREATE
ROLE and ALTER ROLE.
- password_encryption has been changed to a list with comma-separated
elements, for now the possible elements are 'md5' and 'plain'. This
breaks backward compatibility, so if we care about it we should
consider having a new GUC password_encryption_list or similar. Default
is md5, default that does not change backward compatibility.
- Added documentation.
- pg_shadow has been updated, switching to an array with
method:password as elements.

I'll have a look more in-depth at the scram patch as well.

The SCRAM patch (0002~0004) has been rebased to use the new facility.
I have as well fixed a syscache leak... But I haven't been able to
enter much in the details yet.

The patch 0001 to add the multiple verifier facility is in a state
good enough to get some review, so I am registering an entry for it in
the next CF. And I'll continue the work on the SCRAM portion next
week, with hopefully a version ready for CF submission.
Regards,
--
Michael

On Fri, Aug 7, 2015 at 3:22 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Tue, Aug 4, 2015 at 4:20 PM, Michael Paquier wrote:

I have been looking more in depths at this one, which adds essential
infrastructure to support multiple authentication hashes for more protocols.
Here are some comments:
[spec lines]
I am willing to write a patch for the next CF following more or less those
lines, depending of course on the outcome of the discussion we can have
here, so feel free to comment.

OK, attached as 0001 is the patch that respects those lines for the
support of multiple password verifiers in system catalogs. I have
added a new catalog called pg_auth_verifiers that is used at
authentication to fetch a password value depending on the protocol
used. With only this patch attached there are two types of verifiers:
plain and md5. This new catalog is REVOKE'd like pg_authid (pg_authid
could be made readable be this seems sensitive to me so I am not
changing it).

I have as well done the following things:
- Added PASSWORD VERIFIER (md5 = 'hoge', plain = 'hoge') which is used
as well by pg_dump all to be able to specify password verifiers one by
one.

Maybe I'm chiming in too late here but I am sorta unimpressed by this.
If the user's password is stored both MD5-hashed and hashed some other
way in the system catalogs, that's less secure than storing it in the
least secure of those ways. And I'm afraid that if we introduce this
new mechanism, we won't really gain any security, because everybody
will just pg_dump or pg_upgrade and the old passwords will stick
around in the system forever. In fact we might lose security if
somebody changes one password verifier but doesn't realize that the
other one is still floating around, memorializing the old password,
and still available to be used for login.

I think we should look for a solution that either (a) allows SCRAM
authentication without requiring any changes to the contents of
pg_authid, like what Heikki proposed before; or (b) forces a hard
break, where at each password change you can decide if you want the
old or new format (probably based on the current value of some
compatibility GUC).

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

On 08/07/2015 09:26 PM, Robert Haas wrote:

Maybe I'm chiming in too late here but I am sorta unimpressed by this.
If the user's password is stored both MD5-hashed and hashed some other
way in the system catalogs, that's less secure than storing it in the
least secure of those ways. And I'm afraid that if we introduce this
new mechanism, we won't really gain any security, because everybody
will just pg_dump or pg_upgrade and the old passwords will stick
around in the system forever. In fact we might lose security if
somebody changes one password verifier but doesn't realize that the
other one is still floating around, memorializing the old password,
and still available to be used for login.

Yeah, that's certainly a risk. You wouldn't want to keep around
verifiers for authentication methods you don't use.

I think we should look for a solution that either (a) allows SCRAM
authentication without requiring any changes to the contents of
pg_authid, like what Heikki proposed before; or (b) forces a hard
break, where at each password change you can decide if you want the
old or new format (probably based on the current value of some
compatibility GUC).

Yeah, something to force a hard break when you want it would be really
good. Perhaps a command you can run to remove all MD5 hashes, or at
least find all the roles that have them. And a GUC to disallow creating
new ones.

- Heikki

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

On Sat, Aug 8, 2015 at 3:45 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 08/07/2015 09:26 PM, Robert Haas wrote:

Maybe I'm chiming in too late here but I am sorta unimpressed by this.
If the user's password is stored both MD5-hashed and hashed some other
way in the system catalogs, that's less secure than storing it in the
least secure of those ways. And I'm afraid that if we introduce this
new mechanism, we won't really gain any security, because everybody
will just pg_dump or pg_upgrade and the old passwords will stick
around in the system forever. In fact we might lose security if
somebody changes one password verifier but doesn't realize that the
other one is still floating around, memorializing the old password,
and still available to be used for login.

Yeah, that's certainly a risk. You wouldn't want to keep around verifiers
for authentication methods you don't use.

Yep, I cannot refute that. And there is actually the same problem with
the first version of the patch proposed on this thread if that's what
you are referring at below.

I think we should look for a solution that either (a) allows SCRAM
authentication without requiring any changes to the contents of
pg_authid, like what Heikki proposed before; or (b) forces a hard
break, where at each password change you can decide if you want the
old or new format (probably based on the current value of some
compatibility GUC).

FWIW, the patch resets all the existing entries should any
CREATE/ALTER ROLE involving a password should be run, even if
pg_auth_verifiers has entries for method not specified with PASSWORD
VERIFIERS.

Yeah, something to force a hard break when you want it would be really good.
Perhaps a command you can run to remove all MD5 hashes, or at least find all
the roles that have them. And a GUC to disallow creating new ones.

This filtering machinery definitely looks like a GUC to me, something
like password_forbidden_encryption that PASSWORD VERIFIERS looks at
and discards the methods listed in there. This definitely needs to be
separated from password_encryption.
--
Michael

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

On Fri, Aug 7, 2015 at 6:54 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

This filtering machinery definitely looks like a GUC to me, something
like password_forbidden_encryption that PASSWORD VERIFIERS looks at
and discards the methods listed in there. This definitely needs to be
separated from password_encryption.

I don't know what a "password verifier" is and I bet nobody else does
either. Well, I think I sort of know: I think it's basically an
encrypted password. Am I right? Even if I am, I bet the average user
is going to scratch their head and punt.

I don't see that there's any good reason to allow the same password to
be stored in the catalog encrypted more than one way, and I don't
think there's any good reason to introduce the PASSWORD VERIFIER
terminology. I think we should store (1) your password, either
encrypted or unencrypted; and (2) the method used to encrypt it. And
that's it.

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/08/2015 06:27 AM, Robert Haas wrote:

terminology. I think we should store (1) your password, either
encrypted or unencrypted; and (2) the method used to encrypt it.
And that's it.

A petty complaint, but it has always bothered me that we say the
password is encrypted when, at least currently, it is a simple hash
(cryptographic hash yes, but not encrypted, and not even an HMAC). I
think we should try to start using accurate terminology.

- --
Joe Conway
Crunchy Data
Enterprise PostgreSQL
http://crunchydata.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVxhm7AAoJEDfy90M199hlWRsQAIPliVReOYRRD/BJaZlB9Vjs
4YDolZZD9zR2+VPNxG/VaGHJ68rXlnfU/P0GrIQrS67t1xgwPxbUW6TCBsXJDnIE
wo7i5mJn9yn+AowccFiZTToKK8oNjRd33OJ2q00lAGiuaksnBhcJjMCNUHqf1Oz2
rUA/YiTp7RHXOQfiAxSoMKytK2y+rnQA+rnvPiE7XLKYE9rZ5rLiGhV0MPaNOFms
aHZIcYX5Tl2I3RsCexLMMA1qM001wSTyoti7o9gL71EXLV6ea6xt10a++k6oJ19y
oU7WjwKgV2XOGlQNC3/rUEKvuAtQhTlJpx9Q6xmTYidN0QHkZDdpJUblGZoxR2Vz
lT2zZdcpDhENynFZ1nTsd+CNWsn5T5vTVgnuKpG5qIMgT+kSG2JeiS7h+RY4rRtk
bl08tZmQBUBu/3hrRxQVPrt1NISteKXem2OLGphIKQEOmu/Kf43msYHQ+1qY0FTB
TZ96tVJnYTjQZp2P0IdjMf0qpOzK8qkMx2Tb6WehMd9yD1DtxQyKmxGpvssgEmQ7
1n3L/HCKWXF0MbI8QefIsO70ft4hzib5V+G7YmF00dWQM7NhDZYf6ejn1WmCP26u
w9wOHQcCAAKPI2knh3k2Ngdynl8Gofkxr7Le+NW7TGM+bp2U5EStTEH0r70mzEIg
KvB4dWX+tlZowujUmFhL
=VDCN
-----END PGP SIGNATURE-----

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

* Robert Haas (robertmhaas@gmail.com) wrote:

On Fri, Aug 7, 2015 at 6:54 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

This filtering machinery definitely looks like a GUC to me, something
like password_forbidden_encryption that PASSWORD VERIFIERS looks at
and discards the methods listed in there. This definitely needs to be
separated from password_encryption.

I don't know what a "password verifier" is and I bet nobody else does
either. Well, I think I sort of know: I think it's basically an
encrypted password. Am I right? Even if I am, I bet the average user
is going to scratch their head and punt.

Password verifier is actually a well understood term when it comes to
these protocols and their implementations. It is not an encrypted
password but rather a value which allows the server to determine if the
client knows the correct password, without having to store the password
directly, or a simple hash of the password, or have the clear password
sent from the client sent to the server.

I don't see that there's any good reason to allow the same password to
be stored in the catalog encrypted more than one way, and I don't
think there's any good reason to introduce the PASSWORD VERIFIER
terminology. I think we should store (1) your password, either
encrypted or unencrypted; and (2) the method used to encrypt it. And
that's it.

Perhaps we don't want to expose what a password verifier is to users,
but we shouldn't be missing the distinction between hashed passwords,
encrypted passwords, and password verifiers in the code and in the
implementation of SCRAM. We really shouldn't use an incorrect term for
what we're storing in pg_authid either though, which is what we do
today.

I can't see us ever storing encrypted passwords as that implies we'd
need a key stored somewhere and further that the server would be able to
get back to the user's original password, neither of which are things we
want to deal with.

You do have a good point that there is some risk associated with having
multiple values in pg_authid related to a user's password and that we
really want to help users move from the old value in pg_authid to the
new one. I don't believe we should force a hard change as it's going to
cause a lot of trouble for users. We have to also consider that clients
also have to be changed for this.

As discussed previously, in an ideal world, we would handle the old
values and the new ones while introducing password ageing, client
support for detecting that a password needs to be changed, protocol
support for changing passwords which avoids having them get logged in
cleartext to the server log, password complexity, and perhaps used
password history to boot. The main issue here is that we really don't
provide any help for large installations to get their userbase moved off
of the old style today. Password ageing (and good support for it in
clients, etc), would help that greatly.

Unfortunately, it's unlikely that we're going to get all of that done in
one release. As such, I'd suggest our next release support the existing
values in pg_authid, add the password verifier when the password is
changed, and then add a GUC in the following release which disables the
old pg_authid mechanism, defaulting to true, and the release after that
remove support for the old value and the field for it completely.

We should also provide documentation about how to check if there are any
old style values, for users who want to be proactive about moving off of
the old style.

I'm travelling and so I haven't looked over the patch yet (or even read
the entire thread in depth), so apologies if I've got something confused
about what's being proposed.

Thanks!

Stephen

On 08/08/2015 04:27 PM, Robert Haas wrote:

I don't see that there's any good reason to allow the same password to
be stored in the catalog encrypted more than one way,

Sure there is. If you want to be able to authenticate using different
mechanism, you need the same password "encrypted" in different ways.
SCRAM uses verifier that's derived from the password in one way, MD5
authentication needs an MD5 hash, and yet other protocols have other
requirements.

and I don't think there's any good reason to introduce the PASSWORD
VERIFIER terminology. I think we should store (1) your password,
either encrypted or unencrypted; and (2) the method used to encrypt
it. And that's it.

Like Joe and Stephen, I actually find it highly confusing that we call
the MD5 hash an "encrypted password". The term "password verifier" is
fairly common in the specifications of authentication mechanisms. I
think we should adopt 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

On Sat, Aug 8, 2015 at 6:23 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

Like Joe and Stephen, I actually find it highly confusing that we call the
MD5 hash an "encrypted password". The term "password verifier" is fairly
common in the specifications of authentication mechanisms. I think we should
adopt it.

Speaking as someone who hasn't read the specifications I found
"password verifier" surprising. I would have known what "password
hash" was but I misread "verifier" to be something functional like a
PAM plugin. I tend to agree we should just use terminology out of the
specs though even if it's a little opaque, better one opaque piece of
terminology than having to learn and translate between multiple
terminologies.

--
greg

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

On 08/08/2015 10:23 AM, Heikki Linnakangas wrote:

On 08/08/2015 04:27 PM, Robert Haas wrote:

I don't see that there's any good reason to allow the same password to
be stored in the catalog encrypted more than one way,

Sure there is. If you want to be able to authenticate using different
mechanism, you need the same password "encrypted" in different ways.
SCRAM uses verifier that's derived from the password in one way, MD5
authentication needs an MD5 hash, and yet other protocols have other
requirements.

That's correct. However, one of the goals of implementing SCRAM
authentication is to allow security-conscious users to get rid of those
reusable md5 hashes, no?

Obviously the backwards-compatibility issues are pretty major ... it'll
be years before all drivers support SCRAM ... but we also want to
provide a path forwards for secure installations in which no md5 hashes
are stored.

This says "backwards-compatible GUC" to me. Here's one idea on how to
handle this:

1. we drop the parameter password_encryption

2. we add the parameter password_storage, which takes a list:
- plain : plain text
- md5 : current md5 hashes
- scram : new scram hashed passwords
This defaults to 'md5, scram' if not specified.
This list might be extended in the future.

3. All password types in the list are generated. This means having
multiple columns in pg_shadow, or an array. An array would support the
addition of future password storage methods.

4. CREATE ROLE / ALTER ROLE syntax is changed to accept a parameter to
ENCRYPTED in order to support md5, scram, and future methods. If no
parameter is supplied, ENCRYPTED will default to 'md5, scram'.

5. we add the superuser-only function pg_apply_password_policy(). This
applies the policy expressed by password_storage, generating or erasing
passwords for each user.

6. We add a new connection error for "authentication __method__ not
supported for user"

7. Two versions from now, we change the defaults.

I thought about the idea of determining password storage based on what's
in pg_hba.conf, but that seems like way too much implied authorization
to me, and liable to be a big foot-gun.

--Josh Berkus

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

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

On Sun, Aug 9, 2015 at 6:51 AM, Josh Berkus <josh@agliodbs.com> wrote:

Obviously the backwards-compatibility issues are pretty major ... it'll
be years before all drivers support SCRAM ... but we also want to
provide a path forwards for secure installations in which no md5 hashes
are stored.

This says "backwards-compatible GUC" to me. Here's one idea on how to
handle this:

1. we drop the parameter password_encryption
2. we add the parameter password_storage, which takes a list:
- plain : plain text
- md5 : current md5 hashes
- scram : new scram hashed passwords
This defaults to 'md5, scram' if not specified.
This list might be extended in the future.

Perhaps using a different GUC than password_encryption is safer... I
am not that sure. Still that's how I switched password_encryption to
actually handle a list. Default is 'md5' with the first patch, and
'md5,scram' with the scram patch added and it sets the list of
password verifiers created when PASSWORD with ENCRYPTED/UNENCRYPTED is
used.

3. All password types in the list are generated. This means having
multiple columns in pg_shadow, or an array. An array would support the
addition of future password storage methods.

Yeah, the patch switches pg_shadow to an array like that, with as
elements method:value, so you get actually md5:md5blah,scram:stuff in
all the patches applied.

4. CREATE ROLE / ALTER ROLE syntax is changed to accept a parameter to
ENCRYPTED in order to support md5, scram, and future methods. If no
parameter is supplied, ENCRYPTED will default to 'md5, scram'.

Like password ENCRYPTED (md5,scram) or similar? If no method is
passed, I think that we should default to password_storage instead.
Also, I still think that something like PASSWORD VERIFIERS is needed,
users may want to set the verifier user for each method after
calculating it on client-side: we authorize that for md5 even now, and
that's not something this spec authorizes.

5. we add the superuser-only function pg_apply_password_policy(). This
applies the policy expressed by password_storage, generating or erasing
passwords for each user.

pg_upgrade could make use of that to control password aging with an
option to do the cleanup or not. Not sure what the default should be
though. pg_apply_password_policy(roleid) would be useful as well to do
it on a role base.

6. We add a new connection error for "authentication __method__ not
supported for user"

Hm? This would let any user trying to connect with a given method know
that if a method is used or not. What's wrong with failing as we do
now. In case of PASSWORD NULL for example, an attempt of connection
fails all the time with "incorrect password" or similar.

7. Two versions from now, we change the defaults.

Or three. We cannot expect users to change immediately, and it is
wiser to let dust set on the new feature in case critical bugs show up
after the first GA.

Something that sounds more like a detail in this thread by reading
other comments: I think that it is important to store password
verifiers in a different catalog than pg_authid for two reasons:
- that's more user-friendly, a sysadmin could directly join the new
catalog with pg_authid to get all the verifiers for a single user
method
- at password lookup when authorizing connection, there is no need to
fetch all the password verifiers and parse the array with all
verifiers.
- more scalable if we have many verifier methods in the future, though
we are not going to have hundreds of them. Though I am wondering about
per-method validtime and per-method authorization options.
Regards,
--
Michael

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

It'd be nice if the new auth mechanism supports multiple passwords in the
same format as well (not just one per format).

That way you could have two different passwords for a user that are active
at the same time. This would simplify rolling database credentials as it
wouldn't have to be done all at once. You could add the new credentials,
update your app servers one by one, then disable the old ones.

A lot of systems that use API keys let you see the last time a particular
set of keys was used. This helps answer the "Is this going to break
something if I disable it?" question. Having a last used at timestamp for
each auth mechanism (per user) would be useful.

I'm not sure how updates should work when connecting to a read-only slave
though. It would need some way of letting the master know that user X
connected using credentials Y.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

On 08/08/2015 03:21 PM, Michael Paquier wrote:

On Sun, Aug 9, 2015 at 6:51 AM, Josh Berkus <josh@agliodbs.com> wrote:

1. we drop the parameter password_encryption
2. we add the parameter password_storage, which takes a list:
- plain : plain text
- md5 : current md5 hashes
- scram : new scram hashed passwords
This defaults to 'md5, scram' if not specified.
This list might be extended in the future.

Perhaps using a different GUC than password_encryption is safer... I
am not that sure. Still that's how I switched password_encryption to
actually handle a list. Default is 'md5' with the first patch, and
'md5,scram' with the scram patch added and it sets the list of
password verifiers created when PASSWORD with ENCRYPTED/UNENCRYPTED is
used.

Well, generally I feel like if we're going to change the *type* of a GUC
parameter, we ought to change the *name*. It's far easier for users to
figure out that the contents of a parameter need to change if the name
is also changed.

In other words, I think "invalid parameter 'password_encryption'" is an
easier to understand error message than "invalid password_encryption
type 'on'". Besides which, password_encryption was always a misnomer.

Unless you're going to still accept "on, off" in some kind of wierd
backwards-compatibitlity mode? If so, how does that work?

Like password ENCRYPTED (md5,scram) or similar? If no method is
passed, I think that we should default to password_storage instead.

Make sense.

Also, I still think that something like PASSWORD VERIFIERS is needed,
users may want to set the verifier user for each method after
calculating it on client-side: we authorize that for md5 even now, and
that's not something this spec authorizes.

I don't follow this. Mind you, I'm not sure that I need to.

5. we add the superuser-only function pg_apply_password_policy(). This
applies the policy expressed by password_storage, generating or erasing
passwords for each user.

pg_upgrade could make use of that to control password aging with an
option to do the cleanup or not. Not sure what the default should be
though. pg_apply_password_policy(roleid) would be useful as well to do
it on a role base.

No objections to an optional roleid parameter, if you think people will
use it.

6. We add a new connection error for "authentication __method__ not
supported for user"

Hm? This would let any user trying to connect with a given method know
that if a method is used or not. What's wrong with failing as we do
now. In case of PASSWORD NULL for example, an attempt of connection
fails all the time with "incorrect password" or similar.

So, the DBA sets password_storage = 'scram', but doesn't take the md5
lines out of pg_hba.conf.

The app dev tries to connect using a driver which only supports md5.
What error should they get? A user/DBA who is getting "invalid
password" is going to spend a long time debugging it. Also, it would be
very useful to have a distinctive error in the log, so that DBAs could
see who is *trying* to connect with the wrong verifier.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

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

On Sat, Aug 8, 2015 at 1:23 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 08/08/2015 04:27 PM, Robert Haas wrote:

I don't see that there's any good reason to allow the same password to
be stored in the catalog encrypted more than one way,

Sure there is. If you want to be able to authenticate using different
mechanism, you need the same password "encrypted" in different ways. SCRAM
uses verifier that's derived from the password in one way, MD5
authentication needs an MD5 hash, and yet other protocols have other
requirements.

Why do we need to be able to authenticate using more than one
mechanism? If you have some clients that can't support SCRAM yet, you
might as well continue using MD5 across the board until that changes.
You're not going to get much real security out of using MD5 for some
authentication attempts and SCRAM for other ones, and the amount of
infrastructure we're proposing to introduce to support that is pretty
substantial.

and I don't think there's any good reason to introduce the PASSWORD
VERIFIER terminology. I think we should store (1) your password,
either encrypted or unencrypted; and (2) the method used to encrypt
it. And that's it.

Like Joe and Stephen, I actually find it highly confusing that we call the
MD5 hash an "encrypted password". The term "password verifier" is fairly
common in the specifications of authentication mechanisms. I think we should
adopt it.

OK, but it sure looked from Michael's syntax description like you
wrote PASSWORD VERIFIER (md5 'the_actual_password'). Or at least
that was my impression from reading it, maybe I got it wrong. If you
want to introduce ALTER USER ... PASSWORD VERIFIER as alternative
syntax for what we now call ALTER USER ... ENCRYPTED PASSWORD, that
works for me. But a plaintext password shouldn't be called a password
verifier under the terminology you are using here, IIUC.

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

On 08/09/2015 08:09 AM, Robert Haas wrote:

Why do we need to be able to authenticate using more than one
mechanism? If you have some clients that can't support SCRAM yet, you
might as well continue using MD5 across the board until that changes.
You're not going to get much real security out of using MD5 for some
authentication attempts and SCRAM for other ones,

Speaking as someone who has sheperded several clients through
infrastructure upgrades, I have to disagree with this.

First, people don't upgrade large infrastructures with multiple
applications, ETL processes and APIs which connect with the database all
at once. They do it one component at a time, verify that component is
working, and then move on to the next one. Even within a single
application, there could be many servers to upgrade, and you can't do
them all simultaneously.

Now, for shops where they've had the foresight to set up group roles
which own objects so that a new user with SCRAM can be assigned in the
group role, this is no problem. But for the other 98% of our large-app
users, setting up that kind of infrastructure would itself require a
weekend-long downtime, due to the locking required to reassign object
permissions and all of the app testing required.

Second, you're forgetting hosted PostgreSQL, where there may be only one
user available to each database owner. So assigning a new login role
for SCRAM isn't even an option.

Plus all of the above requires that some login roles have a SCRAM
verifier, and others have MD5, for some period. Even if we don't
support multiple verifiers for one login, that still means we need to
deal with "what verifier gets created for a new role" and the required
support functions and GUCs for that. Switching "across the board" on a
per-installation basis is a complete nonstarter for any running application.

Frankly, switching on a per-postmaster basis isn't even worth discussing
in my book, because some languages/platforms will take years longer than
others to support SCRAM.

Overall, it's to the PostgreSQL project's benefit to have users switch
to SCRAM once we have it available. For that reason, we should try to
make it easy for them to make the switch.

However ...

and the amount of
infrastructure we're proposing to introduce to support that is pretty
substantial.

... during my exchange with Michael, I was thinking about the bug
potential of taking the password field and multiplexing it in some way,
which is significant. There is a definite risk of "making this too
complicated" and we'll need to contrast that against ease-of-migration,
because complicated mechanisms tend to be less secure due to user error.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

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