Thread: [HACKERS] Some thoughts about SCRAM implementation

    Hi!
    There's some ongoing discussion about SCRAM (like this thread 
https://www.postgresql.org/message-id/243d8c11-6149-a4bb-0909-136992f74b23%40iki.fi) 
but I wanted to open a new thread that covers these topics and other, 
more general ones. Here are some thoughts based on my perspective on 
SCRAM, which I gained thanks to studying it as part of the effort to 
implement SCRAM in the JDBC driver. FYI, some very early code can be 
found here: https://github.com/ahachete/scram. Although there's still a 
lot to do, I'm doing great progress and I expect to have a working 
version within 2 weeks.
    So here's what I think:

- Work so far is great. Thanks Heikki, Michael and others for the effort 
involved. I love to have SCRAM support in Postgres!

- I think channel binding support should be added. SCRAM brings security 
improvements over md5 and other simpler digest algorithms. But where it 
really shines is together with channel binding. This is the only method 
to prevent MITM attacks. Implementing it should not very difficult. 
There are several possible channel binding mechanisms, but the mandatory 
and probably more typical one is 'tls-unique' which basically means 
getting the byte array from the TLSfinish() message and comparing it 
with the same data sent by the client. That's more or less all it takes 
to implement it. So I would go for it.

- One SCRAM parameter, i, the iteration count, is important, and in my 
opinion should be configurable. AFAIK it is currently hard coded at 
4096, which is the minimum value accepted by the RFC. But it should be 
at least 15K (RFC says), and given that it affects computing time of the 
authentication, it should be configurable. It's also now specified per 
user, which I think its too much granularity (it should suffice to say 
this group of users all require this iteration count).

- The SCRAM RFC states that server should advertise supported 
mechanisms. I see discussion going into not advertising them. I think it 
should be done, I don't see reasons not to do it, and it will become 
compliant with the RFC.

- I don't see why proposed scram mechanism names for pg_hba.conf are not 
following the IANA Registry standard ( 
https://www.iana.org/assignments/sasl-mechanisms/sasl-mechanisms.xhtml#scram), 
which is uppercase (like in SCRAM-SHA-256).

- SCRAM also supports the concept of authzid, which is kind of what 
pg_ident does: authenticate the user as the user sent, but login as the 
user specified here. It could also be supported.

    Based on these comments, I'd like to propose the following changes:

* Implement channel binding.

* Add a GUC for the channel binding technique to use. It could be named 
as "scram_channel_binding_name" or "sasl_channel_binding_name", for 
example. And probably supported value as of today, 'tls-unique'.

* AuthenticationSASL backend message:    - The String field make it a CSV that would be a list of one or 
more supported IANA SCRAM authentication methods. This info comes from 
pg_scram.conf (see below).    - Add another String to specify the variety of channel binding 
supported (if any), like 'tls-unique'. This info comes from the GUC.

* Treat the configuration of scram in pg_hba.conf, which seems 
complicated, similarly to how pg_ident.conf is treated. Basically in 
pg_hba.conf specify a "scram=scram_entry_name" value and then look for a 
pg_scram.conf file for an entry with that name. The pg_scram.conf might 
have the following structure:

name            |    scram_mechanisms    |    i
scramlight    |    SCRAM-SHA-1              |  4096
scramstrong |    SCRAM-SHA-256,SCRAM-SHA-256-PLUS    |    16384


* The nonce length is not specified by the RFC. I see typical 
implementations use 24 chars for the client and 18 for the server. 
Current code uses 10. I think it should not hurt making it at least 16 
or 18.

* It seems to me that the errors defined by the RFC, sent on the 
server-final-message if there were errors, are never sent with the 
current implementation. I think they should be sent as per the standard, 
and also proceed until the last stage even if errors were detected 
earlier (to conform with the RFC).

    Thoughts?
    Álvaro


-- 

Álvaro Hernández Tortosa


-----------
<8K>data

On 04/10/2017 12:39 PM, Álvaro Hernández Tortosa wrote:
> - I think channel binding support should be added. SCRAM brings security
> improvements over md5 and other simpler digest algorithms. But where it
> really shines is together with channel binding. This is the only method
> to prevent MITM attacks. Implementing it should not very difficult.
> There are several possible channel binding mechanisms, but the mandatory
> and probably more typical one is 'tls-unique' which basically means
> getting the byte array from the TLSfinish() message and comparing it
> with the same data sent by the client. That's more or less all it takes
> to implement it. So I would go for it.

We missed the boat for PostgreSQL 10. You're right that it probably 
wouldn't be difficult to implement, but until there's a concrete patch 
to discuss, that's a moot point.

> - One SCRAM parameter, i, the iteration count, is important, and in my
> opinion should be configurable. AFAIK it is currently hard coded at
> 4096, which is the minimum value accepted by the RFC. But it should be
> at least 15K (RFC says), and given that it affects computing time of the
> authentication, it should be configurable. It's also now specified per
> user, which I think its too much granularity (it should suffice to say
> this group of users all require this iteration count).

Yeah, a GUC might be a good idea.

The iteration count is a part of the stored SCRAM verifier, so it's 
determined when you set the user's password. That's why it's per-user.

> - The SCRAM RFC states that server should advertise supported
> mechanisms. I see discussion going into not advertising them. I think it
> should be done, I don't see reasons not to do it, and it will become
> compliant with the RFC.

The SASL spec [RFC4422] says that mechanism negotiation is protocol 
specific. The SCRAM spec [RFC5802] prescribes how negotiation of 
channel-binding is done, and the current implementation is compliant 
with that. (Which is very straightforward when neither the client nor 
the server supports channel binding).

The negotiation of the mechanism is being discussed one the "Letting the 
client choose the protocol to use during a SASL exchange" thread. I'm 
just writing a more concrete proposal based on your suggestion of 
sending a list of SASL mechanisms in the AuthenticationSASL message. 
Stay tuned!

> - I don't see why proposed scram mechanism names for pg_hba.conf are not
> following the IANA Registry standard (
> https://www.iana.org/assignments/sasl-mechanisms/sasl-mechanisms.xhtml#scram),
> which is uppercase (like in SCRAM-SHA-256).

All the existing authentication mechanisms are spelled in lowercase. And 
uppercase is ugly. It's a matter of taste, for sure.

> - SCRAM also supports the concept of authzid, which is kind of what
> pg_ident does: authenticate the user as the user sent, but login as the
> user specified here. It could also be supported.

Yeah, it might be handy for something like pgbouncer, which could then 
have one userid+password to authenticate, but then switch to another 
user. But the SCRAM part of that seems to be just a small part of the 
overall feature. In any case, this is clearly Postgres 11 material.

> * The nonce length is not specified by the RFC. I see typical
> implementations use 24 chars for the client and 18 for the server.
> Current code uses 10. I think it should not hurt making it at least 16
> or 18.

Wouldn't hurt, I guess. IIRC I checked some other implementations, when 
I picked 10, but I don't remember which ones anymore. Got a reference 
for 24/18?

> * It seems to me that the errors defined by the RFC, sent on the
> server-final-message if there were errors, are never sent with the
> current implementation. I think they should be sent as per the standard,
> and also proceed until the last stage even if errors were detected
> earlier (to conform with the RFC).

The server does send "e=invalid-proof", if the password is incorrect (or 
the user doesn't exist). All other errors are reported with 
ereport(ERROR). That is allowed by the spec:

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

It's a bit awkward to use the spec's "e=xxx" mechanism for other errors, 
because the "e=xxx" error code can only be sent in the 
server-final-message. If an error is detected before that stage, the 
server would need to continue the authentication to the end, until it 
could report it. That's why.

As Jeff Janes pointed out at [1], reporting even the invalid password 
case with "e=invalid-proof" actually leads to a different and less 
user-friendly message with psql. So we may still change that too.

[1] 
https://www.postgresql.org/message-id/CAMkU%3D1w3jQ53M1OeNfN8Cxd9O%2BA_9VONJivTbYoYRRdRsLT6vA%40mail.gmail.com

- Heikki

On 04/10/2017 12:39 PM, Álvaro Hernández Tortosa wrote:

- I think channel binding support should be added. SCRAM brings security
improvements over md5 and other simpler digest algorithms. But where it
really shines is together with channel binding. This is the only method
to prevent MITM attacks. Implementing it should not very difficult.
There are several possible channel binding mechanisms, but the mandatory
and probably more typical one is 'tls-unique' which basically means
getting the byte array from the TLSfinish() message and comparing it
with the same data sent by the client. That's more or less all it takes
to implement it. So I would go for it.

We missed the boat for PostgreSQL 10. You're right that it probably wouldn't be difficult to implement, but until there's a concrete patch to discuss, that's a moot point.

    Really? That's a real shame.... I know we're very late in the CF cycle but, again, this would be a real shame.

- One SCRAM parameter, i, the iteration count, is important, and in my
opinion should be configurable. AFAIK it is currently hard coded at
4096, which is the minimum value accepted by the RFC. But it should be
at least 15K (RFC says), and given that it affects computing time of the
authentication, it should be configurable. It's also now specified per
user, which I think its too much granularity (it should suffice to say
this group of users all require this iteration count).

Yeah, a GUC might be a good idea.

    Cool.

The iteration count is a part of the stored SCRAM verifier, so it's determined when you set the user's password. That's why it's per-user.

    Sure. But I think per-user is too much granularity. And if it is not, it should be a parameter exposed to the CREATE USER command, such that it can be (effectively) set per-user. I maintain the best approach could be the suggested pg_scram.conf with the format (or a similar one) that I proposed in the OP.

- The SCRAM RFC states that server should advertise supported
mechanisms. I see discussion going into not advertising them. I think it
should be done, I don't see reasons not to do it, and it will become
compliant with the RFC.

The SASL spec [RFC4422] says that mechanism negotiation is protocol specific. The SCRAM spec [RFC5802] prescribes how negotiation of channel-binding is done, and the current implementation is compliant with that. (Which is very straightforward when neither the client nor the server supports channel binding).

    Yeah. But what I'm saying is that we might want to add an extra String to AuthenticationSASL message for channel binding, so that the message format needs not to be changed when channel binding is added. But the channel binding method name needs to be negotiated, and so there needs to be room for it.

The negotiation of the mechanism is being discussed one the "Letting the client choose the protocol to use during a SASL exchange" thread. I'm just writing a more concrete proposal based on your suggestion of sending a list of SASL mechanisms in the AuthenticationSASL message. Stay tuned!

    Yepp, I will reply there, thanks :)

- I don't see why proposed scram mechanism names for pg_hba.conf are not
following the IANA Registry standard (
https://www.iana.org/assignments/sasl-mechanisms/sasl-mechanisms.xhtml#scram),
which is uppercase (like in SCRAM-SHA-256).

All the existing authentication mechanisms are spelled in lowercase. And uppercase is ugly. It's a matter of taste, for sure.

    And I agree with you. It's ugly. But it's standard. I'd say let's favor standardization vs our own taste.

- SCRAM also supports the concept of authzid, which is kind of what
pg_ident does: authenticate the user as the user sent, but login as the
user specified here. It could also be supported.

Yeah, it might be handy for something like pgbouncer, which could then have one userid+password to authenticate, but then switch to another user. But the SCRAM part of that seems to be just a small part of the overall feature. In any case, this is clearly Postgres 11 material.

    Again, it should be a tiny change, just an extra attribute sent over the message. But I guess time was over... just saying... ;)

* The nonce length is not specified by the RFC. I see typical
implementations use 24 chars for the client and 18 for the server.
Current code uses 10. I think it should not hurt making it at least 16
or 18.

Wouldn't hurt, I guess. IIRC I checked some other implementations, when I picked 10, but I don't remember which ones anymore. Got a reference for 24/18?

    First reference is the RFC example itself (non-mandatory, of course). But then I saw many followed this. As a quick example, GNU SASL defines:

#define SNONCE_ENTROPY_BYTES 18
https://www.gnu.org/software/gsasl/coverage/lib/scram/server.c.gcov.frameset.html

* It seems to me that the errors defined by the RFC, sent on the
server-final-message if there were errors, are never sent with the
current implementation. I think they should be sent as per the standard,
and also proceed until the last stage even if errors were detected
earlier (to conform with the RFC).

The server does send "e=invalid-proof", if the password is incorrect (or the user doesn't exist). All other errors are reported with ereport(ERROR). That is allowed by the spec:

   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.

    OK, agreed. Now... what happens if the client still sends a final message if the server returned error with the server-first-message? I don't really care, but at least we should think of how to react to that.

It's a bit awkward to use the spec's "e=xxx" mechanism for other errors, because the "e=xxx" error code can only be sent in the server-final-message. If an error is detected before that stage, the server would need to continue the authentication to the end, until it could report it. That's why.

    Makes sense.

As Jeff Janes pointed out at [1], reporting even the invalid password case with "e=invalid-proof" actually leads to a different and less user-friendly message with psql. So we may still change that too.

[1] https://www.postgresql.org/message-id/CAMkU%3D1w3jQ53M1OeNfN8Cxd9O%2BA_9VONJivTbYoYRRdRsLT6vA%40mail.gmail.com

- Heikki

    Thanks,

    Álvaro

-- 

Álvaro Hernández Tortosa


-----------
<8K>data

On 2017-04-10 20:28:27 +0200, Álvaro Hernández Tortosa wrote:
> 
> 
> On 10/04/17 13:02, Heikki Linnakangas wrote:
> > On 04/10/2017 12:39 PM, Álvaro Hernández Tortosa wrote:
> > > - I think channel binding support should be added. SCRAM brings security
> > > improvements over md5 and other simpler digest algorithms. But where it
> > > really shines is together with channel binding. This is the only method
> > > to prevent MITM attacks. Implementing it should not very difficult.
> > > There are several possible channel binding mechanisms, but the mandatory
> > > and probably more typical one is 'tls-unique' which basically means
> > > getting the byte array from the TLSfinish() message and comparing it
> > > with the same data sent by the client. That's more or less all it takes
> > > to implement it. So I would go for it.
> > 
> > We missed the boat for PostgreSQL 10. You're right that it probably
> > wouldn't be difficult to implement, but until there's a concrete patch
> > to discuss, that's a moot point.
> 
>     Really? That's a real shame.... I know we're very late in the CF cycle
> but, again, this would be a real shame.

That can equally be said about about a lot of features.  If we don't
stop at some point... Also, we're not late in the CF cycle, the CF cycle
for v10 is over.  It's not like the non-existance of channel binding
removes previously existing features, or makes SCRAM useless.


Greetings,

Andres Freund

On Mon, Apr 10, 2017 at 2:32 PM, Andres Freund <andres@anarazel.de> wrote:
> That can equally be said about about a lot of features.  If we don't
> stop at some point... Also, we're not late in the CF cycle, the CF cycle
> for v10 is over.  It's not like the non-existance of channel binding
> removes previously existing features, or makes SCRAM useless.

+1.

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

On 10/04/17 20:32, Andres Freund wrote:
> On 2017-04-10 20:28:27 +0200, Álvaro Hernández Tortosa wrote:
>>
>> On 10/04/17 13:02, Heikki Linnakangas wrote:
>>> On 04/10/2017 12:39 PM, Álvaro Hernández Tortosa wrote:
>>>> - I think channel binding support should be added. SCRAM brings security
>>>> improvements over md5 and other simpler digest algorithms. But where it
>>>> really shines is together with channel binding. This is the only method
>>>> to prevent MITM attacks. Implementing it should not very difficult.
>>>> There are several possible channel binding mechanisms, but the mandatory
>>>> and probably more typical one is 'tls-unique' which basically means
>>>> getting the byte array from the TLSfinish() message and comparing it
>>>> with the same data sent by the client. That's more or less all it takes
>>>> to implement it. So I would go for it.
>>> We missed the boat for PostgreSQL 10. You're right that it probably
>>> wouldn't be difficult to implement, but until there's a concrete patch
>>> to discuss, that's a moot point.
>>      Really? That's a real shame.... I know we're very late in the CF cycle
>> but, again, this would be a real shame.
> That can equally be said about about a lot of features.  If we don't
> stop at some point... Also, we're not late in the CF cycle, the CF cycle
> for v10 is over.  It's not like the non-existance of channel binding
> removes previously existing features, or makes SCRAM useless.
>
>
> Greetings,
>
> Andres Freund
    I know this is a lost battle. But please bear with me for a minute.
    Let's put ourselves on the foot of potential users. Why would 
anyone want to use SCRAM? What for? The hashing mechanism is better, no 
question. And bring some added benefits, true. So its "better". But the 
real gain comes from using channel binding, which avoids impersonation, 
MITM attacks. This is the deal breaker. SCRAM without channel binding is 
like Coke Zero without caffeine and mixed with water. Don't get me 
wrong, the work behind is great.
    But just a bit more is needed to make it really a big announcement 
and provide real value to (I guess, mostly but very interesting) 
enterprise customers, for which MITM and impersonating are big things. 
The good news is that adding channel binding is like inverse Paretto: a 
20% of extra effort (I bet significantly less) leads to 80% improvement.
    So CF v10 is over. So we're on testing phase. Can't we consider 
this a "missing feature bug"? ^_^

    Álvaro

-- 

Álvaro Hernández Tortosa


-----------
<8K>data

On Tue, Apr 11, 2017 at 9:53 PM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:
>     I know this is a lost battle. But please bear with me for a minute.

I admire your courage.

>     But just a bit more is needed to make it really a big announcement and
> provide real value to (I guess, mostly but very interesting) enterprise
> customers, for which MITM and impersonating are big things. The good news is
> that adding channel binding is like inverse Paretto: a 20% of extra effort
> (I bet significantly less) leads to 80% improvement.

We'll get that into PG11, don't worry. At least Heikki or I will submit a patch.

>     So CF v10 is over. So we're on testing phase. Can't we consider this a
> "missing feature bug"? ^_^

We should really focus on stability. There is still a bit more to do,
and for SCRAM we have added already a lot of infrastructure so this
should be improved first. And then we can work on extending it on a
sane basis.
--
Michael

On Tue, Apr 11, 2017 at 2:53 PM, Álvaro Hernández Tortosa <aht@8kdata.com> wrote:

On 10/04/17 20:32, Andres Freund wrote:

On 2017-04-10 20:28:27 +0200, Álvaro Hernández Tortosa wrote:

On 10/04/17 13:02, Heikki Linnakangas wrote:

On 04/10/2017 12:39 PM, Álvaro Hernández Tortosa wrote:

- I think channel binding support should be added. SCRAM brings security
improvements over md5 and other simpler digest algorithms. But where it
really shines is together with channel binding. This is the only method
to prevent MITM attacks. Implementing it should not very difficult.
There are several possible channel binding mechanisms, but the mandatory
and probably more typical one is 'tls-unique' which basically means
getting the byte array from the TLSfinish() message and comparing it
with the same data sent by the client. That's more or less all it takes
to implement it. So I would go for it.

We missed the boat for PostgreSQL 10. You're right that it probably
wouldn't be difficult to implement, but until there's a concrete patch
to discuss, that's a moot point.

     Really? That's a real shame.... I know we're very late in the CF cycle
but, again, this would be a real shame.

That can equally be said about about a lot of features.  If we don't
stop at some point... Also, we're not late in the CF cycle, the CF cycle
for v10 is over.  It's not like the non-existance of channel binding
removes previously existing features, or makes SCRAM useless.


Greetings,

Andres Freund

    I know this is a lost battle. But please bear with me for a minute.

    Let's put ourselves on the foot of potential users. Why would anyone want to use SCRAM? What for? The hashing mechanism is better, no question. And bring some added benefits, true. So its "better". But the real gain comes from using channel binding, which avoids impersonation, MITM attacks. This is the deal breaker. SCRAM without channel binding is like Coke Zero without caffeine and mixed with water. Don't get me wrong, the work behind is great.

I think you are seriously undervaluing the SCRAM without channel binding.

    I'm not. If I wouldn't appreciate its value, I wouldn't be writing a client library for the jdbc driver.

It improves a lot of things over our current md5 method beyond just using a stronger hashing algorithm.

Sure, channel binding is great. But that's not a dealbreaker, or even close to it.

    I think otherwise. It is close to a dealbreaker. And it's so few extra code lines that it requires....

 

    But just a bit more is needed to make it really a big announcement and provide real value to (I guess, mostly but very interesting) enterprise customers, for which MITM and impersonating are big things. The good news is that adding channel binding is like inverse Paretto: a 20% of extra effort (I bet significantly less) leads to 80% improvement.

I would expect most enterprise customers who care about MITM protection are already using either TLS or ipsec to cover that already. They have benefit from the other parts of SCRAM, but they've already solved those problems.

    Enterprises use checklists. And discard solutions if they don't have "checks" on all the items. One of those is, in my opinion, SCRAM with channel binding. I don't want this to happen to PG, specially when it's so easy to implement.


    But I will conserve my remaining courage (thanks Michael!) credits for future threads ;) I have stated my opinion clearly, I will now go back to the client library.


    Thanks,

    Álvaro

-- 

Álvaro Hernández Tortosa


-----------
<8K>data

On 04/11/2017 04:09 PM, Álvaro Hernández Tortosa wrote:
>      But I will conserve my remaining courage (thanks Michael!) credits
> for future threads ;) I have stated my opinion clearly, I will now go
> back to the client library.

Once you're done with the client library, feel free to post a patch to 
implement channel binding. If it really is trivial, we can discuss it. 
Most likely, it won't go into v10, but someone's got to write the patch 
for v11 anyway, and until there's a patch on the table to discuss, 
there's no point in arguing. Yes, I'm trying to cajole you into writing 
the patch for v11, so that I don't have to :-).

- Heikki

On 11/04/17 15:18, Heikki Linnakangas wrote:
> On 04/11/2017 04:09 PM, Álvaro Hernández Tortosa wrote:
>>      But I will conserve my remaining courage (thanks Michael!) credits
>> for future threads ;) I have stated my opinion clearly, I will now go
>> back to the client library.
>
> Once you're done with the client library, feel free to post a patch to 
> implement channel binding. If it really is trivial, we can discuss it. 
> Most likely, it won't go into v10, but someone's got to write the 
> patch for v11 anyway, and until there's a patch on the table to 
> discuss, there's no point in arguing. Yes, I'm trying to cajole you 
> into writing the patch for v11, so that I don't have to :-).
    LOL. Do you really want a half-baked Java programmer writing a 
patch in C for PostgreSQL? I once tried that and Magnus said my code was 
Java code that happened to compile with a C compiler.... ^_^
    Having said that, I take the bait, I like challenges and putting my 
words behind my code :)

    Álvaro

-- 

Álvaro Hernández Tortosa


-----------
<8K>data

On Tue, Apr 11, 2017 at 02:53:24PM +0200, Álvaro Hernández Tortosa wrote:
>     Let's put ourselves on the foot of potential users. Why would anyone
> want to use SCRAM? What for? The hashing mechanism is better, no question.
> And bring some added benefits, true. So its "better". But the real gain
> comes from using channel binding, which avoids impersonation, MITM attacks.
> This is the deal breaker. SCRAM without channel binding is like Coke Zero
> without caffeine and mixed with water. Don't get me wrong, the work behind
> is great.
> 
>     But just a bit more is needed to make it really a big announcement and
> provide real value to (I guess, mostly but very interesting) enterprise
> customers, for which MITM and impersonating are big things. The good news is
> that adding channel binding is like inverse Paretto: a 20% of extra effort
> (I bet significantly less) leads to 80% improvement.

I don't see why channel binding is a big deal for enterprises because I
assume they are already using SSL:
https://en.wikipedia.org/wiki/Salted_Challenge_Response_Authentication_Mechanism#Channel_binding

I think the big win for SCRAM is the inability to replay md5 packets
after recording 16k sessions (salt was only 32-bit, so a 50% chance of
replay after 16 sessions), and storage of SHA256 hashes instead of MD5
in pg_authid, though the value of that is mostly a check-box item
because collisions are not a problem for the way we use MD5.

--  Bruce Momjian  <bruce@momjian.us>        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 +

Bruce,

* Bruce Momjian (bruce@momjian.us) wrote:
> On Tue, Apr 11, 2017 at 02:53:24PM +0200, Álvaro Hernández Tortosa wrote:
> >     Let's put ourselves on the foot of potential users. Why would anyone
> > want to use SCRAM? What for? The hashing mechanism is better, no question.
> > And bring some added benefits, true. So its "better". But the real gain
> > comes from using channel binding, which avoids impersonation, MITM attacks.
> > This is the deal breaker. SCRAM without channel binding is like Coke Zero
> > without caffeine and mixed with water. Don't get me wrong, the work behind
> > is great.
> >
> >     But just a bit more is needed to make it really a big announcement and
> > provide real value to (I guess, mostly but very interesting) enterprise
> > customers, for which MITM and impersonating are big things. The good news is
> > that adding channel binding is like inverse Paretto: a 20% of extra effort
> > (I bet significantly less) leads to 80% improvement.
>
> I don't see why channel binding is a big deal for enterprises because I
> assume they are already using SSL:

Channel binding should be used with SSL to ensure that there is no
man-in-the-middle attack being performed.  It's necessary when the
end-points aren't performing full, mutual, certificate-based
verification.

> I think the big win for SCRAM is the inability to replay md5 packets
> after recording 16k sessions (salt was only 32-bit, so a 50% chance of
> replay after 16 sessions), and storage of SHA256 hashes instead of MD5
> in pg_authid, though the value of that is mostly a check-box item
> because collisions are not a problem for the way we use MD5.

There are a lot of wins to having SCRAM implemented.  I disagree
strongly that securing PG from attacks based on latent information
gathering (backups which include pg_authid) is just a "check-box" item.

Thanks!

Stephen

On Tue, Apr 11, 2017 at 08:58:30PM -0400, Stephen Frost wrote:
> > >     But just a bit more is needed to make it really a big announcement and
> > > provide real value to (I guess, mostly but very interesting) enterprise
> > > customers, for which MITM and impersonating are big things. The good news is
> > > that adding channel binding is like inverse Paretto: a 20% of extra effort
> > > (I bet significantly less) leads to 80% improvement.
> > 
> > I don't see why channel binding is a big deal for enterprises because I
> > assume they are already using SSL:
> 
> Channel binding should be used with SSL to ensure that there is no
> man-in-the-middle attack being performed.  It's necessary when the
> end-points aren't performing full, mutual, certificate-based
> verification.

And which enterprises are using SSL without certificates?  And I thought
channel binding required certificates anyway, e.g.:
https://en.wikipedia.org/wiki/Salted_Challenge_Response_Authentication_Mechanism#Channel_binding
For instance, for the tls-server-end-point channel binding, it is theserver's TLS certificate.

> > I think the big win for SCRAM is the inability to replay md5 packets
> > after recording 16k sessions (salt was only 32-bit, so a 50% chance of
> > replay after 16 sessions), and storage of SHA256 hashes instead of MD5
> > in pg_authid, though the value of that is mostly a check-box item
> > because collisions are not a problem for the way we use MD5.
> 
> There are a lot of wins to having SCRAM implemented.  I disagree
> strongly that securing PG from attacks based on latent information
> gathering (backups which include pg_authid) is just a "check-box" item.

Well, they have the entire backup so I don't think cracking the password
is a huge win, though the password does potentially give them access to
future data.  And it prevents them from reusing the password on another
server, but _again_, I still think the big win is to prevent replay
after 16k packets are sniffed.

--  Bruce Momjian  <bruce@momjian.us>        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 +

On 04/12/2017 11:22 AM, Magnus Hagander wrote:
> On Wed, Apr 12, 2017 at 3:25 AM, Bruce Momjian <bruce@momjian.us> wrote:
>
>> And which enterprises are using SSL without certificates?  And I thought
>> channel binding required certificates anyway, e.g.:
>>
>>         https://en.wikipedia.org/wiki/Salted_Challenge_Response_
>> Authentication_Mechanism#Channel_binding
>>
>>         For instance, for the tls-server-end-point channel binding, it is
>> the
>>         server's TLS certificate.
>
> AFAIK it does require the TLS certifificates, but it does not require TLS
> certificate *validation*. You can use channel binding with just self-signed
> certs.

tls-server-end-point channel binding type relies on certificates. But 
SCRAM uses "tls-unique" by default, and it does not use certificates. 
It's a bit weird that the wikipedia article uses tls-server-end-point as 
the example, I don't know why anyone would use tls-server-end-point with 
SCRAM.

> That said, I stand by my comment that I don't think it's the enterprises
> that need or want the channel binding. If they care about it, they have
> already put certificate validation in place, and it won't buy them anything.
>
> Because channel binding also only secures the authentication (SCRAM), not
> the actual contents and commands that are then sent across the channel,
> AFAIK?

TLS protects the contents and the commands. The point of channel binding 
is to defeat a MITM attack, where the client connects to a malicious 
server, using TLS, which then connects to the real server, using another 
TLS connection. Channel binding will detect that the client and the real 
server are not communicating over the same TLS connection, but two 
different TLS connections, and make the authentication fail.

SSL certificates, with validation, achieves the same, but channel 
binding achieves it without the hassle of certificates.

- Heikki

On Wed, Apr 12, 2017 at 12:13:03PM +0300, Heikki Linnakangas wrote:
> >That said, I stand by my comment that I don't think it's the enterprises
> >that need or want the channel binding. If they care about it, they have
> >already put certificate validation in place, and it won't buy them anything.
> >
> >Because channel binding also only secures the authentication (SCRAM), not
> >the actual contents and commands that are then sent across the channel,
> >AFAIK?
> 
> TLS protects the contents and the commands. The point of channel binding is
> to defeat a MITM attack, where the client connects to a malicious server,
> using TLS, which then connects to the real server, using another TLS
> connection. Channel binding will detect that the client and the real server
> are not communicating over the same TLS connection, but two different TLS
> connections, and make the authentication fail.
> 
> SSL certificates, with validation, achieves the same, but channel binding
> achieves it without the hassle of certificates.

How does it do that?

--  Bruce Momjian  <bruce@momjian.us>        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 +

On 04/12/2017 06:26 PM, Bruce Momjian wrote:
> On Wed, Apr 12, 2017 at 12:13:03PM +0300, Heikki Linnakangas wrote:
>>> That said, I stand by my comment that I don't think it's the enterprises
>>> that need or want the channel binding. If they care about it, they have
>>> already put certificate validation in place, and it won't buy them anything.
>>>
>>> Because channel binding also only secures the authentication (SCRAM), not
>>> the actual contents and commands that are then sent across the channel,
>>> AFAIK?
>>
>> TLS protects the contents and the commands. The point of channel binding is
>> to defeat a MITM attack, where the client connects to a malicious server,
>> using TLS, which then connects to the real server, using another TLS
>> connection. Channel binding will detect that the client and the real server
>> are not communicating over the same TLS connection, but two different TLS
>> connections, and make the authentication fail.
>>
>> SSL certificates, with validation, achieves the same, but channel binding
>> achieves it without the hassle of certificates.
>
> How does it do that?

Good question, crypto magic? I don't know the details, but the basic 
idea is that you extract a blob of data that uniquely identifies the TLS 
connection. Using some OpenSSL functions, in this case. I think it's a 
hash of some of the TLS handshake messages that were used when the TLS 
connection was established (that's what "tls-unique" means). That data 
is then incorporated in the hash calculations of the SCRAM 
authentication. If the client and the server are not speaking over the 
same TLS connection, they will use different values for the TLS data, 
and the SCRAM computations will not match, and you get an authentication 
failure.

- Heikki

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 04/12/2017 06:26 PM, Bruce Momjian wrote:
>> How does it do that?

> Good question, crypto magic? I don't know the details, but the basic 
> idea is that you extract a blob of data that uniquely identifies the TLS 
> connection. Using some OpenSSL functions, in this case. I think it's a 
> hash of some of the TLS handshake messages that were used when the TLS 
> connection was established (that's what "tls-unique" means). That data 
> is then incorporated in the hash calculations of the SCRAM 
> authentication. If the client and the server are not speaking over the 
> same TLS connection, they will use different values for the TLS data, 
> and the SCRAM computations will not match, and you get an authentication 
> failure.

... which the user can't tell apart from having fat-fingered the password,
I suppose?  Doesn't sound terribly friendly.  A report of a certificate
mismatch is far more likely to lead people to realize there's a MITM.

So this seems more like a hack than like a feature we need so desperately
as to push it into v10 post-freeze.
        regards, tom lane

On Tue, Apr 11, 2017 at 9:20 AM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:
>     LOL. Do you really want a half-baked Java programmer writing a patch in
> C for PostgreSQL? I once tried that and Magnus said my code was Java code
> that happened to compile with a C compiler.... ^_^
>
>     Having said that, I take the bait, I like challenges and putting my
> words behind my code :)

Excellent, because that's how stuff gets done around here.  Saying
that you want something and hoping other people will do it is fine,
but being will to put some effort into it is a lot more likely to get
it done.  Not to be harsh, but showing up 3 days after feature freeze
to complain that a feature pending commit for 14 months is missing
something that you really need isn't really the right way to make
something happen.  I'm pretty sure that the lack of channel binding
was discussed quite a bit earlier than now, so I think there was
adequate opportunity to protest, contribute a patch, etc.  It's not
that I don't have sympathy for the way you feel about this: seeing
features you care about fall out of a release sucks, and I've
experienced a lot of that suckage quite recently, so the pain is fresh
in my mind.  But it's something we all have to live with for the
overall good of the product.  We used to not have a firm feature
freeze, and that was way worse.

In this case, I think it is abundantly clear that SCRAM without
channel binding is still a good feature.  One piece of evidence for
that is that the standard uses the suffix -PLUS to denote the
availability of channel binding.  That clearly conveys that channel
binding has value, but also that not having it does not make the whole
thing useless.  Otherwise, they would have required it to be part of
every implementation, or they would have made you add -CRAPPY if you
didn't have it.  The discussion elsewhere on this thread has
adequately underlined the value of what we've already got, so I won't
further belabor the point here.

Furthermore, I think that the state of this feature as it currently
exists in the tree is actually kind of concerning.  There are
currently four open items pertaining to SCRAM at least two of which
look to my mind an awful lot like stuff that should have ideally been
handled pre-feature-freeze: \password support, and protocol
negotiation.  I'm grateful for the hard work that has gone into this
feature, but these are pretty significant loose ends.  \password
support is a basic usability issue.  Protocol negotiation affects
anyone who may want to make their PG driver work with this feature,
and certainly can't be changed after final release, and ideally not
even after beta.  We really, really need to get that stuff nailed down
ASAP or we're going to have big problems.  So I think we should focus
on those things, not this.

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

Tom, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> ... which the user can't tell apart from having fat-fingered the password,
> I suppose?  Doesn't sound terribly friendly.  A report of a certificate
> mismatch is far more likely to lead people to realize there's a MITM.

We might be able to improve on that.

> So this seems more like a hack than like a feature we need so desperately
> as to push it into v10 post-freeze.

Channel binding certainly isn't a 'hack' and is something we should
support, but I agree that it doesn't need to go into v10.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> ... which the user can't tell apart from having fat-fingered the password,
>> I suppose?  Doesn't sound terribly friendly.  A report of a certificate
>> mismatch is far more likely to lead people to realize there's a MITM.

> We might be able to improve on that.

I'd sure be happier about this being a useful feature if we can.  But
unless someone has a design for that ready-to-go, that's another good
reason to put it off.
        regards, tom lane

On 12/04/17 18:09, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>> On 04/12/2017 06:26 PM, Bruce Momjian wrote:
>>> How does it do that?
>> Good question, crypto magic? I don't know the details, but the basic
>> idea is that you extract a blob of data that uniquely identifies the TLS
>> connection. Using some OpenSSL functions, in this case. I think it's a
>> hash of some of the TLS handshake messages that were used when the TLS
>> connection was established (that's what "tls-unique" means). That data
>> is then incorporated in the hash calculations of the SCRAM
>> authentication. If the client and the server are not speaking over the
>> same TLS connection, they will use different values for the TLS data,
>> and the SCRAM computations will not match, and you get an authentication
>> failure.
    I believe the above is not correct. Channel binding data is *not* 
used for any hash computations. It is simply a byte array that is 
received as an extra user parameter, and the server then gets it by its 
own way (its own TLS data) and do a byte comparison. That's what the 
RFCs say about it.
> ... which the user can't tell apart from having fat-fingered the password,
> I suppose?  Doesn't sound terribly friendly.  A report of a certificate
> mismatch is far more likely to lead people to realize there's a MITM.
    So given what I said before, that won't happen. Indeed, SCRAM RFC 
contains a specific error code for this: "channel-bindings-dont-match".

    Álvaro


-- 

Álvaro Hernández Tortosa


-----------
<8K>data

On 12/04/17 18:38, Robert Haas wrote:
> On Tue, Apr 11, 2017 at 9:20 AM, Álvaro Hernández Tortosa
> <aht@8kdata.com> wrote:
>>      LOL. Do you really want a half-baked Java programmer writing a patch in
>> C for PostgreSQL? I once tried that and Magnus said my code was Java code
>> that happened to compile with a C compiler.... ^_^
>>
>>      Having said that, I take the bait, I like challenges and putting my
>> words behind my code :)
> Excellent, because that's how stuff gets done around here.  Saying
> that you want something and hoping other people will do it is fine,
> but being will to put some effort into it is a lot more likely to get
> it done.  Not to be harsh, but showing up 3 days after feature freeze
> to complain that a feature pending commit for 14 months is missing
> something that you really need isn't really the right way to make
> something happen.  I'm pretty sure that the lack of channel binding
> was discussed quite a bit earlier than now, so I think there was
> adequate opportunity to protest, contribute a patch, etc.  It's not
> that I don't have sympathy for the way you feel about this: seeing
> features you care about fall out of a release sucks, and I've
> experienced a lot of that suckage quite recently, so the pain is fresh
> in my mind.  But it's something we all have to live with for the
> overall good of the product.  We used to not have a firm feature
> freeze, and that was way worse.
    Hi Robert. Not harsh, no worries, but please note a couple of 
things, let me give you a bit of context about my recent comments:

- I haven't complained about not having channel binding support. I was 
just giving my own recommendations for the PostgreSQL community, in the 
belief that contributing this opinion could let -hackes to make a more 
informed decision about whether to include or not a given feature.

- I haven't said either that I need it. I don't use SCRAM, much less 
with channel binding. Rather than looking after myself here, I'm trying 
to sit on the foot of potential users and speak up for them. I might be 
wrong, of course, but this is the only role I'm trying to play here.

- I know how PostgreSQL release cycles work and not meaning to hack them 
anyway. I just thought raising the fact that something that I believe 
might be requested by enterprise users was on the other hand a minor 
addition to a feature, and thus a really high value-to-effort ratio. 
Indeed, I bet adding channel binding is an order of magnitude easier 
than adding saslprep (which is optional too, btw). Ultimately I'm happy 
with SCRAM in PG 10, whether it has cbinding or not, as it is a great 
addition and makes PG10 an even better software.

- Even though I don't really care about SCRAM, and without having any 
prior knowledge about SCRAM, I volunteered some time ago to study SCRAM, 
give a lightning talk about SCRAM and later write a client 
implementation for the jdbc driver. And I have already devoted a very 
fair amount of time in doing so, and will keep doing that until all code 
is done. Code WIP is here FYI: https://github.com/ahachete/scram. So 
it's not that I haven't already put my code behind my words.

- Given all that, I still want to volunteer to not only do the client 
jdbc part and consequently help debugging the server implementation (as 
I believe it is so far the only non-libpq implementation), but also try 
to also stand by my words by writing channel binding code for PostgreSQL 
server. This is a huge effort for me, I only did C programming on the 
year 2001. But still, I want to help from my limited capabilities as 
much as I can.

- Having thoroughly studied the RFC and companion documentation, I 
thought I was in a knowledge position as to give some recommendations 
that other hackers may not know about (unless a deep study of SCRAM 
would have been done). That's it, recommendation, ideas.


>
> In this case, I think it is abundantly clear that SCRAM without
> channel binding is still a good feature.
    I agree. I may have exaggerated before, downplaying SCRAM without 
channel binding. I think it is great. Period. I also still think channel 
binding is very small code addition yet provides even better value. I 
apologize for not picking my previous words more carefully.

>   One piece of evidence for
> that is that the standard uses the suffix -PLUS to denote the
> availability of channel binding.  That clearly conveys that channel
> binding has value, but also that not having it does not make the whole
> thing useless.  Otherwise, they would have required it to be part of
> every implementation, or they would have made you add -CRAPPY if you
> didn't have it.  The discussion elsewhere on this thread has
> adequately underlined the value of what we've already got, so I won't
> further belabor the point here.
>
> Furthermore, I think that the state of this feature as it currently
> exists in the tree is actually kind of concerning.  There are
> currently four open items pertaining to SCRAM at least two of which
> look to my mind an awful lot like stuff that should have ideally been
> handled pre-feature-freeze: \password support, and protocol
> negotiation.  I'm grateful for the hard work that has gone into this
> feature, but these are pretty significant loose ends.  \password
> support is a basic usability issue.  Protocol negotiation affects
> anyone who may want to make their PG driver work with this feature,
> and certainly can't be changed after final release, and ideally not
> even after beta.  We really, really need to get that stuff nailed down
> ASAP or we're going to have big problems.  So I think we should focus
> on those things, not this.
    Absolutely agreed. That's why I have also tried to give all my 
comments (from the perspective of the SCRAM jdbc driver implementator) 
as to the protocol asap, and will continue to do so.

    Thanks,

    Álvaro

-- 

Álvaro Hernández Tortosa


-----------
<8K>data

On 04/12/2017 08:38 PM, Álvaro Hernández Tortosa wrote:
> - Even though I don't really care about SCRAM, and without having any
> prior knowledge about SCRAM, I volunteered some time ago to study SCRAM,
> give a lightning talk about SCRAM and later write a client
> implementation for the jdbc driver. And I have already devoted a very
> fair amount of time in doing so, and will keep doing that until all code
> is done. Code WIP is here FYI: https://github.com/ahachete/scram. So
> it's not that I haven't already put my code behind my words.

That is very much appreciated! You writing a second implementation of 
the client-side support (libpq being the first) is very, very helpful, 
to validate that the protocol is sane, unambiguous, and adequately 
documented.

> On 12/04/17 18:38, Robert Haas wrote:
>> Furthermore, I think that the state of this feature as it currently
>> exists in the tree is actually kind of concerning.  There are
>> currently four open items pertaining to SCRAM at least two of which
>> look to my mind an awful lot like stuff that should have ideally been
>> handled pre-feature-freeze: \password support, and protocol
>> negotiation.  I'm grateful for the hard work that has gone into this
>> feature, but these are pretty significant loose ends.  \password
>> support is a basic usability issue.  Protocol negotiation affects
>> anyone who may want to make their PG driver work with this feature,
>> and certainly can't be changed after final release, and ideally not
>> even after beta.  We really, really need to get that stuff nailed down
>> ASAP or we're going to have big problems.  So I think we should focus
>> on those things, not this.

Yes, we need to nail down the protocol and \password before beta. I am 
working on them now.

- Heikki

On Wed, Apr 12, 2017 at 2:09 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> That is very much appreciated! You writing a second implementation of the
> client-side support (libpq being the first) is very, very helpful, to
> validate that the protocol is sane, unambiguous, and adequately documented.

+1.

> Yes, we need to nail down the protocol and \password before beta. I am
> working on them now.

Good to hear.

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

On Thu, Apr 13, 2017 at 3:21 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Apr 12, 2017 at 2:09 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Yes, we need to nail down the protocol and \password before beta. I am
>> working on them now.
>
> Good to hear.

FWIW, there are patches for each issue.
-- 
Michael

On 4/11/17 09:03, Magnus Hagander wrote:
> I would expect most enterprise customers who care about MITM protection
> are already using either TLS or ipsec to cover that already. They have
> benefit from the other parts of SCRAM, but they've already solved those
> problems.

Yeah, I think if you're concerned about MITM then you would also be
concerned about MITM siphoning off your data.  So you should be using
TLS and then you don't need channel binding.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 14 April 2017 at 20:20, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
>
> Yeah, I think if you're concerned about MITM then you would also be
> concerned about MITM siphoning off your data.  So you should be using
> TLS and then you don't need channel binding.

No. You can use TLS for authentication (by verifying SSL certs in both
directions) in which case TLS will protect against MITM for you. But
if you only use TLS for encryption but still want to use passwords for
authentication then there's no protection against MITM as you don't
know that the party doing the encryption is the same as the one you
authenticated to.

Channel binding is all about tying the authentication mechanism to the
encryption to guarantee that the party doing the encryption is the
same as the party you authenticated to. Otherwise someone could MITM
the TLS connection and relay the raw bytes of of the scram
negotiation. Under our md5 auth that gives them your password, under
scram they won't get the password which is a huge improvement but they
would still have the raw unencrypted data from your traffic.

-- 
greg

On 04/10/2017 09:28 PM, Álvaro Hernández Tortosa wrote:
> On 10/04/17 13:02, Heikki Linnakangas wrote:
>> On 04/10/2017 12:39 PM, Álvaro Hernández Tortosa wrote:
>>> * The nonce length is not specified by the RFC. I see typical
>>> implementations use 24 chars for the client and 18 for the server.
>>> Current code uses 10. I think it should not hurt making it at least 16
>>> or 18.
>>
>> Wouldn't hurt, I guess. IIRC I checked some other implementations,
>> when I picked 10, but I don't remember which ones anymore. Got a
>> reference for 24/18?
>
>      First reference is the RFC example itself (non-mandatory, of
> course). But then I saw many followed this. As a quick example, GNU SASL
> defines:
>
> #define SNONCE_ENTROPY_BYTES 18
> https://www.gnu.org/software/gsasl/coverage/lib/scram/server.c.gcov.frameset.html

Ok, I bumped up the nonce lengths to 18 raw bytes. Thanks!

- Heikki