Subject: Re: [xmpp] Alexey Melnikov's DISCUSS and COMMENT on draft-ietf-xmpp-3920bis-19

From: Peter Saint-Andre <[email-masked]>
Date: Mon, 29 Nov 2010 14:21:59 -0700

[cc'ing [email-masked] to keep the WG in the loop...]

On 11/28/10 2:16 PM, Alexey Melnikov wrote:
> Alexey Melnikov has entered the following ballot position for
> draft-ietf-xmpp-3920bis-19.
> 
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
> 
> Please refer to http://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> [This is the initial version of my comments. I only finished reading section 6.6 at this point.]

OK.

> I am not really surprised, but this is a very well written document. 

But it's long! Sorry about that...

> Some quick comments/questions before I can recommend approving it:
> 
> 1)
> 4.5.3.  Idle Peer
> 
>    Even if the underlying TCP connection is alive and the stream is not
>    broken, the peer might have sent no stanzas for a certain period of
>    time.  In this case, the peer SHOULD close the stream using the
>    handshake described under Section 4.4.
> 
> SHOULD close the stream, or SHOULD use the procedure in Section 4.4 if it decides to close it?

This might be clearer:

   In this case, the peer itself SHOULD close the stream
   (as described under <xref target='streams-close'/>)
   rather than leaving an unused stream open.

>    If the idle peer does not
>    close the stream, the other party MAY either close the stream using
>    the handshake described under Section 4.4 or return a stream error
>    (e.g., <resource-constraint/> if the entity has reached a limit on
>    the number of open TCP connections or <policy-violation/> if the
>    connection has exceeded a local timeout policy).  However, consistent
>    with the order of layers (specified under Section 13.3), the other
>    party is advised to verify that the underlying TCP connection is
>    alive and the stream is unbroken (as described above) before
>    concluding that the peer is idle.  Furthermore, it is preferable to
>    be liberal in accepting idle peers, since experience has shown that
>    doing so improves the reliability of communication over XMPP networks
>    and that it is typically more efficient to maintain a stream between
>    two servers than to aggressively timeout such a stream.
> 
> This is arguing for MAY above.

The SHOULD above is about the idle peer, not the entity to which the
idle peer is connected. I've tried to make that clearer in the proposed
modification above.

> 2)
> 4.6.4.  xml:lang
> 
>    o  If the receiving entity supports a language that closely matches
>       the initiating entity's preferred language (e.g., "de" instead of
>       "de-CH"),
> 
> I think you really need to be referencing Section 3.4 of RFC 4647 for this,
> as the process you describe above is not very formal (and might introduce
> wrong results if coded by a naive implementor).

That is an excellent suggestion. Proposed text:

   o  If the receiving entity supports a language that matches the
      initiating entity's preferred language according to the "lookup
      scheme" specified in Section 3.4 of [LANGMATCH] (e.g., "de"
      instead of "de-CH"), then the value of the 'xml:lang' attribute
      SHOULD be the identifier for the matching language (e.g., "de").

Where [LANGMATCH] = RFC 4647.

> 3)
> 5.3.5.  TLS Renegotiation
> 
>    Support for TLS renegotiation is strictly OPTIONAL.  However,
>    implementations that support TLS renegotiation MUST implement and use
>    the TLS Renegotiation Extension [TLS-NEG].
> 
> This makes [TLS-NEG] a Normative Reference.

Fixed.

> 4)
> 6.4.2.  Initiation
> 
>    If the initiating entity subsequently sends another <auth/> element
>    (even if the ongoing authentication handshake has not yet completed),
>    the server SHOULD discard the ongoing handshake and begin a new
>    handshake for the subsequently requested SASL mechanism.
> 
> What are the possible conditions for violating the SHOULD?

IIRC, that was not discussed within the WG.

> The server really MUST fail the existing handshake, but maybe it doesn't have
> to begin a new handshake (e.g. it might choose to close the connection instead?).

You are suggesting:

   If the initiating entity subsequently sends another <auth/> element
   and the ongoing authentication handshake has not yet completed, the
   server MUST discard the ongoing handshake and SHOULD begin a new
   handshake for the subsequently requested SASL mechanism.

That seems fine.

As for violating the SHOULD, perhaps the initiating entity has attempted
to fall back too many different authentication mechanisms. See similar
text about retries in Section 6.4.5:

   If the initiating entity exceeds the number of retries, the receiving
   entity MUST return a stream error, which SHOULD be <policy-
   violation/>...

Do you think that it would be helpful to specify this scenario more
completely? For example, we could say:

   If the initiating entity subsequently sends another <auth/> element
   and the ongoing authentication handshake has not yet completed, the
   server MUST discard the ongoing handshake and MUST either begin a new
   handshake for the subsequently requested SASL mechanism or return a
   stream error, which SHOULD be <policy-violation/>.

> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> 1.4.  Terminology
> 
>    The term "whitespace" is used to refer to any character that matches
>    production [3] content of [XML], i.e., any instance of the SP, HTAB,
>    CR, or LF rules defined in [ABNF].
> 
> Any single character or multiple characters?

Good catch. That should be:

   .. one or more instances of...

> 3.2.1.  Preferred Process: SRV Lookup
> 
>    3.  If a response is received, it will contain one or more
>        combinations of a port and hostname, each of which is weighted
>        and prioritized as described in [DNS-SRV].  However, if the
>        result of the SRV lookup is a single resource record with a
>        Target of ".", i.e. the root domain, then the initiating entity
>        MUST abort SRV processing at this point (but SHOULD attempt the
>        fallback process described in the next section).
> 
>  [...]
> 
>    7.  If the initiating entity fails to connect using all resolved IP
>        addresses for a given hostname, then it repeats the process of
>        resolution and connection for the next hostname returned by the
>        SRV lookup.
> 
> While this is implied, it might be better to point out that the "next" is
> selected based on priority/weight from [DNS-SRV].

Fixed.

> 3.2.2.  Fallback Processes
> 
>    For client-to-server connections, the fallback MAY be a [DNS-TXT]
> 
> Is this Normative? It looks like it is.

Sure. Fixed.

> 3.2.3.  When Not to Use SRV
> 
>    If the initiating entity has been explicitly configured to associate
>    a particular hostname (and potentially port) with the origin domain
>    of the receiving entity (say, to "hardcode" an association from an
>    origin domain of example.net to a configured hostname of
>    webcm.example.com:80), the initiating entity SHOULD use the
> 
> Is use of port 80 a good example in this document?

Probably not. :) In my working copy I've changed "webcm.example.com:80"
to "apps.example.com".

> 4.2.2.  Stream Features Format
> 
>    A <features/> element MAY contain more than one mandatory feature.
>    This means that the initiating entity can choose among the mandatory
>    features.  For example, perhaps a future technology will perform
>    roughly the same function as TLS, so the receiving entity might
>    advertise support for both TLS and the future technology.
> 
> So if the server requires both TLS and SASL, does this mean that it first
> advertises TLS as required (and SASL is either advertised as optional or not advertised),
> and then once TLS negotiation is complete, SASL needs to be advertised as required
> (on the restarted stream)?

Yes.

I propose the following adjusted text to make this clearer:

   A <features/> element MAY contain more than one mandatory-to-
   negotiate feature.  This means that the initiating entity can choose
   among the mandatory-to-negotiate features at this stage of the stream
   negotiation process.  As an example, perhaps a future technology will
   perform roughly the same function as TLS, so the receiving entity
   might advertise support for both TLS and the future technology at the
   same stage of the stream negotiation process.  However, this applies
   only at a given stage of the stream negotiation process and does not
   apply to features that are be mandatory-to-negotiate at different
   stages (e.g., the receiving entity would not advertise both STARTTLS
   and SASL as mandatory, or both SASL and resource binding as
   mandatory, because TLS would need to be negotiated before SASL and
   SASL would need to be negotiated before resource binding).

> 4.2.7.  Flow Chart
> 
> In the flowchart - is "receive stream features" required after each negotiation
> of a voluntary feature?

Section 4.2.4 states:

   The receiving entity MUST send an updated list of stream features to
   the initiating entity after a stream restart, and MAY do so after
   completing negotiation of a stream feature that does not require a
   stream restart.  The list of updated features MAY be empty if there
   are no further features to be advertised or MAY include any
   combination of features.

I've changed the flow chart to:

                           |                                   |
                           v                                   |
                    +----------------+                         |
                    | receive stream |                         |
+------------------>| features       |                         |
^   {OPTIONAL}      +----------------+                         |
|                          |                                   |

> 4.6.  Stream Attributes
> 
>    The attributes of the root <stream/> element are defined in the
>    following sections.
> 
>       Security Note: Until and unless the confidentiality and integrity
>       of a stream header is ensured via Transport Layer Security as
>       described under Section 5, the attributes provided in a stream
>       header could be tampered with by an attacker.
> 
> Or SASL security layer (e.g. GSSAPI).

Good point. Fixed.

> 4.7.2.  Content Namespace
> 
>    When a content namespace is not declared as the default namespace and
>    so-called "prefix-free canonicalization" is used instead, in rough
>    outline a stream will look something like the following.
> 
>    <stream
>        from=[email-masked]'
>        to='im.example.com'
>        version='1.0'
>        xml:lang='en'
>        xmlns='http://etherx.jabber.org/streams'>
>      <message xmlns='jabber:client'>
>        <body>foo</body>
>      </message>
>    </stream:stream>
> 
> Should the closing tag be </stream> above?

Yes, fixed.

> 4.7.4.  Namespace Declarations and Prefixes
> 
>    An implementation MUST NOT generate namespace prefixes for elements
>    qualified by the content namespace if the content namespace is
>    'jabber:client' or 'jabber:server' (e.g., <foo:presence/>).  An XMPP
> 
> Can you try to explain this again, you lost me here.
> 
>    entity MUST NOT accept data that violates this rule (in particular,
>    an XMPP server MUST NOT route or deliver such data to another
>    entity); instead it MUST either ignore the data or return a stream
>    error (which SHOULD be <bad-namespace-prefix/>).

This point can indeed be confusing and resulted in a long discussion
within the WG recently.

I think an example would help, so perhaps this is a bit clearer:

###

   An implementation MUST NOT generate namespace prefixes for elements
   qualified by the content namespace (i.e., the default namespace for
   data sent over the stream) if the content namespace is 'jabber:
   client' or 'jabber:server'.  For example, the following is illegal:

   <stream:stream
       from=[email-masked]'
       to='im.example.com'
       version='1.0'
       xml:lang='en'
       xmlns='jabber:client'
       xmlns:stream='http://etherx.jabber.org/streams'>
     <foo:message xmlns:foo='jabber:client'>
       <foo:body>foo</foo:body>
     </foo:message>
   </stream:stream>

   An XMPP entity MUST NOT accept data that violates this rule (in
   particular, an XMPP server MUST NOT route or deliver such data to
   another entity); instead it MUST either ignore the data or return a
   stream error (which SHOULD be <bad-namespace-prefix/>).

###

> 5.4.3.1.  Rules
> 
>    7.  Following successful TLS negotiation, all further data
>        transmitted by either party MUST be encrypted.
> 
> Did you really meant "encrypted" here? What about use of NULL ciphers
> with integrity protection?

We forgot about those.

Is this more accurate?

   7.  Following successful TLS negotiation, all further data
       transmitted by either party MUST be protected with the negotiated
       algorithms, keys, and secrets (i.e., encrypted, integrity-
       protected, or both depending on the ciphersuite used).

> 5.4.3.3.  TLS Success
> 
>    R: <stream:features>
>         <mechanisms xmlns='urn:ietf:params:xml:ns:xmpp-sasl'>
>           <mechanism>EXTERNAL</mechanism>
>           <mechanism>PLAIN</mechanism>
>         </mechanisms>
>       </stream:features>
> 
> This should include SCRAM-SHA-1 :-), as it is an MTI.

Good point. Changed to:

R: <stream:features>
     <mechanisms xmlns='urn:ietf:params:xml:ns:xmpp-sasl'>
       <mechanism>EXTERNAL</mechanism>
       <mechanism>SCRAM-SHA-1-PLUS</mechanism>
       <mechanism>SCRAM-SHA-1</mechanism>
       <mechanism>PLAIN</mechanism>
     </mechanisms>
   </stream:features>

> 6.3.3.  Mechanism Preferences
> 
>    Any entity that will act as a SASL client or a SASL server MUST
>    maintain an ordered list of its preferred SASL mechanisms according
>    to the client or server, where the list is ordered according to local
>    policy or user configuration (which SHOULD be in order of perceived
>    strength to enable the strongest authentication possible).  A server
>    MUST offer and a client MUST try SASL mechanisms in preference order.
> 
> Is this clear that server's and client's preference orders are different?

Section 6.4.1 states:

   The <mechanisms/> element MUST contain one <mechanism/> child element
   for each authentication mechanism the receiving entity offers to the
   initiating entity.  The order of <mechanism/> elements in the XML
   indicates the preference order of the SASL mechanisms according to
   the receiving entity; however the initiating entity MUST maintain its
   own preference order independent of the preference order of the
   receiving entity.

I think the last clause really belongs in Section 6.3.3.

Thus Section 6.3.3:

   Any entity that will act as a SASL client or a SASL server MUST
   maintain an ordered list of its preferred SASL mechanisms according
   to the client or server, where the list is ordered according to local
   policy or user configuration (which SHOULD be in order of perceived
   strength to enable the strongest authentication possible).  The
   initiating entity MUST maintain its own preference order independent
   of the preference order of the receiving entity.  A server MUST offer
   and a client MUST try SASL mechanisms in preference order.  For
   example, if the server offers the ordered list "PLAIN SCRAM-SHA-1
   GSSAPI" or "SCRAM-SHA-1 GSSAPI PLAIN" but the client's ordered list
   is "GSSAPI SCRAM-SHA-1", the client MUST try GSSAPI first and then
   SCRAM-SHA-1 but MUST NOT try PLAIN (since PLAIN is not on its list).

and in Section 6.4.1:

   The <mechanisms/> element MUST contain one <mechanism/> child element
   for each authentication mechanism the receiving entity offers to the
   initiating entity.  As noted, the order of <mechanism/> elements in
   the XML indicates the preference order of the SASL mechanisms
   according to the receiving entity (which is not necessarily the
   preference order according to the initiating entity.

> 6.3.7.  Simple User Name
> 
>    Some SASL mechanisms (e.g., CRAM-MD5, DIGEST-MD5, and SCRAM) specify
>    that the authentication identity used in the context of such
>    mechanisms is a "simple user name" (see Section 2 of [SASL] as well
>    as [SASLPREP]).  The exact form of the simple user name in any
>    particular mechanism or deployment thereof is a local matter, and a
>    simple user name does not necessarily map to an application
>    identifier such as a JID or JID component (e.g., a localpart).
>    However, 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 a simple user name equal to the localpart of
>    the user's JID.
> 
> Or bare JID? I believe many implementations based on Cyrus SASL allow full email-like syntax here.

The WG had a long debate about this and settled on localpart as opposed
to bare JID. The matter was unclear in RFC 3920, resulting in confusion
among implementers.

In fact we should add the following to Appendix D:

  o Clarified that a SASL simple user name is the localpart, not
    the bare JID.

> 6.3.9.  Realms
> 
>    The receiving entity MAY include a realm when negotiating certain
>    SASL mechanisms.  If the receiving entity does not communicate a
>    realm, the initiating entity MUST NOT assume that any realm exists.
>    The realm MUST be used only for the purpose of authentication; in
>    particular, an initiating entity MUST NOT attempt to derive an XMPP
>    hostname from the realm information provided by the receiving entity.
> 
> Should this be clearer about which mechanisms are we talking about?
> IMHO, being vague is not being helpful here.

RFC 4422 was vague about this, too. :P

As far as I can see, DIGEST-MD5 has a concept of realm, but the concept
is absent from EXTERNAL, SCRAM, PLAIN, and CRAM-MD5. I'm not sure about
GSSAPI. Does that look right to you? I agree that it would be helpful to
clarify this, but only as examples because future SASL mechanisms might
or might not include the concept of a realm.

Looking forward to your review of Section 7 onward...

Peter

-- 
Peter Saint-Andre
https://stpeter.im/

---

From: Alexey Melnikov <[email-masked]>
Date: Mon, 29 Nov 2010 22:01:40 +0000

>[cc'ing [email-masked] to keep the WG in the loop...]
Hi Peter,
I've deleted items where I am happy with your new text. Remaining 

>   rather than leaving an unused stream open.
That is better.

 [...]

>   stream error, which SHOULD be <policy-violation/>.
If this list of outcomes is exhaustive, then yes.

>   SASL would need to be negotiated before resource binding).
Actually I thought TLS (with possible SCRAM-SHA-1/EXTERNAL/PLAIN) as an 
alternative to SASL GSSAPI (which can provide encryption).
So your new text is slightly wrong on that.
 [...]

>     <foo:message xmlns:foo='jabber:client'>
Is the namespace declaration for "foo" should also be included? Or is 
lack of it is the point here?

Yes.

>  o Clarified that a SASL simple user name is the localpart, not
I am thinking about multidomain servers which don't enforce localpart 
uniqueness across domains. That would be inconvenient.

But anyway, that is only a SHOULD :-).

>or might not include the concept of a realm.
I think both DIGEST-MD5 and GSSAPI use realms (and in somewhat different 
manner).

>Looking forward to your review of Section 7 onward...
I just finished section 12 :-).

---

From: Dave Cridland <[email-masked]>
Date: Mon, 29 Nov 2010 22:06:36 +0000

Nit-picking as things whizz past...

>    rather than leaving an unused stream open.
Why SHOULD? It's unclear to me that leaving an idle S2S stream up is  
a problem, especially given that the other end MAY close it if it is.  
(That is, I do not see an interoperability or security issue here to  
consider).

> idle peer is connected. I've tried to make that clearer in the  

I agree with Alexey, here - this text is arguing that the above  
behaviour is "truly optional".

>    stream error, which SHOULD be <policy-violation/>.
I would rather we picked a single behaviour. I don't care much what  
it is.

>    are no further features to be advertised or MAY include any

Really? I'd not noticed this before (I am aware that we had a  
discussion about unilaterally sending features before and the  
consensus seemed to be against that, but I acknowledge this is a  
different case to an extent). I'm somewhat nervous about how deployed  
clients might respond if we send updated features after resource  
binding, for example.

Consider me fruck out. :-)

>    stream error (which SHOULD be <bad-namespace-prefix/>).

It's not clear to me that an XMPP entity can fail to accept such  
data, because in many (most?) XML processors, the two forms are  
semantically equivalent. There's a sketchy argument that this is a  
SHOULD NOT accept.

Dave.
-- 
Dave Cridland - mailto:[email-masked] - xmpp:[email-masked]
  - acap://acap.dave.cridland.net/byowner/user/dwd/bookmarks/
  - http://dave.cridland.net/
Infotrope Polymer - ACAP, IMAP, ESMTP, and Lemonade

---

From: Alexey Melnikov <[email-masked]>
Date: Mon, 29 Nov 2010 22:09:23 +0000

[...]

> lack of it is the point here?

Never mind, it is in the example.

---

From: Peter Saint-Andre <[email-masked]>
Date: Mon, 29 Nov 2010 15:28:11 -0700

> I've deleted items where I am happy with your new text. Remaining

OK. Further comments (and trimming) inline.

> If this list of outcomes is exhaustive, then yes.

I think so.

A server implementer might think about sending a SASL error at the point
of receiving a subsequent <auth/> element, but I think that's a bad idea
because we don't have a SASL error defined for that scenario. Better to
either accept the new auth handshake attempt or close the stream.

> So your new text is slightly wrong on that.

If the server requires TLS, then it requires TLS. We do not have a
stream feature for "something that ensures the confidentiality and
integrity of the stream, so at this point in the stream negotiation
process it is mandatory to negotiate either TLS or a SASL mechanism that
ensures negotiation of a security layer". In terms of XMPP, the server
would do that by saying TLS is discretionary and then offering only SASL
mechanisms that can ensure negotiation of a security layer.

> lack of it is the point here?

The point is that it's evil to leave the "foo" declaration off the
stream and then attempt to send stanzas (or stanza-like data) that
declare "foo" as a namespace prefix for the default content namespace.

And the "foo" declaration should not be included on the stream header
because the following would make no sense (and might even be illegal
according to the "Namespaces in XML" specification):

   <stream:stream
       from=[email-masked]'
       to='im.example.com'
       version='1.0'
       xml:lang='en'
       xmlns='jabber:client'
       xmlns:foo='jabber:client'
       xmlns:stream='http://etherx.jabber.org/streams'>

> uniqueness across domains. That would be inconvenient.

Indeed, and that was one of the topics discussed by the WG before
settling on localpart.

Yes.

> I think both DIGEST-MD5 and GSSAPI use realms (and in somewhat different

OK. (RFC 2743 isn't very clear on the matter, but that's another topic.)

So:

   The receiving entity MAY include a realm when negotiating certain
   SASL mechanisms (e.g., both the GSSAPI and DIGEST-MD5 mechanisms
   allow the authentication exchange to include a realm, though in
   different ways, whereas the EXTERNAL, SCRAM, and PLAIN mechanisms do
   not).  If the receiving entity does not communicate a realm, the
   initiating entity MUST NOT assume that any realm exists.  The realm
   MUST be used only for the purpose of authentication; in particular,
   an initiating entity MUST NOT attempt to derive an XMPP hostname from
   the realm information provided by the receiving entity.

Peter

-- 
Peter Saint-Andre
https://stpeter.im/

---

From: Peter Saint-Andre <[email-masked]>
Date: Mon, 29 Nov 2010 15:54:42 -0700

On 11/29/10 3:06 PM, Dave Cridland wrote:
> Why SHOULD? It's unclear to me that leaving an idle S2S stream up is a
> problem, especially given that the other end MAY close it if it is.

Yes, MAY seems more sensible here.

> I would rather we picked a single behaviour. I don't care much what it is.

"Begin the new handshake" seems preferable, and most consistent with the
rest of the text. Closing the stream might force the entities to go
through TLS negotiation again (etc.).

> Really? I'd not noticed this before (I am aware that we had a discussion
> about unilaterally sending features before and the consensus seemed to
> be against that, but I acknowledge this is a different case to an
> extent). I'm somewhat nervous about how deployed clients might respond
> if we send updated features after resource binding, for example.
> 

Yes, we had some discussion about this many moons ago. The alternative
would be to forbid sending of stream features except right after sending
a stream header?

> It's not clear to me that an XMPP entity can fail to accept such data,
> because in many (most?) XML processors, the two forms are semantically
> equivalent. There's a sketchy argument that this is a SHOULD NOT accept.

Hmm...

---

From: Dave Cridland <[email-masked]>
Date: Mon, 29 Nov 2010 23:00:06 +0000

On Mon Nov 29 22:54:42 2010, Peter Saint-Andre wrote:
> discussion

That inherently feels like the status quo (ante bellum), and as such  
the safer option.

I would not be averse to this being negotiable at some point, but an  
extension is allowed to override behaviour, right?

Dave.

---

From: Peter Saint-Andre <[email-masked]>
Date: Mon, 29 Nov 2010 16:09:14 -0700

On 11/29/10 4:00 PM, Dave Cridland wrote:
> That inherently feels like the status quo (ante bellum), and as such the
> safer option.
> 

The matter was underspecified in RFC 3920, but opting for safety is fine
with me. Thus we would delete the following clause:

   , and MAY do so after completing negotiation of a stream feature
   that does not require a stream restart

Objections?

Peter

-- 
Peter Saint-Andre
https://stpeter.im/