Add CID to the management status overview

PATCH In review David Sommerseth 5 participants 22 messages Feb 28, 2010 – Oct 22, 2013 1 message not in the archive

lore

Share & tools

lore

David Sommerseth

author

2010-02-28 12:50 UTC #0 lore raw

PATCH 1/1 Add CID to the management status overview

From: Arne Schwabe <[email protected]...>

There are commands in the management interface which require the cid. The only way at the moment to get the cid of connected clients is to have always a management connection established. The patch adds the CID to the status output.

Signed-off-by David Sommerseth <[email protected]...>

---
 multi.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

multi.c +6 -5

index 342871a..f8adb46 100644
@@ -777,7 +777,7 @@ multi_print_status (struct multi_context *m, struct status_output *so, const int
 
 	  status_printf (so, "END");
 	}
-      else if (version == 2 || version == 3)
+      else if (version == 2 || version == 3 )
 	{
 	  const char sep = (version == 3) ? '\t' : ',';

@@ -786,8 +786,8 @@ multi_print_status (struct multi_context *m, struct status_output *so, const int
 	   */
 	  status_printf (so, "TITLE%c%s", sep, title_string);
 	  status_printf (so, "TIME%c%s%c%u", sep, time_string (now, 0, false, &gc_top), sep, (unsigned int)now);
-	  status_printf (so, "HEADER%cCLIENT_LIST%cCommon Name%cReal Address%cVirtual Address%cBytes Received%cBytes Sent%cConnected Since%cConnected Since (time_t)",
-			 sep, sep, sep, sep, sep, sep, sep, sep);
+	  status_printf (so, "HEADER%cCLIENT_LIST%cCommon Name%cReal Address%cVirtual Address%cBytes Received%cBytes Sent%cConnected Since%cConnected Since (time_t)%cCID",
+			 sep, sep, sep, sep, sep, sep, sep, sep, sep);
 	  hash_iterator_init (m->hash, &hi, true);
 	  while ((he = hash_iterator_next (&hi)))
 	    {

@@ -796,14 +796,15 @@ multi_print_status (struct multi_context *m, struct status_output *so, const int
 
 	      if (!mi->halt)
 		{
-		  status_printf (so, "CLIENT_LIST%c%s%c%s%c%s%c" counter_format "%c" counter_format "%c%s%c%u",
+		  status_printf (so, "CLIENT_LIST%c%s%c%s%c%s%c" counter_format "%c" counter_format "%c%s%c%u%c%u",
 				 sep, tls_common_name (mi->context.c2.tls_multi, false),
 				 sep, mroute_addr_print (&mi->real, &gc),
 				 sep, print_in_addr_t (mi->reporting_addr, IA_EMPTY_IF_UNDEF, &gc),
 				 sep, mi->context.c2.link_read_bytes,
 				 sep, mi->context.c2.link_write_bytes,
 				 sep, time_string (mi->created, 0, false, &gc),
-				 sep, (unsigned int)mi->created);
+				 sep, (unsigned int)mi->created,
+				 sep, mi->context.c2.mda_context.cid);
 		}
 	      gc_free (&gc);
 	    }

Signature · 1 line

1.6.6.1

Gert Doering

2010-02-28 14:28 UTC #1 lore raw

Hi,

Quoted 4 lines · David Sommerseth

There are commands in the management interface which require the cid. The only way at the moment to get the cid of connected clients is to have always a management connection established. The patch adds the CID to the status output.

What is "cid"? Client-ID?

I can't ACK this, as I don't understand the implications. The code itself looks harmless, but without knowing who might read the output and whether an additional field will be silently ignored or whether consumer applications may fail, I really can't say.

Is there documentation available for this?

Something I do not like about this patch:

multi.c multi_print_status (struct multi_context *m, struct status_output *so, const int · line 783

Quoted 9 lines

multi.c +1 -1

@@ -777,7 +777,7 @@ multi_print_status (struct multi_context *m, struct status_output *so, const int
 
 	  status_printf (so, "END");
 	}
-      else if (version == 2 || version == 3)
+      else if (version == 2 || version == 3 )
 	{
 	  const char sep = (version == 3) ? '\t' : ',';

In the grand scheme of things, small whitespace changes might later on lead to a merge conflict with another patch in this line (like "introduce version 4" or so), and so I'd avoid changes that are purely cosmetic in a "feature" patch.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

A message that is not in the archive (sent off-list or not yet received): [email protected]...

Gert Doering

2010-02-28 16:46 UTC #3 lore raw

Hi,

Quoted 8 lines · David Sommerseth

Quoted 4 lines
In the grand scheme of things, small whitespace changes might later on lead to a merge conflict with another patch in this line (like "introduce version 4" or so), and so I'd avoid changes that are purely cosmetic in a "feature" patch.
Dang! This one slipped through. I spotted it, fixed it, but for some reason I recreated the patch file and forgot I had fixed it. Gert, I completely agree with you!

Quoted 2 lines

I'll promise to fix this one when merging it into the git tree, is that good enough for you, Gert?

Yes, I'm fine with that.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Arne Schwabe

2012-08-31 13:56 UTC #4 lore raw

Am 28.02.10 17:46, schrieb Gert Doering:

Quoted 7 lines

Hi,
Quoted 4 lines · David Sommerseth
Quoted 4 lines
In the grand scheme of things, small whitespace changes might later on lead to a merge conflict with another patch in this line (like "introduce version 4" or so), and so I'd avoid changes that are purely cosmetic in a "feature" patch.

Quoted 8 lines

Quoted 3 lines
Dang! This one slipped through. I spotted it, fixed it, but for some reason I recreated the patch file and forgot I had fixed it. Gert, I completely agree with you!
:)
Quoted 2 lines
I'll promise to fix this one when merging it into the git tree, is that good enough for you, Gert?
Yes, I'm fine with that.

This patch is very old but since someone asked for this feature on IRC I am bringing this patch up again.

I think it is good to have a stateless way (without being connected to the managament console all the time) to query the CID (client id).

Is it better to

a) break bad parsers when upgrading to 2.3
b) introduce a v4 format which only

Arne

Arne Schwabe

2012-08-31 14:19 UTC #5 lore raw

PATCH 1/1 Add the client id (CID) to the output of the status command

This add adds CID which is needed by a few other management commands to the status output. This will change the output of status in the same way commit ca18a638aa7cf316611f893127ba44131e57083c did.

Signed-off-by Arne Schwabe <[email protected]...>

---
 src/openvpn/multi.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

src/openvpn/multi.c +5 -4

index 9876b80..e5df432 100644
@@ -807,8 +807,8 @@ multi_print_status (struct multi_context *m, struct status_output *so, const int
 	   */
 	  status_printf (so, "TITLE%c%s", sep, title_string);
 	  status_printf (so, "TIME%c%s%c%u", sep, time_string (now, 0, false, &gc_top), sep, (unsigned int)now);
-	  status_printf (so, "HEADER%cCLIENT_LIST%cCommon Name%cReal Address%cVirtual Address%cBytes Received%cBytes Sent%cConnected Since%cConnected Since (time_t)%cUsername",
-			 sep, sep, sep, sep, sep, sep, sep, sep, sep);
+	  status_printf (so, "HEADER%cCLIENT_LIST%cCommon Name%cReal Address%cVirtual Address%cBytes Received%cBytes Sent%cConnected Since%cConnected Since (time_t)%cUsername%cClient ID",
+			 sep, sep, sep, sep, sep, sep, sep, sep, sep, sep);
 	  hash_iterator_init (m->hash, &hi);
 	  while ((he = hash_iterator_next (&hi)))
 	    {

@@ -817,7 +817,7 @@ multi_print_status (struct multi_context *m, struct status_output *so, const int
 
 	      if (!mi->halt)
 		{
-		  status_printf (so, "CLIENT_LIST%c%s%c%s%c%s%c" counter_format "%c" counter_format "%c%s%c%u%c%s",
+		  status_printf (so, "CLIENT_LIST%c%s%c%s%c%s%c" counter_format "%c" counter_format "%c%s%c%u%c%s%c%u",
 				 sep, tls_common_name (mi->context.c2.tls_multi, false),
 				 sep, mroute_addr_print (&mi->real, &gc),
 				 sep, print_in_addr_t (mi->reporting_addr, IA_EMPTY_IF_UNDEF, &gc),

@@ -825,7 +825,8 @@ multi_print_status (struct multi_context *m, struct status_output *so, const int
 				 sep, mi->context.c2.link_write_bytes,
 				 sep, time_string (mi->created, 0, false, &gc),
 				 sep, (unsigned int)mi->created,
-				 sep, tls_username (mi->context.c2.tls_multi, false));
+				 sep, tls_username (mi->context.c2.tls_multi, false),
+                 sep, mi->context.c2.mda_context.cid);
 		}
 	      gc_free (&gc);
 	    }

Signature · 1 line

1.7.9.6 (Apple Git-31.1)

Gert Doering

2013-04-11 09:33 UTC #6 lore raw

[Openvpn-devel] [PATCH applied] Re: Add the client id (CID) to the output of the status command

Your patch has been applied to the master branch (ACK by me).

I'm not sure whether we need this in release/2.3, but if people see the need, please speak up and I'll cherry-pick :-)

commit 662ce6acc065bddf6490b3494725b8b3987b7def (master)

Author: Arne Schwabe
Date:   Fri Aug 31 16:19:13 2012 +0200

     Add the client id (CID) to the output of the status command

     Signed-off-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/7021
     Acked-by: Gert Doering <[email protected]...>
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

2013-04-11 10:52 UTC #7 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Add the client id (CID) to the output of the status command

Hi,

Quoted 6 lines · Gert Doering

Your patch has been applied to the master branch (ACK by me).
I'm not sure whether we need this in release/2.3, but if people see the need, please speak up and I'll cherry-pick :-)
commit 662ce6acc065bddf6490b3494725b8b3987b7def (master)

... unfortunately, it breaks --disable-managemnt builds :-( - good that we have buildbots, stupid me for not noticing it before ACK-and-push.

Arne, could you send a post-patch, please, to #ifdef ENABLE_MANAGEMENT the problematic line?

thanks,

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Samuli Seppänen

2013-04-11 13:02 UTC #8 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Add the client id (CID) to the output of the status command

Quoted 4 lines

Hi,
On Thu, Apr 11, 2013 at 11:33:58AM +0200, Gert Doering wrote:

Quoted 8 lines

Quoted 6 lines
Your patch has been applied to the master branch (ACK by me).
I'm not sure whether we need this in release/2.3, but if people see the need, please speak up and I'll cherry-pick :-)
commit 662ce6acc065bddf6490b3494725b8b3987b7def (master)
... unfortunately, it breaks --disable-managemnt builds :-( - good that we have buildbots, stupid me for not noticing it before ACK-and-push.

I wonder if using a "staging" repo would be too much trouble? Buildbots could track it, and only after all builds have succeeded, the changes would be pushed to master.

Signature · 5 lines

Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

Gert Doering

2013-04-11 13:17 UTC #9 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Add the client id (CID) to the output of the status command

Hi,

Quoted 6 lines · Samuli Seppänen

Quoted 6 lines · Gert Doering
Quoted 5 lines
Your patch has been applied to the master branch (ACK by me).
I'm not sure whether we need this in release/2.3, but if people see the need, please speak up and I'll cherry-pick :-)

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
commit 662ce6acc065bddf6490b3494725b8b3987b7def (master)
... unfortunately, it breaks --disable-managemnt builds :-( - good that we have buildbots, stupid me for not noticing it before ACK-and-push.
I wonder if using a "staging" repo would be too much trouble? Buildbots could track it, and only after all builds have succeeded, the changes would be pushed to master.

You and David explain to me what to do, I do :-)

(Including "how to un-push things pushed to staging if they turn out to be non-useful")

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Samuli Seppänen

2013-04-11 13:25 UTC #10 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Add the client id (CID) to the output of the status command

Quoted 5 lines

Quoted 5 lines
Quoted 2 lines
... unfortunately, it breaks --disable-managemnt builds :-( - good that we have buildbots, stupid me for not noticing it before ACK-and-push.
I wonder if using a "staging" repo would be too much trouble? Buildbots could track it, and only after all builds have succeeded, the changes would be pushed to master.

Quoted 7 lines

You and David explain to me what to do, I do :-)
(Including "how to un-push things pushed to staging if they turn out to be non-useful")
gert

As long as nobody is actually using the staging repo rewriting history should not be an issue. Perhaps one could just do

$ git reset --hard HEAD^1
$ git push origin master

and be done with it?

That said, we might not benefit much from a staging repo if we fix the problems buildbot finds quickly. Sure, the repo might not be 100% correct / functional all the time, but the impact should be fairly low compared to pushing out broken releases.

Any opinions, anyone?

Samuli

Gert Doering

2013-04-11 13:27 UTC #11 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Add the client id (CID) to the output of the status command

Hi,

Quoted 7 lines · Samuli Seppänen

As long as nobody is actually using the staging repo rewriting history should not be an issue. Perhaps one could just do
$ git reset --hard HEAD^1
$ git push origin master
and be done with it?

Will that un-push already-pushed commits? Or just have "one commit less than $remote" in your local repo?

(You see that I still do not understand all of the subtleties of git)

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

David Sommerseth

author

2013-04-11 14:00 UTC #12 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Add the client id (CID) to the output of the status command

Quoted 10 lines · Gert Doering

Hi,
Quoted 7 lines · Samuli Seppänen
As long as nobody is actually using the staging repo rewriting history should not be an issue. Perhaps one could just do
$ git reset --hard HEAD^1
$ git push origin master
and be done with it?

Do not ever do that without fully understanding what it does. Basically it rewrites the git history, and others who have cloned this repo will get ugly warnings from git on the next git-fetch. Whenever such warnings appears, it can indicate that the git tree has been compromised. And if you have some merges involved and do this on a merge commit ... well, things gets even more complicated.

This operation should only be done when no other alternatives will work.
It is the last resort of a git fix.

By all means, use git-reset, but avoid it once you've pushed out a commit you want to remove. If you use git-reset on a local un-pushed tree, that's all fine. That's called cleaning up before you push. But once pushed, rather clean it up differently.

Quoted 2 lines

Will that un-push already-pushed commits? Or just have "one commit less than $remote" in your local repo?

Yes, and it will make users of such a git tree become suspicious.

I rather recommend using 'git revert' instead. Which simply reverts the commit, you add a reason why it was reverted ... and it's all logged as a new separate commit. Much cleaner, and it documents what we've tried and why we didn't use the patch after all. We don't need to be ashamed of pushing out things which breaks, as long as we fix it.

Signature · 3 lines

kind regards,

David Sommerseth

David Sommerseth

author

2013-04-11 14:03 UTC #13 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Add the client id (CID) to the output of the status command

In reply to Samuli Seppänen

Quoted 9 lines · Samuli Seppänen

Quoted 9 lines
Hi,
Quoted 5 lines · Gert Doering
Your patch has been applied to the master branch (ACK by me).
I'm not sure whether we need this in release/2.3, but if people see the need, please speak up and I'll cherry-pick :-)

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
commit 662ce6acc065bddf6490b3494725b8b3987b7def (master)
... unfortunately, it breaks --disable-managemnt builds :-( - good that we have buildbots, stupid me for not noticing it before ACK-and-push.
I wonder if using a "staging" repo would be too much trouble? Buildbots could track it, and only after all builds have succeeded, the changes would be pushed to master.

Tempting thought. But I'd say it will be more maintenance than real benefit in the long run.

Using git-revert will actually document better what we tried and why we didn't like that patch after all. We don't need to try hide our mistakes that well :)

Rather have a policy ... if a patch breaks something, fix it within 24-48 hours, if not use git-revert.

Signature · 3 lines

kind regards,

David Sommerseth

Samuli Seppänen

2013-04-11 14:10 UTC #14 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Add the client id (CID) to the output of the status command

Quoted 16 lines · Samuli Seppänen

Quoted 8 lines
Quoted 8 lines
Hi,
Quoted 5 lines · Gert Doering
Your patch has been applied to the master branch (ACK by me).
I'm not sure whether we need this in release/2.3, but if people see the need, please speak up and I'll cherry-pick :-)
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
commit 662ce6acc065bddf6490b3494725b8b3987b7def (master)
... unfortunately, it breaks --disable-managemnt builds :-( - good that we have buildbots, stupid me for not noticing it before ACK-and-push.
I wonder if using a "staging" repo would be too much trouble? Buildbots could track it, and only after all builds have succeeded, the changes would be pushed to master.

Quoted 6 lines

Tempting thought. But I'd say it will be more maintenance than real benefit in the long run.
Using git-revert will actually document better what we tried and why we didn't like that patch after all. We don't need to try hide our mistakes that well :)

What mistakes? We don't make mistakes!

Quoted 4 lines

Rather have a policy ... if a patch breaks something, fix it within 24-48 hours, if not use git-revert.

Sounds like a plan.

Signature · 5 lines

Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

Gert Doering

2013-04-11 15:11 UTC #15 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Add the client id (CID) to the output of the status command

In reply to David Sommerseth

Hi,

Quoted 2 lines · David Sommerseth

Rather have a policy ... if a patch breaks something, fix it within 24-48 hours, if not use git-revert.

works for me.

Usually these are minor issues anyway, like "symbol not available on particular platforms" (MIN() comes to mind) or "breaks for a certain combination of compile time options", and those can be fixed quickly as soon as spotted by buildbot.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Eike Lohmann

2013-10-22 08:44 UTC #16 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Add the client id (CID) to the output of the status command

Hi,

we are developing a damon wich connects to the management interface from a lokal openvpn server.
This daemon authenticate clients and starts some tasks like "set routes, firewall etc." in the background.
We do this because we can't use client-connect, it locks the openvpn main process for 100-1000ms if you do a lot of stuff there like"SQL query, set routes, set firewall, disconnect clients on other machines. etc."

Our problem right now is, if the server ist running we don't know wich CID are connected and there is no way to query this from status file or via management interface.

In the Changelog of 2.2 and 2.3 is no entry about CID and I want to ask if this feature went into the code or not?

Thanks for any help, Eike.

Example:

[1379691142.394271] 64 bytes from 80.146.164.25: icmp_req=21 ttl=61 time=940 ms

[pid 1929] 1379691140.828976 clone(Process 9597 attached
child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f8f66c8e9d0) = 9597 <0.192637>
[pid 1929] 1379691141.021980 wait4(9597, Process 1929 suspended
<unfinished ...>
[pid 9597] 1379691141.022386 execve("/etc/openvpn/client-connect.sh", ["/etc/openvpn/client-connect.sh", "/tmp/openvpn_cc_2863505f0b808f75"...], [/* 26 vars */]) = 0 <0.021794>

[pid 1929] 1379691142.109748 <... wait4 resumed> [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 9597 <1.087736>

Am 11.04.2013 17:11, schrieb Gert Doering:

Quoted 29 lines

Hi,
Quoted 2 lines · David Sommerseth
Rather have a policy ... if a patch breaks something, fix it within 24-48 hours, if not use git-revert.
works for me.
Usually these are minor issues anyway, like "symbol not available on particular platforms" (MIN() comes to mind) or "breaks for a certain combination of compile time options", and those can be fixed quickly as soon as spotted by buildbot.
gert
------------------------------------------------------------------------------
Precog is a next-generation analytics platform capable of advanced analytics on semi-structured data. The platform includes APIs for building apps and a phenomenal toolset for data science. Developers can use our toolset for easy data analysis & visualization. Get a free account!
http://www2.precog.com/precogplatform/slashdotnewsletter
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Gert Doering

2013-10-22 09:23 UTC #17 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Add the client id (CID) to the output of the status command

Hi,

Quoted 2 lines · Eike Lohmann

In the Changelog of 2.2 and 2.3 is no entry about CID and I want to ask if this feature went into the code or not?

It went into master as commit 662ce6acc065bddf6490b3494725b8b3987b7def, but not into 2.3 (as that train is in "bug fixes and only limited set of new features" mode).

commit 662ce6acc065bddf6490b3494725b8b3987b7def

Author: Arne Schwabe <[email protected]...>
Date:   Fri Aug 31 16:19:13 2012 +0200

    Add the client id (CID) to the output of the status command
    
    This add adds CID which is needed by a few other management commands to
    the status output. This will change the output of status in the same way
    commit ca18a638aa7cf316611f893127ba44131e57083c did.
    
    Signed-off-by: Arne Schwabe <[email protected]...>
    Acked-by: Gert Doering <[email protected]...>
    Message-Id: <[email protected]...>
    URL: http://article.gmane.org/gmane.network.openvpn.devel/7021
    Signed-off-by: Gert Doering <[email protected]...>

The code base we have in master today is fairly well tested, so if I needed that feature on one of my servers, I'd just run "master" (which I *do*, because I want the "export peer-info data to client-connect" functionality - different feature, same argument).

OTOH, you could cherry-pick that commit to the 2.3 branch and use that - it should apply fine (have not checked, but as far as I remember, we didn't do much other work in that area of the code in the time between so there shouldn't be conflicts).

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Arne Schwabe

2013-04-11 11:17 UTC #18 lore raw

PATCH 1/1 Print client id only if compiled with man agent support. Otherwise print an empty string.

In reply to Gert Doering

Also use %lu instead of %u for printing unsigned long int

Signed-off-by Arne Schwabe <[email protected]...>

---
 src/openvpn/multi.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

src/openvpn/multi.c +6 -2

index 1dbdb7f..a2a18c9 100644
@@ -817,7 +817,7 @@ multi_print_status (struct multi_context *m, struct status_output *so, const int
 
 	      if (!mi->halt)
 		{
-		  status_printf (so, "CLIENT_LIST%c%s%c%s%c%s%c" counter_format "%c" counter_format "%c%s%c%u%c%s%c%u",
+		  status_printf (so, "CLIENT_LIST%c%s%c%s%c%s%c" counter_format "%c" counter_format "%c%s%c%u%c%s%c%lu",
 				 sep, tls_common_name (mi->context.c2.tls_multi, false),
 				 sep, mroute_addr_print (&mi->real, &gc),
 				 sep, print_in_addr_t (mi->reporting_addr, IA_EMPTY_IF_UNDEF, &gc),

@@ -826,7 +826,11 @@ multi_print_status (struct multi_context *m, struct status_output *so, const int
 				 sep, time_string (mi->created, 0, false, &gc),
 				 sep, (unsigned int)mi->created,
 				 sep, tls_username (mi->context.c2.tls_multi, false),
-                 sep, mi->context.c2.mda_context.cid);
+#ifdef MANAGEMENT_DEF_AUTH
+				 sep, mi->context.c2.mda_context.cid);
+#else
+				 sep, "");
+#endif
 		}
 	      gc_free (&gc);
 	    }

Signature · 1 line

1.7.9.5

Arne Schwabe

2013-04-11 11:44 UTC #19 lore raw

PATCH v2 1/1 Print client id only if compiled with man agent support. Otherwise print an empty string.

Also use %lu instead of %u for printing unsigned long int

---
 src/openvpn/multi.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

src/openvpn/multi.c +11 -2

index 1dbdb7f..2003a8d 100644
@@ -817,7 +817,12 @@ multi_print_status (struct multi_context *m, struct status_output *so, const int
 
 	      if (!mi->halt)
 		{
-		  status_printf (so, "CLIENT_LIST%c%s%c%s%c%s%c" counter_format "%c" counter_format "%c%s%c%u%c%s%c%u",
+		  status_printf (so, "CLIENT_LIST%c%s%c%s%c%s%c" counter_format "%c" counter_format "%c%s%c%u%c%s%c"
+#ifdef MANAGEMENT_DEF_AUTH
+                         "%lu",
+#else
+                         "%s",
+#endif
 				 sep, tls_common_name (mi->context.c2.tls_multi, false),
 				 sep, mroute_addr_print (&mi->real, &gc),
 				 sep, print_in_addr_t (mi->reporting_addr, IA_EMPTY_IF_UNDEF, &gc),

@@ -826,7 +831,11 @@ multi_print_status (struct multi_context *m, struct status_output *so, const int
 				 sep, time_string (mi->created, 0, false, &gc),
 				 sep, (unsigned int)mi->created,
 				 sep, tls_username (mi->context.c2.tls_multi, false),
-                 sep, mi->context.c2.mda_context.cid);
+#ifdef MANAGEMENT_DEF_AUTH
+				 sep, mi->context.c2.mda_context.cid);
+#else
+				 sep, "");
+#endif
 		}
 	      gc_free (&gc);
 	    }

Signature · 1 line

1.7.9.5

Arne Schwabe

2013-04-11 13:34 UTC #20 lore raw

PATCH v3 1/1 Print client id only if compiled with man agent support. Otherwise print an empty string.

Also use %lu instead of %u for printing unsigned long int

---
 src/openvpn/multi.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

src/openvpn/multi.c +11 -2

index 1dbdb7f..5900523 100644
@@ -817,7 +817,12 @@ multi_print_status (struct multi_context *m, struct status_output *so, const int
 
 	      if (!mi->halt)
 		{
-		  status_printf (so, "CLIENT_LIST%c%s%c%s%c%s%c" counter_format "%c" counter_format "%c%s%c%u%c%s%c%u",
+		  status_printf (so, "CLIENT_LIST%c%s%c%s%c%s%c" counter_format "%c" counter_format "%c%s%c%u%c%s%c"
+#ifdef MANAGEMENT_DEF_AUTH
+				 "%lu",
+#else
+				 "",
+#endif
 				 sep, tls_common_name (mi->context.c2.tls_multi, false),
 				 sep, mroute_addr_print (&mi->real, &gc),
 				 sep, print_in_addr_t (mi->reporting_addr, IA_EMPTY_IF_UNDEF, &gc),

@@ -826,7 +831,11 @@ multi_print_status (struct multi_context *m, struct status_output *so, const int
 				 sep, time_string (mi->created, 0, false, &gc),
 				 sep, (unsigned int)mi->created,
 				 sep, tls_username (mi->context.c2.tls_multi, false),
-                 sep, mi->context.c2.mda_context.cid);
+#ifdef MANAGEMENT_DEF_AUTH
+				 sep, mi->context.c2.mda_context.cid);
+#else
+				 sep);
+#endif
 		}
 	      gc_free (&gc);
 	    }

Signature · 1 line

1.7.9.5

Gert Doering

2013-04-11 14:03 UTC #21 lore raw

[Openvpn-devel] [PATCH applied] Re: Print client id only if compiled with man agent support. Otherwise print an empty string.

ACK. After a long and tiring discussion on ways to make this less ugly, without completely overengineering this simple function at the same time...

Patch has been applied to the master branch and pushed.

commit 01cfa61a41e5162ba2f61383e03ff66741511d3a (master)

Author: Arne Schwabe
Date:   Thu Apr 11 15:34:38 2013 +0200

     Print client id only if compiled with man agent support. Otherwise print an empty string.

     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/7498
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

David Sommerseth

author

2010-03-01 12:04 UTC #22 lore raw

In reply to Gert Doering

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 8 lines · Gert Doering

Hi,
Quoted 4 lines · David Sommerseth
There are commands in the management interface which require the cid. The only way at the moment to get the cid of connected clients is to have always a management connection established. The patch adds the CID to the status output.

Quoted 8 lines

What is "cid"? Client-ID?
I can't ACK this, as I don't understand the implications. The code itself looks harmless, but without knowing who might read the output and whether an additional field will be silently ignored or whether consumer applications may fail, I really can't say.
Is there documentation available for this?

I've been looking at the management/management-notes.txt file. And CID does indicate a Client-ID. In fact, this ID is required by more of the commands issued. But there is no "overview list" which reports it, as far as I can understand. Thus putting this into the status command seems reasonable to me.

The only way to track those CID values is by looking for client notifications as clients connect and authenticates themselves.

kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkuLrVkACgkQDC186MBRfrrFzwCgiVbJKcJWpDi2ZJ7Ipewlan1m f6gAoJjQMfA8uzlcG+ckMO6f1Xyyb1S7
=BnUR
-----END PGP SIGNATURE-----