Re: [HACKERS] Letting the client choose the protocol to use during aSASL exchange - Mailing list pgsql-hackers

On 11/04/17 12:23, Heikki Linnakangas wrote:
> On 04/11/2017 11:55 AM, Álvaro Hernández Tortosa wrote:
>> On 11/04/17 08:50, Heikki Linnakangas wrote:
>>> Oh, I see. According to the SCRAM RFC, "tls-unique" is used by
>>> default. I don't see us implementing anything else any time soon.
>>> PostgreSQL doesn't support any other "channel type" than TLS, and
>>> tls-unique is the only one that makes sense for TLS.
>>>
>>> If we do need it after all, the server could advertise the additional
>>> channel binding types as additional SASL mechanisms in the
>>> AuthenticationSASL message, maybe something like:
>>>
>>> "SCRAM-SHA-256"
>>> "SCRAM-SHA-256-PLUS" (for tls-unique)
>>> "SCRAM-SHA-256-PLUS ssh-unique" (for hypothetical ssh channel binding)
>>>
>>> The same trick can be used to advertise any other SASL mechanism
>>> specific options, if needed in the future.
>>
>>      Why not add an extra field to the message? This scheme has in my
>> opinion some disadvantages:
>>
>> - You assume no extensibility. Maybe Postgres will implement other
>> mechanisms for channel binding. Maybe not. But why limit ourselves?
>> - Apart from tls-unique there are others today, like
>> tls-server-end-point and who knows if in the future TLS 1.x comes with
>> something like 'tls-unique-1.x'
>> - Why have to parse the string (separated by spaces) when you could use
>> different fields and have no parsing at all?
>
> I don't think an option separated by space is any more difficult to 
> parse than a separate field. I'm envisioning that the "parsing" would 
> be simply:
>
> if (strcmp(sasl_mechanism, "SCRAM-SHA-256") == 0) { ... }
> else if (strcmp(sasl_mechanism, "SCRAM-SHA-256-PLUS") == 0) { ... }
> else if (strcmp(sasl_mechanism, "SCRAM-SHA-256-PLUS tls-awesome") == 
> 0) { ... }
>
> This can be extended for more complicated options, if necessary. 
> Although if we find that we need a dozen different options, I think 
> we've done something wrong.
>
>> - How do you advertise different SCRAM mechanisms with different channel
>> binding types? And a mix of SCRAM mechanisms with and without channel
>> binding? If I got it right, with your proposal it would be something 
>> like:
>>
>> Field 1: SCRAM-SHA-256,SCRAM-SHA-256-PLUS tls-unique,SCRAM-SHA-1-PLUS
>> tls-unique,SCRAM-SHA-512-PLUS tls-unique
>>
>> (which is basically a CSV of pairs where the right part of the pair
>> might be empty; too much IMHO for a single field)
>
> Yes, except that in my proposal, the list is not a comma-separated 
> string, but a list of null-terminated strings, similar to how some 
> other lists of options in the FE/BE protocol are transmitted.
    The fact that you null terminate them (fine with me) doesn't change 
my reasoning. How do you separate multiple channel binding methods? And 
do you realize that you will be repeating the channel binding methods 
without reason? A contrived but legal, possible example:

Field1:
SCRAM-SHA-256\0
SCRAM-SHA-512\0
SCRAM-SHA-999\0
SCRAM-SHA-256-PLUS tls-unique tls-awesome yet-another-tls\0
SCRAM-SHA-512-PLUS tls-unique tls-awesome yet-another-tls\0
SCRAM-SHA-999-PLUS tls-unique tls-awesome yet-another-tls\0

vs

Field 1:
SCRAM-SHA-256 SCRAM-SHA-512 SCRAM-SHA-999 SCRAM-SHA-256-PLUS 
SCRAM-SHA-512-PLUS SCRAM-SHA-999-PLUS\0

Field 2:
tls-unique tls-awesome yet-another-tls\0

    Parsing and validation of the first option is significantly more 
complex than the second option.

>
>> vs
>>
>> Field 1:
>> SCRAM-SHA-256,SCRAM-SHA-256-PLUS,SCRAM-SHA-1-PLUS,SCRAM-SHA-512-PLUS
>> (simple CSV)
>> Field 2: tls-unique (String)
>
> What if tls-unique is only supported with SCRAM-SHA-256-PLUS, while 
> SCRAM-SHA-512-PLUS requires tls-awesome? 
    It can't happen. The RFC clearly states that they are orthogonal. 
It is left to the implementations support one or the other, but no 
reason to limit applicability of a given binding method to a given SCRAM 
mechanisms (or viceversa).


> What about possible other options you might need to tack on to 
> mechanisms? This seems less flexible. I'm not saying that either of 
> those cases is very likely, but I don't think it's very likely we'll 
> need extra options or different channel binding types any time soon, 
> anyway.
    As I said, channel binding and SCRAM mechanisms are orthogonal.
    On the flip side, a contrived parsing mechanism with optional 
fields and many that would be repeated all the time seems far from 
ideal. And far less clear.


>
>> Is there any reason not to use two fields? AuthenticationSASL is a
>> new message, I guess we can freely choose its format, right?
>
> Yes, we can choose the format freely.
    Cool.

>
> In summary, I think the simple list of mechanism names is best, because:
>
> * It's simple, and doesn't have any extra fields or options that are 
> not needed right now.
    I think it is too complicated and mixing things that are 
orthogonal. Since protocol is hard to extend, adding an extra field for 
the channel binding mechanisms -even if unused as of PG 10- is a good 
thing. If you need to change the protocol later, that's a very bad thing 
(realistically, we all know it won't be changed and some hack would need 
to be implemented).

> * It's flexible, for future extension. We can add new mechanism 
> entries with extra options, not just new channel binding types, if 
> needed, and existing clients (i.e. v10 clients) will ignore them.
    I think the extra field allows for much more extension possibilities.

    Having said all this, choice is yours :)



>
>>> Perhaps we should reserve a magic user name to mean "same as startup
>>> message", in addition or instead of the empty string. We actually
>>> discussed that already at [1], but we forgot about it since.
>>
>> That works. Please let me know what is the "magic constant" chosen ;P
>
> I was hoping you'd have some good suggestions :-). Unfortunately there 
> is no reserved username prefix or anything like that, so whatever we 
> choose might also be a real username. That's OK, but it could be 
> confusing, if we ever tried to do something different with the SASL 
> username. How about "pg_same_as_startup_message"?
    It doesn't matter if it conflicts with a real name, since it will 
be ignored anyway ^_^ So I'd either pick a short string for saving a few 
bytes (something like "*") or something that looks cool like a function 
name like "pg.get_from_startup_message()" ^_^    (I prefer the former)

    Álvaro

-- 

Álvaro Hernández Tortosa


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