[Bug 14003] Connection: interface to ask for credentials (SASL auth channels)

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=14003

--- Comment #24 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-10-19 09:31:44 PDT ---
(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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.