openssh bugs - Aug 2014 - [Bug 2267] New: Host matching uses modified hostname as well as original

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

            Bug ID: 2267
           Summary: Host matching uses modified hostname as well as
                    original
           Product: Portable OpenSSH
           Version: 6.6p1
          Hardware: All
                OS: All
            Status: NEW
          Severity: normal
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: openssh at richard.birkett.com

Since some of the recent changes to hostname canonicalisation, use of
the HostName config option is now triggering a re-read of the
configuration, trying to find Host sections that match the *new*
hostname.  Arguably this behaviour might be useful, but it's a
significant functional change.

There is also a documentation bug here: the description of Host says
that even canonicalisation will not change the behaviour of Host
matching, whereas the description of CanonicalizeHostname says that it
will!  But even with canonicalisation on, only canonicalised hostnames
should be matched, not any explicit changes specfied by HostName.

More worryingly, the problem seems to affect "Match OriginalHost",
which is also documented only ever to match the text that was given on
the command-line (maybe modified by canonicalisation, depending which
section of the manpage you read).

The double-scan also introduces uncertainty about the order in which
sections are matched, which can have serious functional consequences in
complex config files.  Maybe all options should be thrown away before
the second scan, to avoid surprises.  Or perhaps simply say that
"CanonicalizeHostname yes" only takes effect for config lines that
come
after it in the file, rather than triggering a second scan at all.

The change that introduced the regression seems to be the one labelled
in the ChangeLog as "djm at cvs.openbsd.org 2014/02/23 20:11:36"
(ssh.c
revision 1.400), first released in 6.6p1.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
             Status|NEW                         |ASSIGNED

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
Created attachment 2465
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2465&action=edit
multiple canonicalisation / config parsing fixes

This patch contains a few fixes and one new related feature

1) Fix "match originalhost" to match against the real original
hostname

2) Make configuration reparsing depend on canonicalisation being
enabled, rather than the hostname changing for any reason

3) Improve the documentation to better describe the reparsing behaviour

4) Remove the incorrect assertion in ssh_config(5) that canonicalised
names don't match Host/Match directives (this was always the intention)

5) Add a "canonical" criteria for the "Match" configuration
directive
to allow creating of criteria that are only checked in the second
configuration parsing pass (i.e. Match blocks that work on names after
canonicalisation)

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

--- Comment #2 from Richard Birkett <openssh at richard.birkett.com> ---
That's great, thanks Damien.

A couple of compilation/typo errors:

- ssh-keysign.c doesn't compile, as it's still calling read_config_file
with the old function signature - adding an extra "" agument fixes the
compilation, though I'm not certain whether that's actually correct!

- With canonicalisation enabled, "Match canonical" is giving a
"Missing
Match criteria for canonical" error during the second pass - the
"continue" statement needs to apply to both branches of the "if
(!post_canon)" test.

Functionally, everything's much improved.  With "CanonicalizeHostname
no", the second scan is not happening, which is good.  "Match
host" and
"Match originalhost" seem to do what they're supposed to do. 
There are
a few oddities, though...

- "Host" is still matching the value of a preceding HostName option -
ie. it's behaving like "Match host", instead of "Match
originalhost",
which is what it did pre-6.6.

- With canonicalisation enabled, the second pass is triggered, but all
the tests (Host, Match canonical host, Match canonical originalhost)
now seem to match only the *uncanonicalised* hostname - so
canonicalisation has actually stopped working altogether.

But I really like the "canonical" keyword on Match.  In fact, this
feels like a better solution all round than allowing one option
(CanonicalizeHostname) to magically change the meaning of other options
(Host and Match).

A suggestion: can we deprecate the whole concept of "global"
canonicalisation, and do it specifically when parsing "Match canonical
[original]host"?  That would also avoid the double-parsing, which I
think can still have unintended consequences, even with the extra
checks you've added.

Unfortunately the problem is then deciding how to grandfather the
6.6-style behaviour into the more flexible framework.  Could we perhaps
make "CanonicalizeHostname yes" immediately abort parsing and start
again, with a flag set to treat plain "Host" and "Match
[original]host"
as if they were "Match canonical [original]host"?

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
(In reply to Richard Birkett from comment #2)
> - ssh-keysign.c doesn't compile, as it's still calling
> read_config_file with the old function signature - adding an extra
> "" agument fixes the compilation, though I'm not certain
whether
> that's actually correct!
serves me right for sending out a patch when I should have been asleep.
> - With canonicalisation enabled, "Match canonical" is giving a
> "Missing Match criteria for canonical" error during the second
pass
> - the "continue" statement needs to apply to both branches of the
> "if (!post_canon)" test.
Thanks - I'll fix this in the next revision of the patch.
> Functionally, everything's much improved.  With
> "CanonicalizeHostname no", the second scan is not happening,
which
> is good.  "Match host" and "Match originalhost" seem to
do what
> they're supposed to do.  There are a few oddities, though...
> 
> - "Host" is still matching the value of a preceding HostName
option
> - ie. it's behaving like "Match host", instead of "Match
> originalhost", which is what it did pre-6.6.
That's intentional and only triggered when canonicalisation is enabled.
I don't think it is unreasonable for the configuration parsing
behaviour to change when hostname canonicalisation is enabled.
> - With canonicalisation enabled, the second pass is triggered, but
> all the tests (Host, Match canonical host, Match canonical
> originalhost) now seem to match only the *uncanonicalised* hostname
> - so canonicalisation has actually stopped working altogether.
AFAIK only "match host" was incorrect here. Fixed in next patch.
> But I really like the "canonical" keyword on Match.  In fact,
this
> feels like a better solution all round than allowing one option
> (CanonicalizeHostname) to magically change the meaning of other
> options (Host and Match).
> 
> A suggestion: can we deprecate the whole concept of "global"
> canonicalisation, and do it specifically when parsing "Match
> canonical [original]host"?  That would also avoid the
> double-parsing, which I think can still have unintended
> consequences, even with the extra checks you've added.
> 
> Unfortunately the problem is then deciding how to grandfather the
> 6.6-style behaviour into the more flexible framework.  Could we
> perhaps make "CanonicalizeHostname yes" immediately abort parsing
> and start again, with a flag set to treat plain "Host" and
"Match
> [original]host" as if they were "Match canonical
[original]host"?
I considered that, but a) I don't want to add more flags
(canonicalisation added more than I wanted already) and b) I think it
would be even more difficult to reason about.

I also considered adding a "RereadConfiguration yes|no|if-canon" flag,
but I'd prefer to make something that doesn't need an extra flag and is
backwards compatible when canonicalisation is off (I don't mind
changing behaviour when it is enabled).

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2465|0                           |1
        is obsolete|                            |

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
Created attachment 2466
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2466&action=edit
attempt #2

This should fix the bugs in the previous patch. I've added a
RereadConfig option to control whether a second pass is made through
the configuration files, but I'm unsure whether I'll commit that - the
alternatives is to always reread if canonicalisation is enabled

I've also added negation of Match options, e.g. "Match !canonical
..."
to provide a way to force Match to trigger on the first config pass.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

--- Comment #5 from Richard Birkett <openssh at richard.birkett.com> ---
Hi Damien,

The diff doesn't include the changes to readconf.h, so all of the
defines are missing... can you upload that diff, please?  (Don't worry,
I know all about working-while-asleep!)

I've double-checked the previous patch, though, and Host is definitely
behaving like "Match host" (ie. taking HostName into account), even
with canonicalisation off.  I'll check the new patch as soon as
readconf.h is there.

I'd like to have a go at a proof of concept of my idea, since I still
have concerns about the double-pass parsing and its scope for
surprises.  Obviously it needs to be (a) easy to understand, and
unfortunately (b) backward compatible with the behaviour documented in
6.6 (though luckily the 6.6 docs are a little ambiguous!).

I know 6.7 is already in its final testing phase, but is there any
chance either of our solutions might still get into that?  Or are we
now targetting 6.8 (or 6.7.1)?

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2466|0                           |1
        is obsolete|                            |

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
Created attachment 2467
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2467&action=edit
attempt #3

This one includes readconf.h and ssh-keysign.c that I also missed.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2266

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
Unfortunately this is too late for 6.7 - we're only considering
portability fixes at this stage, but I expect that 6.8 won't be too far
away.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

--- Comment #8 from Richard Birkett <openssh at richard.birkett.com> ---
Thanks for attachment 2467 (attempt #3).

Looks much better!

I've put together a config file that tests all the different hostnames
(original from the command-line and HostName, bare and canonicalised)
against each of the different criteria, and the results are below. 
Each is Y (matched) or N (didn't match).

With canonicalisation off:
                                cmdline         HostName
                               bare  canon     bare  canon
Host                            Y     N         N     N
Match originalhost              Y     N         N     N
Match host                      N     N         Y     N
Match canonical originalhost    N     N         N     N
Match canonical host            N     N         N     N

With canonicalisation on, the second pass gives:
                                cmdline         HostName
                               bare  canon     bare  canon
Host                            Y++   N++       N     N
Match originalhost              Y     N         N     N
Match host                      N     N         Y     N
Match canonical originalhost    Y**   N**       N     N
Match canonical host            N     N         Y**   N**

** looks like a bug
++ also looks like a bug (but is actually the behaviour I'd prefer!)

My proposal would give this:
                                original        HostName
                               bare  canon     bare  canon
Host                            Y     N         N     N
Match originalhost              Y     N         N     N
Match host                      N     N         Y     N
Match canonical originalhost    N     Y         N     N
Match canonical host            N     N         N     Y

(with the global CanonicalizeHostname option causing the first three
lines to mirror the last two).

Incidently, RereadConfig seems to be defaulting to "no" instead of
"if-canon", I think because fill_default_options is called too late. 
So for the above tests I've set it to "if-canon" explicitly.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2467|0                           |1
        is obsolete|                            |

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
Created attachment 2469
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2469&action=edit
Attempt #4

Quite a few changes:

- Add support for dumping the configuration to stdout, much like "sshd
-T". This makes this quite a bit easier to test.

- Remove RereadConfig - the configuration is always re-parsed when
canonicalisation is active.

- Allow "Match canonical all", which is useful to provide a final
catchall config stanza

- Fixes for all the Match/Host consistency problems that I could find

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

--- Comment #10 from Damien Miller <djm at mindrot.org> ---
Created attachment 2470
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2470&action=edit
Regress test

Here is a regress test for the canonicalisation changes. It depends on
the config dump support added in the last patch. Unfortunately it
requires DNS or a modified /etc/hosts to work, so I don't want to
commit it in its current state.

To use it, copy it to the regression tests directory and run:

make SKIP_UNIT=1 LTESTS=clientmatch

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

Simon Deziel <simon at sdeziel.info> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |simon at sdeziel.info

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #11 from Damien Miller <djm at mindrot.org> ---
Patch applied - this will be in OpenSSH 6.8

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

--- Comment #12 from Richard Birkett <openssh at richard.birkett.com> ---
FYI, I am still working on an alternative fix for this issue which
removes the need to do the double-pass at all, and therefore makes the
parsing a lot more deterministic. I know the last diff has now been
pushed upstream, but I'll share my patch shortly anyway, so we have a
choice for 6.8 (or 6.9)!

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2267

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #13 from Damien Miller <djm at mindrot.org> ---
openssh-6.8 is released

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.