[Bug 29018] Allow interactive TLS certificate verification

[bugzilla-daemon at freedesktop.org]

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

--- Comment #7 from Cosimo Cecchi <cosimoc at gnome.org> 2010-07-30 03:11:14 PDT ---
(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

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