Bug 14003

We have an interface for passwords on channels. You can use this to join password protected chat rooms.

http://telepathy.freedesktop.org/spec.html#org.freedesktop.Telepathy.Channel.Interface.Password

Being able to ask for the password after connecting is nice, but I'm more interested in tackling authentication of the server to the client.

Linking a related request for Telepathy-SofiaSIP:
https://sourceforge.net/tracker/?func=detail&atid=756079&aid=1922988&group_id=143636

This is also needed for using non-password-based auth when signing into XMPP (such as the X-GOOGLE-TOKEN SASL mechanism) and presumably for Kerberos support.

Some Empathy users also requested to be able to configure an account whithout saving the password in MC. In such cases, Empathy should request it when trying to connect. See https://bugzilla.gnome.org/show_bug.cgi?id=586562

Eitan, could you keep this bug vaguely up to date with your progress please?

The current proposals appear to be:

http://git.collabora.co.uk/?p=user/eitan/telepathy-spec.git;a=shortlog;h=refs/heads/authentication

http://people.freedesktop.org/~eitani/telepathy-spec-authentication/spec/

http://lists.freedesktop.org/archives/telepathy/2010-March/004307.html

(am I right?)

(In reply to comment #5)
> Eitan, could you keep this bug vaguely up to date with your progress please?
> 

I'll do my best. There is a Wocky branch and Gabble implementation too:

http://git.collabora.co.uk/?p=user/eitan/wocky.git;a=shortlog;h=refs/heads/sasl

http://git.collabora.co.uk/?p=user/eitan/telepathy-gabble.git;a=shortlog;h=refs/heads/sasl

To address the ever-recurring concern with sending plaintext passwords over D-Bus, could CMs that implement TP_PLAINTEXT be advised to implement another mechanism that wraps the password into a secure communication session using, say, Diffie-Hellman key exchange?

(In reply to comment #7)
> To address the ever-recurring concern with sending plaintext passwords over
> D-Bus, could CMs that implement TP_PLAINTEXT be advised to implement another
> mechanism that wraps the password into a secure communication session using,
> say, Diffie-Hellman key exchange?

This sounds good to me. I noticed that the Shared Secrets D-Bus API (as now implemented by Gnome Keyring) does the same: http://people.gnome.org/~stefw/secrets/html/ch06.html

Eitan has a newer branch at: http://git.collabora.co.uk/?p=user/eitan/telepathy-spec.git;a=shortlog;h=refs/heads/sasl-once-again

I pushed some editorial fixes to <http://git.collabora.co.uk/?p=user/wjt/telepathy-spec-wjt.git;a=shortlog;h=refs/heads/sasl-once-again>.

Is there any particular reason why “Sasl” is used rather than “SASL” throughout?

Functionally it looks sane.

2c18994fca484447877892aac9ba9b1553a2b331

commit messages misspells my name quite brutally :(

Other then that things are starting to look nice (no surprise coming from me). 

As a nitpick, the AuthenticationInformation property should probably document it's using the Auth_Details mapping. Also we should define the X-TP-PLAINTEXT/X-TP-PASSWORD SASL mechanisms reasonably soon (as that's the fallback path for handlers not implementating any/specific SASL mechansisms).. I don't consider that to necessarily be a blocker for merging this as a Draft though (but documenting it should be trivial :p)

(In reply to comment #10)
> I pushed some editorial fixes to
> <http://git.collabora.co.uk/?p=user/wjt/telepathy-spec-wjt.git;a=shortlog;h=refs/heads/sasl-once-again>.

Thanks, I cherry picked your commit.

> 
> Is there any particular reason why “Sasl” is used rather than “SASL”
> throughout?
> 

It's camel-cased where required, all other references should be SASL.

> Functionally it looks sane.

I added the Secure property as per our discussion on bug 27645.

(In reply to comment #11)
> 2c18994fca484447877892aac9ba9b1553a2b331
> 
> commit messages misspells my name quite brutally :(
> 

Pardon my bigotry, I fixed it.

> Other then that things are starting to look nice (no surprise coming from me). 
> 
> As a nitpick, the AuthenticationInformation property should probably document
> it's using the Auth_Details mapping.

Added (877bd80)

> Also we should define the
> X-TP-PLAINTEXT/X-TP-PASSWORD SASL mechanisms reasonably soon (as that's the
> fallback path for handlers not implementating any/specific SASL mechansisms)..
> I don't consider that to necessarily be a blocker for merging this as a Draft
> though (but documenting it should be trivial :p)

What is the difference between the two X-TP-* mechanisms? This isn't actually implemented yet in Gabble, fyi.

(If you want review, please set the patch keyword and reference the relevant branch prominently (URL field is good). Reviewing now.)

> +  <interface name="org.freedesktop.Telepathy.Channel.Interface.Sasl.Authentication.DRAFT" tp:causes-havoc="experimental">

I think "SASLAuthentication" would be better than "SaslAuthentication"; that matches what we do for other acronymful interfaces (DTMF and TLS).

> +<node name="/Channel_Interface_Sasl_Authentication"

Channel_Interface_SASL_Authentication (and rename the file accordingly), please. That would mean it comes out as ChannelInterfaceSASLAuthentication, channel_interface_sasl_authentication, etc. in various case conventions.

I don't think http://people.freedesktop.org/~eitani/telepathy-spec-authentication/spec/ is still the right HTML (it seems to have rather different content). Could you upload HTML, please?

I haven't reviewed the general structure in detail yet.

Eitan: any progress on this? Does someone else need to pick this up?

(In reply to comment #16)
> Eitan: any progress on this? Does someone else need to pick this up?

Simon,

This API has already been reviewed and has several implementations (both in CM and UI) out in the wild.

It needs to be merged, probably as a draft for now. Not sure how this fell between the cracks.

The implementation in Gabble seems to be functionally identical to commit 877bd80fde. Stealing this branch to work on it myself.

Branch and HTML, with s/Sasl/SASL/ and no further changes so far:

http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/sasl

http://people.freedesktop.org/~smcv/telepathy-spec-sasl/spec/org.freedesktop.Telepathy.Channel.Type.ServerAuthentication.DRAFT.html

http://people.freedesktop.org/~smcv/telepathy-spec-sasl/spec/org.freedesktop.Telepathy.Channel.Interface.SASLAuthentication.DRAFT.html

ServerAuthentication
====================

ServerAuthentication could do with a high-level explanation of how it happens:

- When is it dispatched? I infer that it's while the Connection is CONNECTING, but that should be stated up-front.

- What properties can it be expected to have? Most of the text from ServerTLSConnection is probably applicable.

- Can you have more than one ServerAuthentication in series? The SASL RFC says you can't have more than one SASL negotiation, if I'm reading it correctly, but you could conceivably have a round of SASL followed by a round of captcha, or vice versa. If you can, would it mean "at least one must succeed" or "all must succeed"?

- Can you have more than one ServerAuthentication in parallel? If you can, would it mean "at least one must succeed" or "all must succeed"?

Are the username and realm in Auth_Details the same as SASL RFC 4422's "authorization identity strings" or are they intended as something to put in the UI presented to the user or what?

Please note the distinction between authentication and authorization identities. A non-empty, non-omitted authorization identity means I want to authenticate with one identity (authentication identity), then act as a different identity. For instance, that could mean "authenticate as root, using your root powers to impersonate eeejay without knowing his password", or conversely, it could mean "the sysadmins can authenticate with their own identities, and are authorized to impersonate announcements@example.com when they do so". We don't necessarily need to support that, but we should get the terminology right so we can support it later if desired.

SASL
====

Abort_Reason is too vague a name - types are a flat namespace. It should be SASL_Abort_Reason.

StartMechanism can't distinguish between no initial data, and initial data of zero length. (Not as stupid as it sounds: RFC 4422 §4(3)(a) requires these to be distinct.)

We don't have a way to emit an 'ay' of additional data with a successful outcome (i.e. carried on a successful StateChanged), which is a SHOULD in RFC 4422 §4(3)(c). Again, "no additional data" is a value distinct from the zero-length string.

Is AvailableMechanisms immutable? (My guess: yes.)

Is Secure immutable? (My guess: yes.)

What is CurrentChallenge when there is no current challenge? (This is not as stupid as it sounds: the empty octet-string is a valid challenge.)

We should document that only the Handler may call StartMechanism.

Do we really need CurrentChallenge at all? Couldn't we just say that the Handler can't call StartMechanism until it has connected to the NewChallenge signal, and it makes no sense for anyone else to call that method?

It's not immediately obvious what the state transitions are; I'll try to dig that out from Gabble.

There are no tp:possible-errors anywhere, which seems like an oversight.

I would assume that:

- Respond() is only valid to be called while In_Progress; similarly, NewChallenge makes no sense unless In_Progress

- Abort() can be called at any time, except when Server_Failed or Client_Failed when it would have no effect (or can it be called in Server_Failed to convert it into Client_Failed? answers on a postcard)

- Close() can be called at any time, and has the same effect as calling Abort() with errors ignored, and then Close()

Can StartMechanism be called in In_Progress, Server_Succeeded, Client_Accepted and Succeeded states, to restart authentication gratuitously?

Why does Abort() take an Abort_Reason, but SASL_State has a DBus_Error_Name? Shouldn't they both have the same (perhaps both)?

We should perhaps write to IANA and register X-GOOGLE-TOKEN as a SASL mechanism, since Google have failed to do so (if I read the RFC correctly, there's no particular provision for "X-" SASL mechanisms, but anyone can register a SASL mechanism, and no particular approval is required).

Do we need a way for the authenticator to get the server's GSSAPI host-based service name (RFC 4422 §4(1))? I expect that that'd be a constant string + "@" + the hostname, for basically every protocol we support.

(In reply to comment #20)
> ServerAuthentication
> ====================
> 
> ServerAuthentication could do with a high-level explanation of how it happens:
> 
> - When is it dispatched? I infer that it's while the Connection is CONNECTING,
> but that should be stated up-front.

Go for it

> - What properties can it be expected to have? Most of the text from
> ServerTLSConnection is probably applicable.

? 

> - Can you have more than one ServerAuthentication in series? The SASL RFC says
> you can't have more than one SASL negotiation, if I'm reading it correctly, but
> you could conceivably have a round of SASL followed by a round of captcha, or
> vice versa. If you can, would it mean "at least one must succeed" or "all must
> succeed"?
>
> - Can you have more than one ServerAuthentication in parallel? If you can,
> would it mean "at least one must succeed" or "all must succeed"?

You can have multiple ServerAuthentication in series, you must defeat them all to get the prize. I haven't seen a use-case in practise for multiple alternate paths, for now the CM chooses your path to the maze and it's up to you to defeat the challenges.

> Are the username and realm in Auth_Details the same as SASL RFC 4422's
> "authorization identity strings" or are they intended as something to put in
> the UI presented to the user or what?
>
> Please note the distinction between authentication and authorization
> identities. A non-empty, non-omitted authorization identity means I want to
> authenticate with one identity (authentication identity), then act as a
> different identity. For instance, that could mean "authenticate as root, using
> your root powers to impersonate eeejay without knowing his password", or
> conversely, it could mean "the sysadmins can authenticate with their own
> identities, and are authorized to impersonate announcements@example.com when
> they do so". We don't necessarily need to support that, but we should get the
> terminology right so we can support it later if desired.

The property is called AuthenticationInformation for a reason. The CM gives you the default/likely authentication identities, the UI can choose a different authorization identity if it so chooses. I probably wouldn't put those in a default UI though (maybe only when you toggle and advanced drop-down) as it's rarely used in practise.

for a specification of realm we might want to refer to the definition in 
rfc2831 (specifically the CM computes a default realm to be used)

> SASL
> ====
> 
> Abort_Reason is too vague a name - types are a flat namespace. It should be
> SASL_Abort_Reason.
++

> StartMechanism can't distinguish between no initial data, and initial data of
> zero length. (Not as stupid as it sounds: RFC 4422 §4(3)(a) requires these to
> be distinct.)

I've not seen an mechanism yet where you can meaningfully start with an empty initial data (e.g. zero length). But i agree that potentially supporting that could be useful, not having a maybe type in D-Bus strikes again though... Your choice for either adding a boolean or split of a StartMechanismWithData (i think i prefer two methods instead of a silly boolean)

> We don't have a way to emit an 'ay' of additional data with a successful
> outcome (i.e. carried on a successful StateChanged), which is a SHOULD in RFC
> 4422 §4(3)(c). Again, "no additional data" is a value distinct from the
> zero-length string.

We do. The additional data is expected to be emitted as a last challenge to which the client responds with Accept (the CurrentChallenge property has some info about it, but it needs to be spelled out more)

> Is AvailableMechanisms immutable? (My guess: yes.)
> 
> Is Secure immutable? (My guess: yes.)

Seems i annoted that in my initial mails but it didn't make it into the spec:

http://www.mail-archive.com/telepathy@lists.freedesktop.org/msg04137.html
http://www.mail-archive.com/telepathy@lists.freedesktop.org/msg04138.html

> What is CurrentChallenge when there is no current challenge? (This is not as
> stupid as it sounds: the empty octet-string is a valid challenge.)

Like you say a bit later, CurrentChallenge is superflous, so i don't think that's an issue

> We should document that only the Handler may call StartMechanism.
Yup, we should, go go go :)

> Do we really need CurrentChallenge at all? Couldn't we just say that the
> Handler can't call StartMechanism until it has connected to the NewChallenge
> signal, and it makes no sense for anyone else to call that method?

Correct. If you care a lot about challenge with no data vs. zero lengt then you might want to add a boolean to that signal, but again i've yet to see a mechanism that actually makes a difference in this case.

> It's not immediately obvious what the state transitions are; I'll try to dig
> that out from Gabble.

Gabble doesn't have the complete set, in particular it can't restart a sasl try.

> There are no tp:possible-errors anywhere, which seems like an oversight.

go go go :)
 
> I would assume that:
> 
> - Respond() is only valid to be called while In_Progress; similarly,
> NewChallenge makes no sense unless In_Progress

Yup, the successfull sequences look like:

StartMechanisms,  => In_Progress, (Challenge, Respond)*,  [=> ServerSucceeded, Accept, => Succeeded || Challenge, Accept, => ClientSucceeded, => Succeeded]

> - Abort() can be called at any time, except when Server_Failed or Client_Failed
> when it would have no effect (or can it be called in Server_Failed to convert
> it into Client_Failed? answers on a postcard)

Yeah, no effect in the Failed cases, wouldn't make much sense (the server failed, well screw you, client failed as well, neh neh neh).

Doesn't really make sense to call abort in Succeeded or Client_Succeeded (You alreadh told the channel everything is fine from your perspective). So Abort can be usefully called in Not_Started (e.g. we don't support any of the sasl mechanisms), In_Progress (server fucked up or user pressed cancel) and Server_Succeeded (the server might claim we're done, but we don't agree)

> - Close() can be called at any time, and has the same effect as calling Abort()
> with errors ignored, and then Close()

Same effect as Abort indeed, accept when Abort doesn't make sense :)

> Can StartMechanism be called in In_Progress, Server_Succeeded, Client_Accepted
> and Succeeded states, to restart authentication gratuitously?

No, SASL_Status correctly documents when you can call StartMechanism. If you want to restart gratuitously you should first Abort and then StartMechanism. (I don't think StartMechanism implicitly calling Abort makes much sense and in general i don't expect restarting to happen unless the server failed us)

> Why does Abort() take an Abort_Reason, but SASL_State has a DBus_Error_Name?
> Shouldn't they both have the same (perhaps both)?

good point, potentially both indeed.

> We should perhaps write to IANA and register X-GOOGLE-TOKEN as a SASL
> mechanism, since Google have failed to do so (if I read the RFC correctly,
> there's no particular provision for "X-" SASL mechanisms, but anyone can
> register a SASL mechanism, and no particular approval is required).

Unless you want to register all custom SASL mechanisms that doesn't make much sense. Even if the RFC doesn't have a provision for it, the X- stuff seems to be the normal practise. It's not up to us to fix the world :)

> Do we need a way for the authenticator to get the server's GSSAPI host-based
> service name (RFC 4422 §4(1))? I expect that that'd be a constant string + "@"
> + the hostname, for basically every protocol we support.

Could be added to the AuthenticationInformation dict i guess?


The one thing i'd like to add to the spec is a mandatory to implement X-TELEPATHY-PASSWORD mechanism, which is a simple mechanism with initial-data '\0user\0password' (basically SASL PLAIN (RFC 4616). When this mechanism is used it's expected that the CM handles the actual authentication (basically as if there was a password in the account information). Reasoning here is that it would allow us to never store a password in the account information and always use an authentication handler (without that handler needed to implement specific sasl mechanisms). (This way Empathy wouldn't need to implement any sasl mechanisms, just get the password from the keyring). Ofcourse if the CM doesn't recognize any of the SASL mechanisms, it doesn't have to expose it to the client (but that seems strange)

(In reply to comment #21)
> for a specification of realm we might want to refer to the definition in 
> rfc2831 (specifically the CM computes a default realm to be used)

Thanks, I'll have a look at that RFC.

> Your
> choice for either adding a boolean or split of a StartMechanismWithData (i
> think i prefer two methods instead of a silly boolean)

I agree with your preference.

> We do. The additional data is expected to be emitted as a last challenge to
> which the client responds with Accept (the CurrentChallenge property has some
> info about it, but it needs to be spelled out more)

Ah, good; a "road map" at the top would probably have made that clear. I'll have a go at writing one.

> > I would assume that:
> > 
> > - Respond() is only valid to be called while In_Progress; similarly,
> > NewChallenge makes no sense unless In_Progress
> 
> Yup, the successfull sequences look like:
> 
> StartMechanisms,  => In_Progress, (Challenge, Respond)*,  [=> ServerSucceeded,
> Accept, => Succeeded || Challenge, Accept, => ClientSucceeded, => Succeeded]

Does your markup here mean that either the part [...|| or the part ||...] happens?

This assumes that it's obvious from the definition of any mechanism that the last Challenge is not actually a challenge, but instead an additional-data blob (i.e. there's no time at which either a challenge or additional data would be equally valid). I haven't checked whether that's true.

> The one thing i'd like to add to the spec is a mandatory to implement
> X-TELEPATHY-PASSWORD mechanism, which is a simple mechanism with initial-data
> '\0user\0password' (basically SASL PLAIN (RFC 4616). When this mechanism is
> used it's expected that the CM handles the actual authentication (basically as
> if there was a password in the account information). Reasoning here is that it
> would allow us to never store a password in the account information and always
> use an authentication handler (without that handler needed to implement
> specific sasl mechanisms). (This way Empathy wouldn't need to implement any
> sasl mechanisms, just get the password from the keyring). Ofcourse if the CM
> doesn't recognize any of the SASL mechanisms, it doesn't have to expose it to
> the client (but that seems strange)

Yeah, I meant to ask about that. Mikhail suggests using D-H or something, like the fd.o secrets service does, either immediately or a bit later.

Is it true that the only distinction between X-TELEPATHY-PASSWORD and PLAIN is that the mechanism name implicitly tells the client who it's authenticating to - if it's X-TELEPATHY-PASSWORD then it's the CM, else it's the server?

It occurs to me that another way to do this would be to say in the AuthenticationInfo who we're authenticating with - it could be as simple as {"talking-to-cm": True} - and use an unmodified SASL mechanism, presumably PLAIN at first and some sort of D-H badgers later, to hand over credentials to it. This is how I'd assumed the "smart CM, dumb protocol" case would work.

(In reply to comment #22)
> (In reply to comment #21)
> > We do. The additional data is expected to be emitted as a last challenge to
> > which the client responds with Accept (the CurrentChallenge property has some
> > info about it, but it needs to be spelled out more)
> 
> Ah, good; a "road map" at the top would probably have made that clear. I'll
> have a go at writing one.
> 
> > > I would assume that:
> > > 
> > > - Respond() is only valid to be called while In_Progress; similarly,
> > > NewChallenge makes no sense unless In_Progress
> > 
> > Yup, the successfull sequences look like:
> > 
> > StartMechanisms,  => In_Progress, (Challenge, Respond)*,  [=> ServerSucceeded,
> > Accept, => Succeeded || Challenge, Accept, => ClientSucceeded, => Succeeded]
> 
> Does your markup here mean that either the part [...|| or the part ||...]
> happens?

Yes. The difference is in whether it's a mechanism with or without additional data (figuring out which one is which is left as an exercise to the reader)

> This assumes that it's obvious from the definition of any mechanism that the
> last Challenge is not actually a challenge, but instead an additional-data blob
> (i.e. there's no time at which either a challenge or additional data would be
> equally valid). I haven't checked whether that's true.

Yes, we're implementing a sasl protocol which has no additional data field in the success message here (See paragraph 3 of RFC 4422).  Every mechanism has to be able to handle this.

We picked this way as you can trivially transform from a protocol doing success-with-additional-data to the challenge-with-additional-data,empty response,succes sequence without any knowledge of the underlying sasl mechanism, while the reverse is impossible.

> > The one thing i'd like to add to the spec is a mandatory to implement
> > X-TELEPATHY-PASSWORD mechanism, which is a simple mechanism with initial-data
> > '\0user\0password' (basically SASL PLAIN (RFC 4616). When this mechanism is
> > used it's expected that the CM handles the actual authentication (basically as
> > if there was a password in the account information). Reasoning here is that it
> > would allow us to never store a password in the account information and always
> > use an authentication handler (without that handler needed to implement
> > specific sasl mechanisms). (This way Empathy wouldn't need to implement any
> > sasl mechanisms, just get the password from the keyring). Ofcourse if the CM
> > doesn't recognize any of the SASL mechanisms, it doesn't have to expose it to
> > the client (but that seems strange)
> 
> Yeah, I meant to ask about that. Mikhail suggests using D-H or something, like
> the fd.o secrets service does, either immediately or a bit later.

We can add that part later, with the same semantics, just a different way of transferring the information, afaik gnome-keyring doesn't even always use the DH part.

> Is it true that the only distinction between X-TELEPATHY-PASSWORD and PLAIN is
> that the mechanism name implicitly tells the client who it's authenticating to
> - if it's X-TELEPATHY-PASSWORD then it's the CM, else it's the server?

You never authenticate to the CM, but that's me nitpicking. All non-X-TELEPATHY-* mechanisms are a direct conversation with the server, while the X-TELEPATHY-* ones is just giving credentials to the CM indeed.

> It occurs to me that another way to do this would be to say in the
> AuthenticationInfo who we're authenticating with - it could be as simple as
> {"talking-to-cm": True} - and use an unmodified SASL mechanism, presumably
> PLAIN at first and some sort of D-H badgers later, to hand over credentials to
> it. This is how I'd assumed the "smart CM, dumb protocol" case would work.

I don't think masking PLAIN is a good idea. Leave it to the UI to choose, yes i want to do PLAIN with the server or let the CM handle it for me.

(In reply to comment #21)
> (In reply to comment #20)
> > ServerAuthentication
> > ====================
> > 
> > ServerAuthentication could do with a high-level explanation of how it happens:
> > 
> > - When is it dispatched? I infer that it's while the Connection is CONNECTING,
> > but that should be stated up-front.

Expanded

> > - What properties can it be expected to have? Most of the text from
> > ServerTLSConnection is probably applicable.

Usual properties added (TargetHandleType == None etc.)

> > - Can you have more than one ServerAuthentication in series?
> > - Can you have more than one ServerAuthentication in parallel?
> You can have multiple ServerAuthentication in series, you must defeat them all
> to get the prize.

Added.

One thing I wonder about this channel type is: is it actually useful to have this split between ServerAuthentication and SASLAuthentication? There are two reasons that might be useful:

- you can do certain things with a ServerAuthentication channel without knowing the specifics

- the SASLAuthentication interface can be common to the ServerAuthentication and HypotheticalEndToEndAuthentication channel types

but do we actually have one of those situations here? I don't think we have the first. If we intend to have the second, I'll have to clean up the wording of the SASL interface to not be server-authentication-specific, and its <tp:requires> is inappropriate.

I'm really not sure whether AuthenticationInformation belongs on the ServerAuthentication channel type: none of the keys you've defined seem useful or indeed possible outside (pseudo-)SASL.

In the emails you linked (thanks!) you mentioned Captcha, EULA and TLS (authenticating via a client certificate?) as other possible values for Authentication_Method. I don't really see anything those have in common with SASL, except that they're all things you need to resolve before the connection will succeed :-)

---------------------------------------------

SASL
====

> > Are the username and realm in Auth_Details the same as SASL RFC 4422's
> > "authorization identity strings" or are they intended as something to put in
> > the UI presented to the user or what?

I haven't expanded on these yet.

> Your
> choice for either adding a boolean or split of a StartMechanismWithData

Split. Also, StartMechanismWithData will fail if the protocol doesn't support the initial response (XMPP does, others might not), and there's an immutable boolean for whether it does.

(I initially tried to define a fallback in terms of StartMechanism + wait for an empty challenge which is not signalled + send response, but that only works for mechanisms which are consistently client-first - DIGEST-MD5 is server-first, unless the protocol has initial data and the client presents some, in which case it's client-first and takes fewer round-trips.)

> > We don't have a way to emit an 'ay' of additional data with a successful
> > outcome
> 
> We do. The additional data is expected to be emitted as a last challenge to
> which the client responds with Accept (the CurrentChallenge property has some
> info about it, but it needs to be spelled out more)

Having re-read the RFC, we essentially fake the fallback path that protocols not supporting additional data would use, which, unlike initial-response, always works.

> > Is AvailableMechanisms immutable? (My guess: yes.)
> > 
> > Is Secure immutable? (My guess: yes.)
> 
> Seems i annoted that in my initial mails but it didn't make it into the spec

It did now.

> Like you say a bit later, CurrentChallenge is superflous

Deleted.

> > We should document that only the Handler may call StartMechanism.

... or indeed, any method. Done.

> If you care a lot about challenge with no data vs. zero lengt then you
> might want to add a boolean to that signal

There's no such distinction, luckily - a challenge carries 0 or more bytes of data and that's all there is to it.

> > It's not immediately obvious what the state transitions are; I'll try to dig
> > that out from Gabble.
> 
> Gabble doesn't have the complete set, in particular it can't restart a sasl
> try.

Hmm, yeah, about that... it raises NotAvailable, which I've used to mean "you're in the wrong state". We should have a separate code for "we failed and the connection is no more". I wonder whether that code should in fact be that the connection just disconnects.

> > There are no tp:possible-errors anywhere, which seems like an oversight.
> 
> go go go :)

Mostly documented now; I haven't done Abort() yet.

> > I would assume that:
> > 
> > - Respond() is only valid to be called while In_Progress; similarly,
> > NewChallenge makes no sense unless In_Progress
> 
> Yup, the successfull sequences look like:
> 
> StartMechanisms,  => In_Progress, (Challenge, Respond)*,  [=> ServerSucceeded,
> Accept, => Succeeded || Challenge, Accept, => ClientSucceeded, => Succeeded]

I *think* I've understood this correctly now...

> > - Close() can be called at any time, and has the same effect as calling Abort()
> > with errors ignored, and then Close()
> 
> Same effect as Abort indeed, accept when Abort doesn't make sense :)

I've put the Close behaviour in the intro.

> > Can StartMechanism be called in In_Progress, Server_Succeeded, Client_Accepted
> > and Succeeded states, to restart authentication gratuitously?
> 
> No, SASL_Status correctly documents when you can call StartMechanism. If you
> want to restart gratuitously you should first Abort and then StartMechanism.

Noted.

> > Why does Abort() take an Abort_Reason, but SASL_State has a DBus_Error_Name?
> > Shouldn't they both have the same (perhaps both)?
> 
> good point, potentially both indeed.

Not sorted yet.

Relatedly, to represent SASL_Abort_Reason_Invalid_Challenge in the state struct, we need a code for "the server is confused". I propose ServiceConfused, which we can also use as the mapping for <internal-server-error/> over on Bug #21735.

> > Do we need a way for the authenticator to get the server's GSSAPI host-based
> > service name (RFC 4422 §4(1))? I expect that that'd be a constant string + "@"
> > + the hostname, for basically every protocol we support.
> 
> Could be added to the AuthenticationInformation dict i guess?

That would make sense.

> The one thing i'd like to add to the spec is a mandatory to implement
> X-TELEPATHY-PASSWORD mechanism, which is a simple mechanism with initial-data
> '\0user\0password' (basically SASL PLAIN (RFC 4616).

Added. I haven't made it m-t-i yet; Gabble doesn't actually implement it.

I'd seen this as a way to deal with the "smart Handler, smart CM, dumb protocol" case, and not "dumb Handler, smart CM, smart protocol", but yes it could equally well be used for that, and that's a reasonable use case for implementing it in conjunction with real SASL mechanisms (which I hadn't considered).

That does require that the CM understands a reasonably wide range of SASL mechanisms, but the old RequestConnection({'password': whatever}) mechanism needed that anyway.

(In reply to comment #24)
> I'm really not sure whether AuthenticationInformation belongs on the
> ServerAuthentication channel type: none of the keys you've defined seem useful
> or indeed possible outside (pseudo-)SASL.

The current draft shuffles them onto SASLAuthentication, but also retains AuthenticationInformation for the moment.

I separated AuthorizationIdentity (the former username) and DefaultRealm (the former realm) into their own properties, and put the former session-id in an ExtraContext a{sv} as "jabber-stream-id". I think it's right that AuthorizationIdentity is separate, but I'm undecided about DefaultRealm.

> > > Are the username and realm in Auth_Details the same as SASL RFC 4422's
> > > "authorization identity strings" or are they intended as something to put in
> > > the UI presented to the user or what?
> 
> I haven't expanded on these yet.

Now done.

> > Gabble doesn't have the complete set, in particular it can't restart a sasl
> > try.
> 
> Hmm, yeah, about that... it raises NotAvailable, which I've used to mean
> "you're in the wrong state". We should have a separate code for "we failed and
> the connection is no more". I wonder whether that code should in fact be that
> the connection just disconnects.

Thinking about it, I think it'd be OK to say NotAvailable, as long as there's a boolean property CanTryAgain, which I've added.

XMPP would usually have CanTryAgain = TRUE, but Wocky doesn't support it, therefore Gabble doesn't either (if I'm reading the implementation correctly).

I think this is ready for another round of review.

Remaining things I'm unsure about:

Should Chan.T.ServerAuthentication be a Chan.T.SASLAuthentication
that exists solely to carry the authentication interface? It doesn't
look to me as though it's likely to be possible to handle this channel
type generically.

(There is precedent: ServerTLSConnection works like that.)

Should Chan.I.SASLAuthentication be a separate object rather than a
channel interface, like TLS certificates are? That'd let us delete
"SASL" from all the property names without worrying about collisions.

If Chan.I.SASLAuthentication does become a separate object, it might be easier to document and understand if there was one object per authentication try?

Updated links for review:

http://people.freedesktop.org/~smcv/telepathy-spec-sasl/spec/Channel_Type_Server_Authentication.htm

http://people.freedesktop.org/~smcv/telepathy-spec-sasl/spec/Channel_Interface_SASL_Authentication.html

http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/sasl

(In reply to comment #26)
> I think this is ready for another round of review.
> 
> Remaining things I'm unsure about:
> 
> Should Chan.T.ServerAuthentication be a Chan.T.SASLAuthentication
> that exists solely to carry the authentication interface? It doesn't
> look to me as though it's likely to be possible to handle this channel
> type generically.

You probably don't want different handerls for the different authentication steps. We could make a ServerSASLAuthentication channel type if you prefer, but needing one channel & interface per authentication method seems a bit odd. 

Basically Chan.T.ServerAuthentication was meant to be a generic channel type for all parts of the ServerAuthentication, with demultiplexing in via the immutable AuthenticationType as to not add loads of channels that are all the same but just carry a different interface..

> (There is precedent: ServerTLSConnection works like that.)
> 
> Should Chan.I.SASLAuthentication be a separate object rather than a
> channel interface, like TLS certificates are? That'd let us delete
> "SASL" from all the property names without worrying about collisions.

Not sure what that buys us
 
> If Chan.I.SASLAuthentication does become a separate object, it might be easier
> to document and understand if there was one object per authentication try?

Maybe, but i don't see it necessarily as a benefit (you need to get a nex proxy, have some signal for the object going away and coming back etc, which just is more hastle for the implementor).

Draft 2 merged, will be in 0.21.5.

Comments from Eitan would be appreciated.

(In reply to comment #29)
> Draft 2 merged, will be in 0.21.5.
> 
> Comments from Eitan would be appreciated.

Wow, thanks for all this. A quick glance looks good. The documentation is sure welcome, but the added properties are a bit confusing (from the conversation above they look necessary). I'll take a deeper look in a bit.

Fixes prompted by re-reading this branch:

ExtraContext should be SASLContext.

Accept, Abort and StatusChanged should be AcceptSASL, AbortSASL and SASLStatusChanged, unless we turn the interface into an independent object.

I think Respond and the properties (other than Secure and ExtraContext) are jargon'y enough to keep as-is without conflicts, and NewChallenge is probably OK too?

Secure should be split into Encrypted (strong encryption is active) and Verified (man-in-the-middle protection is active, e.g. a certificate has been accepted either by the user or the system cert store).

Chan.T.ServerAuthentication.AuthenticationInformation should just go away, I think; I'm reasonably sure it makes more sense in Chan.I.SASLAuthentication.

Chan.T.ServerAuthentication.AuthenticationMethod should be a D-Bus interface name rather than an enum member?

--------------------------------

Not fixed yet:

Collision-avoidance would mean renaming Encrypted and Verified to SASLTransportEncrypted and SASLTransportVerified or something, but they're not really part of this interface at all... in a way, I think we really want the Encryptable interface from Bug #29904, but with a split between Encrypted and Verified (which it currently conflates).

I'm not sure that Verified is useful without Encrypted for SASL, but it's a possibility in XTLS-like mechanisms. Verified and !Encrypted is analogous to a PGP-signed email conversation - I know I'm talking to Fred, but other people might be reading our email too.

Perhaps this?

Chan.I.Securable

    property Encrypted: b, immutable=sometimes
        If true, eavesdropping is prevented, but you might
        not be talking to who you think you are.

        Immutable on ServerAuthentication channels.

    property Verified: b, immutable=sometimes
        If true, tampering is prevented and you are
        talking to who you think you are.

        Immutable on ServerAuthentication channels.

Also to be fixed:

15:59 < jonnylamb> smcv: Why does the SASL_Auth interface not tp:require the 
                   Server_Auth ctype?
16:00 < smcv> jonnylamb: hypothetical Peer_Auth ctype
16:00 < jonnylamb> hm, ok
16:00 < smcv> I should say so, though

(In reply to comment #31)
> Chan.I.Securable
> 
>     property Encrypted: b, immutable=sometimes
>         If true, eavesdropping is prevented, but you might
>         not be talking to who you think you are.
> 
>         Immutable on ServerAuthentication channels.
> 
>     property Verified: b, immutable=sometimes
>         If true, tampering is prevented and you are
>         talking to who you think you are.
> 
>         Immutable on ServerAuthentication channels.

FWIW, that sounds OK to me.

Some implementation thoughts from updating Gabble to the current draft:

Status needs an attribute so it pluralizes to Statuses rather than Statuss.

Discussing this with wjt, we don't think X-WOCKY-JABBER-PASSWORD and -DIGEST should be exposed to D-Bus: nobody's going to implement them anyway. Gabble should just implement X-TELEPATHY-PASSWORD and do the legacy Jabbering internally. This would let us drop jabber-stream-id from SASLContext; we could even drop SASLContext in its entirety?

It makes no sense to call AbortSASL from the Succeeded state: that should raise NotAvailable. Whether it makes sense in Client_Accepted is debatable; eeejay's implementation fails there, too.

(In reply to comment #29)
> Draft 2 merged, will be in 0.21.5.
> 
> Comments from Eitan would be appreciated.

This all looks good. Couldn't think of any useful amendments. r+

AuthorizationIdentity isn't really sufficient; we also need DefaultUsername, for use with SASL mechanisms that deal with a "simple username" as defined in RFC 4422 §2. RFC 3920 §6.3 suggests that we should also be providing the UI with a service name, which is "xmpp" for XMPP.

In XMPP, servers typically[1] expect "smcv@example.com" to authenticate with username "smcv"; this was a SHOULD in RFC 3920. Wocky doesn't appear to support anything else, in fact.

3920bis weakens that SHOULD to "in the absence of local information provided by the server, an XMPP client SHOULD assume that the authentication identity for such a SASL mechanism is the combination of a user name and password, where the simple user name is the localpart of the user's JID".

-------- Simple case: being yourself

When you're authenticating as "yourself", XMPP mandates that you "MUST NOT provide an authorization identity unless the authorization identity is different from the default authorization identity derived from the authentication identity, as described in [SASL]".

So if I connect with:

    RequestConnection({ account: "smcv@example.com" })

and use PLAIN with password "password", I should authenticate like so:

    "\0smcv\0password"

However, I think the authorization identity I'm trying to assert is still "smcv@example.com". So the channel would look like this:

    { "...DefaultUsername": "smcv",
      "...AuthorizationIdentity": "smcv@example.com }

One problem here is that I can't see how a generic SASL client can be expected to know what authorization identity the server is going to derive from its authentication identity (this is particularly tricky for non-username/password things, like SASL EXTERNAL referring to a certificate). So we might violate that "MUST" directive sometimes, without knowing about it...

The semantics I propose for DefaultUsername are: if using a "simple username" SASL mechanism, clients SHOULD default to using the DefaultUsername; also, if the client uses the DefaultUsername, it SHOULD assume that the authorization identity AuthorizationIdentity will be derived from it by the server.

-------- Complex case: being someone else

If I'm using my sysadmin powers to log in as the "announcements" role address, I expect that I'd want to connect with:

    RequestConnection({ account: "announcements@example.com" })

The SASL channel would look like this:

    { "...DefaultUsername": "announcements",
      "...AuthorizationIdentity": "announcements@example.com }

but a sufficiently elaborate UI could give me the opportunity to override the username from "announcements" to "smcv".

My simple username is still smcv, my password is still password, but this time I'm trying to authorize myself to act as announcements@example.com, so the UI would have to perform SASL PLAIN with this string:

    "announcements@example.com\0smcv\0password"

In this case, it really does need to know the AuthorizationIdentity.

[1] http://trac.tools.ietf.org/wg/xmpp/trac/ticket/49

(In reply to comment #31)
> Perhaps this?
> 
> Chan.I.Securable

I've done this.

(In reply to comment #34)
> Status needs an attribute so it pluralizes to Statuses rather than Statuss.

Done.

> we could even drop SASLContext in its entirety?

Done.
 
> It makes no sense to call AbortSASL from the Succeeded state: that should raise
> NotAvailable. Whether it makes sense in Client_Accepted is debatable; eeejay's
> implementation fails there, too.

Done.

(In reply to comment #36)
> AuthorizationIdentity isn't really sufficient; we also need DefaultUsername,
> for use with SASL mechanisms that deal with a "simple username" as defined in
> RFC 4422 §2. RFC 3920 §6.3 suggests that we should also be providing the UI
> with a service name, which is "xmpp" for XMPP.

Done!

> +        <tp:dbus-ref namespace="ofdT">Channel</tp:dbus-ref>s. The two
> +        properties are immutable and can be used to make decisions on
> +        how cautious to be about transferring sensitive data.</p>

In cosimoc's e2e encryption branch, these properties are mutable but can only get more secure. I think that's correct in general. When securing a Text channel, say, I think we probably do want them mutable - it starts off unsecured, you upgrade it to e2e security, and the channel remains the same, but the message path behind the scenes changes to a secure one.

My suggested compromise was to have them tp:immutable="sometimes", and specify that for the special case of ServerAuthentication channels, they are indeed immutable.

When we support e2e encryption, we can also add the securing state (not tried -> trying -> succeeded or failed, where Encrypted and Verified stop changing after the state has reached succeeded).

> </tt>". where

(in DefaultUsername) Nitpicking: dot -> comma

(In reply to comment #38)
> My suggested compromise was to have them tp:immutable="sometimes", and specify
> that for the special case of ServerAuthentication channels, they are indeed
> immutable.

Done.

> > </tt>". where
> 
> (in DefaultUsername) Nitpicking: dot -> comma

Fixed.

Thanks, r+, please merge.

Thanks, merged. Removing patch keyword and URL.

I've added some text tweaks based on discussion with Pekka, see my sasl branch:

http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/sasl

I think this is also ready for undraft now:

http://people.freedesktop.org/~smcv/telepathy-spec-undraft/spec/
http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/undraft

(In reply to comment #42)
> I've added some text tweaks based on discussion with Pekka, see my sasl branch:

Looks like r+

++ to both your spec branches there. GOGOOGOGOGOGOLRELELELELELEASE.

Fixed in 0.21.5