| Message ID | 20171110163946.7656-1-fkooman@tuxed.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel] Implement "status 4" (JSON) for management interface | expand |
Hi François, This is not a full review, but as just discussed face-to-face, here are some points from me: 1) Could we also please please enable this for non-management interface use case? I guess it's just adding the status version number 4 in option/config validator (and the manpage). 2) With 1), perhaps it would be more clear to use *version* in your message/description so that it's about the --status-version option, not the --status option. 3) I believe the output structure is not valid JSON with the "END" string at the end. (FYI I'm about to upload a patch to include more documentation on the --status-version option and to include a new field as well.) -- Gert On 10-11-17 17:39, François Kooman wrote: > Parsing CSV can be cumbersome in depending software, so instead > offer JSON as well using "status 4". > > The "status 4" command uses the same keys/values as "status 2" > and "status 3", just formatted as JSON. > --- > src/openvpn/multi.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 96 insertions(+) > > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index 82a0b9d9..3e82431b 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -1014,6 +1014,102 @@ multi_print_status(struct multi_context *m, struct status_output *so, const int > > status_printf(so, "END"); > } > + else if (version == 4) > + { > + /* > + * Status file version 4 (JSON) > + */ > + status_printf(so, "{"); > + status_printf(so, "\"TITLE\": \"%s\",", title_string); > + status_printf(so, "\"TIME\": \"%s\",", time_string(now, 0, false, &gc_top)); > + /* UNIX_TIME added here instead of making it part of TIME as well > + as in status 2/3 */ > + status_printf(so, "\"UNIX_TIME\": %u,", (unsigned int)now); > + status_printf(so, "\"CLIENT_LIST\": ["); > + > + /* JSON needs a "," between items, but not at the last position, > + because I was unable to figure out how to determine if we > + iterate over the last item, we inverse it by prepending > + additional items with a comma */ > + int is_first = 1; > + hash_iterator_init(m->hash, &hi); > + while ((he = hash_iterator_next(&hi))) > + { > + struct gc_arena gc = gc_new(); > + const struct multi_instance *mi = (struct multi_instance *) he->value; > + > + if (!mi->halt) > + { > + if(0 == is_first) { > + status_printf(so, ","); > + } else { > + is_first = 0; > + } > + status_printf(so, "{"); > + status_printf(so, "\"Common Name\": \"%s\",", tls_common_name(mi->context.c2.tls_multi, false)); > + status_printf(so, "\"Real Address\": \"%s\",", mroute_addr_print(&mi->real, &gc)); > + status_printf(so, "\"Virtual Address\": \"%s\",", print_in_addr_t(mi->reporting_addr, IA_EMPTY_IF_UNDEF, &gc)); > + status_printf(so, "\"Virtual IPv6 Address\": \"%s\",", print_in6_addr(mi->reporting_addr_ipv6, IA_EMPTY_IF_UNDEF, &gc)); > + status_printf(so, "\"Bytes Received\": " counter_format ",", mi->context.c2.link_read_bytes); > + status_printf(so, "\"Bytes Sent\": " counter_format ",", mi->context.c2.link_write_bytes); > + status_printf(so, "\"Connected Since\": \"%s\",", time_string(mi->created, 0, false, &gc)); > + status_printf(so, "\"Connected Since (time_t)\": %u,", (unsigned int)mi->created); > + status_printf(so, "\"Username\": \"%s\",", tls_username(mi->context.c2.tls_multi, false)); > +#ifdef MANAGEMENT_DEF_AUTH > + status_printf(so, "\"Client ID\": %lu,", mi->context.c2.mda_context.cid); > +#endif > + status_printf(so, "\"Peer ID\": %" PRIu32, mi->context.c2.tls_multi ? mi->context.c2.tls_multi->peer_id : UINT32_MAX); > + status_printf(so, "}"); > + } > + gc_free(&gc); > + } > + hash_iterator_free(&hi); > + > + status_printf(so, "],"); > + status_printf(so, "\"ROUTING_TABLE\": ["); > + > + is_first = 1; > + hash_iterator_init(m->vhash, &hi); > + while ((he = hash_iterator_next(&hi))) > + { > + struct gc_arena gc = gc_new(); > + const struct multi_route *route = (struct multi_route *) he->value; > + > + if (multi_route_defined(m, route)) > + { > + const struct multi_instance *mi = route->instance; > + const struct mroute_addr *ma = &route->addr; > + char flags[2] = {0, 0}; > + > + if (route->flags & MULTI_ROUTE_CACHE) > + { > + flags[0] = 'C'; > + } > + if(0 == is_first) { > + status_printf(so, ","); > + } else { > + is_first = 0; > + } > + status_printf(so, "{\"Virtual Address\": \"%s%s\",", mroute_addr_print(ma, &gc), flags); > + status_printf(so, "\"Common Name\": \"%s\",", tls_common_name(mi->context.c2.tls_multi, false)); > + status_printf(so, "\"Real Address\": \"%s\",", mroute_addr_print(&mi->real, &gc)); > + status_printf(so, "\"Last Ref\": \"%s\",", time_string(route->last_reference, 0, false, &gc)); > + status_printf(so, "\"Last Ref (time_t)\": %u}", (unsigned int)route->last_reference); > + } > + gc_free(&gc); > + } > + hash_iterator_free(&hi); > + status_printf(so, "]"); > + > + if (m->mbuf) > + { > + status_printf(so, ",\"GLOBAL_STATS\": {"); > + status_printf(so, "\"Max bcast/mcast queue length\": %d", mbuf_maximum_queued(m->mbuf)); > + status_printf(so, "}"); > + } > + status_printf(so, "}"); > + status_printf(so, "END"); > + } > else > { > status_printf(so, "ERROR: bad status format version number"); > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 11/11/2017 04:55 PM, Gert van Dijk wrote: > Hi François, Hi Gert, > This is not a full review, but as just discussed face-to-face, here are > some points from me: Thanks for taking the time to look at it! > 1) Could we also please please enable this for non-management interface > use case? I guess it's just adding the status version number 4 in > option/config validator (and the manpage). Yes, I will add this as well. > 2) With 1), perhaps it would be more clear to use *version* in your > message/description so that it's about the --status-version option, not > the --status option. This got confused because in the management interface (via telnet) you'd use "status" and not "status-version". > 3) I believe the output structure is not valid JSON with the "END" > string at the end. All (multiline) responses end with "END", so that's why I did that as well. Ideally I'd construct a JSON object that does not include newlines at all and just responds as: SUCCESS: { ... } > (FYI I'm about to upload a patch to include more documentation on the > --status-version option and to include a new field as well.) Great! Cheers, François ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi > 3) I believe the output structure is not valid JSON with the "END" > > string at the end. > > All (multiline) responses end with "END", so that's why I did that as > well. Ideally I'd construct a JSON object that does not include newlines > at all and just responds as: > > SUCCESS: { ... } > One of the niceties of JSON is its readability which is greatly reduced if formatted without line breaks. Multi-line with the same format used for management and console would be nice. Except the former gets the extra footer line with "END". Selva <div dir="ltr">Hi<div><br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span> > 3) I believe the output structure is not valid JSON with the "END"<br> > string at the end.<br> <br> </span>All (multiline) responses end with "END", so that's why I did that as<br> well. Ideally I'd construct a JSON object that does not include newlines<br> at all and just responds as:<br> <br> SUCCESS: { ... }<br></blockquote><div><br></div><div>One of the niceties of JSON is its readability which is greatly reduced</div><div>if formatted without line breaks. Multi-line with the same</div><div>format used for management and console would be nice. Except the</div><div>former gets the extra footer line with "END".</div><div><br></div><div>Selva</div></div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Sun, Nov 12, 2017 at 12:21:06PM -0500, Selva wrote: > One of the niceties of JSON is its readability which is greatly reduced > if formatted without line breaks. +1 the difference in efficienty is not large, but "human readability for manual proofreading" is good. gert
On 11/12/2017 10:42 PM, Gert Doering wrote: > On Sun, Nov 12, 2017 at 12:21:06PM -0500, Selva wrote: >> One of the niceties of JSON is its readability which is greatly reduced >> if formatted without line breaks. > > +1 > > the difference in efficienty is not large, but "human readability for > manual proofreading" is good. Selva, Gert, You know, the funny thing is that my main reason for implementing JSON support for status was to have it easily _machine_ readable, for higher level programming languages where JSON is "first class citizen", and not primarily human readability. Besides you could easily do this to make JSON human readable: $ cat /path/to/status.json | python -mjson.tool Thinking some more about it now, it doesn't really make sense to implement JSON at all, because CSV _is_ actually meant to be machine readable and a lot more efficient in the amount of data it uses (much less duplication). So it seems my reasons for implementing JSON support seems to completely miss the point. For machine readable output it would make more sense to use a binary format instead of JSON. What do you guys think? Are there any other reasons to implement JSON support? Cheers, François ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On 13/11/17 17:44, François Kooman wrote: > On 11/12/2017 10:42 PM, Gert Doering wrote: >> On Sun, Nov 12, 2017 at 12:21:06PM -0500, Selva wrote: >>> One of the niceties of JSON is its readability which is greatly reduced >>> if formatted without line breaks. >> >> +1 >> >> the difference in efficienty is not large, but "human readability for >> manual proofreading" is good. > > Selva, Gert, > > You know, the funny thing is that my main reason for implementing JSON > support for status was to have it easily _machine_ readable, for higher > level programming languages where JSON is "first class citizen", and not > primarily human readability. Besides you could easily do this to make > JSON human readable: > > $ cat /path/to/status.json | python -mjson.tool > > Thinking some more about it now, it doesn't really make sense to > implement JSON at all, because CSV _is_ actually meant to be machine > readable and a lot more efficient in the amount of data it uses (much > less duplication). So it seems my reasons for implementing JSON support > seems to completely miss the point. For machine readable output it would > make more sense to use a binary format instead of JSON. > > What do you guys think? Are there any other reasons to implement JSON > support? Although JSON is a bloated language (meaning that there is lots of overhead next to the data itself), I think that it allows you to easily extend the objects being sent back and forth with a fairly high chance to keep backwards compatibility. This is barely true for csv. A binary presentation per se is not bad, but then you need to make sure that the format is properly encoded/decoded (especially if the data is transferred across the network from/to different platforms) and this can be error prone. Honestly I did not read the rest of the thread, but if this data is supposed to be exposed through an API (i.e. REST API or any other interface) and it is meant to travel across the network, I wouldn't mind seeing the JSON format being used. JSON consumers nowadays are extremely easy to implement compared to others. Regarding the JSON "prettiness", I think that printing it in a nice readable format makes it just extremely easy to debug and possibly spot errors on the fly (even when not searching for them). For this reason I'd rather add a couple of tabs and \n to make it easier to read. my2cents Cheers,
Hi, Since I don't use the mgmt interface / status file a lot, I didn't chip in earlier. But since this quite closely matches my initial thoughts I'll do so anyway. On 13-11-17 10:54, Antonio Quartulli wrote: > On 13/11/17 17:44, François Kooman wrote: >> On 11/12/2017 10:42 PM, Gert Doering wrote: >>> On Sun, Nov 12, 2017 at 12:21:06PM -0500, Selva wrote: >>>> One of the niceties of JSON is its readability which is greatly reduced >>>> if formatted without line breaks. >>> >>> +1 >>> >>> the difference in efficienty is not large, but "human readability for >>> manual proofreading" is good. >> >> Selva, Gert, >> >> You know, the funny thing is that my main reason for implementing JSON >> support for status was to have it easily _machine_ readable, for higher >> level programming languages where JSON is "first class citizen", and not >> primarily human readability. Besides you could easily do this to make >> JSON human readable: >> >> $ cat /path/to/status.json | python -mjson.tool >> >> Thinking some more about it now, it doesn't really make sense to >> implement JSON at all, because CSV _is_ actually meant to be machine >> readable and a lot more efficient in the amount of data it uses (much >> less duplication). So it seems my reasons for implementing JSON support >> seems to completely miss the point. For machine readable output it would >> make more sense to use a binary format instead of JSON. >> >> What do you guys think? Are there any other reasons to implement JSON >> support? This was my original question: why should we *add* JSON? I agree that if we would have to pick something now, I'd prefer JSON over CSV. But we already have CSV, and as far as I understand it suffices just fine. > Although JSON is a bloated language (meaning that there is lots of > overhead next to the data itself), I think that it allows you to easily > extend the objects being sent back and forth with a fairly high chance > to keep backwards compatibility. > > This is barely true for csv. The biggest difference is that CSV is 'flat' while JSON is "hierarchical". But since we already support CSV (and want to keep supporting it, I guess?), I think we're kind of bound to a flat structure anyway. > A binary presentation per se is not bad, but then you need to make sure > that the format is properly encoded/decoded (especially if the data is > transferred across the network from/to different platforms) and this can > be error prone. Slightly offtopic: I recently came across CBOR, which seems a good attempt to merge the benefits of JSON with a binary representation: https://tools.ietf.org/html/rfc7049 Haven't really used it yet though, so no practical experiences unfortunately. > Honestly I did not read the rest of the thread, but if this data is > supposed to be exposed through an API (i.e. REST API or any other > interface) and it is meant to travel across the network, I wouldn't mind > seeing the JSON format being used. > > JSON consumers nowadays are extremely easy to implement compared to others. Even compared to CSV? I can't really come up with a situation where I can consume JSON easily, but can't consume CSV. > Regarding the JSON "prettiness", I think that printing it in a nice > readable format makes it just extremely easy to debug and possibly spot > errors on the fly (even when not searching for them). For this reason > I'd rather add a couple of tabs and \n to make it easier to read. +1, one of the strong sides of JSON is that is easy to make it both user and machine readable. -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 13/11/17 11:15, Steffan Karger wrote: [...snip...] >>> You know, the funny thing is that my main reason for implementing JSON >>> support for status was to have it easily _machine_ readable, for higher >>> level programming languages where JSON is "first class citizen", and not >>> primarily human readability. Besides you could easily do this to make >>> JSON human readable: >>> >>> $ cat /path/to/status.json | python -mjson.tool >>> >>> Thinking some more about it now, it doesn't really make sense to >>> implement JSON at all, because CSV _is_ actually meant to be machine >>> readable and a lot more efficient in the amount of data it uses (much >>> less duplication). So it seems my reasons for implementing JSON support >>> seems to completely miss the point. For machine readable output it would >>> make more sense to use a binary format instead of JSON. >>> >>> What do you guys think? Are there any other reasons to implement JSON >>> support? > > This was my original question: why should we *add* JSON? I agree that > if we would have to pick something now, I'd prefer JSON over CSV. But > we already have CSV, and as far as I understand it suffices just fine. The tooling libraries around JSON vs CSV are more commonly more mature these days for JSON; plus it is so widely used the interwebs are filled with examples. And this does not only go for scripted languages; even there are also nice libraries for C and C++ too, giving you a machine parseable object with better predictability on the result. If I had a project parsing a CSV and had been given a chance to use JSON instead; I'd migrate over ASAP as long as the risks of breaking something badly was not very high (It would depend on where in the call chain such changes happens, and how much happens after that point). So I'm fine with adding --status 4 which dumps JSON. >> Although JSON is a bloated language (meaning that there is lots of >> overhead next to the data itself), I think that it allows you to easily >> extend the objects being sent back and forth with a fairly high chance >> to keep backwards compatibility. >> >> This is barely true for csv. > > The biggest difference is that CSV is 'flat' while JSON is > "hierarchical". But since we already support CSV (and want to keep > supporting it, I guess?), I think we're kind of bound to a flat > structure anyway. Yeah, we cannot rip out CSV now. We could probably propose that further into the future once JSON is widely used. And we could provide some tooling which converts JSON to CSV outside of OpenVPN at that time. >> A binary presentation per se is not bad, but then you need to make sure >> that the format is properly encoded/decoded (especially if the data is >> transferred across the network from/to different platforms) and this can >> be error prone. > > Slightly offtopic: I recently came across CBOR, which seems a good > attempt to merge the benefits of JSON with a binary representation: > https://tools.ietf.org/html/rfc7049 > > Haven't really used it yet though, so no practical experiences > unfortunately. I think that if we want to have a binary approach, that should be a variant of --status 4; like --status 4_binary. >> Honestly I did not read the rest of the thread, but if this data is >> supposed to be exposed through an API (i.e. REST API or any other >> interface) and it is meant to travel across the network, I wouldn't mind >> seeing the JSON format being used. >> >> JSON consumers nowadays are extremely easy to implement compared to others. > > Even compared to CSV? I can't really come up with a situation where I > can consume JSON easily, but can't consume CSV. The library tooling is in my experience far easier and predictable. And the JSON libraries have generally been far better at "object-ifying" JSON to objects you use in your code than what CSV parses have been. But I'll admit, I've mostly avoided CSV for the last decade or so if I could use JSON or even XML. CSV just looks structured - until the layout is changed. While JSON (and even XML for that matter, _not_ saying we should have XML support!) is not strictly bound to the order of fields, so adding new fields is a no-brainer with JSON compared to CSV. And before you say "We only append fields to CSV", some of those changes we have done in the past to --status 3 have even broken some users setup - because the CSV parser was stupid. The risk of that happening to JSON is far less. >> Regarding the JSON "prettiness", I think that printing it in a nice >> readable format makes it just extremely easy to debug and possibly spot >> errors on the fly (even when not searching for them). For this reason >> I'd rather add a couple of tabs and \n to make it easier to read. > > +1, one of the strong sides of JSON is that is easy to make it both user > and machine readable. +1 from me too. Just a few words to the management interface vs --status file. We should support JSON via both channels. And implementing that shouldn't be rocket science as it should be the same function producing that result. But we should consider if we want to make use of a JSON library producing the JSON streams. The reason is to ensure the output is according to the specification and that escaping if contents is consistent and proper. Imagine if someone puts a double-quote into the CN field of a certificate? CN="} Lets explode things, O=Hacktivist0 Or other characters which needs escaping. I also see the proposed patch adds "END" at the very end. To me that makes more sense in text/plain representations to indicate that this is the end of a list of unknown length. JSON have that implicitly with the last '}'. Just my few cents for this round :)
On 11/13/2017 01:16 PM, David Sommerseth wrote: > CSV just looks structured - until the layout is changed. While JSON > (and even XML for that matter, _not_ saying we should have XML support!) > is not strictly bound to the order of fields, so adding new fields is a > no-brainer with JSON compared to CSV. And before you say "We only > append fields to CSV", some of those changes we have done in the past to > --status 3 have even broken some users setup - because the CSV parser > was stupid. The risk of that happening to JSON is far less. This is a very good point. This was one of the problems I had before parsing OpenVPN's CSV when it changed (a little) between 2.3 and 2.4. > Just a few words to the management interface vs --status file. We > should support JSON via both channels. And implementing that shouldn't > be rocket science as it should be the same function producing that result. Agreed, also talked about with Gert (van Dijk) @ Hackathon. > But we should consider if we want to make use of a JSON library > producing the JSON streams. The reason is to ensure the output is > according to the specification and that escaping if contents is > consistent and proper. Imagine if someone puts a double-quote into the > CN field of a certificate? > > CN="} Lets explode things, O=Hacktivist0 > > Or other characters which needs escaping. Another very good point. I do have some experience with json-c [1], would it be acceptable to add this as a project dependency? That is the main reason I started out by just outputting a string and conveniently forgot escaping issues... So for now my plan is: 1. use json-c to generate (human readable) JSON; 2. also support --status (command line) in addition to "status" in management interface 3. do not print "END" marker any longer This is an excellent way for me to gain some more experience with writing C (and OpenVPN), even if in the end the patch is not accepted. I do think, like Steffan said, we need a good reason to support JSON, especially if it adds an additional external dependency. The future-proof-ness of the JSON format could be a good selling point, maybe new fields would only be added to the JSON format and not to the other status formats to promote adoption of the JSON format and slowly phasing out the CSV format(s). Cheers, François [1] https://github.com/json-c/json-c/ ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 13/11/17 14:44, François Kooman wrote: > On 11/13/2017 01:16 PM, David Sommerseth wrote: [...snip...] >> But we should consider if we want to make use of a JSON library >> producing the JSON streams. The reason is to ensure the output is >> according to the specification and that escaping if contents is >> consistent and proper. Imagine if someone puts a double-quote into the >> CN field of a certificate? >> >> CN="} Lets explode things, O=Hacktivist0 >> >> Or other characters which needs escaping. > > Another very good point. I do have some experience with json-c [1], > would it be acceptable to add this as a project dependency? That is the > main reason I started out by just outputting a string and conveniently > forgot escaping issues... Yes, I think that should be fine. I see it is available in RHEL6 (the oldest Linux distribution we support, json-c-0.11-13.el6), so this should work out just fine. So keep in mind to not use a bleeding fresh version only available in the latest faster-moving Linux distros. What would be needed though is to enable/disable this via ./configure (tackled in configure.ac). We basically can have three approaches here: a) If json-c is found, enable --status 4 by default. If not found, this feature is unavailable. b) Require json-c to be installed, which enables --status 4. To build without json-c dependency, --disable-json must be used with ./configure c) Do not require json-c by default, but if adding --enable-json to ./configure then json-c must be present. I don't have any strong opinion either way, but I believe alternative c) is the one which will give the least fuzz in the community - thus overall be more acceptable. > So for now my plan is: > > 1. use json-c to generate (human readable) JSON; > 2. also support --status (command line) in addition to "status" in > management interface > 3. do not print "END" marker any longer Sounds like a good plan! > This is an excellent way for me to gain some more experience with > writing C (and OpenVPN), even if in the end the patch is not accepted. I > do think, like Steffan said, we need a good reason to support JSON, > especially if it adds an additional external dependency. The > future-proof-ness of the JSON format could be a good selling point, > maybe new fields would only be added to the JSON format and not to the > other status formats to promote adoption of the JSON format and slowly > phasing out the CSV format(s). That's a good attitude :) I for one is not hard to get sold on the JSON format though, even though I don't have any particular use right now. But I can easily see this being preferred for others outside the core project. And this can simplify some ideas I do have pondered on in my own infrastructure. Again the alternative c) approach will fit nicely into this too. I am definitely willing to enable a JSON feature in the Fedora builds when patches hits an upstream release. And if that works out nice, I'll consider to add that even in the EPEL builds too - I'll most likely mention that on the Fedora devel mailing list to get some thoughts on the EPEL side before enabling it.
Hi, On Mon, Nov 13, 2017 at 3:40 PM, David Sommerseth <openvpn@sf.lists. topphemmelig.net> wrote: > On 13/11/17 14:44, François Kooman wrote: > > On 11/13/2017 01:16 PM, David Sommerseth wrote: > [...snip...] > >> But we should consider if we want to make use of a JSON library > >> producing the JSON streams. The reason is to ensure the output is > >> according to the specification and that escaping if contents is > >> consistent and proper. Imagine if someone puts a double-quote into the > >> CN field of a certificate? > >> > >> CN="} Lets explode things, O=Hacktivist0 > >> > >> Or other characters which needs escaping. > > > > Another very good point. I do have some experience with json-c [1], > > would it be acceptable to add this as a project dependency? That is the > > main reason I started out by just outputting a string and conveniently > > forgot escaping issues... > > Yes, I think that should be fine. I see it is available in RHEL6 (the > oldest Linux distribution we support, json-c-0.11-13.el6), so this > should work out just fine. So keep in mind to not use a bleeding fresh > version only available in the latest faster-moving Linux distros. > What would be needed though is to enable/disable this via ./configure > (tackled in configure.ac). We basically can have three approaches here: > > a) If json-c is found, enable --status 4 by default. If not found, > this feature is unavailable. > There is no libjson-c for Windows, I guess... Probably no one cares if --status 4 is not supported on Windows. > > > 1. use json-c to generate (human readable) JSON; > > 2. also support --status (command line) in addition to "status" in > > management interface > > 3. do not print "END" marker any longer > I suppose that means no "END" as a part of the JSON output (ie., not written to console or file) but if the output is multi-line, management still needs to get a terminating "END\n" line. Else we will be breaking the management interface protocol as it stands now. Selva <div dir="ltr">Hi,<br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 13, 2017 at 3:40 PM, David <span class="" id=":aj5.1" tabindex="-1">Sommerseth</span> <span dir="ltr"><<a href="mailto:openvpn@sf.lists.topphemmelig.net" target="_blank"><span class="" id=":aj5.2" tabindex="-1">openvpn</span>@sf.lists.<span class="" id=":aj5.3" tabindex="-1">topphemmelig</span>.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">On 13/11/17 14:44, François Kooman wrote:<br> > On 11/13/2017 01:16 PM, David Sommerseth wrote:<br> </span>[...snip...]<br> <span class="gmail-">>> But we should consider if we want to make use of a JSON library<br> >> producing the JSON streams. The reason is to ensure the output is<br> >> according to the specification and that escaping if contents is<br> >> consistent and proper. Imagine if someone puts a double-quote into the<br> >> CN field of a certificate?<br> >><br> >> CN="} Lets explode things, O=Hacktivist0<br> >><br> >> Or other characters which needs escaping.<br> ><br> > Another very good point. I do have some experience with json-c [1],<br> > would it be acceptable to add this as a project dependency? That is the<br> > main reason I started out by just outputting a string and conveniently<br> > forgot escaping issues...<br> <br> </span>Yes, I think that should be fine. I see it is available in RHEL6 (the<br> oldest Linux distribution we support, json-c-0.11-13.el6), so this<br> should work out just fine. So keep in mind to not use a bleeding fresh<br> version only available in the latest faster-moving Linux distros.</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> What would be needed though is to enable/disable this via ./configure<br> (tackled in <a href="http://configure.ac" rel="noreferrer" target="_blank">configure.ac</a>). We basically can have three approaches here:<br> <br> a) If json-c is found, enable --status 4 by default. If not found,<br> this feature is unavailable.<br></blockquote><div><div><br></div><div>There is no <span class="" id=":aj5.4" tabindex="-1">libjson</span>-c for Windows, I guess... Probably no one cares if</div><div>--status 4 is not supported on Windows.</div><div> </div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-"><br> > 1. use json-c to generate (human readable) JSON;<br> > 2. also support --status (command line) in addition to "status" in<br> > management interface<br> > 3. do not print "END" marker any longer<br></span></blockquote><div><br></div><div>I suppose that means no "END" as a part of the <span class="" id=":aj5.5" tabindex="-1">JSON</span> output (<span class="" id=":aj5.6" tabindex="-1">ie</span>., not written</div><div>to console or file) but if the output is multi-line, management still needs to get a</div><div>terminating "END\n" line. Else we will be breaking the management</div><div>interface protocol as it stands now.</div><div><br></div><div><span class="" id=":aj5.7" tabindex="-1">Selva</span></div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Mon, Nov 13, 2017 at 01:16:46PM +0100, David Sommerseth wrote: > But we should consider if we want to make use of a JSON library > producing the JSON streams. The reason is to ensure the output is > according to the specification and that escaping if contents is > consistent and proper. Imagine if someone puts a double-quote into the > CN field of a certificate? > > CN="} Lets explode things, O=Hacktivist0 > > Or other characters which needs escaping. I'm not convinced we want to import a new library dependency and a heap of #ifdef for this. Escaping on *output* is pretty trivial ("characters from <this set> need to be encoded <like this>") - and as long as we do not need to parse *incoming* JSON, a full-blown new library is mainly adding complications (like, configure flags, #ifdefs, library version dependencies, ...). But you knew that this response would be coming :-) gert
Hi, On 14/11/17 16:31, Gert Doering wrote: > Hi, > > On Mon, Nov 13, 2017 at 01:16:46PM +0100, David Sommerseth wrote: >> But we should consider if we want to make use of a JSON library >> producing the JSON streams. The reason is to ensure the output is >> according to the specification and that escaping if contents is >> consistent and proper. Imagine if someone puts a double-quote into the >> CN field of a certificate? >> >> CN="} Lets explode things, O=Hacktivist0 >> >> Or other characters which needs escaping. > > I'm not convinced we want to import a new library dependency and a heap > of #ifdef for this. > > Escaping on *output* is pretty trivial ("characters from <this set> > need to be encoded <like this>") - and as long as we do not need to parse > *incoming* JSON, a full-blown new library is mainly adding complications > (like, configure flags, #ifdefs, library version dependencies, ...). > > But you knew that this response would be coming :-) +1 if we want to do output only, we should not need another library. Especially if we are not going to do complex stuff. Cheers,
On 14/11/17 09:31, Gert Doering wrote: > Hi, > > On Mon, Nov 13, 2017 at 01:16:46PM +0100, David Sommerseth wrote: >> But we should consider if we want to make use of a JSON library >> producing the JSON streams. The reason is to ensure the output is >> according to the specification and that escaping if contents is >> consistent and proper. Imagine if someone puts a double-quote into the >> CN field of a certificate? >> >> CN="} Lets explode things, O=Hacktivist0 >> >> Or other characters which needs escaping. > > I'm not convinced we want to import a new library dependency and a heap > of #ifdef for this. > > Escaping on *output* is pretty trivial ("characters from <this set> > need to be encoded <like this>") - and as long as we do not need to parse > *incoming* JSON, a full-blown new library is mainly adding complications > (like, configure flags, #ifdefs, library version dependencies, ...). > > But you knew that this response would be coming :-) Not surprised ;-) But I do disagree. I tend to dislike re-inventing wheels when others do the job for you. Quite some years ago, I would probably have agreed with you. But I've been in projects where I implemented producing XML data manually in a similar manner. After fixing numerous of escaping errors, I switched to a library and all these bugs disappeared and never came back. It is so easy to think you know exactly what needs to be escaped beforehand and firmly believe you have covered each corner-case, but it always sneaks back at you - as you didn't spot the hidden doors. And to me this feels like it actually contradicts our netlink discussion on the Hackathon, where you initially wanted to use an external library ... ;-) That said, json-c is packaged and available on Linux, FreeBSD and even OpenWRT/LEDE got a recent version available. I don't know the situation on macOS (but that's similar to FreeBSD, I'd presume) and Windows is also not clear to me (if this is a relevant feature there). json-c is also fairly small (approx 7k lines of code, stripped binary lib ~60KB) and is being maintained (last commits from late October). So this isn't a big massive dependency. With ./configure --enable-json we also don't change the current behaviour in OpenVPN. Which should only require a single #ifdef ENABLE_JSON in the multi_print_status() [multi.c:859] function. But I also see that the status printing stuff could be improved as well, to simplify the implementation a bit too (which is a different task).
Hi, On Tue, Nov 14, 2017 at 11:56:10AM +0100, David Sommerseth wrote: > And to me this feels like it actually contradicts our netlink discussion > on the Hackathon, where you initially wanted to use an external library > ... ;-) It's a matter of complexity. Netlink is binary, complex, and hard to verify (unless you write your own decoder, which might have bugs on its own). JSON is very trivial to produce (unlike XML, or netlink). The escaping rules on producing are also very easy - basically, encode things in double quotes, and escape the set of { BS, FF, NL, CR, Tab, Backslash, Quote } with a single backslash before the corresponding character. https://stackoverflow.com/questions/19176024/how-to-escape-special-characters-in-building-a-json-string For *parsing*, I'd agree with you... gert
Hi, On Tue, Nov 14, 2017 at 3:31 AM, Gert Doering <gert@greenie.muc.de> wrote: > Hi, > > On Mon, Nov 13, 2017 at 01:16:46PM +0100, David Sommerseth wrote: >> But we should consider if we want to make use of a JSON library >> producing the JSON streams. The reason is to ensure the output is >> according to the specification and that escaping if contents is >> consistent and proper. Imagine if someone puts a double-quote into the >> CN field of a certificate? >> >> CN="} Lets explode things, O=Hacktivist0 >> >> Or other characters which needs escaping. > > I'm not convinced we want to import a new library dependency and a heap > of #ifdef for this. > > Escaping on *output* is pretty trivial ("characters from <this set> > need to be encoded <like this>") - and as long as we do not need to parse > *incoming* JSON, a full-blown new library is mainly adding complications > (like, configure flags, #ifdefs, library version dependencies, ...). +1 Best regards, Jon Bullard ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 14/11/17 09:31, Gert Doering wrote: > On Mon, Nov 13, 2017 at 01:16:46PM +0100, David Sommerseth wrote: >> But we should consider if we want to make use of a JSON library >> producing the JSON streams. The reason is to ensure the output is >> according to the specification and that escaping if contents is >> consistent and proper. Imagine if someone puts a double-quote into the >> CN field of a certificate? >> >> CN="} Lets explode things, O=Hacktivist0 >> >> Or other characters which needs escaping. > I'm not convinced we want to import a new library dependency and a heap > of #ifdef for this. > > Escaping on *output* is pretty trivial ("characters from <this set> > need to be encoded <like this>") - and as long as we do not need to parse > *incoming* JSON, a full-blown new library is mainly adding complications > (like, configure flags, #ifdefs, library version dependencies, ...). > > But you knew that this response would be coming :-) > > +1 JJK ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 14/11/17 12:02, Gert Doering wrote: > JSON is very trivial to produce (unlike XML, or netlink). The escaping > rules on producing are also very easy - basically, encode things in double > quotes, and escape the set of { BS, FF, NL, CR, Tab, Backslash, Quote } > with a single backslash before the corresponding character. > > <https://stackoverflow.com/questions/19176024/how-to-escape-special-characters-in-building-a-json-string> Right, all those are single byte characters - and that's fairly simple ... but that ignores various quirks which easily appears with multi-byte characters - especially when "looping" through a value, byte by byte. We support UTF-8 in certificate subject fields. So this escaper needs to handle those classes the stackoverflow mentions, plus beware of multi-byte strings (so we need to use the plethora of mb* related functions). In a clean 8-bit ASCII only-world, things are less complicated. Heiko and I have looked into the "simple" world of revamping the argv parser (to avoid our own "homebrewed" printf-like processing and base it on what is in the C library) and even this pre-parsing we need to do have popped up with surprises. The argv caller scope mostly covers parsing strings which is defined by us developers so the variations are not as broad, and luckily format strings is not expected to contain UTF-8 chars. But I do not for a second think processing certificate subject strings will as easy as those values we need to parse (typically CN) are not generated by us but a broad range of users. Who knows what kind of funny tricks they'll throw at our code? But sure, we can start with our own string parser to escape JSON and see how it works and then revisit using a library if the maintenance gets too annoying.
Hi, On Tue, Nov 14, 2017 at 7:40 AM, David Sommerseth <openvpn@sf.lists.topphemmelig.net> wrote: > > On 14/11/17 12:02, Gert Doering wrote: >> JSON is very trivial to produce (unlike XML, or netlink). The escaping >> rules on producing are also very easy - basically, encode things in double >> quotes, and escape the set of { BS, FF, NL, CR, Tab, Backslash, Quote } >> with a single backslash before the corresponding character. My reading of the RFC [1] (maybe not the only RFC for JSON?) is that all control characters must be escaped (all characters < 0x20). And the quotation mark that must be escaped is the double-quote (0x22), not the single-quote (0x27). Also, the escaping needs to be of the form \uXXXX where XXXX is the URF-16 code unit for the character. That's easy for these characters -- I think it's printf("\\u%04d", ch) since all the characters we are escaping are in the Basic Multilingual Plane (U+0000 through U+FFFF). > <https://stackoverflow.com/questions/19176024/how-to-escape-special-characters-in-building-a-json-string> > > Right, all those are single byte characters - and that's fairly simple > ... but that ignores various quirks which easily appears with multi-byte > characters - especially when "looping" through a value, byte by byte. > We support UTF-8 in certificate subject fields. So this escaper needs > to handle those classes the stackoverflow mentions, plus beware of > multi-byte strings (so we need to use the plethora of mb* related > functions). > > In a clean 8-bit ASCII only-world, things are less complicated. > > Heiko and I have looked into the "simple" world of revamping the argv > parser (to avoid our own "homebrewed" printf-like processing and base it > on what is in the C library) and even this pre-parsing we need to do > have popped up with surprises. The argv caller scope mostly covers > parsing strings which is defined by us developers so the variations are > not as broad, and luckily format strings is not expected to contain > UTF-8 chars. But I do not for a second think processing certificate > subject strings will as easy as those values we need to parse (typically > CN) are not generated by us but a broad range of users. Who knows what > kind of funny tricks they'll throw at our code? How does OpenVPN currently handle escaping when generating CSV? If we don't do it correctly for multi-byte strings for CSV, do we need to for JSON? From what I can tell, the only problematic character that needs to be escaped is the backslash (0x5C). All the other characters that need to be escaped are less than 0x40, so they will never appear as part of a multi-byte sequence. That one character makes this difficult, but IMO the problem is simple enough that it could be done within OpenVPN. That would be my preference, instead of adding another dependency. Best regards, Jon Bullard [1] https://www.ietf.org/rfc/rfc4627.txt ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Tue, Nov 14, 2017 at 7:17 AM, Jan Just Keijser <janjust@nikhef.nl> wrote: > On 14/11/17 09:31, Gert Doering wrote: > >> On Mon, Nov 13, 2017 at 01:16:46PM +0100, David Sommerseth wrote: >> >>> But we should consider if we want to make use of a JSON library >>> producing the JSON streams. The reason is to ensure the output is >>> according to the specification and that escaping if contents is >>> consistent and proper. Imagine if someone puts a double-quote into the >>> CN field of a certificate? >>> >>> CN="} Lets explode things, O=Hacktivist0 >>> >>> Or other characters which needs escaping. >>> >> I'm not convinced we want to import a new library dependency and a heap >> of #ifdef for this. >> >> Escaping on *output* is pretty trivial ("characters from <this set> >> need to be encoded <like this>") - and as long as we do not need to parse >> *incoming* JSON, a full-blown new library is mainly adding complications >> (like, configure flags, #ifdefs, library version dependencies, ...). >> >> But you knew that this response would be coming :-) >> >> >> +1 Piling on... with another +1 :) I would even say that if an external library is required reconsider whether we really need JSON output. Selva <div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 14, 2017 at 7:17 AM, Jan Just Keijser <span dir="ltr"><<a href="mailto:janjust@nikhef.nl" target="_blank">janjust@nikhef.nl</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 14/11/17 09:31, Gert Doering wrote:<br> </span><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Mon, Nov 13, 2017 at 01:16:46PM +0100, David Sommerseth wrote:<br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> But we should consider if we want to make use of a JSON library<br> producing the JSON streams. The reason is to ensure the output is<br> according to the specification and that escaping if contents is<br> consistent and proper. Imagine if someone puts a double-quote into the<br> CN field of a certificate?<br> <br> CN="} Lets explode things, O=Hacktivist0<br> <br> Or other characters which needs escaping.<br> </blockquote> I'm not convinced we want to import a new library dependency and a heap<br> of #ifdef for this.<br> <br> Escaping on *output* is pretty trivial ("characters from <this set><br> need to be encoded <like this>") - and as long as we do not need to parse<br> *incoming* JSON, a full-blown new library is mainly adding complications<br> (like, configure flags, #ifdefs, library version dependencies, ...).<br> <br> But you knew that this response would be coming :-)<br> <br> <br> </blockquote></span> +1</blockquote><div><br></div><div><br></div><div>Piling on... with another +1 :)</div><div><br></div><div>I would even say that if an external library is required reconsider whether we really</div><div>need JSON output.</div><div><br></div><div>Selva</div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Fri, Nov 10, 2017 at 05:39:46PM +0100, François Kooman wrote: > Parsing CSV can be cumbersome in depending software, so instead > offer JSON as well using "status 4". > > The "status 4" command uses the same keys/values as "status 2" > and "status 3", just formatted as JSON. So... we had a nice and long discussion about this last year, which I think can be wrapped like this - JSON is nice because good parser support exist on the receiving end today - human readability is a plus for manually verifying output - if we do JSON, we should do it for management *and* status file - most voices argued for "do not require an external library" (for *creation*). Not sure if we managed to convince David in the end, though :-) ... but then it just died off, and the patch is sitting in patchwork (where I moved it to "changes requested" just now). Is there interest in moving forward, or shall we just let it go? gert
On 10/7/18 9:10 AM, Gert Doering wrote: > Hi, Hi Gert, > - most voices argued for "do not require an external library" > (for *creation*). Not sure if we managed to convince David in > the end, though :-) In the meantime I had some "great" experience with manually generating JSON in another project. That was a terrible mistake ;-) So I would not recommend following this path any longer. Maybe not even JSON, but something typed and/or even binary. > ... but then it just died off, and the patch is sitting in patchwork > (where I moved it to "changes requested" just now). > > Is there interest in moving forward, or shall we just let it go? I'm not really interested in it anymore, and I brought it up ;-) So if no one else is interested, maybe better let it go... Cheers, François
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 82a0b9d9..3e82431b 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -1014,6 +1014,102 @@ multi_print_status(struct multi_context *m, struct status_output *so, const int status_printf(so, "END"); } + else if (version == 4) + { + /* + * Status file version 4 (JSON) + */ + status_printf(so, "{"); + status_printf(so, "\"TITLE\": \"%s\",", title_string); + status_printf(so, "\"TIME\": \"%s\",", time_string(now, 0, false, &gc_top)); + /* UNIX_TIME added here instead of making it part of TIME as well + as in status 2/3 */ + status_printf(so, "\"UNIX_TIME\": %u,", (unsigned int)now); + status_printf(so, "\"CLIENT_LIST\": ["); + + /* JSON needs a "," between items, but not at the last position, + because I was unable to figure out how to determine if we + iterate over the last item, we inverse it by prepending + additional items with a comma */ + int is_first = 1; + hash_iterator_init(m->hash, &hi); + while ((he = hash_iterator_next(&hi))) + { + struct gc_arena gc = gc_new(); + const struct multi_instance *mi = (struct multi_instance *) he->value; + + if (!mi->halt) + { + if(0 == is_first) { + status_printf(so, ","); + } else { + is_first = 0; + } + status_printf(so, "{"); + status_printf(so, "\"Common Name\": \"%s\",", tls_common_name(mi->context.c2.tls_multi, false)); + status_printf(so, "\"Real Address\": \"%s\",", mroute_addr_print(&mi->real, &gc)); + status_printf(so, "\"Virtual Address\": \"%s\",", print_in_addr_t(mi->reporting_addr, IA_EMPTY_IF_UNDEF, &gc)); + status_printf(so, "\"Virtual IPv6 Address\": \"%s\",", print_in6_addr(mi->reporting_addr_ipv6, IA_EMPTY_IF_UNDEF, &gc)); + status_printf(so, "\"Bytes Received\": " counter_format ",", mi->context.c2.link_read_bytes); + status_printf(so, "\"Bytes Sent\": " counter_format ",", mi->context.c2.link_write_bytes); + status_printf(so, "\"Connected Since\": \"%s\",", time_string(mi->created, 0, false, &gc)); + status_printf(so, "\"Connected Since (time_t)\": %u,", (unsigned int)mi->created); + status_printf(so, "\"Username\": \"%s\",", tls_username(mi->context.c2.tls_multi, false)); +#ifdef MANAGEMENT_DEF_AUTH + status_printf(so, "\"Client ID\": %lu,", mi->context.c2.mda_context.cid); +#endif + status_printf(so, "\"Peer ID\": %" PRIu32, mi->context.c2.tls_multi ? mi->context.c2.tls_multi->peer_id : UINT32_MAX); + status_printf(so, "}"); + } + gc_free(&gc); + } + hash_iterator_free(&hi); + + status_printf(so, "],"); + status_printf(so, "\"ROUTING_TABLE\": ["); + + is_first = 1; + hash_iterator_init(m->vhash, &hi); + while ((he = hash_iterator_next(&hi))) + { + struct gc_arena gc = gc_new(); + const struct multi_route *route = (struct multi_route *) he->value; + + if (multi_route_defined(m, route)) + { + const struct multi_instance *mi = route->instance; + const struct mroute_addr *ma = &route->addr; + char flags[2] = {0, 0}; + + if (route->flags & MULTI_ROUTE_CACHE) + { + flags[0] = 'C'; + } + if(0 == is_first) { + status_printf(so, ","); + } else { + is_first = 0; + } + status_printf(so, "{\"Virtual Address\": \"%s%s\",", mroute_addr_print(ma, &gc), flags); + status_printf(so, "\"Common Name\": \"%s\",", tls_common_name(mi->context.c2.tls_multi, false)); + status_printf(so, "\"Real Address\": \"%s\",", mroute_addr_print(&mi->real, &gc)); + status_printf(so, "\"Last Ref\": \"%s\",", time_string(route->last_reference, 0, false, &gc)); + status_printf(so, "\"Last Ref (time_t)\": %u}", (unsigned int)route->last_reference); + } + gc_free(&gc); + } + hash_iterator_free(&hi); + status_printf(so, "]"); + + if (m->mbuf) + { + status_printf(so, ",\"GLOBAL_STATS\": {"); + status_printf(so, "\"Max bcast/mcast queue length\": %d", mbuf_maximum_queued(m->mbuf)); + status_printf(so, "}"); + } + status_printf(so, "}"); + status_printf(so, "END"); + } else { status_printf(so, "ERROR: bad status format version number");