Re: [Curdle] Ben Campbell's Discuss on draft-ietf-curdle-ssh-ext-info-12: (with DISCUSS and COMMENT)

[denis bider <[e-mail]>]

> I do not see a response to my comment on the “*” note in section 2.4.
> Did that change as part of the rewrite of that section that you mention
below?

Aye, sorry for not mentioning that. Most of the text was kept, but it was
slightly rearranged. The sentence containing the inappropriate MUST was
replaced with:

"The timing of the second opportunity is chosen for the following reasons."


> Don’t get me started on center embedding in English :-)

:D


> The issue is redundant normative language, where it becomes
> ambiguous which piece of text is authoritative.

Aye - it does violate the one-definition rule.

I have changed the first mention as follows:

"Implementers' attention is called to Section 2.5., in particular the
requirement to tolerate any sequence of bytes - including null bytes at any
position - in an unknown extension's extension-value."


I have previously contemplated whether to add explicit mention of the
OpenSSH behavior up to 7.5. On the one hand, it is poor practice to
reference individual implementations. But on the other hand, this is a
widespread implementation that everyone will want to interoperate with. If
we provide no guidance about how to handle this bug, it will throw
implementers for a spin, and worse, the compatibility workarounds may not
be ideal. (E.g. people may avoid sending binary extension-values to ANY
version of OpenSSH, instead of just versions up to 7.5.)

For this reason, I have provisionally added the following section under the
"delay-compression" extension:


3.2.3.  Compatibility Note: OpenSSH up to 7.5

  This extension uses a binary extension-value encoding. OpenSSH clients
  up to and including version 7.5 advertise support to receive
  SSH_MSG_EXT_INFO, but disconnect on receipt of an extension-value
  containing null bytes. This is an error fixed in OpenSSH version 7.6.

  Implementations that wish to interoperate with OpenSSH 7.5 and earlier
  are advised to check the remote party's SSH version string, and omit
  this extension if an affected version is detected. Affected versions
  do not implement this extension, so there is no harm in omitting it.
  The extension SHOULD NOT be omitted if the detected OpenSSH version is
  7.6 or higher. This would make it harder for the OpenSSH project to
  implement this extension in a higher version, if they so choose.


This might be unusual, but since the bug exists in current versions, it
will stay relevant for 5+ years. I think it may be worthwhile to ensure
everyone's workarounds are consistent.

The alternative would be to change the spec to disallow binary
extension-values, but I think this is long-term inferior (especially since
OpenSSH has committed to fixing the bug).

denis






On Wed, Sep 13, 2017 at 10:22 PM, Ben Campbell <[e-mail]> wrote:

> Hi,
>
> Thanks for the response. Comments inline. I deleted sections that do not
> seem to need further discussion.
>
> I do not see a response to my comment on the “*” note in section 2.4. Did
> that change as part of the rewrite of that section that you mention below?
>
> Thanks!
>
> Ben.
>
>
> > On Sep 13, 2017, at 11:05 PM, denis bider <[e-mail]>
> wrote:
> >
> > > I don't think allowing specific extensions
> > > to add ordering requirement works.
> >
> > OK, fair point.
> >
> > That makes two votes against this. There is currently no known extension
> that needs this, and if one does arise, there exist other possible
> approaches for what is required. I have removed.
>
> Thanks! I will clear my discuss.
>
> […]
>
> >
> >
> > > -2.1, first paragraph: "Applications implementing this mechanism
> > > MUST add to the field "kex_algorithms", in their KEXINIT packet
> > > sent for the first key exchange, one of the following indicator names:"
> > > That's hard to parse.
> >
> > I'm guessing you're not a German speaker. :)
>
> Don’t get me started on center embedding in English :-)
>
> > The issue raised here seems to be that one needs to read to the end of
> the sentence to fully understand what is being added ("one of the following
> indicator names").
> >
> > No problem, rearranged sentence.
> >
> >
> > > -2.5, 2nd to last paragraph: "... applications MUST tolerate
> > > any sequence of bytes; including null bytes at any position;
> > > in an unknown extension’s extension-value."
> > > Redundant to similar normative statement in 2.3.
> >
> > Aye, this is redundant on purpose. If this is not followed, it is
> devastating to compatibility. OpenSSH in particular has this issue in
> currently released versions (including 7.5, the latest). They stated
> they'll fix this for 7.6, but a workaround is required to NOT send
> extension-values containing null bytes to OpenSSH servers.
> >
> > The bug is not on purpose, it's because they have multiple functions to
> decode an SSH string - some that allow for binary data, some that don't.
> They accidentally used the one that doesn't allow binary data to decode
> extension-values.
> >
> > That's bad - and easy to proliferate as long as the most commonly used
> extensions don't exercise binary data. Fortunately, OpenSSH doesn't hide
> its version string, so the compatibility workaround is straightforward.
> That's not the case for all implementations.
> >
> > This is super important not to miss. The spec is somewhat big, and
> implementers might read one part at one time, and another part at another.
> I would prefer that this is not missed by anyone.
>
> I have no objection to reinforcing a requirement. The issue is redundant
> normative language, where it becomes ambiguous which piece of text is
> authoritative. It’s not a real issue here because the statements are
> consistent (which is why I marked that as “editorial”. ), but it can become
> a maintenance issue in the future if the resulting RFC is updated or
> obsoleted.
>
> So my suggestion, which you can freely ignore, is to change one of the
> occurrences to use descriptive (that is, non 2119)  language.
>
> >
> > denis
> >
> >
> >
> > On Wed, Sep 13, 2017 at 1:30 PM, Ben Campbell <[e-mail]> wrote:
> > Ben Campbell has entered the following ballot position for
> > draft-ietf-curdle-ssh-ext-info-12: Discuss
> >
> > 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 https://www.ietf.org/iesg/statement/discuss-criteria.
> html
> > for more information about IESG DISCUSS and COMMENT positions.
> >
> >
> > The document, along with other ballot positions, can be found here:
> > https://datatracker.ietf.org/doc/draft-ietf-curdle-ssh-ext-info/
> >
> >
> >
> > ----------------------------------------------------------------------
> > DISCUSS:
> > ----------------------------------------------------------------------
> >
> > I plan to ballot "yes", but I want to discuss one point first. Alexey
> mentioned
> > this in his comments, but I think it's discuss worthy. Hopefully it's an
> easy
> > fix, and it may well be because I've missed something obvious. If people
> think
> > it really, really needs to be this way, I will clear--but I want to
> discuss it
> > first:
> >
> > - 2.5: The relative order in which extensions appear in an
> >   EXT_INFO message MUST be ignored by default; but an extension MAY
> >   specify that the order matters for that extension, in a specific way.
> >
> > I don't think allowing specific extensions to add ordering requirement
> works.
> > It opens up the possibility of incompatible ordering requirements across
> > extensions. As far as I can tell, the only control over this is the "IETF
> > Consensus" requirement for adding new extensions. I'm open to arguments
> that
> > this is good enough, but my knee-jerk response is that it puts an undue
> burden
> > on the consensus process for little return.
> >
> >
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> >
> > Substantive:
> >
> > -2.3: "This message is sent immediately after SSH_MSG_NEWKEYS, without
> delay."
> > That seems to be contradicted in section 2.4, which talks about two other
> > potential times.
> >
> > -2.4, last paragraph (asterisk note): "The message MUST be sent at this
> point
> > for the following reasons:" That's a confusing use of MUST. Do you mean
> that
> > the message MUST NOT be sent unless the reasons are true? Or that if the
> > reasons are true, the message MUST be sent? Or is this just a statement
> of
> > fact, in which case the 2119 keyword is not appropriate?
> >
> > -2.5, 2nd paragraph: "or it MAY be sufficient that only one party
> includes it"
> > That seems like a statement of fact rather than a grant of permission.
> If so,
> > the 2119 MAY is not appropriate.
> >
> > Editorial:
> >
> > -2.1, first paragraph: "Applications implementing this mechanism MUST
> add to
> > the field
> >   "kex_algorithms", in their KEXINIT packet sent for the first key
> >   exchange, one of the following indicator names:"
> >
> > That's hard to parse.  Suggestion:
> > "Applications implementing this mechanism MUST add one of the
> >  following indicator names to the "kex_algorithms" field for the first
> >  key exchange:"
> >
> > -2.5, 2nd to last paragraph: "... applications MUST
> >   tolerate any sequence of bytes; including null bytes at any position;
> >   in an unknown extension’s extension-value."
> >
> > Redundant to similar normative statement in 2.3.
> >
> >
> > _______________________________________________
> > Curdle mailing list
> > [e-mail]
> > https://www.ietf.org/mailman/listinfo/curdle
> >
>
>