[Bug 2267] Host matching uses modified hostname as well as original

[bugzilla-daemon at mindrot.org]

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.