This bug is to track the effort to add the ability to specify authentications that are "required, but not sufficient" to sshd. E.g. require public-key authentication before accepting password authentication.
Created attachment 807 [details] Patch to add RequiredAuthentications{1,2} options to sshd_config The configuration options aren't finalised yet and are subject to change. Otherwise, this patch is ready for testing. To use, specify these options in sshd_config: RequiredAuthentications1 method[,method...] RequiredAuthentications2 method[,method...] E.g. RequiredAuthentications1 rsa RequiredAuthentications2 public-key
This is probably going to change substantially, so don't use this patch as the basis of anything.
Created attachment 941 [details] Adds "RequireBothPasswordAndPubKey" authorization option to SSHd I'll attach my patch as well so it exists someplace else besides the mailing list archives. My patch is much more specific to just password/public key and only for SSH2, although I wrestled a bit with how to make it general. It adds a "RequireBothPasswordAndPubKey" authorization option to the SSHD server. It is a hack but I tried to keep it as clean as possible.
It seems that the request in bug id 1072 is so close to this one. However, they are mutually exclusive. It would be nice to hear from Openssh on which side they would decide to step.
No, 1072 is a completely different issue
Thnx for the reply. Just tested the AUTH-SELECT patch but before proceeding further let me put a secnario. If I configure to deny public key for a user with directive PubKeyAuthDeny of AUTH-SELECT patch. The user is allowed to do password auth although denied by public key. This is an expected one. If I also add requiredauthentications as public-key, both public key and password are denied for the user. This breaks the meaning tht a user denied by public key is allowed through password as per the directive PubKeyAuthDeny.
Comment on attachment 807 [details] Patch to add RequiredAuthentications{1,2} options to sshd_config This bit doesn't seem right: >+ /* Loop until the required authmethods are done */ >+ if (authenticated && options.required_auth1 != NULL) { >+ if (auth_remove_from_list(&options.required_auth1, >+ meth_name) != 1) >+ fatal("INTERNAL ERROR: authenticated method " >+ "\"%s\" not in required list \"%s\"", >+ meth_name, options.required_auth1); >+ debug2("do_authloop: required list now: %s", >+ options.required_auth1 == NULL ? >+ "DONE" : options.required_auth1); >+ authenticated = 0; Unless I'm misreading it, this can't complete. Once the last method in the list is successful, "authenticated" is set and options.required_auth1 is non-null. The final method is removed and "authenticated" set is zeroed so the loop goes around again...
auth_remove_from_list() returns NULL when the list is empty, so the normal authentication path will continue.
(In reply to comment #8) > auth_remove_from_list() returns NULL when the list is empty, so the normal > authentication path will continue. No, auth_remove_from_list() sets options.required_auth1 to NULL, but by that time you're already inside the "if" block and authenticated is zeroed unconditionally. I have updated the patch to -current and this part doesn't seem to work, although auth1.c has changed a lot and it's quite possible I missed something merging the patch.
Created attachment 1121 [details] Update attachment 807 [details] to -current. Updated patch 807 to -current. No (deliberate :-) changes.
Created attachment 1122 [details] Corrected patch 1121.
Comment on attachment 1122 [details] Corrected patch 1121. I think this is what is needed: >+ /* Loop until the required authmethods are done */ >+ if (authenticated && options.required_auth1 != NULL) { >+ if (auth_remove_from_list(&options.required_auth1, >+ meth_name) != 1) >+ fatal("INTERNAL ERROR: authenticated method " >+ "\"%s\" not in required list \"%s\"", >+ meth_name, options.required_auth1); >+ debug2("do_authloop: required list now: %s", >+ options.required_auth1 == NULL ? >+ "DONE" : options.required_auth1); if (options.required_auth1 == NULL) return; >+ authenticated = 0;
I don't follow: you only enter that block if authenticated is set and the list still has one or more entries. Once you are in the block, it is because you have hit a required method. When you exit the block, if you have completed all the required methods then the list is set to NULL and any subsequent authentication against an enabled method will succeed. Returning without clearing "authenticated" means that the last method in the list will succeed authentication, which is incorrect: these options are "required, but not sufficient" authentications to be completed first.
Created attachment 1123 [details] Fix a null pointer deref when given an invalid method name.
(In reply to comment #13) > Returning without clearing "authenticated" means that the last method in the > list will succeed authentication, which is incorrect: these options are > "required, but not sufficient" authentications to be completed first. Ah, I think this is the difference. I thought that all listed methods needed to be completed, but this seems to mean that you need all of the listed methods plus one more?
Yes, that is why I say they are "required, but not sufficient". Maybe this is too confusing... Perhaps we could do a syntax like: Authentications public-key,* hostbased,gssapi Where completion of an entire list completes authentication, and '*' (which may only appear at the end of a list) means "any method not yet completed".
Created attachment 1455 [details] Updates the RequireBothPasswordAndPubKey patch to 4.7p1 Made several small changes to update the original patch to 4.7p1.
*** Bug 1435 has been marked as a duplicate of this bug. ***
Created attachment 1518 [details] Updates RequireBothPasswordAndPubKey patch to current cvs snapshot Works with 5.0p1.
Created attachment 1521 [details] Update requiredauthentications patch to -current.
Would it be reasonable to add an option, similar to this one, that requires both public key and keyboard-iterative/pam authentication? I've experimented adapting this patch but auth2-chall and auth-pam behave quite differently than auth2-passwd and don't seem amenable to that model. Any suggestions?
Created attachment 1567 [details] Ports patch 1521 (BSD) to portable & -current The RequiredMethods patch is more general than RequireBothPasswordAndPubKey, but is written for OpenSSH/BSD. This patch works for Openssh/Portable. Note: sshd chokes if you specify RequiredAuthentications1 publickey; use RequiredAuthentications1 password RequiredAuthentications2 publickey Todo: get privsep working; test other authentication methods combinations.
I think we should take a look at this for the release after the next one (ie 5.4).
Created attachment 1667 [details] Updates RequierdMethods patch to -current Use the following sshd_config: UsePrivilegeSeparation no UsePAM no RequiredAuthentications1 password RequiredAuthentications2 publickey Get following error w/ UsePrivilegeSeparation yes debug2: input_userauth_request: try method password debug3: mm_auth_password entering debug3: mm_request_send entering: type 10 mm_request_receive_expect: read: rtype 10 != type 24 Need to add RequiredMethods logic to
Yes, let's get this going again.
The configuration below is incorrect. When using protocol 2, it should read: ... RequiredAuthentications2 password Also, there's no need to specify publickey in conjunction with other authentication methods because it will always be tried first (as specified in the rfc). You could use the following config if you want to use hostbased and password together (protocol 2): ... RequiredAuthentications2 hostbased RequiredAuthentications2 password (In reply to comment #24) > Created an attachment (id=1667) [details] > Updates RequierdMethods patch to -current > > Use the following sshd_config: > UsePrivilegeSeparation no > UsePAM no > RequiredAuthentications1 password > RequiredAuthentications2 publickey > > Get following error w/ UsePrivilegeSeparation yes > > debug2: input_userauth_request: try method password > debug3: mm_auth_password entering > debug3: mm_request_send entering: type 10 > mm_request_receive_expect: read: rtype 10 != type 24 > > Need to add RequiredMethods logic to
err, i mean: RequiredAuthentications2 hostbased,password (In reply to comment #26) > The configuration below is incorrect. When using protocol 2, it should > read: > > ... > RequiredAuthentications2 password > > Also, there's no need to specify publickey in conjunction with other > authentication methods because it will always be tried first (as > specified in the rfc). > > You could use the following config if you want to use hostbased and > password together (protocol 2): > > ... > RequiredAuthentications2 hostbased > RequiredAuthentications2 password > > (In reply to comment #24) > > Created an attachment (id=1667) [details] [details] > > Updates RequierdMethods patch to -current > > > > Use the following sshd_config: > > UsePrivilegeSeparation no > > UsePAM no > > RequiredAuthentications1 password > > RequiredAuthentications2 publickey > > > > Get following error w/ UsePrivilegeSeparation yes > > > > debug2: input_userauth_request: try method password > > debug3: mm_auth_password entering > > debug3: mm_request_send entering: type 10 > > mm_request_receive_expect: read: rtype 10 != type 24 > > > > Need to add RequiredMethods logic to
Created attachment 1768 [details] Works with privilege separation Added a bit of logic to auth2.c to get it working with privilege separation. Tested combinations of publickey+password, publickey+hostbased and password+hostbased authentication.
We are freezing for the OpenSSH 5.6 release. Retargetting these bugs to the next release.
Created attachment 1955 [details] Implements required methods Updated the required methods patch to -current Doesn't work with SELinux "./configure ... --with-selinux"
Updated patch to -current
Is there a patch for 5.8p1? This is a feature we really want, so if not then I might have time to update the existing patch. Right now it blows up with a lot of .rej files.
Created attachment 1999 [details] Updated to -current Updated patch to -current and 5.8p1. Appears to work with sshd_config->RequiredAuthentications2 publickey,password. Needs more testing.
Retarget unresolved bugs/features to 6.0 release
Retarget unresolved bugs/features to 6.0 release (try again - bugzilla's "change several" isn't)
Created attachment 2079 [details] Another approach to solution of the problem I've created another patch which solves the similar problem. There is new configuration items TwoFactorAuthentication and Second.*Authentication. If the TwoFactorAuthentication is not set the sshd work as usual. If is set then after the successful authentication the Second set without the method successfully used in first authentication is enabled and then is the second authentication cycle started. There is no need to work with short names like "kbdint" in the configuration file. This schema may be easily enlarged to new potential authentication method.
Paul, why do you say (in comment #30) that the patch doesn't work with SELinux? I tried your latest patch from comment #33, which I think is just updated to apply to the latest OpenSSH rather than really changed... and it seems to work fine. Is there something known to be wrong? What remains to be fixed before this patch can be merged?
The patch didn't work for me when I tested it with SELinux at that time. SELinux policy is constantly updated, so I'm not surprised it's working now. I'll check it out on my systems now.
(In reply to comment #33) > Created attachment 1999 [details] > Updated to -current > > Updated patch to -current and 5.8p1. Appears to work with > sshd_config->RequiredAuthentications2 publickey,password. I take it we've dropped the 'necessary but not sufficient' bit? This looks wrong: Don't we want SSHCFG_ALL in each of the new additions here?: @@ -451,6 +456,8 @@ static struct { { "trustedusercakeys", sTrustedUserCAKeys, SSHCFG_ALL }, { "authorizedprincipalsfile", sAuthorizedPrincipalsFile, SSHCFG_ALL }, { "kexalgorithms", sKexAlgorithms, SSHCFG_GLOBAL }, + { "requiredauthentications1", sRequiredAuthentications1 }, + { "requiredauthentications2", sRequiredAuthentications2 }, { "ipqos", sIPQoS, SSHCFG_ALL }, { NULL, sBadOption, 0 } };
Yes, my mistake. I'll add it in the next patch.
Created attachment 2084 [details] Another approach to solution of the problem (updated)
My use case for this is to run a PAM stack *after* pubkey authentication, and one environment in which I want to do that is for something like gitolite — where multiple people each have their own SSH key installed, but there is only one local user. We want to use keys *and* a one-time password. It would be really useful if the PAM stack could know *which* SSH key was used to authenticate. Then we can have an OTP setup for each human being rather than just having a single shared one. This kind of thing should probably do it. This makes the two-step authentication much more useful for us. diff --git a/auth2-pubkey.c b/auth2-pubkey.c index 137887e..68f1a6a 100644 --- a/auth2-pubkey.c +++ b/auth2-pubkey.c @@ -350,6 +350,12 @@ user_key_allowed2(struct passwd *pw, Key *key, char *file) verbose("Accepted certificate ID \"%s\" " "signed by %s CA %s via %s", key->cert->key_id, key_type(found), fp, file); +#ifdef USE_PAM + if (options.use_pam) { + do_pam_putenv("SSH_PUBKEY_TYPE", "X509"); + do_pam_putenv("SSH_PUBKEY", key->cert->key_id); + } +#endif xfree(fp); found_key = 1; break; @@ -365,6 +371,12 @@ user_key_allowed2(struct passwd *pw, Key *key, char *file) fp = key_fingerprint(found, SSH_FP_MD5, SSH_FP_HEX); verbose("Found matching %s key: %s", key_type(found), fp); +#ifdef USE_PAM + if (options.use_pam) { + do_pam_putenv("SSH_PUBKEY_TYPE", key_type(found)); + do_pam_putenv("SSH_PUBKEY", fp); + } +#endif xfree(fp); break; }
(In reply to comment #33) Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7ffff7f3f7e0 (LWP 3257)] 0x00007ffff7f9c32a in input_userauth_info_response (type=<optimized out>, seq=<optimized out>, ctxt=0x7ffff8213b90) at auth2-chall.c:344 344 userauth_finish(authctxt, authenticated, "keyboard-interactive", (gdb) p kbdintctxt->device->name Cannot access memory at address 0x0 (gdb) p kbdintctxt->device $7 = (KbdintDevice *) 0x0 I don't quite understand how the extra 'submethod' argument to userauth_finish() and auth_log() are relevant to this patch. Normally I would expect them to be part of a separate patch. It appears to be entirely cosmetic... part from the SEGV that it causes. So I fixed it thus without worrying too much about what it *should* have been: --- auth2-chall.c~ 2011-09-26 20:50:00.741593219 +0100 +++ auth2-chall.c 2011-09-26 22:18:41.119608430 +0100 @@ -342,7 +342,7 @@ input_userauth_info_response(int type, u } } userauth_finish(authctxt, authenticated, "keyboard-interactive", - kbdintctxt->device->name); + kbdintctxt->device?kbdintctxt->device->name:NULL); } void Note: This SEGV wasn't trivial to find. The symptom was just that mm_request_receive() got -EPIPE after the child process died. No hint about the SEGV was visible because a handler was installed. Even when running it in gdb it didn't show up until I set 'follow-fork-mode child'. Is this not a really bad thing?
Oh, that fixes the fact that the patch breaks keyboard-interactive authentication when it's the only form of authentication. But RequiredAuthentications2 publickey,keyboard-interactive still doesn't work: INTERNAL ERROR: authenticated method "keyboard-interactive/pam" not in required list "keyboard-interactive"
Created attachment 2096 [details] Updated version of original patch. Oh, *now* I see why we were splitting 'keyboard-interactive/pam' into the method 'keyboard-interactive' and the submethod 'pam', and why it's a necessary part of this patch. I've been spoiled by git users who put that kind of information into the commit comments. Here's an updated patch against the current git mirror, which should fix that by doing the same thing in monitor.c.
Hi & thanks for the patch. Question: I try to use this patch for the following issue. Default should be RequiredAuthentications2 publickey, password but within a "Match Address x.x.x.x/x" container I want to use simple publickey auth. Is it possible or could someone add a way to make it possible cause it seems like RequiredAuthentications2 will enforce to have two authentication methods always- Thanks for any help. Regards, Marc
I have few comments to patch: There are two parts with RequiredMethods[12] in sshd_config.5 and the second one overwrites RevokedKeys part. I would rename RequiredMethods part to RequiredAuthentications[12] Patch removes TrustedUserCAKeys from sshd_config.5 Are all these changes intended?
Retarget from 6.0 to 6.1
Retarget 6.0 => 6.1
Created attachment 2138 [details] fixes of original patch (In reply to comment #46) > Created attachment 2096 [details] > Updated version of original patch. Fix missing braces around block in monitor.c:449-451 which causes that condition on monitor.c:457 is never true. Fix possible dereferenced NULL value in auth2.c:449 Fix dereference before null check in servconf.c:1233 and 1246 which causes segfault if argument of RequiredAuthentications[12] is empty.
(In reply to comment #46) > Created attachment 2096 [details] > Updated version of original patch. > auth2_check_required(const char *list): + /* Activate method if it isn't already */ + if (*(m->enabled) == -1) + *(m->enabled) = 1; I think that enable a method which is not explicitly enabled and might be disabled by default is not a good idea. An user should be warmed about the incorrect configuration is this case.
(In reply to comment #46) > Created attachment 2096 [details] > Updated version of original patch. - userauth_finish(authctxt, authenticated, method); - xfree(method); + userauth_finish(authctxt, authenticated, "keyboard-interactive", + kbdintctxt->device?kbdintctxt->device->name:NULL); kbdintctxt points to authctxt->kbdintctxt, but authctxt->kbdintctxt can be set to NULL in previous call of auth2_challenge_*() while kbdintctxt not. And there is probably accidentally removed line with xfree(method); - userauth_finish(authctxt, authenticated, method); + userauth_finish(authctxt, authenticated, "keyboard-interactive", + authctxt->kbdintctxt != NULL ? kbdintctxt->device->name : NULL); xfree(method);
Created attachment 2177 [details] updated patch This is my latest patch based on "Updated version of original patch" and my previous comments. It's rebased for latest cvs sources.
Created attachment 2178 [details] Fixes syntax error in 2177
Retarget uncompleted bugs from 6.1 => 6.2
Retarget bugs from 6.1 => 6.2
Created attachment 2192 [details] new multiple required authentication methods patch Here's a patch I'm working on. It adds an AuthenticationMethods option that lists the possible paths to successful authentication. E.g. AuthenticationMethods publickey,password gssapi-with-mic,password publickey,keyboard-interactive When attempting to authenticate, only methods that are at the start of one of the paths listed will be offered. Each successful authentication removes the successful method from the head of each path. E.g. for the example above, the client would be offered "publickey,gssapi-with-mic" for the first round. If they completed publickey authentication they would be offered "password,gssapi-with-mic,keyboard-interactive". Finally, if they completed password or keyboard-interactive then they would be considered authenticated. The patch is only for SSH2 and will fatal if protocol 1 is enabled. We can't support arbitrary orders for protocol 1 and I'm not going to make an OpenSSH-only extension for a dead protocol. The patch also tries to warn you early if you have selected authentication paths that are impossible to satisfy with the set of enabled authentication methods (e.g if you asked for publickey,password and has PasswordAuthentication=no). This warning won't catch cases where AuthenticationMethods are set late via Match blocks though.
Slightly tweaked patch committed. This will be in OpenSSH 6.2, due early next year.
It doesn't work for me with "UsePAM yes" and "AuthenticationMethods password,publickey". After successful password authentication I get: debug3: PAM: sshpam_passwd_conv called with 1 messages debug1: PAM: password authentication accepted for plautrba debug3: mm_answer_authpassword: sending result 1 debug3: mm_request_send entering: type 11 debug3: auth2_update_methods_lists: updating methods list after "password" debug3: authentication methods list 0 remaining: "publickey" debug3: monitor_child_preauth: method password: partial Failed password for plautrba from 127.0.0.1 port 60646 ssh2 debug3: mm_auth_password: user authenticated [preauth] debug3: mm_do_pam_account entering [preauth] debug3: mm_request_send entering: type 46 [preauth] debug3: mm_request_receive_expect entering: type 47 [preauth] debug3: mm_request_receive entering [preauth] debug3: mm_request_receive entering debug3: monitor_read: checking request 46 debug1: do_pam_account: called debug3: PAM: do_pam_account pam_acct_mgmt = 0 (Success) debug3: mm_request_send entering: type 47 debug3: auth2_update_methods_lists: updating methods list after "unknown" auth2_update_methods_lists: method not in AuthenticationMethods
Created attachment 2196 [details] fix of multiple required authentication methods auth2.c: - don't call do_pam_account() for partial authentication success - authctxt->failures shouldn't be increased for partial success - auth_log() should log "Accepted method" for partial success monitor.c: - authctxt->failures shouldn't be increased for partial success - auth_log() should log "Accepted method" for partial success
I've just read the mailing list. My patch doesn't reset partial in while loop in monitor_child_preauth() and also doesn't work for keyboard-interactive.
This should be working as of last November last year. Please file any problems in a separate bug.
mark bugs closed by openssh-6.2 release as CLOSED