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

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


Field 1:

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.


Field 1:
(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.


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 Hernández Tortosa


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

Reply via email to