[PATCH] smbstatus output for sessions with authentication still in progress

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] smbstatus output for sessions with authentication still in progress

Samba - samba-technical mailing list
Hi!

Currently smbstatus displays -1 for user and group info when it hits a session
where authentication is still in progress.

The attached patch changes this to display

    PID  Username   Group    Machine                          Protocol Version ....
    6604 (auth in progress)  127.0.0.1 (ipv4:127.0.0.1:47930) SMB3_11 ....

instead, which should help users make sense of the output.

Please review & push if happy. Thanks!

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] smbstatus output for sessions with authentication still in progress

Samba - samba-technical mailing list
On Tue, Nov 21, 2017 at 05:53:48PM +0100, Ralph Böhme via samba-technical wrote:

> Hi!
>
> Currently smbstatus displays -1 for user and group info when it hits a session
> where authentication is still in progress.
>
> The attached patch changes this to display
>
>     PID  Username   Group    Machine                          Protocol Version ....
>     6604 (auth in progress)  127.0.0.1 (ipv4:127.0.0.1:47930) SMB3_11 ....
>
> instead, which should help users make sense of the output.
>
> Please review & push if happy. Thanks!

Very slight NAK on this. The reason is if someone has
set numeric_only=true then it changes the output for
an auth in progress from "-1 -1" to "(auth in progress)"
which may break scripts that are expecting only numbers
(even invalid ones :-) in this field.

Can you add an if statement that re-adds the "-1 -1" output
in the "The session is not fully authenticated yet."
clause ?

Sorry for being nit-picky :-).

Jeremy.

> --
> Ralph Boehme, Samba Team       https://samba.org/
> Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

> From 964b5f9a2d6bb9d300b0bb4b794ff1d5a58e97a1 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Tue, 4 Jul 2017 12:22:00 +0200
> Subject: [PATCH] smbstatus: correctly denote not fully authenticated sessions
>
> Currently for sessions where authentication is still in progress we
> print uid and gid as -1.
>
> With this change we nicely list them like this:
>
> PID  Username   Group    Machine                          Protocol Version ....
> 6604 (auth in progress)  127.0.0.1 (ipv4:127.0.0.1:47930) SMB3_11 ....
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/utils/status.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/source3/utils/status.c b/source3/utils/status.c
> index abc0d26df53..f69326e888a 100644
> --- a/source3/utils/status.c
> +++ b/source3/utils/status.c
> @@ -365,7 +365,7 @@ static int traverse_sessionid(const char *key, struct sessionid *session,
>        void *private_data)
>  {
>   TALLOC_CTX *mem_ctx = (TALLOC_CTX *)private_data;
> - fstring uid_str, gid_str;
> + fstring uid_gid_str;
>   struct server_id_buf tmp;
>   char *machine_hostname = NULL;
>   int result = 0;
> @@ -380,24 +380,22 @@ static int traverse_sessionid(const char *key, struct sessionid *session,
>  
>   Ucrit_addPid(session->pid);
>  
> - fstrcpy(uid_str, "-1");
> + if (session->uid == -1 && session->gid == -1) {
> + /*
> + * The session is not fully authenticated yet.
> + */
> + fstrcpy(uid_gid_str, "(auth in progress)");
> + } else {
> + fstring uid_str, gid_str;
>  
> - if (session->uid != -1) {
>   if (numeric_only) {
>   fstr_sprintf(uid_str, "%u", (unsigned int)session->uid);
> - } else {
> - fstrcpy(uid_str, uidtoname(session->uid));
> - }
> - }
> -
> - fstrcpy(gid_str, "-1");
> -
> - if (session->gid != -1) {
> - if (numeric_only) {
>   fstr_sprintf(gid_str, "%u", (unsigned int)session->gid);
>   } else {
> + fstrcpy(uid_str, uidtoname(session->uid));
>   fstrcpy(gid_str, gidtoname(session->gid));
>   }
> + fstr_sprintf(uid_gid_str, "%s %s", uid_str, gid_str);
>   }
>  
>   machine_hostname = talloc_asprintf(mem_ctx, "%s (%s)",
> @@ -457,9 +455,9 @@ static int traverse_sessionid(const char *key, struct sessionid *session,
>   }
>  
>  
> - d_printf("%-7s %-12s %-12s %-41s %-17s %-20s %-21s\n",
> + d_printf("%-7s %-25s %-41s %-17s %-20s %-21s\n",
>   server_id_str_buf(session->pid, &tmp),
> - uid_str, gid_str,
> + uid_gid_str,
>   machine_hostname,
>   session_dialect_str(session->connection_dialect),
>   encryption,
> --
> 2.13.6
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] smbstatus output for sessions with authentication still in progress

Samba - samba-technical mailing list
On Tue, Nov 21, 2017 at 03:07:26PM -0800, Jeremy Allison wrote:

> On Tue, Nov 21, 2017 at 05:53:48PM +0100, Ralph Böhme via samba-technical wrote:
> > Hi!
> >
> > Currently smbstatus displays -1 for user and group info when it hits a session
> > where authentication is still in progress.
> >
> > The attached patch changes this to display
> >
> >     PID  Username   Group    Machine                          Protocol Version ....
> >     6604 (auth in progress)  127.0.0.1 (ipv4:127.0.0.1:47930) SMB3_11 ....
> >
> > instead, which should help users make sense of the output.
> >
> > Please review & push if happy. Thanks!
>
> Very slight NAK on this. The reason is if someone has
> set numeric_only=true then it changes the output for
> an auth in progress from "-1 -1" to "(auth in progress)"
> which may break scripts that are expecting only numbers
> (even invalid ones :-) in this field.
>
> Can you add an if statement that re-adds the "-1 -1" output
> in the "The session is not fully authenticated yet."
> clause ?
sure. Updated patch attached.

> Sorry for being nit-picky :-).

No prob.

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] smbstatus output for sessions with authentication still in progress

Samba - samba-technical mailing list
On Wed, Nov 22, 2017 at 11:01:36AM +0100, Ralph Böhme wrote:

> On Tue, Nov 21, 2017 at 03:07:26PM -0800, Jeremy Allison wrote:
> > On Tue, Nov 21, 2017 at 05:53:48PM +0100, Ralph Böhme via samba-technical wrote:
> > > Hi!
> > >
> > > Currently smbstatus displays -1 for user and group info when it hits a session
> > > where authentication is still in progress.
> > >
> > > The attached patch changes this to display
> > >
> > >     PID  Username   Group    Machine                          Protocol Version ....
> > >     6604 (auth in progress)  127.0.0.1 (ipv4:127.0.0.1:47930) SMB3_11 ....
> > >
> > > instead, which should help users make sense of the output.
> > >
> > > Please review & push if happy. Thanks!
> >
> > Very slight NAK on this. The reason is if someone has
> > set numeric_only=true then it changes the output for
> > an auth in progress from "-1 -1" to "(auth in progress)"
> > which may break scripts that are expecting only numbers
> > (even invalid ones :-) in this field.
> >
> > Can you add an if statement that re-adds the "-1 -1" output
> > in the "The session is not fully authenticated yet."
> > clause ?
>
> sure. Updated patch attached.
>
> > Sorry for being nit-picky :-).
>
> No prob.

LGTM. Pushed ! Thanks Ralph.

> --
> Ralph Boehme, Samba Team       https://samba.org/
> Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

> From 43f44641629469a757bce9e03dc4595e12a46aa1 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Wed, 22 Nov 2017 10:43:19 +0100
> Subject: [PATCH 1/2] s3/smbstatus: add a NULL check
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/utils/status.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/source3/utils/status.c b/source3/utils/status.c
> index abc0d26df53..dd196b64a47 100644
> --- a/source3/utils/status.c
> +++ b/source3/utils/status.c
> @@ -386,7 +386,12 @@ static int traverse_sessionid(const char *key, struct sessionid *session,
>   if (numeric_only) {
>   fstr_sprintf(uid_str, "%u", (unsigned int)session->uid);
>   } else {
> - fstrcpy(uid_str, uidtoname(session->uid));
> + const char *uid_name = uidtoname(session->uid);
> +
> + if (uid_name == NULL) {
> + return -1;
> + }
> + fstrcpy(uid_str, uid_name);
>   }
>   }
>  
> @@ -396,6 +401,11 @@ static int traverse_sessionid(const char *key, struct sessionid *session,
>   if (numeric_only) {
>   fstr_sprintf(gid_str, "%u", (unsigned int)session->gid);
>   } else {
> + const char *gid_name = gidtoname(session->gid);
> +
> + if (gid_name == NULL) {
> + return -1;
> + }
>   fstrcpy(gid_str, gidtoname(session->gid));
>   }
>   }
> --
> 2.13.6
>
>
> From 7579145be144ac3451ad0bc71464d43d7ca5488e Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Tue, 4 Jul 2017 12:22:00 +0200
> Subject: [PATCH 2/2] smbstatus: correctly denote not fully authenticated
>  sessions
>
> Currently for sessions where authentication is still in progress we
> print uid and gid as -1.
>
> With this change we nicely list them like this:
>
> PID  Username   Group    Machine                          Protocol Version ....
> 6604 (auth in progress)  127.0.0.1 (ipv4:127.0.0.1:47930) SMB3_11 ....
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/utils/status.c | 61 ++++++++++++++++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 27 deletions(-)
>
> diff --git a/source3/utils/status.c b/source3/utils/status.c
> index dd196b64a47..dfb1d921a42 100644
> --- a/source3/utils/status.c
> +++ b/source3/utils/status.c
> @@ -365,7 +365,7 @@ static int traverse_sessionid(const char *key, struct sessionid *session,
>        void *private_data)
>  {
>   TALLOC_CTX *mem_ctx = (TALLOC_CTX *)private_data;
> - fstring uid_str, gid_str;
> + fstring uid_gid_str;
>   struct server_id_buf tmp;
>   char *machine_hostname = NULL;
>   int result = 0;
> @@ -380,33 +380,40 @@ static int traverse_sessionid(const char *key, struct sessionid *session,
>  
>   Ucrit_addPid(session->pid);
>  
> - fstrcpy(uid_str, "-1");
> -
> - if (session->uid != -1) {
> - if (numeric_only) {
> - fstr_sprintf(uid_str, "%u", (unsigned int)session->uid);
> + if (numeric_only) {
> + fstr_sprintf(uid_gid_str, "%-12u %-12u",
> +     (unsigned int)session->uid,
> +     (unsigned int)session->gid);
> + } else {
> + if (session->uid == -1 && session->gid == -1) {
> + /*
> + * The session is not fully authenticated yet.
> + */
> + fstrcpy(uid_gid_str, "(auth in progress)");
>   } else {
> - const char *uid_name = uidtoname(session->uid);
> -
> - if (uid_name == NULL) {
> - return -1;
> + /*
> + * In theory it should not happen that one of
> + * session->uid and session->gid is valid (ie != -1)
> + * while the other is not (ie = -1), so we a check for
> + * that case that bails out would be reasonable.
> + */
> + const char *uid_name = "-1";
> + const char *gid_name = "-1";
> +
> + if (session->uid != -1) {
> + uid_name = uidtoname(session->uid);
> + if (uid_name == NULL) {
> + return -1;
> + }
>   }
> - fstrcpy(uid_str, uid_name);
> - }
> - }
> -
> - fstrcpy(gid_str, "-1");
> -
> - if (session->gid != -1) {
> - if (numeric_only) {
> - fstr_sprintf(gid_str, "%u", (unsigned int)session->gid);
> - } else {
> - const char *gid_name = gidtoname(session->gid);
> -
> - if (gid_name == NULL) {
> - return -1;
> + if (session->gid != -1) {
> + gid_name = gidtoname(session->gid);
> + if (gid_name == NULL) {
> + return -1;
> + }
>   }
> - fstrcpy(gid_str, gidtoname(session->gid));
> + fstr_sprintf(uid_gid_str, "%-12s %-12s",
> +     uid_name, gid_name);
>   }
>   }
>  
> @@ -467,9 +474,9 @@ static int traverse_sessionid(const char *key, struct sessionid *session,
>   }
>  
>  
> - d_printf("%-7s %-12s %-12s %-41s %-17s %-20s %-21s\n",
> + d_printf("%-7s %-25s %-41s %-17s %-20s %-21s\n",
>   server_id_str_buf(session->pid, &tmp),
> - uid_str, gid_str,
> + uid_gid_str,
>   machine_hostname,
>   session_dialect_str(session->connection_dialect),
>   encryption,
> --
> 2.13.6
>