Bug 2398 - AuthenticationMethods doesn't have default value (inconsistency) and it accept empty value
Summary: AuthenticationMethods doesn't have default value (inconsistency) and it accep...
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sshd (show other bugs)
Version: 6.8p1
Hardware: Other Linux
: P5 enhancement
Assignee: Damien Miller
URL:
Keywords:
Depends on:
Blocks: V_7_3
  Show dependency treegraph
 
Reported: 2015-05-14 00:20 AEST by Jakub Jelen
Modified: 2016-08-02 10:41 AEST (History)
3 users (show)

See Also:


Attachments
proposed patch (3.83 KB, patch)
2015-05-14 00:20 AEST, Jakub Jelen
no flags Details | Diff
clear AuthenticationMethods=any in fill_default_server_options() (2.01 KB, patch)
2016-06-17 14:42 AEST, Damien Miller
no flags Details | Diff
with manual bits (2.01 KB, patch)
2016-06-17 14:47 AEST, Damien Miller
no flags Details | Diff
really with manual bits (3.33 KB, patch)
2016-06-17 14:56 AEST, Damien Miller
dtucker: ok+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelen 2015-05-14 00:20:11 AEST
Created attachment 2620 [details]
proposed patch

Based on previous bugzilla, we realized that it would be useful to have default value for this server option, because of:

 * First of all, it is inconsistent with all other options that are available in openssh.

 * Another issue is usability. You can't reset this option in match block to it's default value if other match block or default config changed this option.

Ex: I want to have all users to authenticate using public key and password, but I want exception for localhost to use any authentication method available. It would be nice, if we could do something like this:

>Match Address ::1
>    AuthenticationMethods any
>Match Address *
>    AuthenticationMethods publickey,password

There can be used workaround:
>Match Address !::1
>    AuthenticationMethods publickey,password

but it doesn't work, as stated in bz2397. Also it can get quite messy if you have more blocks like that.

To have this feature working, we need to choose value for ANY (proposed "any"), use this value as default (enforced by fill_default_server_options) and make sure that it is handled everywhere in the code consistently. There are few design consideration, before posting a patch:
 * We can't use just num_auth_methods == 0, because this is considered as not-defined and it can't override previously definde authentication methods
 * We can use enforce num_auth_methods == 1 && strcmp(auth_methods[0], "any"), but it is not much elegant from my POV, but best I have got.
 * We can use num_auth_methods == -1, but it would require few changes in more data types in application (currently defined as u_int, so we can't store here -1).


Also as I can see, there was not properly propagated change to bz2281 from our bugzilla which covered also empty values of AuthenticationMethods (also covered in attached patch).
Comment 1 Damien Miller 2016-02-26 14:44:30 AEDT
Retarget to openssh-7.3
Comment 2 Damien Miller 2016-02-26 14:47:25 AEDT
Retarget to openssh-7.3
Comment 3 Damien Miller 2016-06-17 14:42:46 AEST
Created attachment 2836 [details]
clear AuthenticationMethods=any in fill_default_server_options()

I think it would be simpler to handle it similarly to the way we do CLEAR_ON_NONE options, i.e. clear it in fill_default_server_options() if it holds a special value.
Comment 4 Damien Miller 2016-06-17 14:47:27 AEST
Created attachment 2837 [details]
with manual bits

Revised diff with a mention of AuthenticationMethods=any in sshd_config.5
Comment 5 Damien Miller 2016-06-17 14:56:12 AEST
Created attachment 2838 [details]
really with manual bits
Comment 6 Damien Miller 2016-06-17 15:08:22 AEST
patch applied, with an extra test for AuthenticationMethod=""

Will be released in openssh-7.3 - thanks!
Comment 7 Damien Miller 2016-08-02 10:41:34 AEST
Close all resolved bugs after 7.3p1 release