Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Tue, Sep 26, 2017 at 11:09 AM, Michael Paquier
 wrote:
> On Mon, Sep 25, 2017 at 11:22 PM, Peter Eisentraut
>  wrote:
>> I think the channel-binding negotiation on the client side is wrong.
>> The logic in the patch is
>>
>> +#ifdef USE_SSL
>> +   if (state->ssl_in_use)
>> +   appendPQExpBuffer(, "p=%s", SCRAM_CHANNEL_TLS_UNIQUE);
>> +   else
>> +   appendPQExpBuffer(, "y");
>> +#else
>> +   appendPQExpBuffer(, "n");
>> +#endif
>>
>> But if SSL is compiled in but not used, the client does not in fact
>> support channel binding (for that connection), so it should send "n".
>
> For others, details about this flag are here:
>gs2-cbind-flag  = ("p=" cb-name) / "n" / "y"
>  ;; "n" -> client doesn't support channel binding.
>  ;; "y" -> client does support channel binding
>  ;;but thinks the server does not.
>  ;; "p" -> client requires channel binding.
>  ;; The selected channel binding follows "p=".
>
> And channel binding description is here:
> https://tools.ietf.org/html/rfc5802#section-6

Changed the patch to do that. Note that if the client enforces the
SASL mechanism to SCRAM-SHA-256-PLUS in a non-SSL context then libpq
complains. This can be done in libpq using pgsaslname.

>> The "y" flag should be sent if ssl_in_use but the client did not see the
>> server advertise SCRAM-SHA256-PLUS.  That logic is missing entirely in
>> this patch.
>
> Okay. I think I get your point here. I agree that the client is
> deficient here. This needs some more work.

Hm. I take back the argument that we can use the backend version here.
conn->sversion is only filled at the end of authentication. So the
best thing I think can be done here is to check if channel binding has
been advertised or not, and send "y" if the client thinks that the
server should do support it. If the client has chosen not to use
channel binding, by for example enforcing pgsaslname in libpq to
SCRAM-SHA-256, then "n" should be sent. I have changed the patch
accordingly, doing at the same time some refactoring in pg_SASL_init.
pg_fe_scram_init() gets initialized only when the mechanism is
selected.

>> You have the server reject a client that does not support channel
>> binding ("n") on all SSL connections.  I don't think this is correct.
>> It is up to the client to use channel binding or not, even on SSL
>> connections.
>
> It seems that I got confused with the meaning of "y" mixed with
> ssl_in_use. The server should reject "y" instead only if SSL is in
> use.

Okay. In order to address that, what just needs to be done is to
remove the error message that I have added in the backend when "n" is
sent by the client. So this thing is ripped off. This way, when a v10
libpq connects to a v11 backend, the connection is able to work even
with SSL connections.

>> We should update pg_hba.conf to allow a method specification of
>> "scram-sha256-plus", i.e., only advertise the channel binding variant to
>> the client.  Then you could make policy decisions like rejecting clients
>> that do not use channel binding on SSL connections.  This could be a
>> separate patch later.
>
> OK, I agree that there could be some value in that. This complicates a
> bit hba rule checks, but nothing really complicated either.

This would be useful, but I have let it for now to not complicate the
series more.

>> The error message in the "p" case if SSL is not in use is a bit
>> confusing: "but the server does not need it".  I think this could be
>> left at the old message "but it is not supported".  This ties into my
>> interpretation from above that whether channel binding is "supported"
>> depends on whether SSL is in use for a particular connection.
>
> Check.

Okay, I switched the error message to that (diff with previous patch series):
@@ -850,7 +850,7 @@ read_client_first_message(scram_state *state, char *input)
if (!state->ssl_in_use)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
-errmsg("client supports SCRAM channel
binding, but server does not need it for non-SSL connections")));
+errmsg("client requires SCRAM channel
binding, but it is not supported")));

>> Some small code things:
>> - prefer to use size_t over int for length (tls_finish_len etc.)
>> - tls_finish should be tls_finished
>> - typos: certificate_bash -> certificate_hash
>
> Yes, thanks for spotting those.

Fixed.

>> In the patch for tls-server-end-point, I think the selection of the hash
>> function deviates slightly from the RFC.  The RFC only says to
>> substitute MD5 and SHA-1.  It doesn't say to turn SHA-224 into SHA-256,
>> for example.  There is also the problem that the code as written will
>> turn any unrecognized hash method into SHA-256.  If think the code
>> should single out MD5 and 

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Tue, Oct 3, 2017 at 1:30 AM, Robert Haas  wrote:
> On Fri, Sep 15, 2017 at 6:29 PM, Michael Paquier
>  wrote:
>> I would like to point out that per the RFC, if the client attempts a
>> SSL connection with SCRAM and that the server supports channel
>> binding, then it has to publish the SASL mechanism for channel
>> binding, aka SCRAM-PLUS. If the client tries to force the use of SCRAM
>> even if SCRAM-PLUS is specified, this is seen as a downgrade attack by
>> the server which must reject the connection. So this parameter has
>> meaning only if you try to connect to a PG10 server using a PG11
>> client (assuming that channel binding gets into PG11). If you connect
>> with a PG11 client to a PG11 server with SSL, the server publishes
>> SCRAM-PLUS, the client has to use it, hence this turns out to make
>> cbind=disable and prefer meaningless in the long-term. If the client
>> does not use SSL, then there is no channel binding, and cbind=require
>> loses its value. So cbind's fate is actually linked to sslmode.
>
> That seems problematic.  What if the client supports SCRAM but not
> channel binding?

Peter has outlined here that my interpretation of the RFC was wrong on
the client side to begin with:
https://www.postgresql.org/message-id/f74525e4-6c53-c653-6860-a8cc8d7c8...@2ndquadrant.com
If a client does not support channel binding (it is not compiled with
SSL or the connection is done without SSL), it should not send 'y' but
'n'. It should be up to the client to decide if it wants to use
channel binding or not. libpq is also going to need some extra logic
to send 'y' when it thinks that the server should have channel binding
support. This can be done by looking at the backend version.
-- 
Michael


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Robert Haas]

On Fri, Sep 15, 2017 at 6:29 PM, Michael Paquier
 wrote:
> I would like to point out that per the RFC, if the client attempts a
> SSL connection with SCRAM and that the server supports channel
> binding, then it has to publish the SASL mechanism for channel
> binding, aka SCRAM-PLUS. If the client tries to force the use of SCRAM
> even if SCRAM-PLUS is specified, this is seen as a downgrade attack by
> the server which must reject the connection. So this parameter has
> meaning only if you try to connect to a PG10 server using a PG11
> client (assuming that channel binding gets into PG11). If you connect
> with a PG11 client to a PG11 server with SSL, the server publishes
> SCRAM-PLUS, the client has to use it, hence this turns out to make
> cbind=disable and prefer meaningless in the long-term. If the client
> does not use SSL, then there is no channel binding, and cbind=require
> loses its value. So cbind's fate is actually linked to sslmode.

That seems problematic.  What if the client supports SCRAM but not
channel binding?

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


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Mon, Sep 25, 2017 at 11:22 PM, Peter Eisentraut
 wrote:
> Here is a review of the meat of the code, leaving aside the discussion
> of the libpq connection parameters.
>
> Overall, the structure of the code makes sense and it fits in well with
> the existing SCRAM code.

Thanks for the review, Peter.

> I think the channel-binding negotiation on the client side is wrong.
> The logic in the patch is
>
> +#ifdef USE_SSL
> +   if (state->ssl_in_use)
> +   appendPQExpBuffer(, "p=%s", SCRAM_CHANNEL_TLS_UNIQUE);
> +   else
> +   appendPQExpBuffer(, "y");
> +#else
> +   appendPQExpBuffer(, "n");
> +#endif
>
> But if SSL is compiled in but not used, the client does not in fact
> support channel binding (for that connection), so it should send "n".

For others, details about this flag are here:
   gs2-cbind-flag  = ("p=" cb-name) / "n" / "y"
 ;; "n" -> client doesn't support channel binding.
 ;; "y" -> client does support channel binding
 ;;but thinks the server does not.
 ;; "p" -> client requires channel binding.
 ;; The selected channel binding follows "p=".

And channel binding description is here:
https://tools.ietf.org/html/rfc5802#section-6

> The "y" flag should be sent if ssl_in_use but the client did not see the
> server advertise SCRAM-SHA256-PLUS.  That logic is missing entirely in
> this patch.

Okay. I think I get your point here. I agree that the client is
deficient here. This needs some more work.

> You have the server reject a client that does not support channel
> binding ("n") on all SSL connections.  I don't think this is correct.
> It is up to the client to use channel binding or not, even on SSL
> connections.

It seems that I got confused with the meaning of "y" mixed with
ssl_in_use. The server should reject "y" instead only if SSL is in
use.

> We should update pg_hba.conf to allow a method specification of
> "scram-sha256-plus", i.e., only advertise the channel binding variant to
> the client.  Then you could make policy decisions like rejecting clients
> that do not use channel binding on SSL connections.  This could be a
> separate patch later.

OK, I agree that there could be some value in that. This complicates a
bit hba rule checks, but nothing really complicated either.

> The error message in the "p" case if SSL is not in use is a bit
> confusing: "but the server does not need it".  I think this could be
> left at the old message "but it is not supported".  This ties into my
> interpretation from above that whether channel binding is "supported"
> depends on whether SSL is in use for a particular connection.

Check.

> Some small code things:
> - prefer to use size_t over int for length (tls_finish_len etc.)
> - tls_finish should be tls_finished
> - typos: certificate_bash -> certificate_hash

Yes, thanks for spotting those.

> In the patch for tls-server-end-point, I think the selection of the hash
> function deviates slightly from the RFC.  The RFC only says to
> substitute MD5 and SHA-1.  It doesn't say to turn SHA-224 into SHA-256,
> for example.  There is also the problem that the code as written will
> turn any unrecognized hash method into SHA-256.  If think the code
> should single out MD5 and SHA-1 only and then use EVP_get_digestbynid()
> for the rest.  (I don't know anything about the details of OpenSSL APIs,
> but that function sounded right to me.)

Relevant bits from the RFC: https://tools.ietf.org/html/rfc5929#section-4.1
   o  if the certificate's signatureAlgorithm uses a single hash
  function, and that hash function is either MD5 [RFC1321] or SHA-1
  [RFC3174], then use SHA-256 [FIPS-180-3];
   o  if the certificate's signatureAlgorithm uses no hash functions or
  uses multiple hash functions, then this channel binding type's
  channel bindings are undefined at this time (updates to is channel
  binding type may occur to address this issue if it ever arises).

OK. Hm. I think that we need as well to check for the case where
EVP_get_digestbynid() returns NULL then and issue an ERROR on it. That
seems to be the second point outlined by the RFC I am quoting here.

This patch got the feedback I was looking for, and this requires some
rework anyway. So I am marking the patch as returned with feedback for
now. This won't make it in time for this CF.

Thanks Peter for looking at it and pointing out some deficiencies.
-- 
Michael


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Peter Eisentraut]

On 9/10/17 22:37, Michael Paquier wrote:
>>> With the tests directly in the patch, things are easy to run. WIth
>>> PG10 stabilization work, of course I don't expect much feedback :)
>>> But this set of patches looks like the direction we want to go so as
>>> JDBC and libpq users can take advantage of channel binding with SCRAM.
>>
>> Attached is a new patch set, rebased as of c6293249.
> 
> And again a new set to fix the rotten bits caused by 85f4d63.

Here is a review of the meat of the code, leaving aside the discussion
of the libpq connection parameters.

Overall, the structure of the code makes sense and it fits in well with
the existing SCRAM code.


I think the channel-binding negotiation on the client side is wrong.
The logic in the patch is

+#ifdef USE_SSL
+   if (state->ssl_in_use)
+   appendPQExpBuffer(, "p=%s", SCRAM_CHANNEL_TLS_UNIQUE);
+   else
+   appendPQExpBuffer(, "y");
+#else
+   appendPQExpBuffer(, "n");
+#endif

But if SSL is compiled in but not used, the client does not in fact
support channel binding (for that connection), so it should send "n".

The "y" flag should be sent if ssl_in_use but the client did not see the
server advertise SCRAM-SHA256-PLUS.  That logic is missing entirely in
this patch.


You have the server reject a client that does not support channel
binding ("n") on all SSL connections.  I don't think this is correct.
It is up to the client to use channel binding or not, even on SSL
connections.


We should update pg_hba.conf to allow a method specification of
"scram-sha256-plus", i.e., only advertise the channel binding variant to
the client.  Then you could make policy decisions like rejecting clients
that do not use channel binding on SSL connections.  This could be a
separate patch later.


The error message in the "p" case if SSL is not in use is a bit
confusing: "but the server does not need it".  I think this could be
left at the old message "but it is not supported".  This ties into my
interpretation from above that whether channel binding is "supported"
depends on whether SSL is in use for a particular connection.


Some small code things:

- prefer to use size_t over int for length (tls_finish_len etc.)

- tls_finish should be tls_finished

- typos: certificate_bash -> certificate_hash


In the patch for tls-server-end-point, I think the selection of the hash
function deviates slightly from the RFC.  The RFC only says to
substitute MD5 and SHA-1.  It doesn't say to turn SHA-224 into SHA-256,
for example.  There is also the problem that the code as written will
turn any unrecognized hash method into SHA-256.  If think the code
should single out MD5 and SHA-1 only and then use EVP_get_digestbynid()
for the rest.  (I don't know anything about the details of OpenSSL APIs,
but that function sounded right to me.)

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


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Sat, Sep 16, 2017 at 12:42 AM, Robert Haas  wrote:
> On Thu, Sep 14, 2017 at 12:58 PM, Peter Eisentraut
>  wrote:
>> Second thoughts, to make things simpler.  All we need for channel
>> binding is a connection flag that says "I require channel binding".  It
>> could be modeled after the sslmode parameter, e.g., cbind=disable (maybe
>> for debugging), cbind=prefer (default), cbind=require.  If you specify
>> "require", then libpq would refuse to proceed unless scram-sha2-256-plus
>> (or future similar mechanisms) was offered for authentication.
>
> +1, although I think cbind is too brief.  I'd spell it out.

I would like to point out that per the RFC, if the client attempts a
SSL connection with SCRAM and that the server supports channel
binding, then it has to publish the SASL mechanism for channel
binding, aka SCRAM-PLUS. If the client tries to force the use of SCRAM
even if SCRAM-PLUS is specified, this is seen as a downgrade attack by
the server which must reject the connection. So this parameter has
meaning only if you try to connect to a PG10 server using a PG11
client (assuming that channel binding gets into PG11). If you connect
with a PG11 client to a PG11 server with SSL, the server publishes
SCRAM-PLUS, the client has to use it, hence this turns out to make
cbind=disable and prefer meaningless in the long-term. If the client
does not use SSL, then there is no channel binding, and cbind=require
loses its value. So cbind's fate is actually linked to sslmode.

>> We don't even need a parameter that specifies which channel binding type
>> to use.  If libpq implements tls-unique, it should always use that.  We
>> might need a flag for testing other types, but that should not be an
>> in-the-user's-face option.
>
> I'm not so sure about this part.  Why don't we want to let users control this?

That's at least useful for at least testing, tls-unique should be the
default, but we need as well end-point which we much likely would like
to be able to enforce from the client, and being able to enforce the
type of channel binding used does not betray the RFC.
-- 
Michael


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Robert Haas]

On Thu, Sep 14, 2017 at 12:58 PM, Peter Eisentraut
 wrote:
> Second thoughts, to make things simpler.  All we need for channel
> binding is a connection flag that says "I require channel binding".  It
> could be modeled after the sslmode parameter, e.g., cbind=disable (maybe
> for debugging), cbind=prefer (default), cbind=require.  If you specify
> "require", then libpq would refuse to proceed unless scram-sha2-256-plus
> (or future similar mechanisms) was offered for authentication.

+1, although I think cbind is too brief.  I'd spell it out.

> We don't even need a parameter that specifies which channel binding type
> to use.  If libpq implements tls-unique, it should always use that.  We
> might need a flag for testing other types, but that should not be an
> in-the-user's-face option.

I'm not so sure about this part.  Why don't we want to let users control this?

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


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Fri, Sep 15, 2017 at 1:58 AM, Peter Eisentraut
 wrote:
> Second thoughts, to make things simpler.  All we need for channel
> binding is a connection flag that says "I require channel binding".  It
> could be modeled after the sslmode parameter, e.g., cbind=disable (maybe
> for debugging), cbind=prefer (default), cbind=require.  If you specify
> "require", then libpq would refuse to proceed unless scram-sha2-256-plus
> (or future similar mechanisms) was offered for authentication.
>
> We don't even need a parameter that specifies which channel binding type
> to use.  If libpq implements tls-unique, it should always use that.  We
> might need a flag for testing other types, but that should not be an
> in-the-user's-face option.

JDBC folks are willing to have end-point, and we should have a way to
enforce it in my opinion for at least testing with an implementation
at client-level in Postgres core. I agree that without the JDBC needs
having a on/off switch would be sufficient, and actually RFC compliant
as tls-unique is mandatory.
-- 
Michael


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Peter Eisentraut]

On 9/12/17 19:03, Michael Paquier wrote:
> Once channel binding is involved though.. This needs to be extended
> and this needs careful thoughts:
> * "scram-sha-256" means that the version without channel binding is
> accepted. "!scram-sha-256" means that scram without channel binding is
> refused.
> * "scram-sha-256-plus" means that all channel bindings are accepted.
> "!scram-sha-256-plus" means that no channel binding are accepted.
> After that there is some filtering per channel binding name. Do we
> want a separate parameter or just filter with longer names like
> "scram-sha-256-plus-tls-unique" and
> "scram-sha-256-plus-tls-server-end-point"? The last one gets
> particularly long, this does not help users with typos :)

Second thoughts, to make things simpler.  All we need for channel
binding is a connection flag that says "I require channel binding".  It
could be modeled after the sslmode parameter, e.g., cbind=disable (maybe
for debugging), cbind=prefer (default), cbind=require.  If you specify
"require", then libpq would refuse to proceed unless scram-sha2-256-plus
(or future similar mechanisms) was offered for authentication.

We don't even need a parameter that specifies which channel binding type
to use.  If libpq implements tls-unique, it should always use that.  We
might need a flag for testing other types, but that should not be an
in-the-user's-face option.

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


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Tue, Sep 12, 2017 at 11:38 PM, Peter Eisentraut
 wrote:
> It seems we should start by sorting out the mechanism by which the
> client can control what authentication mechanisms it accepts.  In your
> patch set you introduce a connection parameter saslname.  I think we
> should expand that to non-SASL mechanisms and have it be some kind of
> whitelist or blacklist.  It might be reasonable for a client to require
> "gssapi" or "cert" for example or do an exclusion like "!password !md5
> !ldap".
>
> Thoughts?

That looks like a sensible approach to begin with at the end: there
have been complains that a client can be tricked into using MD5 by a
rogue server even if it was willing to use SCRAM. So what about a
parameter called pgauthfilter, which uses a comma-separated list of
keywords. As you say, using an exclamation point to negate an
authentication method is fine for me. For SCRAM, we could just use
"scram-sha-256" as keyword.

Once channel binding is involved though.. This needs to be extended
and this needs careful thoughts:
* "scram-sha-256" means that the version without channel binding is
accepted. "!scram-sha-256" means that scram without channel binding is
refused.
* "scram-sha-256-plus" means that all channel bindings are accepted.
"!scram-sha-256-plus" means that no channel binding are accepted.
After that there is some filtering per channel binding name. Do we
want a separate parameter or just filter with longer names like
"scram-sha-256-plus-tls-unique" and
"scram-sha-256-plus-tls-server-end-point"? The last one gets
particularly long, this does not help users with typos :)
-- 
Michael


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Peter Eisentraut]

On 9/10/17 22:37, Michael Paquier wrote:
> On Mon, Aug 21, 2017 at 9:51 PM, Michael Paquier
>  wrote:
>> On Tue, Jun 20, 2017 at 1:11 PM, Michael Paquier
>>  wrote:
>>> With the tests directly in the patch, things are easy to run. WIth
>>> PG10 stabilization work, of course I don't expect much feedback :)
>>> But this set of patches looks like the direction we want to go so as
>>> JDBC and libpq users can take advantage of channel binding with SCRAM.
>>
>> Attached is a new patch set, rebased as of c6293249.
> 
> And again a new set to fix the rotten bits caused by 85f4d63.

It seems we should start by sorting out the mechanism by which the
client can control what authentication mechanisms it accepts.  In your
patch set you introduce a connection parameter saslname.  I think we
should expand that to non-SASL mechanisms and have it be some kind of
whitelist or blacklist.  It might be reasonable for a client to require
"gssapi" or "cert" for example or do an exclusion like "!password !md5
!ldap".

Thoughts?

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


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Mon, Aug 21, 2017 at 9:51 PM, Michael Paquier
 wrote:
> On Tue, Jun 20, 2017 at 1:11 PM, Michael Paquier
>  wrote:
>> With the tests directly in the patch, things are easy to run. WIth
>> PG10 stabilization work, of course I don't expect much feedback :)
>> But this set of patches looks like the direction we want to go so as
>> JDBC and libpq users can take advantage of channel binding with SCRAM.
>
> Attached is a new patch set, rebased as of c6293249.

And again a new set to fix the rotten bits caused by 85f4d63.
-- 
Michael


0001-Refactor-routine-to-test-connection-to-SSL-server.patch
Description: Binary data


0002-Support-channel-binding-tls-unique-in-SCRAM.patch
Description: Binary data


0003-Add-connection-parameters-saslname-and-saslchannelbi.patch
Description: Binary data


0004-Implement-channel-binding-tls-server-end-point-for-S.patch
Description: Binary data

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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Tue, Jun 20, 2017 at 1:11 PM, Michael Paquier
 wrote:
> With the tests directly in the patch, things are easy to run. WIth
> PG10 stabilization work, of course I don't expect much feedback :)
> But this set of patches looks like the direction we want to go so as
> JDBC and libpq users can take advantage of channel binding with SCRAM.

Attached is a new patch set, rebased as of c6293249.
-- 
Michael


0001-Refactor-routine-to-test-connection-to-SSL-server.patch
Description: Binary data


0002-Support-channel-binding-tls-unique-in-SCRAM.patch
Description: Binary data


0003-Add-connection-parameters-saslname-and-saslchannelbi.patch
Description: Binary data


0004-Implement-channel-binding-tls-server-end-point-for-S.patch
Description: Binary data

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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Wed, Jun 21, 2017 at 4:04 AM, Álvaro Hernández Tortosa
 wrote:
> In the coming weeks, and once my PR for pgjdbc has been added, I will
> work towards another patch to implement channel binding. Should be
> reasonably easy now, thanks to this.

So you basically have an equivalent of OpenSSL stuff in java, right?
- SSL_get_peer_certificate to get the X509 point of the server.
- X509_digest to hash it.
- OBJ_find_sigid_algs and X509_get_signature_nid to guess the
signature algorithm of a certificate. I think that this part can be
tricky depending on the SSL implementation, but I have designed a
generic API for this purpose.
That's all it took me to get end-point to work. Plus the error
handling of course.
-- 
Michael


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Álvaro Hernández Tortosa]

On 20/06/17 06:11, Michael Paquier wrote:

On Tue, Jun 6, 2017 at 3:40 PM, Michael Paquier
 wrote:

As far as I can see, there are a couple of things that I still need to
work on to make people happy:
- Rework the generic APIs for TLS finish and endpoint so as any
implementation can use channel binding without inducing any extra code
footprint to be-secure.c and fe-secure.c.
- Implement endpoint, as Alvaro is saying for JDBC that would be nicer.
- Have a couple of tests for channel binding to allow people to test
the feature easily. Those will be in src/test/ssl/. It would be nice
as well to be able to enforce the channel binding type on libpq-side,
which is useful at least for testing. So we are going to need an
environment variable for this purpose, and a connection parameter.

Okay, here we go. Attached is a set of four patches:
- 0001 is some refactoring for the SSL tests so as other test suite in
src/test/ssl can take advantage of the connection routines. There is
nothing fancy here.
- 0002 is the implementation of tls-unique as channel binding. This
has been largely reworked since last submission, I have found on the
way a couple of bugs and some correctness issues.
- 0003 is a patch to add as connection parameters saslname and
saslchannelbinding. With support of more SASL mechanisms (PG10 has
SCRAM-SHA-256, I am adding SCRAM-SHA-256-PLUS here), saslname can be
used to enforce on the client-side the value of the SASL mechanism
chosen. saslchannelbinding does the same for the channel binding name.
This is very useful for testing, and a set of tests are added in
src/test/ssl/ for tls-unique and the SASL mechanisms. The tests cover
many scenarios, like downgrade attacks for example.
- 0004 is the implementation of tls-server-end-point, as Alvaro has
asked. Per RFC 5929, the binding data needs to be a hash of the server
certificate. If the signature algorithm of the certificate is MD5 or
SHA-1, then SHA-256 is used. Other signature algos like SHA-384 or 512
are used to hash the data. The hashed data is then encoded in base64
and sent to the server for verification. Tests using saslchannelname
have been added as well. It took me a while to find out that
OBJ_find_sigid_algs(X509_get_signature_nid(X509*)) needs to be used to
find out the algorithm of a certificate with OpenSSL.

With the tests directly in the patch, things are easy to run. WIth
PG10 stabilization work, of course I don't expect much feedback :)
But this set of patches looks like the direction we want to go so as
JDBC and libpq users can take advantage of channel binding with SCRAM.


This is awesome, Michael.

In the coming weeks, and once my PR for pgjdbc has been added, I 
will work towards another patch to implement channel binding. Should be 
reasonably easy now, thanks to this.


Appreciated!

Álvaro



--

Álvaro Hernández Tortosa


---
<8K>data



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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Tue, Jun 6, 2017 at 3:40 PM, Michael Paquier
 wrote:
> As far as I can see, there are a couple of things that I still need to
> work on to make people happy:
> - Rework the generic APIs for TLS finish and endpoint so as any
> implementation can use channel binding without inducing any extra code
> footprint to be-secure.c and fe-secure.c.
> - Implement endpoint, as Alvaro is saying for JDBC that would be nicer.
> - Have a couple of tests for channel binding to allow people to test
> the feature easily. Those will be in src/test/ssl/. It would be nice
> as well to be able to enforce the channel binding type on libpq-side,
> which is useful at least for testing. So we are going to need an
> environment variable for this purpose, and a connection parameter.

Okay, here we go. Attached is a set of four patches:
- 0001 is some refactoring for the SSL tests so as other test suite in
src/test/ssl can take advantage of the connection routines. There is
nothing fancy here.
- 0002 is the implementation of tls-unique as channel binding. This
has been largely reworked since last submission, I have found on the
way a couple of bugs and some correctness issues.
- 0003 is a patch to add as connection parameters saslname and
saslchannelbinding. With support of more SASL mechanisms (PG10 has
SCRAM-SHA-256, I am adding SCRAM-SHA-256-PLUS here), saslname can be
used to enforce on the client-side the value of the SASL mechanism
chosen. saslchannelbinding does the same for the channel binding name.
This is very useful for testing, and a set of tests are added in
src/test/ssl/ for tls-unique and the SASL mechanisms. The tests cover
many scenarios, like downgrade attacks for example.
- 0004 is the implementation of tls-server-end-point, as Alvaro has
asked. Per RFC 5929, the binding data needs to be a hash of the server
certificate. If the signature algorithm of the certificate is MD5 or
SHA-1, then SHA-256 is used. Other signature algos like SHA-384 or 512
are used to hash the data. The hashed data is then encoded in base64
and sent to the server for verification. Tests using saslchannelname
have been added as well. It took me a while to find out that
OBJ_find_sigid_algs(X509_get_signature_nid(X509*)) needs to be used to
find out the algorithm of a certificate with OpenSSL.

With the tests directly in the patch, things are easy to run. WIth
PG10 stabilization work, of course I don't expect much feedback :)
But this set of patches looks like the direction we want to go so as
JDBC and libpq users can take advantage of channel binding with SCRAM.
-- 
Michael


0001-Refactor-routine-to-test-connection-to-SSL-server.patch
Description: Binary data


0002-Support-channel-binding-tls-unique-in-SCRAM.patch
Description: Binary data


0003-Add-connection-parameters-saslname-and-saslchannelbi.patch
Description: Binary data


0004-Implement-channel-binding-tls-server-end-point-for-S.patch
Description: Binary data

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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Tom Lane]

Robert Haas  writes:
> I don't think it's true that we force the latest TLS version to be
> used.  The comment says:

> /*
>  * We use SSLv23_method() because it can negotiate use of the highest
>  * mutually supported protocol version, while alternatives like
>  * TLSv1_2_method() permit only one specific version.  Note
> that we don't
>  * actually allow SSL v2 or v3, only TLS protocols (see below).
>  */

> IIUC, this is specifically so that we don't force the use of TLS 1.2
> or TLS 1.1 or TLS 1.0.

Right.  IIUC, there's no way (at least in older OpenSSL versions) to say
directly "we only want TLS >= 1.0", so we have to do it like this.
I found a comment on the web saying "SSLv23_method would be better named
AutoNegotiate_method", which seems accurate.

regards, tom lane


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Robert Haas]

On Tue, Jun 6, 2017 at 2:32 AM, Michael Paquier
 wrote:
>>> At the end,
>>> everything has been rejected as Postgres enforces the use of the
>>> newest one when doing the SSL handshake.
>>
>> TLS implementations, or TLS versions?  What does the TLS version have
>> to do with this issue?
>
> I really mean *version* here.

I don't think it's true that we force the latest TLS version to be
used.  The comment says:

/*
 * We use SSLv23_method() because it can negotiate use of the highest
 * mutually supported protocol version, while alternatives like
 * TLSv1_2_method() permit only one specific version.  Note
that we don't
 * actually allow SSL v2 or v3, only TLS protocols (see below).
 */

IIUC, this is specifically so that we don't force the use of TLS 1.2
or TLS 1.1 or TLS 1.0.

It could well be that there's something I don't understand here.

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


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Fri, Jun 2, 2017 at 10:25 AM, Robert Haas  wrote:
> > On Thu, Jun 1, 2017 at 9:13 PM, Michael Paquier
> >  wrote:
> >> It seems to me that any testing in this area won't fly high as long as
> >> there is no way to enforce the list of TLS implementations that a
> >> server allows. There have been discussions about being able to control
> >> that after the OpenSSL vulnerabilities that were protocol-specific and
> >> there were even patches adding GUCs for this purpose. At the end,
> >> everything has been rejected as Postgres enforces the use of the
> >> newest one when doing the SSL handshake.
> >
> > TLS implementations, or TLS versions?  What does the TLS version have
> > to do with this issue?
> 
> I really mean *version* here. Unlike OpenSSL, the Windows TLS
> implementation does not offer an API to choose the latest TLS version
> available:
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa380513(v=vs.85).aspx
> It is up to the server and the client to negotiate that, so it seems
> to me that we want some control in this area, which would be important
> for testing as well because the TLS finish message differs a bit
> across versions, in length mainly. On top of that per the aggressive
> updates that Windows does from time to time they may as well forcibly
> expose users to a broken TLS implementation...
> MacOS has something similar to OpenSSL, with
> SSLGetProtocolVersionMax(), which is nice.

We mainly need to know what version was used, right..?  Isn't that
available?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Fri, Jun 2, 2017 at 10:08 AM, Stephen Frost  wrote:
> I don't have any issue with asking that Michael, or someone, to go look
> at other OpenSSL-using implementations which support channel binding.

I don't see the implementation of other TLS/SSL as a requirement for
channel binding with OpenSSL, and I have no plans to investigate that.
What is clearly a requirement though is to design a set of API generic
enough for the backend and the frontend to fetch the TLS unique
message, and what's needed for endpoint so as any implementation can
plug in easily with PG's channel binding code.

> What I find somewhat objectionable is the notion that if we don't have 5
> different TLS/SSL implementations supported in PG and that we've tested
> that channel binding works correctly among all combinations of all of
> them, then we can't accept a patch implementing it.  I'm exaggerating
> for effect here, of course, and you've agreed that a full, committable,
> implementation isn't necessary, and I agreed with that, but we still
> seem to have a different level of expectation regarding what needs doing
> here.  I don't know that we'll ever be able to nail down the exact
> location of the line in the sand that needs to be crossed here, or agree
> to it, so I'd suggest that we let them continue their efforts to work
> together and provide a patch which they feel is ready to be commented on
> by committers.  Perhaps we'll find that, regardless of where the line
> was drawn exactly, they've crossed it.

As far as I can see, there are a couple of things that I still need to
work on to make people happy:
- Rework the generic APIs for TLS finish and endpoint so as any
implementation can use channel binding without inducing any extra code
footprint to be-secure.c and fe-secure.c.
- Implement endpoint, as Alvaro is saying for JDBC that would be nicer.
- Have a couple of tests for channel binding to allow people to test
the feature easily. Those will be in src/test/ssl/. It would be nice
as well to be able to enforce the channel binding type on libpq-side,
which is useful at least for testing. So we are going to need an
environment variable for this purpose, and a connection parameter.
-- 
Michael


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Fri, Jun 2, 2017 at 10:25 AM, Robert Haas  wrote:
> On Thu, Jun 1, 2017 at 9:13 PM, Michael Paquier
>  wrote:
>> It seems to me that any testing in this area won't fly high as long as
>> there is no way to enforce the list of TLS implementations that a
>> server allows. There have been discussions about being able to control
>> that after the OpenSSL vulnerabilities that were protocol-specific and
>> there were even patches adding GUCs for this purpose. At the end,
>> everything has been rejected as Postgres enforces the use of the
>> newest one when doing the SSL handshake.
>
> TLS implementations, or TLS versions?  What does the TLS version have
> to do with this issue?

I really mean *version* here. Unlike OpenSSL, the Windows TLS
implementation does not offer an API to choose the latest TLS version
available:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa380513(v=vs.85).aspx
It is up to the server and the client to negotiate that, so it seems
to me that we want some control in this area, which would be important
for testing as well because the TLS finish message differs a bit
across versions, in length mainly. On top of that per the aggressive
updates that Windows does from time to time they may as well forcibly
expose users to a broken TLS implementation...
MacOS has something similar to OpenSSL, with
SSLGetProtocolVersionMax(), which is nice.
-- 
Michael


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Robert Haas]

On Thu, Jun 1, 2017 at 9:13 PM, Michael Paquier
 wrote:
> On Fri, Jun 2, 2017 at 10:08 AM, Stephen Frost  wrote:
>> What I find somewhat objectionable is the notion that if we don't have 5
>> different TLS/SSL implementations supported in PG and that we've tested
>> that channel binding works correctly among all combinations of all of
>> them, then we can't accept a patch implementing it.
>
> It seems to me that any testing in this area won't fly high as long as
> there is no way to enforce the list of TLS implementations that a
> server allows. There have been discussions about being able to control
> that after the OpenSSL vulnerabilities that were protocol-specific and
> there were even patches adding GUCs for this purpose. At the end,
> everything has been rejected as Postgres enforces the use of the
> newest one when doing the SSL handshake.

TLS implementations, or TLS versions?  What does the TLS version have
to do with this issue?

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


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Fri, Jun 2, 2017 at 10:08 AM, Stephen Frost  wrote:
> What I find somewhat objectionable is the notion that if we don't have 5
> different TLS/SSL implementations supported in PG and that we've tested
> that channel binding works correctly among all combinations of all of
> them, then we can't accept a patch implementing it.

It seems to me that any testing in this area won't fly high as long as
there is no way to enforce the list of TLS implementations that a
server allows. There have been discussions about being able to control
that after the OpenSSL vulnerabilities that were protocol-specific and
there were even patches adding GUCs for this purpose. At the end,
everything has been rejected as Postgres enforces the use of the
newest one when doing the SSL handshake.
-- 
Michael


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, Jun 1, 2017 at 11:50 AM, Stephen Frost  wrote:
> > I certainly wouldn't object to someone working on this, but I feel like
> > it's a good deal more work than perhaps you're realizing (and I tend to
> > think trying to use the Windows SSL implementation would increase the
> > level of effort, not minimize it).
> 
> I agree that it's a fair amount of work, but if nobody does it, then I
> think it's pretty speculative to suppose that the feature actually
> works correctly.

We've considered "works with OpenSSL" (and, to some extent, JDBC, but
I'm pretty sure that came later and just happened to be able to work...)
to be sufficient to include things like client-side certificate based
authentication, so this is raising the bar quite a bit for a feature
that, while important and valuable, frankly isn't as important as
client-side cert auth.

> > Perhaps it wouldn't be too bad to
> > write a one-off piece of code which just connects and then returns the
> > channel binding information on each side and then one could hand-check
> > that what's returned matches what it's supposed to based on the RFC, but
> > if it doesn't, then what?
> 
> Then something's broken and we need to fix it before we start
> committing patches.

... Or that implementation doesn't follow the RFC, which is what I was
getting at.

> > In the specific case we're talking about,
> > there's two approaches in the RFC and it seems like we should support
> > both because at least our current JDBC implementation only works with
> > one, and ideally we can allow for that to be extended to other methods,
> > but I wouldn't want to implement a method that only works on Windows SSL
> > because that implementation, for whatever reason, doesn't follow either
> > of the methods available in the RFC.
> 
> I am very skeptical that if two people on two different SSL
> interpretations both do something that appears to comply with the RFC,
> we can just assume those two people will get the same answer.  In an
> ideal world, that would definitely work, but in the real world, I
> think it needs to be tested or you don't really know.  I agree that if
> a given SSL implementation is such that it can't support either of the
> possible channel binding methods, then that's not our problem; people
> using that SSL implementation just can't get channel binding, and if
> they don't like that they can switch SSL implementations.  But I also
> think that if two SSL implementations both claim to support what's
> needed to make channel binding work and we don't do any testing that
> they actually interoperate, then I don't think we can really know that
> we've got it right.

I'm all for doing testing, as I've tried to point out a few times, and I
would like to see an implementation which is able to be extended in the
future to other channel binding methods, as we clearly need to support
at least the two listed in the RFC based on this discussion and there
might be a still better way down the road anyway.

> Another way of validating Michael's problem which I would find
> satisfactory is to go look at some other OpenSSL-based implementations
> of channel binding.  If in each case they are using the same functions
> that Michael used in the same way, then that's good evidence that his
> implementation is doing the right thing, especially if any of those
> implementations also support other SSL implementations and have
> verified that the OpenSSL mechanism does in fact interoperate.

I don't have any issue with asking that Michael, or someone, to go look
at other OpenSSL-using implementations which support channel binding.

> I don't really know why we're arguing about this.  It seems blindingly
> obvious to me that we can't just take it on faith that the functions
> Michael identified for this purpose are the right ones and will work
> perfectly in complete compliance with the RFC.  We are in general very
> reluctant to make such assumptions and if we were going to start,
> changes that affect wire protocol compatibility wouldn't be my first
> choice.  Is it really all that revolutionary or controversial to
> suggest that this patch has not yet had enough validation to really
> know that it does what we want?  To me, it seems like verifying that a
> patch which supposedly implements a standard interoperates with
> something other than itself is so obvious that it should barely need
> to be mentioned, let alone become a bone of contention.

As I said in an earlier reply, I'm hopefuly that we're closer to
agreement here than we are disagreement, but there seems to be some
confusion regarding my position on this specific patch.  I'm not
advocating for this patch to be committed as-is or even anytime soon,
and I'm unsure of where I gave that impression.  I'm encouraged by the
ongoing discussion between Michael and Alvaro and hope to see a new
patch down the road which incorporates the results of 

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Álvaro Hernández Tortosa]

On 01/06/17 17:50, Stephen Frost wrote:

Robert,

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

On Wed, May 31, 2017 at 7:59 PM, Stephen Frost  wrote:

If your comments regarding the requirement that we have interoperability
testing of this feature before accepting it were intended to mean that
we need to make sure that the JDBC driver is able to work with the
chosen solution (or, perhaps, that we support multuple options, one of
which works with the JDBC changes), and that we review the other TLS
libraries with the goal of making sure that what we agree on in core
should work with those also, then that's great and exactly what I'd like
to see also.

OK, great.  That's what I mean (mostly - see below).

Glad to hear it.


If your comments regarding the requirement that we have interoperability
testing of this feature before accepting it were intended to mean that
we must have a complete alternative TLS solution, while that would
actually play a great deal to a goal I've had for a while to have PG
support multiple TLS implementations (LibNSS being top-of-mind, at least
for me, as I've mentioned a couple times), I can't agree that we should
require that before accepting channel binding as a feature.  To do so
would be akin to requiring that we support multiple TLS implementations
before we agreed to support client-side certificates, in my opinion.

I don't think that we need to have a committed patch allowing multiple
SSL implementations before we accept a patch for channel binding, but
I think it might be a good idea for someone to hack either libpq or
the server enough to make it work with some other SSL implementation
(Windows SSL would probably be the best candidate) and test that what
we think will work for channel binding actually does work.  It
wouldn't need to be a committable patch, but it should be enough to
make a successful connection in the laboratory.  Now, there might also
be other ways besides that of testing that interoperability is
possible, so don't take me as being of the view that someone has to
necessarily do exactly that thing that I just said.  But I do think
that we probably should do more than say "hey, we've got this
undocumented SSL API, and this other Windows API, and it looks like
they do about the same thing, so we're good".  There's sometimes a big
difference between what sounds like it should work on paper and what
actually does work.

I certainly wouldn't object to someone working on this, but I feel like
it's a good deal more work than perhaps you're realizing (and I tend to
think trying to use the Windows SSL implementation would increase the
level of effort, not minimize it).  Perhaps it wouldn't be too bad to
write a one-off piece of code which just connects and then returns the
channel binding information on each side and then one could hand-check
that what's returned matches what it's supposed to based on the RFC, but
if it doesn't, then what?  In the specific case we're talking about,
there's two approaches in the RFC and it seems like we should support
both because at least our current JDBC implementation only works with
one, and ideally we can allow for that to be extended to other methods,
but I wouldn't want to implement a method that only works on Windows SSL
because that implementation, for whatever reason, doesn't follow either
of the methods available in the RFC.


To make things even more complicated, I think the RFC is not 
helping very much, as the definition is not very clear itself:


"
Description: The first TLS Finished message sent (note: the Finished
   struct, not the TLS record layer message containing it) in the most
   recent TLS handshake of the TLS connection being bound to (note: TLS
   connection, not session, so that the channel binding is specific to
   each connection regardless of whether session resumption is used).
   If TLS renegotiation takes place before the channel binding
   operation, then the first TLS Finished message sent of the latest/
   inner-most TLS connection is used.  Note that for full TLS
   handshakes, the first Finished message is sent by the client, while
   for abbreviated TLS handshakes (session resumption), the first
   Finished message is sent by the server.
"
https://tools.ietf.org/html/rfc5929#section-3.1

If you read further, it becomes even less clear what it is and that 
if there are re-negotiations, it gets more complicated. Server-end-point 
is kind of better specified:


"
The hash of the TLS server's certificate [RFC5280] as it
   appears, octet for octet, in the server's Certificate message.
"


Álvaro



--

Álvaro Hernández Tortosa


---
<8K>data



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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Álvaro Hernández Tortosa]

On 01/06/17 18:11, Bruce Momjian wrote:

On Wed, May 31, 2017 at 09:37:02AM -0400, Robert Haas wrote:

On Tue, May 30, 2017 at 11:49 PM, Stephen Frost  wrote:

... and I don't believe that we should be asking the
implementors of channel binding to also implement support for multiple
TLS libraries in PostgreSQL in order to test that their RFC-following
(at least, as far as they can tell) implementation actually works.

You're of course free to believe what you wish, but that sounds
short-sighted to me.  If we implement channel binding and it turns out
not to be interoperable with other SSL implementations, then what?  We
can't change it later without breaking compatibility with our own
prior implementation.  Note that Álvaro Hernández Tortosa said about
two hours before you sent this email that it doesn't seem possible to
implement something comparable in Java's standard SSL stack.  If
that's the case, adopting this implementation is dooming everyone who
connects to the database server using JDBC to be unable to use channel
binding.  And that's a large percentage of our user base.

Just to step back, exactly how does channel binding work?  Is each side
of the SSL connection hashing the password hash with the shared SSL
session secret in some way that each side knows the other end knows
the password hash, but not disclosing the secret or password hash?  Is
there some other way JDBC can get that information?



In short, channel binding augments SCRAM (could be also other 
authentication methods, I guess) by adding to the mix of data to be 
exchanged, data that comes off-band. Normal SCRAM exchange involves 
user, a unique token, a salt and some other information. If you add into 
the mix off-band information, that is uniquely known by only client and 
server, you can avoid MITM attacks. It's not as simple as "hashing" the 
password, though. SCRAM involves 4 steps (2 messages each way) with 
somehow complex hashing of data in the last 2 steps.


There are basically 2 channel binding options, that is, 2 possible 
data pieces that could be taken off-band of the SCRAM authentication and 
throw them into this mix:


- tls-unique. It's like a unique identifier for each client-server SSL 
connection. It should be get from the SSL channel and there doesn't seem 
to be a super consistent way of getting it from the channel (in OpenSSL 
is an undocumented API, it's not available in normal Java stack, etc).
- tls-server-end-point. Channel binding data is just a hash of a SSL 
certificate, as is. As such, seems to be easier to be supported across 
multiple OSs/SSL stacks.


However, SCRAM RFC states that if channel binding is implemented, 
at least tls-unique must be implemented, with others being optional 
(there is as of today a third one, but only applies to telnet). So in my 
opinion, I'd implement both of the above, for maximum flexibility, and 
add a field to the required scram authentication febe message with a 
list (aka CSV) of the channel binding mechanisms supported by this 
server. In this way, I believe covers support for several channel 
binding mechanisms and could be extended in the future should other 
mechanisms appear.



Álvaro

--

Álvaro Hernández Tortosa


---
<8K>data



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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Robert Haas]

On Thu, Jun 1, 2017 at 11:50 AM, Stephen Frost  wrote:
> I certainly wouldn't object to someone working on this, but I feel like
> it's a good deal more work than perhaps you're realizing (and I tend to
> think trying to use the Windows SSL implementation would increase the
> level of effort, not minimize it).

I agree that it's a fair amount of work, but if nobody does it, then I
think it's pretty speculative to suppose that the feature actually
works correctly.

> Perhaps it wouldn't be too bad to
> write a one-off piece of code which just connects and then returns the
> channel binding information on each side and then one could hand-check
> that what's returned matches what it's supposed to based on the RFC, but
> if it doesn't, then what?

Then something's broken and we need to fix it before we start
committing patches.

> In the specific case we're talking about,
> there's two approaches in the RFC and it seems like we should support
> both because at least our current JDBC implementation only works with
> one, and ideally we can allow for that to be extended to other methods,
> but I wouldn't want to implement a method that only works on Windows SSL
> because that implementation, for whatever reason, doesn't follow either
> of the methods available in the RFC.

I am very skeptical that if two people on two different SSL
interpretations both do something that appears to comply with the RFC,
we can just assume those two people will get the same answer.  In an
ideal world, that would definitely work, but in the real world, I
think it needs to be tested or you don't really know.  I agree that if
a given SSL implementation is such that it can't support either of the
possible channel binding methods, then that's not our problem; people
using that SSL implementation just can't get channel binding, and if
they don't like that they can switch SSL implementations.  But I also
think that if two SSL implementations both claim to support what's
needed to make channel binding work and we don't do any testing that
they actually interoperate, then I don't think we can really know that
we've got it right.

Another way of validating Michael's problem which I would find
satisfactory is to go look at some other OpenSSL-based implementations
of channel binding.  If in each case they are using the same functions
that Michael used in the same way, then that's good evidence that his
implementation is doing the right thing, especially if any of those
implementations also support other SSL implementations and have
verified that the OpenSSL mechanism does in fact interoperate.

I don't really know why we're arguing about this.  It seems blindingly
obvious to me that we can't just take it on faith that the functions
Michael identified for this purpose are the right ones and will work
perfectly in complete compliance with the RFC.  We are in general very
reluctant to make such assumptions and if we were going to start,
changes that affect wire protocol compatibility wouldn't be my first
choice.  Is it really all that revolutionary or controversial to
suggest that this patch has not yet had enough validation to really
know that it does what we want?  To me, it seems like verifying that a
patch which supposedly implements a standard interoperates with
something other than itself is so obvious that it should barely need
to be mentioned, let alone become a bone of contention.

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


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Bruce Momjian]

On Wed, May 31, 2017 at 09:37:02AM -0400, Robert Haas wrote:
> On Tue, May 30, 2017 at 11:49 PM, Stephen Frost  wrote:
> > ... and I don't believe that we should be asking the
> > implementors of channel binding to also implement support for multiple
> > TLS libraries in PostgreSQL in order to test that their RFC-following
> > (at least, as far as they can tell) implementation actually works.
> 
> You're of course free to believe what you wish, but that sounds
> short-sighted to me.  If we implement channel binding and it turns out
> not to be interoperable with other SSL implementations, then what?  We
> can't change it later without breaking compatibility with our own
> prior implementation.  Note that Álvaro Hernández Tortosa said about
> two hours before you sent this email that it doesn't seem possible to
> implement something comparable in Java's standard SSL stack.  If
> that's the case, adopting this implementation is dooming everyone who
> connects to the database server using JDBC to be unable to use channel
> binding.  And that's a large percentage of our user base.

Just to step back, exactly how does channel binding work?  Is each side
of the SSL connection hashing the password hash with the shared SSL
session secret in some way that each side knows the other end knows
the password hash, but not disclosing the secret or password hash?  Is
there some other way JDBC can get that information?

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

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


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Wed, May 31, 2017 at 7:59 PM, Stephen Frost  wrote:
> > If your comments regarding the requirement that we have interoperability
> > testing of this feature before accepting it were intended to mean that
> > we need to make sure that the JDBC driver is able to work with the
> > chosen solution (or, perhaps, that we support multuple options, one of
> > which works with the JDBC changes), and that we review the other TLS
> > libraries with the goal of making sure that what we agree on in core
> > should work with those also, then that's great and exactly what I'd like
> > to see also.
> 
> OK, great.  That's what I mean (mostly - see below).

Glad to hear it.

> > If your comments regarding the requirement that we have interoperability
> > testing of this feature before accepting it were intended to mean that
> > we must have a complete alternative TLS solution, while that would
> > actually play a great deal to a goal I've had for a while to have PG
> > support multiple TLS implementations (LibNSS being top-of-mind, at least
> > for me, as I've mentioned a couple times), I can't agree that we should
> > require that before accepting channel binding as a feature.  To do so
> > would be akin to requiring that we support multiple TLS implementations
> > before we agreed to support client-side certificates, in my opinion.
> 
> I don't think that we need to have a committed patch allowing multiple
> SSL implementations before we accept a patch for channel binding, but
> I think it might be a good idea for someone to hack either libpq or
> the server enough to make it work with some other SSL implementation
> (Windows SSL would probably be the best candidate) and test that what
> we think will work for channel binding actually does work.  It
> wouldn't need to be a committable patch, but it should be enough to
> make a successful connection in the laboratory.  Now, there might also
> be other ways besides that of testing that interoperability is
> possible, so don't take me as being of the view that someone has to
> necessarily do exactly that thing that I just said.  But I do think
> that we probably should do more than say "hey, we've got this
> undocumented SSL API, and this other Windows API, and it looks like
> they do about the same thing, so we're good".  There's sometimes a big
> difference between what sounds like it should work on paper and what
> actually does work.

I certainly wouldn't object to someone working on this, but I feel like
it's a good deal more work than perhaps you're realizing (and I tend to
think trying to use the Windows SSL implementation would increase the
level of effort, not minimize it).  Perhaps it wouldn't be too bad to
write a one-off piece of code which just connects and then returns the
channel binding information on each side and then one could hand-check
that what's returned matches what it's supposed to based on the RFC, but
if it doesn't, then what?  In the specific case we're talking about,
there's two approaches in the RFC and it seems like we should support
both because at least our current JDBC implementation only works with
one, and ideally we can allow for that to be extended to other methods,
but I wouldn't want to implement a method that only works on Windows SSL
because that implementation, for whatever reason, doesn't follow either
of the methods available in the RFC.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Robert Haas]

On Wed, May 31, 2017 at 7:59 PM, Stephen Frost  wrote:
> If your comments regarding the requirement that we have interoperability
> testing of this feature before accepting it were intended to mean that
> we need to make sure that the JDBC driver is able to work with the
> chosen solution (or, perhaps, that we support multuple options, one of
> which works with the JDBC changes), and that we review the other TLS
> libraries with the goal of making sure that what we agree on in core
> should work with those also, then that's great and exactly what I'd like
> to see also.

OK, great.  That's what I mean (mostly - see below).

> If your comments regarding the requirement that we have interoperability
> testing of this feature before accepting it were intended to mean that
> we must have a complete alternative TLS solution, while that would
> actually play a great deal to a goal I've had for a while to have PG
> support multiple TLS implementations (LibNSS being top-of-mind, at least
> for me, as I've mentioned a couple times), I can't agree that we should
> require that before accepting channel binding as a feature.  To do so
> would be akin to requiring that we support multiple TLS implementations
> before we agreed to support client-side certificates, in my opinion.

I don't think that we need to have a committed patch allowing multiple
SSL implementations before we accept a patch for channel binding, but
I think it might be a good idea for someone to hack either libpq or
the server enough to make it work with some other SSL implementation
(Windows SSL would probably be the best candidate) and test that what
we think will work for channel binding actually does work.  It
wouldn't need to be a committable patch, but it should be enough to
make a successful connection in the laboratory.  Now, there might also
be other ways besides that of testing that interoperability is
possible, so don't take me as being of the view that someone has to
necessarily do exactly that thing that I just said.  But I do think
that we probably should do more than say "hey, we've got this
undocumented SSL API, and this other Windows API, and it looks like
they do about the same thing, so we're good".  There's sometimes a big
difference between what sounds like it should work on paper and what
actually does work.

> I would be rather surprised if the solution which Michael and Alvaro
> come to results in a solution which requires us to break compatibility
> down the road, though it's of course a risk we need to consider.  The
> ongoing discussion around finding a way to support both methods outlined
> in the RFC sounds exactly correct to me and I encourage further
> discussion there.

Sure, I have no objection to the discussion.  Discussion is always
cool; what I'm worried about is a tls-unique implementation that is
supremely unportable, and there's no current evidence that the
currently-proposed patch is anything else.  There is some optimism,
but optimism <> evidence.

> I'm certainly on-board with the general idea of having a way for the
> client to require both SCRAM and channel binding and I agree that's a
> hole in our current system, but that is really an orthogonal concern.

Orthogonal but *very* closely related.

> entirely technical perspective.  If I were one of the individuals
> working on this feature, I'd be rather put-off and upset at the
> suggestion that the good work they're doing is viewed by the PostgreSQL
> community and one of our major committers as only being done to check a
> box or to be "buzzword compliant" and ultimately without technical
> merit.

I did not say that the feature was "ultimately without technical
merit"; I said that the patch as submitted seemed like it might not
interoperate, and that without a libpq option to force it to be used
it wouldn't add any real security.  I stand by those statements and I
think it is 100% appropriate for me to raise those issues.  Other
people, including you, do the same thing about other patches all the
time.  It is not a broadside against the contributors or the patch; it
is a short, specific list of concerns that are IMHO quite justifiable.

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


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, May 30, 2017 at 11:49 PM, Stephen Frost  wrote:
> > ... and I don't believe that we should be asking the
> > implementors of channel binding to also implement support for multiple
> > TLS libraries in PostgreSQL in order to test that their RFC-following
> > (at least, as far as they can tell) implementation actually works.
> 
> You're of course free to believe what you wish, but that sounds
> short-sighted to me.  If we implement channel binding and it turns out
> not to be interoperable with other SSL implementations, then what?  We
> can't change it later without breaking compatibility with our own
> prior implementation.  Note that Álvaro Hernández Tortosa said about
> two hours before you sent this email that it doesn't seem possible to
> implement something comparable in Java's standard SSL stack.  If
> that's the case, adopting this implementation is dooming everyone who
> connects to the database server using JDBC to be unable to use channel
> binding.  And that's a large percentage of our user base.

I'm hopeful that we're closer to agreement here than we are the
opposite.

It was absolutely not my intent that the ongoing discussion between
Alvaro and Michael be stopped or changed, quite the opposite, in fact.

If your comments regarding the requirement that we have interoperability
testing of this feature before accepting it were intended to mean that
we need to make sure that the JDBC driver is able to work with the
chosen solution (or, perhaps, that we support multuple options, one of
which works with the JDBC changes), and that we review the other TLS
libraries with the goal of making sure that what we agree on in core
should work with those also, then that's great and exactly what I'd like
to see also.

If your comments regarding the requirement that we have interoperability
testing of this feature before accepting it were intended to mean that
we must have a complete alternative TLS solution, while that would
actually play a great deal to a goal I've had for a while to have PG
support multiple TLS implementations (LibNSS being top-of-mind, at least
for me, as I've mentioned a couple times), I can't agree that we should
require that before accepting channel binding as a feature.  To do so
would be akin to requiring that we support multiple TLS implementations
before we agreed to support client-side certificates, in my opinion.

I would be rather surprised if the solution which Michael and Alvaro
come to results in a solution which requires us to break compatibility
down the road, though it's of course a risk we need to consider.  The
ongoing discussion around finding a way to support both methods outlined
in the RFC sounds exactly correct to me and I encourage further
discussion there.

> And then what happens when we do add support for Windows SSL or macOS
> SSL?  It sounds like you're equally willing to throw people using
> those implementations under the bus.  So we'll end up with channel
> binding that works only when everybody's running the same operating
> system and nobody's using Java.  Uggh.  At that point you might as
> well just go back to using SSL certificates to solve this problem.  If
> you use SSL certificates, then (1) it should work with any SSL
> implementation and (2) the client can force SSL certificate
> verification, whereas currently the client cannot force even the use
> of SCRAM, let alone channel binding.

I hope it's clear that it's not my intent to throw anyone "under the
bus."  The only reason that "SSL certificates should work with any SSL
implementation" is because those SSL implementations are based on RFCs
which define how they should work and what I was encouraging up-thread
is exactly that we should be looking at RFCs to determine the right path
forward.  There are cases, of course, where the RFCs have alternatives
and the ideal approach is to find a way to work with all of those
alternatives, or at least implement a solution which is flexible, such
that later changes could add support without breaking existing users.

I'm certainly on-board with the general idea of having a way for the
client to require both SCRAM and channel binding and I agree that's a
hole in our current system, but that is really an orthogonal concern.

> So what is being proposed here does not (yet, anyway) provide any
> actual security and seems to have poor prospects for interoperability,
> whereas we have an existing feature (SSL certificates) that has
> neither of those problems.  Are you sure you're not putting
> buzzword-compliance ahead of real progress?

Adding support for channel binding is quite important and valuable, in
its own right.  I concur that we want to provide ways for the client to
require SCRAM and to require channel binding, but I don't see any
particular reason that adding support for channel binding has to be held
up behind that.

As for your question, I have to say that I find 

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Tue, May 30, 2017 at 6:50 PM, Álvaro Hernández Tortosa
 wrote:
> Actually this Python patch seems to ultimately rely on the same OpenSSL
> functions as your implementation.

Yup. My point is that even if those APIs are undocumented, I think
that they are not going to disappear either.

> I don't have anything against implementing tls-unique, specially as per
> the RFC it is mandatory (if channel binding support is provided). However,
> given the use of undocumented API, given that at least the Java driver would
> have a very difficult time implementing it (basically replacing Java's
> standard SSL stack with a completely external one, BouncyCastle, which adds
> load, size and complexity), I would rather focus the efforts on
> tls-server-end-point which only needs to access the certificate data,
> something that most APIs/frameworks/languages seem to cover much more
> naturally than tls-unique.

Point taken. Well, this brings us to the point where we should have
both tls-unique and end-point to allow JDBC to work with it. Now,
thinking about it, do we really need to advertise the available
channel binding types when the client chooses the -PLUS mechanism?
Wouldn't it make sense to let the client choose what it thinks is
better and just fail the exchange with the backend if the channel type
is not supported?

> Both are equally safe and effective and so having support for both seems
> sensible to me.

I have some automatic setups that would be really simplified by just
using libpq with SSL connections if there is channel binding. And that
involves certificate deployments, I think I am not the only one to see
that present even if JDBC just supports end-point.
-- 
Michael


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Robert Haas]

On Tue, May 30, 2017 at 11:49 PM, Stephen Frost  wrote:
> ... and I don't believe that we should be asking the
> implementors of channel binding to also implement support for multiple
> TLS libraries in PostgreSQL in order to test that their RFC-following
> (at least, as far as they can tell) implementation actually works.

You're of course free to believe what you wish, but that sounds
short-sighted to me.  If we implement channel binding and it turns out
not to be interoperable with other SSL implementations, then what?  We
can't change it later without breaking compatibility with our own
prior implementation.  Note that Álvaro Hernández Tortosa said about
two hours before you sent this email that it doesn't seem possible to
implement something comparable in Java's standard SSL stack.  If
that's the case, adopting this implementation is dooming everyone who
connects to the database server using JDBC to be unable to use channel
binding.  And that's a large percentage of our user base.

And then what happens when we do add support for Windows SSL or macOS
SSL?  It sounds like you're equally willing to throw people using
those implementations under the bus.  So we'll end up with channel
binding that works only when everybody's running the same operating
system and nobody's using Java.  Uggh.  At that point you might as
well just go back to using SSL certificates to solve this problem.  If
you use SSL certificates, then (1) it should work with any SSL
implementation and (2) the client can force SSL certificate
verification, whereas currently the client cannot force even the use
of SCRAM, let alone channel binding.

So what is being proposed here does not (yet, anyway) provide any
actual security and seems to have poor prospects for interoperability,
whereas we have an existing feature (SSL certificates) that has
neither of those problems.  Are you sure you're not putting
buzzword-compliance ahead of real progress?

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


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> but without actual interoperability testing it sounds pretty
> speculative to me.

I'm all for interoperability testing.

When we have multiple implementations of TLS using different libraries
with various versions of PostgreSQL and libpq and are able to test those
against other versions of PostgreSQL and libpq compiled with other TLS
libraries, I'll be downright ecstatic.  We are a small ways from that
right now, however, and I don't believe that we should be asking the
implementors of channel binding to also implement support for multiple
TLS libraries in PostgreSQL in order to test that their RFC-following
(at least, as far as they can tell) implementation actually works.

I'm not exactly sure what to characterize that as, given that the old
fall-back of "feature creep" feels woefully inadequate as a description.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Álvaro Hernández Tortosa]

On 31/05/17 03:39, Michael Paquier wrote:

On Tue, May 30, 2017 at 5:59 PM, Robert Haas  wrote:

That sounds like undue optimism to me.  Unless somebody's tested that
Michael's proposed implementation, which uses undocumented OpenSSL
APIs, actually interoperates properly with a SCRAM + channel binding
implementation based on some other underlying SSL implementation, we
can't really know that it's going to work.  It's not like we're
calling SSL_do_the_right_thing_for_channel_binding_thing_per_rfc5929().
We're calling SSL_do_something_undocumented() and hoping that
something_undocumented ==
the_right_thing_for_channel_binding_thing_per_rfc5929.  Could be true,
but without actual interoperability testing it sounds pretty
speculative to me.

Hm? Python is using that stuff for a certain amount of years now, for
the same goal of providing channel binding for SSL authentication. You
can look at this thread:
https://bugs.python.org/issue12551
So qualifying that as a speculative method sounds a quite hard word to me.


Actually this Python patch seems to ultimately rely on the same 
OpenSSL functions as your implementation.


I don't have anything against implementing tls-unique, specially as 
per the RFC it is mandatory (if channel binding support is provided). 
However, given the use of undocumented API, given that at least the Java 
driver would have a very difficult time implementing it (basically 
replacing Java's standard SSL stack with a completely external one, 
BouncyCastle, which adds load, size and complexity), I would rather 
focus the efforts on tls-server-end-point which only needs to access the 
certificate data, something that most APIs/frameworks/languages seem to 
cover much more naturally than tls-unique.


Both are equally safe and effective and so having support for both 
seems sensible to me.



Álvaro

--

Álvaro Hernández Tortosa


---
<8K>data



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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Tue, May 30, 2017 at 5:59 PM, Robert Haas  wrote:
> That sounds like undue optimism to me.  Unless somebody's tested that
> Michael's proposed implementation, which uses undocumented OpenSSL
> APIs, actually interoperates properly with a SCRAM + channel binding
> implementation based on some other underlying SSL implementation, we
> can't really know that it's going to work.  It's not like we're
> calling SSL_do_the_right_thing_for_channel_binding_thing_per_rfc5929().
> We're calling SSL_do_something_undocumented() and hoping that
> something_undocumented ==
> the_right_thing_for_channel_binding_thing_per_rfc5929.  Could be true,
> but without actual interoperability testing it sounds pretty
> speculative to me.

Hm? Python is using that stuff for a certain amount of years now, for
the same goal of providing channel binding for SSL authentication. You
can look at this thread:
https://bugs.python.org/issue12551
So qualifying that as a speculative method sounds a quite hard word to me.
-- 
Michael


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Robert Haas]

On Tue, May 30, 2017 at 1:00 PM, Stephen Frost  wrote:
> All-in-all, this sounds like it's heading in the right direction, at
> least at a high level.  Glad to see that there's been consideration of
> other TLS implementations, and as such I don't think we need to be
> overly concerned about the specifics of the OpenSSL API here.

That sounds like undue optimism to me.  Unless somebody's tested that
Michael's proposed implementation, which uses undocumented OpenSSL
APIs, actually interoperates properly with a SCRAM + channel binding
implementation based on some other underlying SSL implementation, we
can't really know that it's going to work.  It's not like we're
calling SSL_do_the_right_thing_for_channel_binding_thing_per_rfc5929().
We're calling SSL_do_something_undocumented() and hoping that
something_undocumented ==
the_right_thing_for_channel_binding_thing_per_rfc5929.  Could be true,
but without actual interoperability testing it sounds pretty
speculative to me.

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


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Daniel Gustafsson]

> On 30 May 2017, at 16:50, Tom Lane  wrote:
> 
> Robert Haas  writes:
>> On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
>>  wrote:
>>> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
>>> small flag about the stability and future of those APIs.
> 
>> It seems to me that the question is not just whether those APIs will
>> be available in future versions of OpenSSL, but whether they will be
>> available in every current and future version of every SSL
>> implementation that we may wish to use in core or that any client may
>> wish to use.  We've talked before about being able to use the Windows
>> native SSL implementation rather than OpenSSL and it seems that there
>> would be significant advantages in having that capability.
> 
> Another thing of the same sort that should be on our radar is making
> use of Apple's TLS code on macOS.  The handwriting on the wall is
> unmistakable that they intend to stop shipping OpenSSL before long,
> and I do not think we really want to be in a position of having to
> bundle OpenSSL into our distribution on macOS.
> 
> I'm not volunteering to do that, mind you.  But +1 for not tying new
> features to any single TLS implementation.

Big +1.  The few settings we have already make it hard to provide other
implementations as drop-in replacements (Secure Transport doesn’t support
.crl files for example, only CRL loaded in Keychains).

cheers ./daniel

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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Daniel Gustafsson]

> On 30 May 2017, at 18:25, Michael Paquier  wrote:
> 
> On macos though it is another story, I am not seeing anything:
> https://developer.apple.com/reference/security/secure_transport#symbols

The Secure Transport documentation unfortunately excel at being incomplete and
hard to search in.  That being said, I think you’re right, AFAICT there is no
published API for this (SSLProcessFinished() seems like the best match but is
not public).

> Depending on the SSL implementation the server is compiled with, it
> will be up to the backend to decide if it should advertise to the
> client the -PLUS mechanism or not, so we can stay modular here.

+1

cheers ./daniel

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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Tue, May 30, 2017 at 8:14 AM, Stephen Frost  wrote:
> > The specific APIs are, certainly, going to be different between
> > different TLS implementations and I don't believe we need to be
> > particularly concerned with that (or if they change in the future), as
> > long as we abstract those APIs.  Perhaps there's some question as to
> > just how to abstract them, but I'm at least a bit less concerned with
> > that as I expect we'll be able to adjust those abstraction layers later
> > (presuming they aren't exposed, which I wouldn't expect them to be).
> 
> The current patch fetches the TLS finish message data just after the
> handshake in be_tls_open_server() using the dedicated OpenSSL API. We
> could definitely go with a more generic routine on our side that
> sticks with the concepts of be_tls_get_compression():
> - For the backend, this routine returns an allocated string with the
> contents of the expected TLS message, and its size:
> char *be_tls_get_tls_finish(Port *, int *)
> - For the frontend, a routine to get the generated TLS finish message:
> char *pgtls_get_tls_finish(PGconn *, int *)

Those look pretty reasonable to me and seem to go along with the
concepts from the RFC, making them hopefully the right API.

> > If that wasn't the case then, for example, it wouldn't be possible to do
> > channel binding with a LibNSS client and an OpenSSL server or with other
> > combinations and I find that rather hard to believe.
> 
> As far as I can see, Windows has some APIs to get the TLS finish message:
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa382420(v=vs.85).aspx

Good.

> On macos though it is another story, I am not seeing anything:
> https://developer.apple.com/reference/security/secure_transport#symbols

That's a bit unfortunate, if that's the case.  Perhaps Apple will see
fit to address that deficiency though.

> Depending on the SSL implementation the server is compiled with, it
> will be up to the backend to decide if it should advertise to the
> client the -PLUS mechanism or not, so we can stay modular here.

Right, of course.

All-in-all, this sounds like it's heading in the right direction, at
least at a high level.  Glad to see that there's been consideration of
other TLS implementations, and as such I don't think we need to be
overly concerned about the specifics of the OpenSSL API here.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Tue, May 30, 2017 at 8:14 AM, Stephen Frost  wrote:
> Work has been done in that area already, as well as for LibNSS support.
>
> I've not had a chance to closely look over the proposed approach here,
> but generally speaking, we need to be looking for a standard way to
> generate and transmit the channel binding information across the
> wire.

The RFC of SCRAM specifies that the client sends the type of channel
binding in its first message data, and then provides the contents of
the TLS message it generated in the challenge response.

> The specific APIs are, certainly, going to be different between
> different TLS implementations and I don't believe we need to be
> particularly concerned with that (or if they change in the future), as
> long as we abstract those APIs.  Perhaps there's some question as to
> just how to abstract them, but I'm at least a bit less concerned with
> that as I expect we'll be able to adjust those abstraction layers later
> (presuming they aren't exposed, which I wouldn't expect them to be).

The current patch fetches the TLS finish message data just after the
handshake in be_tls_open_server() using the dedicated OpenSSL API. We
could definitely go with a more generic routine on our side that
sticks with the concepts of be_tls_get_compression():
- For the backend, this routine returns an allocated string with the
contents of the expected TLS message, and its size:
char *be_tls_get_tls_finish(Port *, int *)
- For the frontend, a routine to get the generated TLS finish message:
char *pgtls_get_tls_finish(PGconn *, int *)

> I skimmed over RFC5929, which looks to be the correct RFC when it comes
> to channel binding with TLS.  Hopefully the various TLS implementations
> work in a similar manner, following that RFC (or whichever is relevant).

That's the one I use as a reference.

> If that wasn't the case then, for example, it wouldn't be possible to do
> channel binding with a LibNSS client and an OpenSSL server or with other
> combinations and I find that rather hard to believe.

As far as I can see, Windows has some APIs to get the TLS finish message:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa382420(v=vs.85).aspx
On macos though it is another story, I am not seeing anything:
https://developer.apple.com/reference/security/secure_transport#symbols

Depending on the SSL implementation the server is compiled with, it
will be up to the backend to decide if it should advertise to the
client the -PLUS mechanism or not, so we can stay modular here.
-- 
Michael


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Stephen Frost]

Tom, all,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
> >  wrote:
> >> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
> >> small flag about the stability and future of those APIs.
> 
> > It seems to me that the question is not just whether those APIs will
> > be available in future versions of OpenSSL, but whether they will be
> > available in every current and future version of every SSL
> > implementation that we may wish to use in core or that any client may
> > wish to use.  We've talked before about being able to use the Windows
> > native SSL implementation rather than OpenSSL and it seems that there
> > would be significant advantages in having that capability.
> 
> Another thing of the same sort that should be on our radar is making
> use of Apple's TLS code on macOS.  The handwriting on the wall is
> unmistakable that they intend to stop shipping OpenSSL before long,
> and I do not think we really want to be in a position of having to
> bundle OpenSSL into our distribution on macOS.
> 
> I'm not volunteering to do that, mind you.  But +1 for not tying new
> features to any single TLS implementation.

Work has been done in that area already, as well as for LibNSS support.

I've not had a chance to closely look over the proposed approach here,
but generally speaking, we need to be looking for a standard way to
generate and transmit the channel binding information across the
wire.  The specific APIs are, certainly, going to be different between
different TLS implementations and I don't believe we need to be
particularly concerned with that (or if they change in the future), as
long as we abstract those APIs.  Perhaps there's some question as to
just how to abstract them, but I'm at least a bit less concerned with
that as I expect we'll be able to adjust those abstraction layers later
(presuming they aren't exposed, which I wouldn't expect them to be).

I skimmed over RFC5929, which looks to be the correct RFC when it comes
to channel binding with TLS.  Hopefully the various TLS implementations
work in a similar manner, following that RFC (or whichever is relevant).

If that wasn't the case then, for example, it wouldn't be possible to do
channel binding with a LibNSS client and an OpenSSL server or with other
combinations and I find that rather hard to believe.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Tom Lane]

Robert Haas  writes:
> On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
>  wrote:
>> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
>> small flag about the stability and future of those APIs.

> It seems to me that the question is not just whether those APIs will
> be available in future versions of OpenSSL, but whether they will be
> available in every current and future version of every SSL
> implementation that we may wish to use in core or that any client may
> wish to use.  We've talked before about being able to use the Windows
> native SSL implementation rather than OpenSSL and it seems that there
> would be significant advantages in having that capability.

Another thing of the same sort that should be on our radar is making
use of Apple's TLS code on macOS.  The handwriting on the wall is
unmistakable that they intend to stop shipping OpenSSL before long,
and I do not think we really want to be in a position of having to
bundle OpenSSL into our distribution on macOS.

I'm not volunteering to do that, mind you.  But +1 for not tying new
features to any single TLS implementation.

regards, tom lane


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
 wrote:
> On 25/05/17 17:17, Michael Paquier wrote:
>> Please find attached a patch to add support for channel binding for
>> SCRAM, to mitigate MITM attacks using this protocol. Per RFC5802
>> (https://tools.ietf.org/html/rfc5802), servers supporting channel
>> binding need to add support for tls-unique, and this is what this
>> patch does.
>
> This is awesome, Michael :) Thank you! You have prevented me eventually
> writing the patch and having then PostgreSQL source tainted with Java-to-C
> "transpiled" code ^_^

The thing would have been reviewed and rewritten a couple of times :)
So that would have unlikely (hopefully) finished in the shape of a
Java-C-like patch.

> After a deeper analysis, I have some concerns/comments here:
>
> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
> small flag about the stability and future of those APIs.

Those are 5 lines of code each in OpenSSL, not that hard to maintain.
Those APIs are clunky by the way as there is no way to know in advance
the length of the message for memory allocation.

> - More importantly, some drivers (not libpq-based) may have unexpected
> difficulties implementing tls-unique. In particular, there is not an API in
> Java to get the finished message. I expect other languages to maybe have
> similar limitations. For Java I see two options:
> * Also implement tls-server-end-point, which rather relies on the server
> certificate. This information seems to be exposed as part of the Java APIs:
> https://docs.oracle.com/javase/8/docs/api/java/security/cert/Certificate.html#getEncoded--
> * Use the BouncyCastle library (http://bouncycastle.org/), which
> explicitly supports tls-unique
> (https://www.bouncycastle.org/docs/pkixdocs1.5on/org/bouncycastle/est/TLSUniqueProvider.html#getTLSUnique()
> ,
> https://www.bouncycastle.org/docs/tlsdocs1.5on/org/bouncycastle/tls/ChannelBinding.html#tls_unique
> ). This would require, however, non-trivial changes to the driver and, I
> expect, a lot of extra effort.

I am wondering how other languages are regarding that. Python has
added a method in 2011:
https://hg.python.org/cpython/rev/cb44fef5ea1d

> - It seems to me that tls-unique might be a bit fragile. In particular, it
> requires the server to be aware of TSL renegotiations and avoid performing
> one while the SCRAM authentication is being performed. I don't know if this
> is already guaranteed by the current patch, but it seems to me it requires
> complex interactions between levels of abstraction that are quite separate
> (SSL and SCRAM). This is explained by the RFC:

RFC 5802, section 6 gives a good level of details on the matter:
https://tools.ietf.org/html/rfc5802#section-6
There is indeed interaction between SSL and scram, and the patch makes
use of USE_SSL for this purpose.

> "
> server applications MUST NOT request TLS renegotiation during phases of the
> application protocol during which application-layer authentication occurs
> "
> (https://tools.ietf.org/html/rfc5929#section-3.1)

The SSL hanshake happens only once at connection obtention, before the
authentication exchange happens. So there is no renegociation. SSL
renegotiation has been removed in PG anyway, disabled on
back-branches, and removed as well in TLS 1.3 (right? I don't recall
the spec in details).

> Based on this facts, I'd suggest to either implement
> tls-server-end-point or implement both tls-server-end-point and tls-unique.
> The latter would require a more complete protocol message to advertise
> separately SCRAM mechanisms and channel binding names. One of such
> structures could be the one I suggested earlier:
> https://www.postgresql.org/message-id/df8c6e27-4d8e-5281-96e5-131a4e638...@8kdata.com

The RFC says that any server implementing channel binding must
implement tls-unique, so having only end-point is not compliant.

>> Before even that, the server needs to send to the client the list of
>> SASL mechanisms that are supported. This adds SCRAM-SHA-256-PLUS if
>> the server has been built with SSL support to the list.
>
> And I'd say the list of channel binding names supported.

 It seems to me as well that this gives us an indication that it
should be the default channel binding used, or if the exchange list
has no channel names included, the client can assume that tls-unique
will be used. Such an approach has as well the merit to keep the patch
simple. In short, I would advocate an incremental approach that adds
tls-unique first. This has value anyway as there is no need for
certificate configuration. This also does not require a message format
extension.
-- 
Michael


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