If we focus purely on a c2s connection TLSCarrier doesn't really seem suitable (well, it has bits that make no sense in this environment).

I think i'm in favour of having one channel type specificall for the use-case of c2s so it's very clear what this channel does and doesn't have bits that only make sense in some use-cases. The TLSCertificate interface does make sense as something we can re-use for both things (as in it's just a common bit to represent a certificate ``challenge'')..

I'm wondering if we can just have a ServerTLSConnection channel type and slap the TLSCertificate on it and basically call it a day?

Thanks for the feedback.

I implemented your suggestions in the 'tls-connection2' branch of my telepathy-spec repo here [1].

Changes:
- removed TLSCertificateCarrier, and added ServerTLSConnection channel in his place.
- removed the Fingerprint property to the TLSCertificate object. My rationale for removing it is it's not an important information to know off-hand for the C2S use case, and for the C2C use-case we might want to add a method for calculating it (with a parameter specifying the digest algorithm), or calculate it completely on the client-side. Either way, it's something we can add it later.

Review and comments welcome.

[1] http://git.collabora.co.uk/?p=user/cosimoc/telepathy-spec.git;a=shortlog;h=refs/heads/tls-connection2

Had a nice chat with smcv about the interface. We agreed that while the model is a bit odd (one channel that just points to another object), it makes sense for the reasons we've discussed earlier.

* Authentication_TLS_Certificate should document which properties are immutable (CertificateType ad CertificateChainData i assume)
* Channel_Type_Server_TLS_Connection should document this as well.

We were pondering if it would be useful to put the immutable properties of the Autehntication_TLS_Certificate object in the ServerCertificate property on the channel. But i think me and smcv agreed in the end that that might be a bit too much data for a signal.

The biggest update is to change the Reject method call on TLSCertificate to pass onwards some more information. Using http://people.collabora.co.uk/~cosimoc/tls-connection-spec/Connection.html#org.freedesktop.Telepathy.Connection.ConnectionError as an example (given that if we reject a certificate while connecting, it should signal a ConnectionError with the reason).. Keep the enum though as that's useful for clients that don't understand a dbus error.

So Reject would look like Reject (enum reason, DBus_Error_Name error, a{sv}: Details). Do document which reasons map to specific DBus_Error_names by default, like connection errors do.

We came up with to use-cases for some extra information (so well-known keys for the details):
  - Give a hint whether or not the rejection was automatically done or at the request of a user (in
    the latter case empathy should not show its awfullbar for example)
  - Give a bit more details when things went wrong, e.g. If there is a hostname mismatch have a key with the (expected, received) hostname pairs

Thanks for the review;

I implemented your suggestions on top of my branch [1].

Overview of changes:

* fixed what I think is a c/p typo in the Connection error enumeration
* documented immutable properties
* synced the TLS_Certificate_Error enum with the values of Cert.* already there in errors.xml
* added more arguments to Reject() as for your review
* split the 'StateChanged' signal into two 'Certificate_Verified' and 'Certificate_Rejected' signals, so the latter can carry the error details
* synced the Cert.* errors in errors.xml to add references to the TLS_Certificate_Error enum and also added there two values from that enum, which were previously missing.

The HTML version can be found here [2]

[1] http://git.collabora.co.uk/?p=user/cosimoc/telepathy-spec.git;a=shortlog;h=refs/heads/tls-connection2
[2] http://people.collabora.co.uk/~cosimoc/tls-connection-spec/

The API looks fine to me, i could nitpick about the naming (e.g. why have an Accept method, but the signal is CertificateVerified ?), and the documentation needs some work. But that's more smcv's area :)

Commit 2c33eaae4 affects Connection API, but it is indeed an obvious copypasta fix. Please cherry-pick it into master, Reviewed-by: me.

The API seems the right shape.

Authn.TLSCertificate
====================

While we're on the subject of TLS, could you answer the question I asked in <https://bugs.freedesktop.org/show_bug.cgi?id=23931#c6> (whether Limit_Exceeded and Insecure should be enum members or not), then we can add Limit_Exceeded and Insecure codes? TLS_Certificate_Reject_Reason contains a new member Insecure_Algorithm that doesn't correspond to a D-Bus error name yet; we should unify the naming one way or the other.

I'm not sure that TLS_Certificate_Reject_Reason needs both None and Other; what's the difference between them? Could they usefully be merged, perhaps with the merged version called Unknown? I think "Unknown" should be numerically zero.

Either the methods should gain a Certificate prefix, or the signals should lose it. I'd vote for the signals losing their prefix: the usual namespace-collision reasons don't apply here, because the TLSCertificate is an independent object.

CertificateVerified() should be [Certificate]Accepted(): the user could accept a certificate about which nothing has been verified (leap of faith), and the naming should be consistent with Accept(). Similarly, TLS_Certificate_State_Verified should be ..._Accepted.

The Accepted signal should be documented as "The _State_ of this certificate has changed to Accepted", with a hyperlink (<tp:member-ref>) to the property.

The Rejected signal should be documented with a hyperlink to State too. The Rejected signal's Reason argument should be documented as "The new value of _RejectReason_", with a hyperlink.

RejectReason should have rationale, defined behaviour in non-Rejected states, and a hyperlink to State, something like this:

    RejectReason: u (TLS_Certificate_Reject_Reason)
        If the _State_ is Rejected, the reason why the certificate was
        rejected.

        | Clients that do not understand the _RejectError_, which may be
        | implementation-specific, can use this property to classify
        | rejection reasons into common categories.

        Otherwise, this property is not meaningful, and SHOULD be set to
        Unknown.

If we want clients to be able to state-recover the rejection reason, there should be RejectError and RejectDetails properties:

    RejectError: s (DBus_Error_Name)
        If the _State_ is Rejected, the reason why the certificate
        was rejected; this MAY correspond to the _RejectReason_, or MAY
        be a more specific D-Bus error name, perhaps implementation-specific.

        If the State is not Rejected, this property is not meaningful,
        and SHOULD be set to an empty string.

    RejectDetails: a{sv} (String_Variant_Map)
        If the _State_ is Rejected, additional information about why the
        certificate was rejected.

        If the State is not Rejected, this property is not meaningful,
        and SHOULD be set to an empty map.

and the arguments to the Rejected signal should be documented as "The new value of _Foo_". If we don't care about state recovery for some reason, it would be consistent to delete the RejectReason property too.

If the RejectError, RejectDetails properties are added, then I'd be inclined to document the parameters to Reject like "The new value of _RejectError_", and move the list of well-known RejectDetails keys to the RejectDetails documentation.

There should be a "debug-message" detail, the same as for Connection.

In "user-requested", instead of "Whether the error was...", I'd prefer "True if the error was...". "rejectal" should be "rejection" and "opposed to" should be "as opposed to" (but I'd prefer it worded as "...rejection of the certificate; False if there was an unrecoverable error..." anyway).

If we're going to define "expected-hostname", I think it should be more like this:

    expected-hostname (s)
        If the rejection reason is Hostname_Mismatch, the hostname that the
        server was expected to have.

    certificate-hostname (s)
        If the rejection reason is Hostname_Mismatch, the hostname of the
        certificate that was presented.

        | For instance, if you try to connect to gmail.com but are presented
        | with a TLS certificate issued to evil.example.org, the error
        | details for Hostname_Mismatch SHOULD include:
        |
        | { "expected-hostname": "gmail.com",
        |   "certificate-hostname": "evil.example.org" }

Ideally, Connection.ConnectionError should also include these:

    expected-hostname (s), certificate-hostname (s)
        The same details defined in _TLSCertificate.DRAFT.RejectDetails_

and the description of HostnameMismatch in errors.xml should refer to them too.

TLS_Certificate_State_None would perhaps make more sense as TLS_Certificate_State_Pending?

> The RAW PEM-encoded trust chain of this TLS certificate. 

RAW isn't an acronym, I think you mean "raw". I don't think non-X.509 certificates can be PEM-encoded, so this doesn't really make sense; we should specify an encoding for PGP keys, perhaps like this?

    Certificate_Data - ay
        The raw data contained in a TLS certificate.

        For X.509 certificates (_CertificateType_ = "x509"), this MUST be in
        PEM format (Base64-encoded DER enclosed in
        "-----BEGIN CERTIFICATE-----" and "-----END CERTIFICATE-----" markers.

        For PGP certificates (_CertificateType_ = "pgp"), this MUST be a
        binary OpenPGP key as defined by section 11.1 of _RFC 4880_.

    CertificateChainData - aay (Certificate_Data[])
        One or more TLS certificates forming a trust chain, each encoded as
        specified by _Certificate_Data_.

Would DER or ASN.1 be a better form for X.509 certificates? The PEM format seems rather poorly-specified (I couldn't find a good reference for it), but if it's the de facto standard and things typically understand it better than DER, keep it. I'd prefer to be able to cite chapter and verse of an RFC for this sort of thing, though.

Chan.T.ServerTLSConnection
==========================

> Channels of this kind are never requested, are anonymous

"always have _Requested_ = False, _TargetHandleType_ = None and _TargetHandle_ = 0, and cannot be requested with methods such as _CreateChannel_" with hyperlinks, please.

> MAY just close the channel

Please hyperlink to Close.

(In reply to comment #6)
> Commit 2c33eaae4 affects Connection API, but it is indeed an obvious copypasta
> fix. Please cherry-pick it into master, Reviewed-by: me.

Done.

> While we're on the subject of TLS, could you answer the question I asked in
> <https://bugs.freedesktop.org/show_bug.cgi?id=23931#c6> (whether Limit_Exceeded
> and Insecure should be enum members or not), then we can add Limit_Exceeded and
> Insecure codes? TLS_Certificate_Reject_Reason contains a new member
> Insecure_Algorithm that doesn't correspond to a D-Bus error name yet; we should
> unify the naming one way or the other.

I already added Insecure_Algorithm to the D-Bus errors in a previous commit (e243001481350703aec717fcea57c7c40412ad4b); we could probably add it to the enum in Connection too, I think it's worth.
I think Limit_Exceeded only makes sense if we also document limits for the number of bits for an acceptable certificate and the depth of the certificate chain, and I don't think it's something worth doing for now.

> I'm not sure that TLS_Certificate_Reject_Reason needs both None and Other;
> what's the difference between them? Could they usefully be merged, perhaps with
> the merged version called Unknown? I think "Unknown" should be numerically
> zero.

Fixed in c06111e50266f3b725495a7ba12fae18dd8349e2

> Either the methods should gain a Certificate prefix, or the signals should lose
> it. I'd vote for the signals losing their prefix: the usual namespace-collision
> reasons don't apply here, because the TLSCertificate is an independent object.

Fixed in 9b3f6c0d38ba6ba1fcd53389fe221d72c82eb303

> CertificateVerified() should be [Certificate]Accepted(): the user could accept
> a certificate about which nothing has been verified (leap of faith), and the
> naming should be consistent with Accept(). Similarly,
> TLS_Certificate_State_Verified should be ..._Accepted.
> 
> The Accepted signal should be documented as "The _State_ of this certificate
> has changed to Accepted", with a hyperlink (<tp:member-ref>) to the property.

Fixed in c099c797b4d5ea117dde33e797be201eff4adc1d

> The Rejected signal should be documented with a hyperlink to State too. The
> Rejected signal's Reason argument should be documented as "The new value of
> _RejectReason_", with a hyperlink.

Fixed in 0d77ec3bfebed2d75eea06a071461c4f00c7cedb

> RejectReason should have rationale, defined behaviour in non-Rejected states,
> and a hyperlink to State, something like this:
> 
>     RejectReason: u (TLS_Certificate_Reject_Reason)
>         If the _State_ is Rejected, the reason why the certificate was
>         rejected.
> 
>         | Clients that do not understand the _RejectError_, which may be
>         | implementation-specific, can use this property to classify
>         | rejection reasons into common categories.
> 
>         Otherwise, this property is not meaningful, and SHOULD be set to
>         Unknown.
> 
> If we want clients to be able to state-recover the rejection reason, there
> should be RejectError and RejectDetails properties:
> 
>     RejectError: s (DBus_Error_Name)
>         If the _State_ is Rejected, the reason why the certificate
>         was rejected; this MAY correspond to the _RejectReason_, or MAY
>         be a more specific D-Bus error name, perhaps implementation-specific.
> 
>         If the State is not Rejected, this property is not meaningful,
>         and SHOULD be set to an empty string.
> 
>     RejectDetails: a{sv} (String_Variant_Map)
>         If the _State_ is Rejected, additional information about why the
>         certificate was rejected.
> 
>         If the State is not Rejected, this property is not meaningful,
>         and SHOULD be set to an empty map.
> 
> and the arguments to the Rejected signal should be documented as "The new value
> of _Foo_". If we don't care about state recovery for some reason, it would be
> consistent to delete the RejectReason property too.

Should be fixed with 00a73ea162aa1e4d2533757b52c506c5747e5935

> If the RejectError, RejectDetails properties are added, then I'd be inclined to
> document the parameters to Reject like "The new value of _RejectError_", and
> move the list of well-known RejectDetails keys to the RejectDetails
> documentation.
> 
> There should be a "debug-message" detail, the same as for Connection.

Fixed in 57878f426edc890a053b81c29048c4eb25921b4d

> In "user-requested", instead of "Whether the error was...", I'd prefer "True if
> the error was...". "rejectal" should be "rejection" and "opposed to" should be
> "as opposed to" (but I'd prefer it worded as "...rejection of the certificate;
> False if there was an unrecoverable error..." anyway).
> 
> If we're going to define "expected-hostname", I think it should be more like
> this:
> 
>     expected-hostname (s)
>         If the rejection reason is Hostname_Mismatch, the hostname that the
>         server was expected to have.
> 
>     certificate-hostname (s)
>         If the rejection reason is Hostname_Mismatch, the hostname of the
>         certificate that was presented.
> 
>         | For instance, if you try to connect to gmail.com but are presented
>         | with a TLS certificate issued to evil.example.org, the error
>         | details for Hostname_Mismatch SHOULD include:
>         |
>         | { "expected-hostname": "gmail.com",
>         |   "certificate-hostname": "evil.example.org" }

Fixed in 03cfdbadc393286dcbb6bdea89f0191515ad7d62

> Ideally, Connection.ConnectionError should also include these:
> 
>     expected-hostname (s), certificate-hostname (s)
>         The same details defined in _TLSCertificate.DRAFT.RejectDetails_
> 
> and the description of HostnameMismatch in errors.xml should refer to them too.

Fixed in 5b1cabf90f174b353186299a0296828f977be878
 
> TLS_Certificate_State_None would perhaps make more sense as
> TLS_Certificate_State_Pending?

Fixed in 19475972054c91cb842dae3d21b488bcb0d0052b

> > The RAW PEM-encoded trust chain of this TLS certificate. 
> 
> RAW isn't an acronym, I think you mean "raw". I don't think non-X.509
> certificates can be PEM-encoded, so this doesn't really make sense; we should
> specify an encoding for PGP keys, perhaps like this?
> 
>     Certificate_Data - ay
>         The raw data contained in a TLS certificate.
> 
>         For X.509 certificates (_CertificateType_ = "x509"), this MUST be in
>         PEM format (Base64-encoded DER enclosed in
>         "-----BEGIN CERTIFICATE-----" and "-----END CERTIFICATE-----" markers.
> 
>         For PGP certificates (_CertificateType_ = "pgp"), this MUST be a
>         binary OpenPGP key as defined by section 11.1 of _RFC 4880_.
> 
>     CertificateChainData - aay (Certificate_Data[])
>         One or more TLS certificates forming a trust chain, each encoded as
>         specified by _Certificate_Data_.
> 
> Would DER or ASN.1 be a better form for X.509 certificates? The PEM format
> seems rather poorly-specified (I couldn't find a good reference for it), but if
> it's the de facto standard and things typically understand it better than DER,
> keep it. I'd prefer to be able to cite chapter and verse of an RFC for this
> sort of thing, though.

After having looked at the documentation of the TLS libraries we will use, and having talked at GUADEC with some crypto people, I got convinced we should just use DER here instead of PEM, not because it's poorly specified (see RFC 1421 [1]), but because PEM is just DER encoded in a different way, and DER is the most simple and widely supported format anyway.

I changed all of this in 83812bda2eb0e876255140b00034886f281abb0d

> Chan.T.ServerTLSConnection
> ==========================
> 
> > Channels of this kind are never requested, are anonymous
> 
> "always have _Requested_ = False, _TargetHandleType_ = None and _TargetHandle_
> = 0, and cannot be requested with methods such as _CreateChannel_" with
> hyperlinks, please.

Fixed in 5d903175a252b6e543c904ae0f7220ce5f9f9ef7
 
> > MAY just close the channel
> 
> Please hyperlink to Close.

Fixed in 4e150176864c7789c466c19ecb50bc78d323c082

[1] http://tools.ietf.org/html/rfc1421

In RejectError:
> is not meaningful, ans SHOULD

"and"

In RejectDetails:
> +           <pre>
> +             { 'expected-hostname': 'gmail.com' }
> +             { 'certificate-hostname': 'evil.example.org' }
> +           </pre>

The conventional pseudocode for dictionaries is to use Python/JSON syntax, in which the whole dictionary is wrapped in {}, and items are separated by commas:

    { 'expected-hostname': 'gmail.com',
      'certificate-hostname': 'evil.example.org' }

or 

    {
      'expected-hostname': 'gmail.com',
      'certificate-hostname': 'evil.example.org',
    }

or however you want to indent it.

In Connection:
>                that indicates the hostname that could not be found.</p>
>            </tp:rationale>
> +
> +         <dl>
> +           <dt>expected-hostname (s), certificate-hostname (s)</dt>

Instead of making a new <dl>, I'd prefer including these in the existing <dl>, above the <tp:rationale>. Indeed, now that we have a practical use for ConnectionErrorDetails, you could delete the "maybe we'll use this in future" rationale :-)

(In reply to comment #7)
> I already added Insecure_Algorithm to the D-Bus errors in a previous commit
> (e243001481350703aec717fcea57c7c40412ad4b); we could probably add it to the
> enum in Connection too, I think it's worth.

Please do keep all three places in sync. Also, Bug #23931 tries to add Revoked, which I hadn't spotted; is that available in all three lists too?

I believe Vivek called it Insecure rather than Insecure_Algorithm in Bug #23931 because the algorithm isn't necessarily the part that's insecure: consider 256-bit RSA, for instance :-)

> I think Limit_Exceeded only makes sense if we also document limits for the
> number of bits for an acceptable certificate and the depth of the certificate
> chain, and I don't think it's something worth doing for now.

As I understand it, Limit_Exceeded was intended to represent limits hard-coded into GNUTLS, to stop people performing denial of service attacks by providing massive certificates that will take hours of computation to verify? It's probably more useful as a Connection_Error than here, but we should keep the TLS-related things in sync.
 
> After having looked at the documentation of the TLS libraries we will use, and
> having talked at GUADEC with some crypto people, I got convinced we should just
> use DER here instead of PEM, not because it's poorly specified (see RFC 1421
> [1]), but because PEM is just DER encoded in a different way, and DER is the
> most simple and widely supported format anyway.

Sounds good to me.

(The reason I thought PEM-for-certificates was poorly specified is that RFC 1421 seems to be the reference document for PEM, but it makes no mention of the text "BEGIN CERTIFICATE" that in practice everyone uses, or indeed (AFAICS) how to encode a standalone certificate!)

I forgot to mention before: in the crypto libraries we'll be using, is it more convenient for CertificateChainData to be an aay, or would it be more conventional to have the entire certificate chain as a single blob of bytes?

Also, should we be specifying an order for the certificate chain, at least for the X.509 case? I think there's some convention that the cert being verified should be at one end, intermediate CAs should be in the middle, and the root CA cert (if present) should be at the other, but I can never remember which direction the chain goes in.

(In reply to comment #8)
> In RejectError:
> > is not meaningful, ans SHOULD
> 
> "and"

I fixed the typo in 361b443981dc7550169344bc9c0522152caa36e2

> The conventional pseudocode for dictionaries is to use Python/JSON syntax, in
> which the whole dictionary is wrapped in {}, and items are separated by commas:
> 
>     { 'expected-hostname': 'gmail.com',
>       'certificate-hostname': 'evil.example.org' }
> 
> or 
> 
>     {
>       'expected-hostname': 'gmail.com',
>       'certificate-hostname': 'evil.example.org',
>     }
> 
> or however you want to indent it.

I went for the latter form, and fixed it in 6ae8d8ada1c0550f75111c4c4c438e966edd6715
 
> Instead of making a new <dl>, I'd prefer including these in the existing <dl>,
> above the <tp:rationale>. Indeed, now that we have a practical use for
> ConnectionErrorDetails, you could delete the "maybe we'll use this in future"
> rationale :-)

This makes sense, I changed it in 565766c307fdba26cf17d29eeb2d9cbb651790e2
 
> Please do keep all three places in sync. Also, Bug #23931 tries to add Revoked,
> which I hadn't spotted; is that available in all three lists too?
> 
> I believe Vivek called it Insecure rather than Insecure_Algorithm in Bug #23931
> because the algorithm isn't necessarily the part that's insecure: consider
> 256-bit RSA, for instance :-)

Yeah, good point. I synced the three lists in 9c351609b05427552dc669a7a0bc763cf16633c5, which also renames Insecure_Algorithm to Insecure.

> As I understand it, Limit_Exceeded was intended to represent limits hard-coded
> into GNUTLS, to stop people performing denial of service attacks by providing
> massive certificates that will take hours of computation to verify? It's
> probably more useful as a Connection_Error than here, but we should keep the
> TLS-related things in sync.

By looking at GnuTLS and OpenSSL documentation, it seems those limits are not hardcoded, and there are APIs to tweak them (e.g. [1]). My take was that it's kind of pointless to expose that kind of detail in an error, if we can't act on the limits themselves.

[1] http://www.gnu.org/software/gnutls/manual/gnutls.html#index-gnutls_005fcertificate_005fset_005fverify_005flimits-100

(In reply to comment #7)
> I think Limit_Exceeded only makes sense if we also document limits for the
> number of bits for an acceptable certificate and the depth of the certificate
> chain, and I don't think it's something worth doing for now.

Why would the Telepathy spec be the thing choosing those limits? It's an implementation detail of the connection manager (and/or the cert-verifying UI), surely?

Or, do you mean that the CM and/or UI should advertise their limits on D-Bus? I don't see any need for that - the certificate came from the server, so either we're willing to accept it, or we're not, and capability discovery isn't useful.

I think it's useful to tell the user why a cert was rejected: They may 
not understand the reason themselves, or be able to do anything about 
it, but it's handy for whoever the user runs to to be able to see what 
happened, otherwise they're left with a frustrating "cause of death: 
it died" situation.

(In reply to comment #9)
> I forgot to mention before: in the crypto libraries we'll be using, is it more
> convenient for CertificateChainData to be an aay, or would it be more
> conventional to have the entire certificate chain as a single blob of bytes?

At least OpenSSL and GnuTLS deal with both single certificates and certificate lists, so it's probably more useful to have it here as a list.

> Also, should we be specifying an order for the certificate chain, at least for
> the X.509 case? I think there's some convention that the cert being verified
> should be at one end, intermediate CAs should be in the middle, and the root CA
> cert (if present) should be at the other, but I can never remember which
> direction the chain goes in.

Yeah, good point. I find the usual order is the peer's certificate first, with the root CA on the other end. I added some notes about this in 478315b4c70a4085e9329c6b6f338a3ac6067565

(In reply to comment #11)
> Why would the Telepathy spec be the thing choosing those limits? It's an
> implementation detail of the connection manager (and/or the cert-verifying UI),
> surely?
> 
> Or, do you mean that the CM and/or UI should advertise their limits on D-Bus? I
> don't see any need for that - the certificate came from the server, so either
> we're willing to accept it, or we're not, and capability discovery isn't
> useful.

Agreed. I originally meant that those limits could be advertised on the bus, but it probably doesn't make much sense.

(In reply to comment #12)
> I think it's useful to tell the user why a cert was rejected: They may 
> not understand the reason themselves, or be able to do anything about 
> it, but it's handy for whoever the user runs to to be able to see what 
> happened, otherwise they're left with a frustrating "cause of death: 
> it died" situation.

Fair enough; I went ahead and added Limit_Exceeded as a possible value in 17df3f31a5c5b3d1cd4e5493728a47854b72345c

In Connection.xml:
> -      <tp:enumvalue suffix="Cert_Other_Error" value="13">
...
> +      <tp:enumvalue suffix="Cert_Other_Error" value="16">

This is an incompatible change to a non-DRAFT interface; I hope you can see why it's incompatible?

You'll have to add the new members at the very end of the enum instead, with correspondingly large numbers; this will mean that Cert_Other_Error isn't the last cert error, but such is life.

In errors.xml:
> -        This corresponds to Insecure_Algorithm
> +        This corresponds to Cert_Insecure in the 
> +       <tp:type>Connection_Status_Reason</tp:type> enum and to Insecure_Algorithm

I think you mean Insecure; I notice you've also added trailing whitespace on the line above, please fix that too.

After fixing these, I think this should be ready to merge as a draft, but please let me have another look first (since it touches stable API).

(In reply to comment #14)
> This is an incompatible change to a non-DRAFT interface; I hope you can see why
> it's incompatible?
> 
> You'll have to add the new members at the very end of the enum instead, with
> correspondingly large numbers; this will mean that Cert_Other_Error isn't the
> last cert error, but such is life.

Oh yeah, my bad. I fixed this in d87005f010963798fccfca01912baf94ac694503

> In errors.xml:
> > -        This corresponds to Insecure_Algorithm
> > +        This corresponds to Cert_Insecure in the 
> > +       <tp:type>Connection_Status_Reason</tp:type> enum and to Insecure_Algorithm
> 
> I think you mean Insecure; I notice you've also added trailing whitespace on
> the line above, please fix that too.

Fixed in b054b7b6b62ca8b2cffbf71505197926bbed8fb2

I think this is ready to go as a draft. After merging it, please leave this bug open with the patch keyword removed (for the un-drafting process), but close Bug #23931 :-)

(In reply to comment #16)
> I think this is ready to go as a draft. After merging it, please leave this bug
> open with the patch keyword removed (for the un-drafting process), but close
> Bug #23931 :-)

Thanks, I merged this to master.

Draft 1 was in spec 0.19.11.

Do we have CM implementations of the draft? Please reference them as bugs that block this one.

Do we have a client implementation of the draft? I believe we do, in Empathy?

Any feedback from the implementation process?

(In reply to comment #18)
> Draft 1 was in spec 0.19.11.
> 
> Do we have CM implementations of the draft? Please reference them as bugs that
> block this one.

Yes, we have an implementation in Gabble, that was bug 29458 (now FIXED).

> Do we have a client implementation of the draft? I believe we do, in Empathy?

Yes, see https://bugzilla.gnome.org/show_bug.cgi?id=626848

> Any feedback from the implementation process?

Yes; thanks for reminding me this. It might happen that the verification process for a certificate finds more than a reason why the certificate would not be valid (e.g. the certificate could be at the same time self-signed and not matching the right hostname).
If you see e.g. Firefox, when you connect to a site whose certificate has more than one issue, it displays all of them at the same time in the UI; this isn't currently do-able with this specification, as the reject reason is a single enumeration value.

So, I think it'd be good to change Reject() on Auth.TLSCertificate to take an array of (u: Reason) instead of a single one; this would also imply that the RejectReason property becomes RejectReasons (au).
If you think it's a good idea too, I will provide a tp-spec branch for this, and fix Gabble/Empathy accordingly.

We'd like to undraft this today or tomorrow, for Empathy 2.32. I think the only pending change is this one:

(In reply to comment #19)
> It might happen that the verification
> process for a certificate finds more than a reason why the certificate would
> not be valid (e.g. the certificate could be at the same time self-signed and
> not matching the right hostname).
> If you see e.g. Firefox, when you connect to a site whose certificate has more
> than one issue, it displays all of them at the same time in the UI; this isn't
> currently do-able with this specification, as the reject reason is a single
> enumeration value.

If we do this, the D-Bus error name and the details should also be repetitive. Two straw-man APIs:

Repeated rejection
==================

Add struct TLS_Certificate_Rejection = ( u, s, a{sv} )

Change Reject so if it's called repeatedly, the second and subsequent calls are secondary rejection reasons, which the CM MAY either use or ignore [1]

Allow Rejected to be emitted repeatedly, once per Reject call

Remove the Reject* properties and replace them with Rejections: a(us{asv}), TLS_Certificate_Rejection[], defined such that the first rejection in the list MAY be assumed to be "the most important"

Update Gabble with whichever semantics from [1] are easier

Update Empathy and make it just look at the first thing in Rejections (defensive programming: if Rejections is empty, treat it as unknown error)

Multi-rejection
===============

The same, except change the signature of Reject to a(usa{sv}) -> nothing, forbid calling Reject with an empty list, change the signature of Rejected to a(usa{sv}), and keep the second and subsequent calls to Reject as ignored

+        The reason why a TLS certificate was rejected.

How about:

    Struct representing one reason why a TLS certificate was rejected.

    Since there can be multiple things wrong with a TLS certificate,
    arrays of this type are used to represent lists of reasons for
    rejection. The most important reason SHOULD be placed first in the
    list.

> +          The new value of thje <tp:member-ref>Rejections</tp:member-ref> property.

s/thje/the/

Gabble
======

> +  DEBUG ("TLS certificate rejected with rejections %p, long %u.",
> +      rejections, rejections->len);

length %u

Assuming you consider an empty list of rejections to be an internal error, this should probably g_assert (rejections->len >= 1).

> +  rejection = g_ptr_array_index (rejections, 0);

Assuming you consider an empty list of rejections to be an internal error, this needs a g_assert (rejections->len >= 1) before you dereference it.

> +  g_ptr_array_unref (rejections);

This will leak the GValueArrays. You need to use g_boxed_free (GABBLE_ARRAY_TYPE_TLS_CERTIFICATE_REJECTION_LIST, rejections) to free the contents too.

> +  tp_clear_pointer (&self->priv->rejections, g_ptr_array_unref);

Likewise. You can use tp_clear_boxed to combine the effects of tp_clear_pointer and g_boxed_free. (This occurs at least twice in the TLSCertificate.)

> +  DEBUG ("Reject() called on the TLS certificate with rejections %p, "
> +      "long %u; current state %u", rejections, rejections->len,

length %u

You should either raise InvalidArgument if the list has length 0, or treat an empty list as equivalent to this one-element list:

    [(Unknown, TP_ERROR_STR_CERT_INVALID, {})]

(probably by copying the empty list to the property and then 

Your choice. Please mention which way you want to go in the spec.

Empathy
=======

Guillaume pointed out a memory leak on IRC. I don't have any objections beyond that.

(In reply to comment #22)
> Gabble
> ======

I fixed these issues with the code; also, I set the behaviour of Reject() to reply 'Invalid_Argument' if the list doesn't have at least one element, and changed the spec accordingly.

> Empathy
> =======
> 
> Guillaume pointed out a memory leak on IRC. I don't have any objections beyond
> that.

Fixed.

This was undrafted in 0.19.13.