[Bug 29904] Support end-to-end encryption and authentication

[bugzilla-daemon at freedesktop.org]

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

Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|telepathy-bugs at lists.freede |cosimoc at gnome.org
                   |sktop.org                   |

--- Comment #3 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-09-01 04:19:23 PDT ---
General comments:

Every property should either hyperlink to the signal that provides its change
notification, or say that it is immutable for the lifetime of the object, or
say that it can change but has no change notification (in the latter case,
which should be unusual, you should explain when it would make sense to poll
it).

(In reply to comment #0)
> To accomplish that, I added basically three fundamental types to the
> specification: the Encryptable interface, the ClientAuthentication channel
> type, and the Authentication.Proposal transient object. On top of them I also
> drafted some protocol-specific implementations.

Terminology: of these, I'd say only Authentication.Proposal is a fundamental
type (i.e. least derived object - the others appear on a Channel), and if you
say "the Encryptable channel interface" or "Chan.I.Encryptable" it's easier to
see what's going on at a glance :-)

There are lots of mentions of "Encryption" and "Authentication" here where I'm
not sure that the usage is actually valid - it seems strange for
EncryptionInterface to have Chan.I.XTLSAuthentication as a value. Perhaps this
indicates that the interface should be called Chan.I.XTLSEncryption,
Chan.I.XTLSSecurity or just Chan.I.XTLS, or that the property should be called
something else.

EncryptionInterface should document what sort of interface it could be, perhaps
with wording like "The interface that will appear on a
Chan.T.ClientAuthentication channel representing the encryption mechanism, such
as Chan.I.XTLSAuthentication".

AuthenticationChannel needs to be "nullable" - in Telepathy we do this by
reserving the object path '/' to be the pseudo-null value (see
ofdT.Account.Connection for appropriate wording). Otherwise, there's no sane
value for the property at times when there's no authentication channel (and
every property must have a value at all times, even if that value is useless).

Relatedly, I suspect EncryptionStateChanged should have signature (u: State, o:
Authentication_Channel) so AuthenticationChannel has change notification?

> I thought this being a requestable property here could also play nice with the
> fact that a CM could advertise the possible protocols in
> RequestableChannelClasses.

Yeah, about that. It's worth documenting what a CM is expected to have in its
RCCs (so that clients probing for support will look for the right things). As
far as I can see, for your current proposal, that text would look something
like this:

-------------------------------->8-----------------------------------

A connection manager supporting end-to-end secured channels should include
EncryptionRequired in the allowed properties of each requestable channel class
that supports this.

The connection manager should also duplicate the "basic" requestable channel
class once for each supported value of the EncryptionInterface property, with
that property added to the fixed set.

For instance, if you support encrypting text channels with either XTLS or OTR,
and you support StreamedMedia channels which cannot be encrypted, you might
have these requestable channel classes:

Fixed = { ...ChannelType: ...Text, ...TargetHandleType: CONTACT }
Allowed = [ ...TargetHandle, ...TargetID,
...Chan.I.Encryptable.EncryptionRequired ]

Fixed = { ...ChannelType: ...Text, ...TargetHandleType: CONTACT,
...Chan.I.Encryptable.EncryptionInterface: ...Chan.I.XTLSAuthentication }
Allowed = [ ...TargetHandle, ...TargetID,
...Chan.I.Encryptable.EncryptionRequired ]

Fixed = { ...ChannelType: ...Text, ...TargetHandleType: CONTACT,
...Chan.I.Encryptable.EncryptionInterface: ...Chan.I.OTRAuthentication }
Allowed = [ ...TargetHandle, ...TargetID,
...Chan.I.Encryptable.EncryptionRequired ]

Fixed = { ...ChannelType: ...StreamedMedia, ...TargetHandleType: CONTACT }
Allowed = [ ...TargetHandle, ...TargetID, ...Chan.T.StreamedMedia.InitialAudio
]

-------------------------------->8-----------------------------------

I'm a little worried about potential combinatorial explosion if we go in this
direction for EncryptionInterface, though...

I wonder whether it's actually appropriate for clients that upgrade to e2e to
specify how in UpgradeEncryption? It seems to be a design goal that Empathy can
flip the "encrypt me" bit and Seahorse or something can pop up to do the actual
crypto handshaking? If that's the case, then we could detect from handlers'
filters, or perhaps handler capability tokens, what sorts of handshake we have
UI for.

An alternative API for upgrades would be for a second channel to pop up,
replacing the first, using a more general form of Conference
(Chan.I.Continuable?). Naive clients not understanding Continuable would handle
it as if it was a new channel. That approach is probably harder to work with
for clients, but might lead to easier conceptual design.

> The method does nothing if EncryptionRequired  is already True.

This implicitly assumes that there are only two levels of
encryption/authentication - "not enough to mention" (which includes "none") and
"fully secure" - and it doesn't matter which specific mechanism you're using.
This is probably true, but it's worth thinking about.

> ----------------------------
> Ch.Type.ClientAuthentication
> ----------------------------
> 
> It's the channel that will be dispatched to authenticate the encryption
> process. The base object also keeps track of the channels around that use this
> authentication, by using the TargetChannels property.
> 
> Channel.Interface.XTLSAuthentication is an XTLS interface for this kind of
> channel, and one might also write Channel.Interface.OTRAuthentication on the
> same line.

The Description could do with an overview of this object's life cycle: when
you'll get one, how you proceed, and when you can consider it dead.

AuthenticationProposals and XTLSAuthenticationStateReason should document what
their change notification is (looks like the answer is ProposalsReceived and
XTLSAuthenticationStateReason, respectively).

Is it valid for ProposalsReceived to be emitted with a set of proposals that is
smaller than the previous value of AuthenticationProposals? (My guess: no.
Please answer in the form of a spec patch :-)

In each XTLS_Authentication_State it would be good to document where to go
next, with a hyperlink. My guesses:

* Not_Started: you say SendMechanisms but you're missing a <tp:member-ref>

* L_M_S: just wait

* R_M_R: call methods on the AuthenticationProposals?

* Success: Close()?

* Failed: Abort()?

How does XTLSAuthentication.Abort() differ from Channel.Close()? When would you
call one? When would you call the other? Should Abort() take some sort of
reason code?

Are there any sensible values for XTLS_Authentication_State_Change_Reason that
are less vague than "Rejected" or "Error"?

The Mechanism type's name is far too vague. Authentication_Mechanism, please -
remember that types are a flat namespace (this one would come out as
TP_STRUCT_TYPE_MECHANISM in current telepathy-glib).

SendMechanisms' argument should probably be documented as Mechanism[] rather
than Mechanism_List? (Either works, I believe.)

> -------------
> Auth.Proposal
> -------------
> 
> It's a transient object that can be used to complete the authentication step.
> Each proposal implements one interface, which defines the protocol-specific
> methods to accept it.
> 
> How exactly the proposal objects are used and constructed is up to the
> interface of the ClientAuthentication channel.
> 
> I drafted implementation for Auth.Proposal.X509 and Auth.Proposal.SRP, to be
> used with the XTLSAuthentication interface.

Proposal.Interface should perhaps be Proposal.Type or Proposal.ProposalType,
consistent with Channel.ChannelType (the model is that channels have exactly
one type, and n interfaces). Proposal should probably have an Interfaces
property for future expansion, too.

X509
====

X509 should have a brief overview of how to operate it in its Description (just
mentioning all the methods in the right order would be valuable). A hyperlink
to the appropriate RFC or XEP would be extremely valuable.

X509_Authentication_State_R_C_R should hyperlink Accept.

In X509StateChanged, what does "a good path for accepting a remote certificate
or sending one" mean? Does it mean "the handler should respond according to the
state, perhaps by accepting a remote certificate or sending a local
certificate"?

Is RemoteCertificate mutable? (Guess: yes.) What is its value if there is no
remote certificate yet? (Guess: '/'.) What is its change notification?

Likewise for LocalCertificate. If there is no change notification, that's
probably OK, but you should explicitly say so.

Is RequestedIdentity mutable? What is its change notification?

Is LocalX509Fingerprint mutable? What is its change notification?

Which properties are valid to pass to SendMechanisms and which are mandatory?
(Guess: LocalCertificate is optional, LocalX509Fingerprint is mandatory.)

SRP
===

Are there any less vague error codes that could be used?

There is an error code for Invalid_Username but I don't see any mention of a
username anywhere in the interface?

This badly needs an introduction, if only via a hyperlink to the relevant RFC,
to explain what "password" means in this context. Based on spec meetings, my
understanding is that the two parties are meant to communicate out of band
(phone call, in person, etc.) and agree on a password.

> There's a last object, which is Ch.Type.ClientTLSConnection, which is a way to
> exchange TLS certificates between clients; this is usually needed after SRP,
> and a channel of this kind would use the same Encryptable/ClientAuthentication
> process described before to signal that the exchange is linked to a previously
> authenticated session.

ClientTLSConnection doesn't seem like the right name for its functionality, if
it's meant to pop up with EncryptionRequired=TRUE already - it's not
establishing any sort of TLS'd connection, just exchanging certs.
ClientCertificateExchange, perhaps?

What is the target handle type of a ClientTLSConnection? I assume only CONTACT
makes sense?

The introduction should explain how the two possibilities (incoming and
outgoing) work.

Say what requestable channel classes are expected?

Is RequestedIdentity our identity, or that of the other guy?

Is Incoming redundant with Requested? If so, we can delete it.

ReceiveCertificate() and ProvideCertificate() should raise a specified error
(Invalid_Argument, probably) if called on a channel of the wrong direction.

Did you get the Requested semantics of ProvideCertificate and
ReceiveCertificate backwards? They look backwards to me but perhaps I'm not
seeing it clearly enough.

Why is the received certificate a TLSCertificate and not just a
Certificate_Data[]? I can't see what it would mean to accept or reject it at
this stage, since it's just being provided for future reference?

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