Hi all,
Following a discussion of last June and the resulting bug (2408), in
order to improve 2 factor authentication, I've worked on a set of
patches to expose at least basic authentication information and more
when possible. By exposing such information to PAM, it would be possible
to differentiate the cases when PAM is called first and when it is
called after a valid authentication (e.g. when using
AuthenticationMethods gssapi-with-mic,keyboard-interactive:pam
publickey,keyboard-interactive:pam keyboard-interactive:pam)
The design of these patches is rather simple:
- Authentication method can "publish" detailed information about the
client, if the authentication was successful
- In the main auth 2 loop, after a successful authentication, the
authentication method is recorded, including details if provided
- When calling PAM or a shell, export this information via an
environment variable
I've tried to keep the patches as small and atomic as possible.
The larger change is probably the introduction of 'pubkey_format' in
order to only have one function to print a public key.
This set of patch could be extended to cover more ground, but I not sure
how and thus I'm open to suggestions and ideas on how:
- to expose the same kind of detailed information from the priviledged
thread in case of priviledge separation
- to produce the same kind of information from other authentication
methods (I'm particularly interested in getting the kerbors principal
from gss-serv-krb5 for example)
Resolving such questions would probably make this feature complete, but
I fear that they need complex modifications of the existing code. Do you
think that this design and this first step could be considered and
merged first, without closing the door for further improvement?
Thanks in advance,
Vincent
PS: I've posted these patches as a single patch on the bugzilla, but as
I didn't get any feedback, I'm trying my luck here directly
>From 99f2f709afedceed0cc14235f46e90081e4aa432 Mon Sep 17 00:00:00 2001
From: Vincent Brillault <git@xxxxxxxxx>
Date: Wed, 18 Nov 2015 20:01:30 +0100
Subject: [PATCH 1/8] Store details about successful auth methods
---
auth.h | 3 +++
auth2.c | 13 +++++++++++++
session.c | 3 +++
3 files changed, 19 insertions(+)
diff --git a/auth.h b/auth.h
index fffbe6c..391c0a7 100644
--- a/auth.h
+++ b/auth.h
@@ -81,6 +81,9 @@ struct Authctxt {
struct sshkey **prev_userkeys;
u_int nprev_userkeys;
+
+ char *last_details;
+ char *auth_details;
};
/*
* Every authentication method has to handle authentication requests for
diff --git a/auth2.c b/auth2.c
index 7177962..99eb750 100644
--- a/auth2.c
+++ b/auth2.c
@@ -293,6 +293,7 @@ userauth_finish(Authctxt *authctxt, int authenticated, const char *method,
const char *submethod)
{
char *methods;
+ char *prev_auth_details;
int partial = 0;
if (!authctxt->valid && authenticated)
@@ -323,6 +324,18 @@ userauth_finish(Authctxt *authctxt, int authenticated, const char *method,
if (authctxt->postponed)
return;
+ if (authenticated || partial) {
+ prev_auth_details = authctxt->auth_details;
+ xasprintf(&authctxt->auth_details, "%s%s%s%s%s",
+ prev_auth_details ? prev_auth_details : "",
+ prev_auth_details ? ", " : "", method,
+ authctxt->last_details ? ": " : "",
+ authctxt->last_details ? authctxt->last_details : "");
+ free(authctxt->last_details);
+ authctxt->last_details = NULL;
+ free(prev_auth_details);
+ }
+
#ifdef USE_PAM
if (options.use_pam && authenticated) {
if (!PRIVSEP(do_pam_account())) {
diff --git a/session.c b/session.c
index 26f4742..c1f5ab2 100644
--- a/session.c
+++ b/session.c
@@ -2735,6 +2735,9 @@ do_cleanup(Authctxt *authctxt)
if (authctxt == NULL)
return;
+ free(authctxt->auth_details);
+ authctxt->auth_details = NULL;
+
#ifdef USE_PAM
if (options.use_pam) {
sshpam_cleanup();
--
2.4.10
>From bb0c1ccd71f1c52e926d7b6ad726da71efb7518c Mon Sep 17 00:00:00 2001
From: Vincent Brillault <git@xxxxxxxxx>
Date: Thu, 19 Nov 2015 20:26:26 +0100
Subject: [PATCH 2/8] Export auth_details to child env as SSH_USER_AUTH
---
session.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/session.c b/session.c
index c1f5ab2..a54dbe0 100644
--- a/session.c
+++ b/session.c
@@ -1307,6 +1307,10 @@ do_setup_env(Session *s, const char *shell)
}
#endif /* USE_PAM */
+ if (s->authctxt->auth_details)
+ child_set_env(&env, &envsize, "SSH_USER_AUTH",
+ s->authctxt->auth_details);
+
if (auth_sock_name != NULL)
child_set_env(&env, &envsize, SSH_AUTHSOCKET_ENV_NAME,
auth_sock_name);
--
2.4.10
>From 807787f8e09f703305074a8c5e0abb4eb5777e3a Mon Sep 17 00:00:00 2001
From: Vincent Brillault <git@xxxxxxxxx>
Date: Thu, 19 Nov 2015 20:26:53 +0100
Subject: [PATCH 3/8] Expose auth_details to pam via "SSH_USER_AUTH"
Whenever the pam device is called, update the "SSH_USER_AUTH" PAM
environment variable (Doing it outside this module exposes us to a NULL
sshpam_handle).
When a session will be later created, this variable, still part of the
PAM environment variable will be copied to the child environment before
being overriden by the latest value stored in auth_details. As a result,
using the same variable name as the final one is key to prevent the
final environment to be poluted with an outdated value.
---
auth-pam.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/auth-pam.c b/auth-pam.c
index d94c828..815c833 100644
--- a/auth-pam.c
+++ b/auth-pam.c
@@ -688,6 +688,10 @@ sshpam_init_ctx(Authctxt *authctxt)
return (NULL);
}
+ /* Notify PAM about any already successful auth methods */
+ if (authctxt->auth_details)
+ do_pam_putenv("SSH_USER_AUTH", authctxt->auth_details);
+
ctxt = xcalloc(1, sizeof *ctxt);
/* Start the authentication thread */
--
2.4.10
>From c087cdfe4a7e6cf752a0fff4830eb32b4991b8b3 Mon Sep 17 00:00:00 2001
From: Vincent Brillault <git@xxxxxxxxx>
Date: Wed, 18 Nov 2015 21:11:07 +0100
Subject: [PATCH 4/8] Extract pubkey_format from pubkey_auth_info
---
auth.h | 1 +
auth2-pubkey.c | 50 ++++++++++++++++++++++++++++++--------------------
2 files changed, 31 insertions(+), 20 deletions(-)
diff --git a/auth.h b/auth.h
index 391c0a7..8a55f72 100644
--- a/auth.h
+++ b/auth.h
@@ -130,6 +130,7 @@ int auth_rsa_key_allowed(struct passwd *, BIGNUM *, Key **);
int auth_rhosts_rsa_key_allowed(struct passwd *, char *, char *, Key *);
int hostbased_key_allowed(struct passwd *, const char *, char *, Key *);
int user_key_allowed(struct passwd *, Key *, int);
+char *pubkey_format(const Key *key);
void pubkey_auth_info(Authctxt *, const Key *, const char *, ...)
__attribute__((__format__ (printf, 3, 4)));
void auth2_record_userkey(Authctxt *, struct sshkey *);
diff --git a/auth2-pubkey.c b/auth2-pubkey.c
index b1b5b74..45a861a 100644
--- a/auth2-pubkey.c
+++ b/auth2-pubkey.c
@@ -214,40 +214,50 @@ done:
return authenticated;
}
-void
-pubkey_auth_info(Authctxt *authctxt, const Key *key, const char *fmt, ...)
+char *
+pubkey_format(const Key *key)
{
- char *fp, *extra;
- va_list ap;
- int i;
-
- extra = NULL;
- if (fmt != NULL) {
- va_start(ap, fmt);
- i = vasprintf(&extra, fmt, ap);
- va_end(ap);
- if (i < 0 || extra == NULL)
- fatal("%s: vasprintf failed", __func__);
- }
+ char *fp, *result;
if (key_is_cert(key)) {
fp = sshkey_fingerprint(key->cert->signature_key,
options.fingerprint_hash, SSH_FP_DEFAULT);
- auth_info(authctxt, "%s ID %s (serial %llu) CA %s %s%s%s",
+ xasprintf(&result, "%s ID %s (serial %llu) CA %s %s",
key_type(key), key->cert->key_id,
(unsigned long long)key->cert->serial,
key_type(key->cert->signature_key),
- fp == NULL ? "(null)" : fp,
- extra == NULL ? "" : ", ", extra == NULL ? "" : extra);
+ fp == NULL ? "(null)" : fp);
free(fp);
} else {
fp = sshkey_fingerprint(key, options.fingerprint_hash,
SSH_FP_DEFAULT);
- auth_info(authctxt, "%s %s%s%s", key_type(key),
- fp == NULL ? "(null)" : fp,
- extra == NULL ? "" : ", ", extra == NULL ? "" : extra);
+ xasprintf(&result, "%s %s", key_type(key),
+ fp == NULL ? "(null)" : fp);
free(fp);
}
+
+ return result;
+}
+
+void
+pubkey_auth_info(Authctxt *authctxt, const Key *key, const char *fmt, ...)
+{
+ char *extra, *pubkey;
+ va_list ap;
+ int i;
+
+ extra = NULL;
+ if (fmt != NULL) {
+ va_start(ap, fmt);
+ i = vasprintf(&extra, fmt, ap);
+ va_end(ap);
+ if (i < 0 || extra == NULL)
+ fatal("%s: vasprintf failed", __func__);
+ }
+
+ pubkey = pubkey_format(key);
+ auth_info(authctxt, "%s%s%s", pubkey, extra == NULL ? "" : ", ",
+ extra == NULL ? "" : extra);
free(extra);
}
--
2.4.10
>From 27d52173c07ad8aacddd919a6c4f4f382800a518 Mon Sep 17 00:00:00 2001
From: Vincent Brillault <git@xxxxxxxxx>
Date: Wed, 18 Nov 2015 21:32:06 +0100
Subject: [PATCH 5/8] auth2-pubkey: fill last_details on success
---
auth2-pubkey.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/auth2-pubkey.c b/auth2-pubkey.c
index 45a861a..170cd24 100644
--- a/auth2-pubkey.c
+++ b/auth2-pubkey.c
@@ -79,7 +79,7 @@ userauth_pubkey(Authctxt *authctxt)
{
Buffer b;
Key *key = NULL;
- char *pkalg, *userstyle;
+ char *pkalg, *userstyle, *pubkey;
u_char *pkblob, *sig;
u_int alen, blen, slen;
int have_sig, pktype;
@@ -168,7 +168,8 @@ userauth_pubkey(Authctxt *authctxt)
#ifdef DEBUG_PK
buffer_dump(&b);
#endif
- pubkey_auth_info(authctxt, key, NULL);
+ pubkey = pubkey_format(key);
+ auth_info(authctxt, "%s", pubkey);
/* test for correct signature */
authenticated = 0;
@@ -176,9 +177,12 @@ userauth_pubkey(Authctxt *authctxt)
PRIVSEP(key_verify(key, sig, slen, buffer_ptr(&b),
buffer_len(&b))) == 1) {
authenticated = 1;
+ authctxt->last_details = pubkey;
/* Record the successful key to prevent reuse */
auth2_record_userkey(authctxt, key);
key = NULL; /* Don't free below */
+ } else {
+ free(pubkey);
}
buffer_free(&b);
free(sig);
--
2.4.10
>From b3d808b192bbfad8f19942d1124d574a7d14d694 Mon Sep 17 00:00:00 2001
From: Vincent Brillault <git@xxxxxxxxx>
Date: Wed, 18 Nov 2015 21:36:55 +0100
Subject: [PATCH 6/8] auth2-hostbased: fill last_details on success
---
auth2-hostbased.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/auth2-hostbased.c b/auth2-hostbased.c
index e2327cf..856efc9 100644
--- a/auth2-hostbased.c
+++ b/auth2-hostbased.c
@@ -60,7 +60,7 @@ userauth_hostbased(Authctxt *authctxt)
{
Buffer b;
Key *key = NULL;
- char *pkalg, *cuser, *chost, *service;
+ char *pkalg, *cuser, *chost, *service, *pubkey;
u_char *pkblob, *sig;
u_int alen, blen, slen;
int pktype;
@@ -132,15 +132,21 @@ userauth_hostbased(Authctxt *authctxt)
buffer_dump(&b);
#endif
- pubkey_auth_info(authctxt, key,
- "client user \"%.100s\", client host \"%.100s\"", cuser, chost);
+ pubkey = pubkey_format(key);
+ auth_info(authctxt,
+ "%s, client user \"%.100s\", client host \"%.100s\"",
+ pubkey, cuser, chost);
/* test for allowed key and correct signature */
authenticated = 0;
if (PRIVSEP(hostbased_key_allowed(authctxt->pw, cuser, chost, key)) &&
PRIVSEP(key_verify(key, sig, slen, buffer_ptr(&b),
- buffer_len(&b))) == 1)
+ buffer_len(&b))) == 1) {
authenticated = 1;
+ authctxt->last_details = pubkey;
+ } else {
+ free(pubkey);
+ }
buffer_free(&b);
done:
--
2.4.10
>From 11a367de8fd87314fd6516306620136889e03c20 Mon Sep 17 00:00:00 2001
From: Vincent Brillault <git@xxxxxxxxx>
Date: Wed, 18 Nov 2015 23:53:15 +0100
Subject: [PATCH 7/8] privsep: Expose success auth methods
Unfortunately, in the monitor thread, not the same amount of data is
available when an key-base authentication succeed. It could be possible
to extract the key information of all key that pass through
mm_answer_keyverify, but linking it to the authentication success would
be dangerous.
Simply exposing the successul methods would already be a progress
---
monitor.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/monitor.c b/monitor.c
index 395a6f6..c6b2236 100644
--- a/monitor.c
+++ b/monitor.c
@@ -335,6 +335,7 @@ monitor_child_preauth(Authctxt *_authctxt, struct monitor *pmonitor)
{
struct mon_table *ent;
int authenticated = 0, partial = 0;
+ char *prev_auth_details;
debug3("preauth child monitor started");
@@ -366,6 +367,14 @@ monitor_child_preauth(Authctxt *_authctxt, struct monitor *pmonitor)
auth_submethod = NULL;
authenticated = (monitor_read(pmonitor, mon_dispatch, &ent) == 1);
+ if (authenticated) {
+ prev_auth_details = authctxt->auth_details;
+ xasprintf(&authctxt->auth_details, "%s%s%s",
+ prev_auth_details ? prev_auth_details : "",
+ prev_auth_details ? ", " : "", auth_method);
+ free(prev_auth_details);
+ }
+
/* Special handling for multiple required authentications */
if (options.num_auth_methods != 0) {
if (!compat20)
--
2.4.10
>From 2da3ca1fd0d48db8e6c7ebda0bf9002da4cbd45b Mon Sep 17 00:00:00 2001
From: Vincent Brillault <git@xxxxxxxxx>
Date: Thu, 19 Nov 2015 21:31:29 +0100
Subject: [PATCH 8/8] Mention SSH_USER_AUTH in the man page
---
ssh.1 | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/ssh.1 b/ssh.1
index 5b35b6c..1af89b3 100644
--- a/ssh.1
+++ b/ssh.1
@@ -1402,6 +1402,10 @@ server IP address, and server port number.
This variable contains the original command line if a forced command
is executed.
It can be used to extract the original arguments.
+.It Ev SSH_USER_AUTH
+This variable contains, for SSH2 only, a comma-separated list of authentication
+methods that were successfuly used to authenticate. When possible, these
+methods are extended with detailed information on the credential used.
.It Ev SSH_TTY
This is set to the name of the tty (path to the device) associated
with the current shell or command.
--
2.4.10
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
