Re: libedit: stop ignoring SIGINT
On Mon, Aug 09, 2021 at 07:20:31AM -0600, Theo de Raadt wrote: > Ingo Schwarze wrote: > > > as mentioned earlier, deraadt@ reported that sftp(1) ignores Ctrl-C. > > Fixing that without longjmp(3) requires making editline(3) better > > behaved. > > If a library interface encourages use longjmp(), that library should be > thrown into the dustbin of history. Our src tree has very few longjmp > these days. Thank you for the effort to discourage addition of more. > > > The following patch causes el_gets(3) and el_wgets(3) to return > > failure when read(2)ing from the terminal is interrupted by a > > signal other than SIGCONT or SIGWINCH. That allows the calling > > program to decide what to do, usually either exit the program or > > provide a fresh prompt to the user. > > Looks good. > > > * bc(1) > >It behaves well with the patch: Ctrl-C discards the current > >input line and starts a new input line. > >The reason why this already works even without the patch > >is that bc(1) does very scary stuff inside the signal handler: > >pass a file-global EditLine pointer on the heap to el_line(3) > >and access fields inside the returned struct. Needless to > >say that no signal handler should do such things... > > Otto -- if you are short of time, at minimum mark the variable usage > line with /* XXX signal race */ as we have done throughout the tree. I > would encourage anyone who sees such problems inside any signal handler > to show such comment-adding diffs. If these problems are documented, > they can be fixed eventually, usually through event-loop logic, but I'll > admit many of the comments are in non-event-loop programs. Added the comment. Don't know what I was thinking when I did that change. -Otto > > > * ftp(1) > >It behaves well with the patch: Ctrl-C discards the current > >input line and provides a fresh prompt. > >The reason why it already works without the patch is that ftp(1) > >uses setjmp(3)/longjmp(3) to forcefully grab back control > >from el_gets(3) without waiting for it to return. > > Horrible isn't it. > > > * sftp(1) > >Behaviour is improved with the patch: Ctrl-C now exits sftp(1). > >If desired, i can supply a very simple follow-up patch to sftp.c > >to instead behave like ftp(1) and bc(1), i.e. discard the > >current input line and provide a fresh prompt. > >Neither doing undue work in the signal handler nor longjmp(3) > >will be required for that (if this patch gets committed). > > I suspect dtucker will want to decide on the interactive behaviour, > but what you describe sounds right. > > > Also note that deraadt@ pointed out in private mail to me that the > > fact that read__fixio() clears FIONBIO is probably a symptom of > > botched editline(3) API design. That might be worth fixing, too, > > as far as that is feasible, but it is unrelated to the sftp(1) > > Ctrl-C issue; let's address one topic at a time. > > I mentioned rarely having seen a good outcome from code mixing any of > the 3: FIONBIO, FIONREAD, and select/poll. Often the non-blocking was > added to select/poll code to hide some sort of bug, or the select/poll > code was added amateurishly to older code without removing the FIONBIO. > There are a few situations you need both approaches mixed, but it isn't > the general case, and thus FIONBIO has a "polled IO" smell for me. >
Re: [EXTERNAL] Re: CVS: cvs.openbsd.org: src
Once initially defined engineid should be static, a lot of SNMP polling applications use engineid as device identifier. Consistency and uniqueness are both important. Some polling system will not allow you to add another snmp device If engineid is a duplicate. Network devices usually derive engineid from lowest "numbered" interface. Unfortunately you can manually set them, this is bad when someone creates a configuration base template which has engineid defined, then you end up with 100's of network devices with the same engineid. I've spent quite a bit of time cleaning up from the mess this created. diana
Re: CVS: cvs.openbsd.org: src
On Mon, 2021-08-09 at 21:58 +0100, Stuart Henderson wrote: > On 2021/08/09 22:35, Martijn van Duren wrote: > > Moving to tech@ > > > > On Mon, 2021-08-09 at 20:56 +0100, Stuart Henderson wrote: > > > On 2021/08/09 12:14, Martijn van Duren wrote: > > > > CVSROOT:/cvs > > > > Module name:src > > > > Changes by: mart...@cvs.openbsd.org2021/08/09 12:14:53 > > > > > > > > Modified files: > > > > usr.sbin/snmpd : parse.y snmpd.c snmpd.conf.5 snmpd.h snmpe.c > > > > util.c > > > > > > > > Log message: > > > > Allow setting the engineid. > > > > > > > > The previous engineid was based aronud the engine boottime and a random > > > > value, which gives problems when sending/receiving unacknowledged PDUs > > > > (trapv2) over SNMPv3 with authentication enabled, which need a > > > > consistent > > > > engineid across restarts to determine the correct user from the sender. > > > > > > > > The new default engineid takes a sha256 hash (chosen for its longer > > > > output) > > > > of gethostname(3) and places the first 27 bytes after the new format > > > > number > > > > 129. This should give us a very low probability of collisions, assuming > > > > all machines have a unique name. > > > > > > what happens if there's a collision? i'm not sure it's safe to assume > > > unique names. > > > > The engineid is used to load the engine context of the originator agent. > > For unacknowledged pdu's this means at least loading the remote users > > (unlike get requests the users, keys and engineid are from the > > originator). > > > > If there's a collision for trapv2 this means that if a user exists on > > both remote agents but with different keys one of them will fail. > > > > But if you want to enter a new user of a new system and you find that > > that engineid already exist it should be a pretty big red flag that > > there's a collision and the new system can explicitly set the engineid. > > Similar if you ever want to set up something like "this engineid can > > only send from this ip": If you see that a new systems engineid already > > has such restriction it means you have to manually set the engineid. > > > > For existing get requests we don't have any risk: For acknowledged PDUs > > (get*/set) the engineid will be discovered (RFC3414 Section 4). And > > people can't have any hefty restrictions in place as is, because > > previously it was randomly generated at startup, so no persistent > > engineid to check on. > > > > I don't see any big risks here. > > > > A minor risk would be that if a hostname ever changes traps won't be > > received anymore, since the engineid changes. But changing a hostname > > seems like something that doesn't happen very often and brings with it > > a lot more migration research. > > > > But considering the question if there was a better idea for generating > > a consistent engineid by jmatthew@ on the 1st without any feedback I > > went with this one. > > If you still feel like there's a risk here after my previous reasoning > > feel free to come up with an alternative. We still have time to change > > it, and even then we have the range 130-255 to implement new formats > > (although then we come back to the discussion about defaults without > > negotiation) > > > > > > > The other formats as specified in SNMP-FRAMEWORK-MIB (RFC3411) are also > > > > supported as well as arbitrary formats in the range 128-255 for other > > > > private enterprise numbers in hex format. > > > > > > > > OK jmatthew@ > > > > > > > > > > > > > Thanks. I haven't looked closely at this before, I just know that it's > common for agents to set it randomly at startup (net-snmp docs say "must > be consistent through time and should not change or conflict with another > agent's engineID. Ever." which isn't exactly clear which is the stronger > requirement - consistency or not conflicting). I'd say the not changing. Because it determines how a password is changed to an auth-/priv-key, which is needed to decipher an incoming unacknowledged trap. However, users will always find some legitimate reason to change it. But that should be their burden. If during setup it's found that a conflict occurs it should be fine to change the engineid, but if you change it in production authentication might fail. > > Regarding the commit, the manual doesn't mention that there's a default. > Should it? e.g. maybe Manpages: not my strong suit :-) I've documented it under hosthash: "This format is the default" Maybe it should be more clear. Does this read better (including some extra explanation for your prior questions)? Index: snmpd.conf.5 === RCS file: /cvs/src/usr.sbin/snmpd/snmpd.conf.5,v retrieving revision 1.54 diff -u -p -r1.54 snmpd.conf.5 --- snmpd.conf.59 Aug 2021 19:13:08 - 1.54 +++ snmpd.conf.59 Aug 2021 22:16:18 - @@ -158,6 +158,12 @@ specifies the
Re: CVS: cvs.openbsd.org: src
On 2021/08/09 22:35, Martijn van Duren wrote: > Moving to tech@ > > On Mon, 2021-08-09 at 20:56 +0100, Stuart Henderson wrote: > > On 2021/08/09 12:14, Martijn van Duren wrote: > > > CVSROOT:/cvs > > > Module name:src > > > Changes by: mart...@cvs.openbsd.org2021/08/09 12:14:53 > > > > > > Modified files: > > > usr.sbin/snmpd : parse.y snmpd.c snmpd.conf.5 snmpd.h snmpe.c > > > util.c > > > > > > Log message: > > > Allow setting the engineid. > > > > > > The previous engineid was based aronud the engine boottime and a random > > > value, which gives problems when sending/receiving unacknowledged PDUs > > > (trapv2) over SNMPv3 with authentication enabled, which need a consistent > > > engineid across restarts to determine the correct user from the sender. > > > > > > The new default engineid takes a sha256 hash (chosen for its longer > > > output) > > > of gethostname(3) and places the first 27 bytes after the new format > > > number > > > 129. This should give us a very low probability of collisions, assuming > > > all machines have a unique name. > > > > what happens if there's a collision? i'm not sure it's safe to assume > > unique names. > > The engineid is used to load the engine context of the originator agent. > For unacknowledged pdu's this means at least loading the remote users > (unlike get requests the users, keys and engineid are from the > originator). > > If there's a collision for trapv2 this means that if a user exists on > both remote agents but with different keys one of them will fail. > > But if you want to enter a new user of a new system and you find that > that engineid already exist it should be a pretty big red flag that > there's a collision and the new system can explicitly set the engineid. > Similar if you ever want to set up something like "this engineid can > only send from this ip": If you see that a new systems engineid already > has such restriction it means you have to manually set the engineid. > > For existing get requests we don't have any risk: For acknowledged PDUs > (get*/set) the engineid will be discovered (RFC3414 Section 4). And > people can't have any hefty restrictions in place as is, because > previously it was randomly generated at startup, so no persistent > engineid to check on. > > I don't see any big risks here. > > A minor risk would be that if a hostname ever changes traps won't be > received anymore, since the engineid changes. But changing a hostname > seems like something that doesn't happen very often and brings with it > a lot more migration research. > > But considering the question if there was a better idea for generating > a consistent engineid by jmatthew@ on the 1st without any feedback I > went with this one. > If you still feel like there's a risk here after my previous reasoning > feel free to come up with an alternative. We still have time to change > it, and even then we have the range 130-255 to implement new formats > (although then we come back to the discussion about defaults without > negotiation) > > > > > The other formats as specified in SNMP-FRAMEWORK-MIB (RFC3411) are also > > > supported as well as arbitrary formats in the range 128-255 for other > > > private enterprise numbers in hex format. > > > > > > OK jmatthew@ > > > > > > > Thanks. I haven't looked closely at this before, I just know that it's common for agents to set it randomly at startup (net-snmp docs say "must be consistent through time and should not change or conflict with another agent's engineID. Ever." which isn't exactly clear which is the stronger requirement - consistency or not conflicting). Regarding the commit, the manual doesn't mention that there's a default. Should it? e.g. maybe Index: snmpd.conf.5 === RCS file: /cvs/src/usr.sbin/snmpd/snmpd.conf.5,v retrieving revision 1.54 diff -u -p -r1.54 snmpd.conf.5 --- snmpd.conf.59 Aug 2021 19:13:08 - 1.54 +++ snmpd.conf.59 Aug 2021 20:57:10 - @@ -153,6 +153,7 @@ generation for the .Ar auth and .Ar key . +By default, a hash of the system hostname is used. .Ar enterprise specifies the private enterprise number of the instance and can be either an integer or .
Re: CVS: cvs.openbsd.org: src
Moving to tech@ On Mon, 2021-08-09 at 20:56 +0100, Stuart Henderson wrote: > On 2021/08/09 12:14, Martijn van Duren wrote: > > CVSROOT:/cvs > > Module name:src > > Changes by: mart...@cvs.openbsd.org2021/08/09 12:14:53 > > > > Modified files: > > usr.sbin/snmpd : parse.y snmpd.c snmpd.conf.5 snmpd.h snmpe.c > > util.c > > > > Log message: > > Allow setting the engineid. > > > > The previous engineid was based aronud the engine boottime and a random > > value, which gives problems when sending/receiving unacknowledged PDUs > > (trapv2) over SNMPv3 with authentication enabled, which need a consistent > > engineid across restarts to determine the correct user from the sender. > > > > The new default engineid takes a sha256 hash (chosen for its longer output) > > of gethostname(3) and places the first 27 bytes after the new format number > > 129. This should give us a very low probability of collisions, assuming > > all machines have a unique name. > > what happens if there's a collision? i'm not sure it's safe to assume > unique names. The engineid is used to load the engine context of the originator agent. For unacknowledged pdu's this means at least loading the remote users (unlike get requests the users, keys and engineid are from the originator). If there's a collision for trapv2 this means that if a user exists on both remote agents but with different keys one of them will fail. But if you want to enter a new user of a new system and you find that that engineid already exist it should be a pretty big red flag that there's a collision and the new system can explicitly set the engineid. Similar if you ever want to set up something like "this engineid can only send from this ip": If you see that a new systems engineid already has such restriction it means you have to manually set the engineid. For existing get requests we don't have any risk: For acknowledged PDUs (get*/set) the engineid will be discovered (RFC3414 Section 4). And people can't have any hefty restrictions in place as is, because previously it was randomly generated at startup, so no persistent engineid to check on. I don't see any big risks here. A minor risk would be that if a hostname ever changes traps won't be received anymore, since the engineid changes. But changing a hostname seems like something that doesn't happen very often and brings with it a lot more migration research. But considering the question if there was a better idea for generating a consistent engineid by jmatthew@ on the 1st without any feedback I went with this one. If you still feel like there's a risk here after my previous reasoning feel free to come up with an alternative. We still have time to change it, and even then we have the range 130-255 to implement new formats (although then we come back to the discussion about defaults without negotiation) > > > The other formats as specified in SNMP-FRAMEWORK-MIB (RFC3411) are also > > supported as well as arbitrary formats in the range 128-255 for other > > private enterprise numbers in hex format. > > > > OK jmatthew@ > > >
Re: snmpd: tweak listen on
On 2021/08/09 20:55, Martijn van Duren wrote: > Updated diff after my engineid commit. ok > Index: snmpd.conf.5 > === > RCS file: /cvs/src/usr.sbin/snmpd/snmpd.conf.5,v > retrieving revision 1.53 > diff -u -p -r1.53 snmpd.conf.5 > --- snmpd.conf.5 9 Aug 2021 18:14:53 - 1.53 > +++ snmpd.conf.5 9 Aug 2021 18:54:56 - > @@ -96,9 +96,22 @@ reduced during bulk updates. > The default is > .Ic no . > .It Ic listen on Oo Ic tcp | udp Oc Ar address Oo Ic port Ar port Oc Op Ar > flags > -Specify the local address > +Specify the local > +.Ar address > .Xr snmpd 8 > should listen on for incoming SNMP messages. > +If > +.Ar address > +is set to > +.Cm any , > +it resolves to 0.0.0.0 and ::. > +Multiple > +.Ic listen on > +statements are supported. > +If no > +.Ic listen on > +statement is present, the default is > +.Ic listen on Cm any . > .Pp > The > .Ar flags while most snmpd users will likely know what 0.0.0.0 and :: are, this might be a bit friendlier (and is shorter) Specify the local .Ar address .Xr snmpd 8 should listen on for incoming SNMP messages, or .Cm any to listen on all local IPv4 and IPv6 addresses. Multiple .Ic listen on statements are supported. If no .Ic listen on statement is present, the default is .Ic listen on Cm any .
Re: snmpd: allow sending traps with SNMPv3
On Tue, 2021-07-27 at 21:28 +0200, Martijn van Duren wrote:
> This diff allows sending traps in SNMPv3 messages.
> It defaults to the global seclevel, but it can be specified on a per
> rule basis.
>
> Diff requires both previous setting engineid and ober_dup diff.
>
> Tested with netsnmp's snmptrapd and my WIP diff.
>
> The other 2 outstanding diffs are for receiving SNMPv3 traps.
>
> OK?
>
> martijn@
>
Resending now that the engineid diff is in.
Still awaiting the commit of ober_dup diff[0].
OK once that one goes in?
Also, rereading the diff, splitting the trap receiver in two might be a
bit clutch. Once again invoking the manpage gurus.
martijn@
[0] https://marc.info/?l=openbsd-tech=162698527126249=2
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/snmpd/parse.y,v
retrieving revision 1.65
diff -u -p -r1.65 parse.y
--- parse.y 9 Aug 2021 18:14:53 - 1.65
+++ parse.y 9 Aug 2021 19:41:38 -
@@ -134,10 +134,10 @@ typedef struct {
%token HANDLE DEFAULT SRCADDR TCP UDP PFADDRFILTER PORT
%token STRING
%token NUMBER
-%typehostcmn
+%typeusmuser community optcommunity
%typesrcaddr port
%typeoptwrite yesno seclevel listenopt listenopts
-%type objtype cmd
+%type objtype cmd hostauth hostauthv3 usmauthopts usmauthopt
%type oid hostoid trapoid
%type auth
%type enc
@@ -242,13 +242,13 @@ main : LISTEN ON listenproto
free($3);
}
| TRAP RECEIVER host
- | TRAP HANDLE hostcmn trapoid cmd {
- struct trapcmd *cmd = $5.data;
+ | TRAP HANDLE trapoid cmd {
+ struct trapcmd *cmd = $4.data;
- cmd->cmd_oid = $4;
+ cmd->cmd_oid = $3;
if (trapcmd_add(cmd) != 0) {
- free($4);
+ free($3);
free(cmd);
yyerror("duplicate oid");
YYERROR;
@@ -267,8 +267,8 @@ main: LISTEN ON listenproto
| PFADDRFILTER yesno{
conf->sc_pfaddrfilter = $2;
}
- | SECLEVEL seclevel {
- conf->sc_min_seclevel = $2;
+ | seclevel {
+ conf->sc_min_seclevel = $1;
}
| USER STRING {
const char *errstr;
@@ -720,15 +720,93 @@ hostoid : /* empty */
{ $$ = NULL; }
| OBJECTID oid { $$ = $2; }
;
-hostcmn: /* empty */ { $$ = NULL; }
- | COMMUNITY STRING { $$ = $2; }
+usmuser: USER STRING {
+ if (strlen($2) > SNMPD_MAXUSERNAMELEN) {
+ yyerror("User name too long: %s", $2);
+ free($2);
+ YYERROR;
+ }
+ $$ = $2;
+ }
+ ;
+
+community : COMMUNITY STRING {
+ if (strlen($2) > SNMPD_MAXCOMMUNITYLEN) {
+ yyerror("Community too long: %s", $2);
+ free($2);
+ YYERROR;
+ }
+ $$ = $2;
+ }
+ ;
+
+optcommunity : /* empty */ { $$ = NULL; }
+ | community { $$ = $1; }
+ ;
+
+usmauthopt : usmuser {
+ $$.data = $1;
+ $$.value = -1;
+ }
+ | seclevel {
+ $$.data = 0;
+ $$.value = $1;
+ }
+ ;
+
+usmauthopts: /* empty */ {
+ $$.data = NULL;
+ $$.value = -1;
+ }
+ | usmauthopts usmauthopt{
+ if ($2.data != NULL) {
+ if ($$.data != NULL) {
+ yyerror("user redefined");
+ free($2.data);
+ YYERROR;
+ }
+ $$.data = $2.data;
+ } else {
+ if ($$.value != -1) {
+ yyerror("seclevel redefined");
+
Re: [patch] Preserve keymap adjustments on suspend/resume
Pardon my impatience -- I am a newbie and I don't really know what should happen next (if anything?). I have skipped any introduction from my original email, imitating the communication style in this list -- straight to the point and because it was already long. Although this is my first patch to OpenBSD I am familiar with inner workings of operating systems from few books[1] and commercial development of a driver for Windows (CE and NT) back in 2005. Thanks to the very clear organization of OpenBSD code it is a pleasure to study and figure things out. I have spent a week on massaging this problem, discovering corner cases, testing and ironing out regressions. I did my best to keep the patch at minimum while retaining correctness and clarity. Some changes like the move of t_keymap field (apart from the change of type) were done to re-align it with NetBSD. I was in doubt about locking, but tracing the path of ioctl handling I have not found anything preventing two CPU cores from executing wskbd ioctls in parallel on amd64 (and it is a NOLOCK syscall). In fact, now I wonder why wskbd_softc is not protected. Now a little bit on the motivation... My current day job does not involve system programming anymore and I miss it often. Also, with kids growing up I feel I can afford to contribute to important open projects with my experience and skills. I fiddle with OpenBSD for a long time and I admire how simple, stable and well-documented it is. In our age of pervasive electronic waste, robust software often makes a difference between a useful computer and garbage. I realize that changes in wscons related to custom keymaps and USB keyboards is something of little priority. Nevertheless, this is one annoying thing I randomly picked up and got to the bottom of it on my short staycation and it was a sufficiently low entry barrier. I hope it may be useful for other OpenBSD users, especially on laptop computers which may not have a USB keyboard connected yet at boot time. My longer-term aspiration is extending and maintaining hardware support, especially on ARM (I own a Pinebook Pro and few RPis). I would appreciate a bit of guidance or feedback. Best regards, Sergii Rudchenko [1] Of which the most prominent are: - "Modern Operating Systems (3rd edition)" by Andrew S. Tannenbaum - "The Design And Implementation Of The Freebsd Operating System" by Marshall Kirk McKusick and George V. Neville-Neil - "Linux Kernel Development (2nd edition)" by Robert Love P.S. Sorry for the git patch with a prefix, I came across https://www.openbsd.org/faq/faq5.html#Diff too late. As far I understand it should be fine with -p1
Re: snmpd: tweak listen on
On Mon, 2021-08-09 at 11:57 +0200, Martijn van Duren wrote:
> On Sun, 2021-08-08 at 14:44 +0100, Stuart Henderson wrote:
> > > This is probably is a bad example.
> > > Reading it like this: you're correct that we listen on all interfaces
> > > by default, but that's not listed in snmpd.conf(5). So that should
> > > probably be fixed (including mentioning that setting one "listen on"
> > > disables the all interfaces default).
> >
> > Let's handle that separately. (it would be convenient to support
> > "any" to mean any v4+v6 as well).
> >
> > > Second, your examples enable snmpv2c on all interfaces, while you
> > > enable an implicit snmpv3 on 127.0.0.1. This should probably be the
> >
> > I wasn't intending that they should all be uncommented at once,
> > just showing some common options. And actually it seems snmpd
> > doesn't allow listening to 0.0.0.0 as well as a specific v4 address
> > (and similarly for :: and v6) so while it's a convenient idea to
> > allow v2c on localhost for quick testing while using v3 for external
> > traffic, it doesn't actually work.
>
> This diff fixes all of the above:
> - Allow any to be used resolving to 0.0.0.0 and ::
> - Set SO_REUSEADDR on sockets, so we can listen on both any and
> localhost
> - Document that we listen on any by default
>
> The listen on text is starting to get quite large, so I hope that one of
> our man guru's can either confirm that it's still readable enough or
> help me to polish it.
>
> martijn@
>
Updated diff after my engineid commit.
This also incorperates some manpage feedback from schwarze@.
OK?
martijn@
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/snmpd/parse.y,v
retrieving revision 1.65
diff -u -p -r1.65 parse.y
--- parse.y 9 Aug 2021 18:14:53 - 1.65
+++ parse.y 9 Aug 2021 18:54:56 -
@@ -135,8 +135,9 @@ typedef struct {
%token STRING
%token NUMBER
%typehostcmn
+%typelistenproto listenflag listenflags
%typesrcaddr port
-%typeoptwrite yesno seclevel listenopt listenopts
+%typeoptwrite yesno seclevel
%type objtype cmd
%type oid hostoid trapoid
%type auth
@@ -202,7 +203,7 @@ yesno : STRING {
}
;
-main : LISTEN ON listenproto
+main : LISTEN ON listen_udptcp
| engineid_local {
if (conf->sc_engineid_len != 0) {
yyerror("Redefinition of engineid");
@@ -288,15 +289,16 @@ main : LISTEN ON listenproto
}
;
-listenproto: UDP listen_udp
- | TCP listen_tcp
- | listen_udp
+listenproto: /* empty */ { $$ = SOCK_DGRAM; }
+ | UDP { $$ = SOCK_DGRAM; }
+ | TCP listen_tcp{ $$ = SOCK_STREAM; }
+ ;
-listenopts : /* empty */ { $$ = 0; }
- | listenopts listenopt { $$ |= $2; }
+listenflags: /* empty */ { $$ = 0; }
+ | listenflags listenflag { $$ |= $2; }
;
-listenopt : READ { $$ = ADDRESS_FLAG_READ; }
+listenflag : READ { $$ = ADDRESS_FLAG_READ; }
| WRITE { $$ = ADDRESS_FLAG_WRITE; }
| NOTIFY { $$ = ADDRESS_FLAG_NOTIFY; }
| SNMPV1 { $$ = ADDRESS_FLAG_SNMPV1; }
@@ -304,71 +306,50 @@ listenopt : READ { $$ = ADDRESS_FLAG_REA
| SNMPV3 { $$ = ADDRESS_FLAG_SNMPV3; }
;
-listen_udp : STRING port listenopts{
+listen_udptcp : listenproto STRING port listenflags {
struct sockaddr_storage ss[16];
- int nhosts, i;
- char *port = $2;
+ int nhosts, j;
+ char *address[2], *port = $3;
+ size_t addresslen = 1, i;
if (port == NULL) {
- if (($3 & ADDRESS_FLAG_PERM) ==
+ if (($4 & ADDRESS_FLAG_PERM) ==
ADDRESS_FLAG_NOTIFY)
port = SNMPTRAP_PORT;
else
port = SNMP_PORT;
}
- nhosts = host($1, port, SOCK_DGRAM, ss, nitems(ss));
- if (nhosts < 1) {
- yyerror("invalid address: %s", $1);
- free($1);
- free($2);
- YYERROR;
+ if (strcmp($2, "any") == 0) {
+ addresslen = 2;
+ address[0] = "0.0.0.0";
+ address[1] = "::";
+ } else {
+
Re: dhcpleased(8): ignore servers / parts of lease
On 2021-08-09 09:56 -06, "Theo de Raadt" wrote: > Using the word "security", you've got to be kidding. > > If a dhcp server on a L2 segment can be "rogue" about one thing, it can > most certainly lie about any other answer, or act out in many other > ways. > > The only way to avoid "rogue" DHCP servers on a segment is to not ask > DHCP questions on that segment. > > This is not a security feature. It is purely for selecting aspects of > the answer from TRUSTED DHCP servers. ...and filtering out non-malicious "rogue" DHCP servers. Say you are at a conference or on a hotel wifi with not-stellar L2 security and some jackass spins up a linksys. you can either a) go to a different hotel b) ignore the stupid dhcp server and maybe get work done It's convenient, not a security feature. > > Andras Vinter wrote: > >> The Linux dhclient supports it and it's actually a nice to have >> feature as it can increase the security by keeping out the rogue DHCP >> servers from an entire LAN range. But probably you can achieve similar >> functionality with the interface restriction. >> >> On Mon, Aug 9, 2021 at 3:33 PM Stuart Henderson wrote: >> > >> > On 2021/08/09 15:03, Andras Vinter wrote: >> > > It's probably an overkill for first implementation, but in the future >> > > I think we should support subnet definitions in CIDR notation (e.x.: >> > > 192.168.0.0/24) and IP ranges for fine control (e.x.: >> > > 192.168.0.100-192.168.0.254). >> > >> > dhclient never needed that. >> > >> -- I'm not entirely sure you are real.
Re: dhcpleased(8): ignore servers / parts of lease
Using the word "security", you've got to be kidding. If a dhcp server on a L2 segment can be "rogue" about one thing, it can most certainly lie about any other answer, or act out in many other ways. The only way to avoid "rogue" DHCP servers on a segment is to not ask DHCP questions on that segment. This is not a security feature. It is purely for selecting aspects of the answer from TRUSTED DHCP servers. Andras Vinter wrote: > The Linux dhclient supports it and it's actually a nice to have > feature as it can increase the security by keeping out the rogue DHCP > servers from an entire LAN range. But probably you can achieve similar > functionality with the interface restriction. > > On Mon, Aug 9, 2021 at 3:33 PM Stuart Henderson wrote: > > > > On 2021/08/09 15:03, Andras Vinter wrote: > > > It's probably an overkill for first implementation, but in the future > > > I think we should support subnet definitions in CIDR notation (e.x.: > > > 192.168.0.0/24) and IP ranges for fine control (e.x.: > > > 192.168.0.100-192.168.0.254). > > > > dhclient never needed that. > > >
Re: dhcpleased(8): ignore servers / parts of lease
The Linux dhclient supports it and it's actually a nice to have feature as it can increase the security by keeping out the rogue DHCP servers from an entire LAN range. But probably you can achieve similar functionality with the interface restriction. On Mon, Aug 9, 2021 at 3:33 PM Stuart Henderson wrote: > > On 2021/08/09 15:03, Andras Vinter wrote: > > It's probably an overkill for first implementation, but in the future > > I think we should support subnet definitions in CIDR notation (e.x.: > > 192.168.0.0/24) and IP ranges for fine control (e.x.: > > 192.168.0.100-192.168.0.254). > > dhclient never needed that. >
Re: libedit: stop ignoring SIGINT
On Mon, 2021-08-09 at 15:17 +0200, Ingo Schwarze wrote: > Hi Martijn, > > Martijn van Duren wrote on Mon, Aug 09, 2021 at 02:15:42PM +0200: > > > If we're stripping down fixio to a single function, why not > > inline the whole thing? > > I deliberately tried to: > > 1. Keep patches that change behaviour as small as possible to make > review as simple as possible for people who fear they might be > affected but are not specifically interested in editline(3). > > 2. Make sure that reorg / cleanup patches do not change behaviour. > > > Also, as far as my brain explains things to me, it could theoretically > > be possible that the signal handler kicks in right after we return a -1 > > from read(2), making it possible to get something like an EIO and > > entering the sig switch statement. Assuming that a signal handler > > doesn't clobber our errno (which libedit doesn't do, but who knows what > > bad code is out there) we should probably only check the signal type > > when we know we have an EINTR. > > Yes, that looks like a race condition bug that so far escaped my > attention. But it seems unrelated to what should happen with signals > in general, so can we keep fixing that bug separate? > > > Finally, I don't understand why we only have a single retry on EAGAIN? > > Not having *any* retries inside read_char() would look like a worthy > long-term goal to me, but i'm not yet completely sure that can be > reached. I would prefer to steer into the direction of fewer magic > retries rather than more of them. Either way, EAGAIN seems unrelated > to SIGINT, so i'd prefer to keep topics separate. > > > If the application keeps resetting the FIONBIO in such a furious way > > that it happens more then once in a single call (no idea how that could > > happen) it might be an uphill battle, but we just burn away cpu cycles. > > It is not and should probably not be treated like something fatal. > > Actually, if the application sets FIONBIO at all (even once), chances > are quite low in the first place that stuff works as inteded by the > author of the application. So i certainly wouldn't worry about an > application setting FIONBIO in a loop, we have significantly worse > problems than that. But again, that's a separate topic. > > > Diff below does this. (minus 27 LoC) > > I do not in general object to cleaning this code up, and getting rid > of read__fixio() indeed seems to be a long-term goal. But i hope > we will be able to remove all the (mostly broken) functionality > from read__fixio() in a step-by-step manner, and once the function > is empty, it will fade away without having to disturb the code in > read_char(). Either way - separate topic... > > The most important short-term goal seems to fix sftp(1), including > the steps required to get that done in a clean way. > > > The following ports use libedit (there might be a better way of > > finding them, but this works): > > Hmm, thanks, that list feels useful. > > > So if we decide which of our interpretations should take precedence > > it might be a good idea to put it into snaps for a while. > > I don't think so in this case. Let's not over-use the feature of > putting stuff in snaps. I think that should be reserved for stuff > that is quite important and somewhat urgent and can't easily be > tested in a less disruptive way. But here, testing a program is > quite feasible once you know which program to test. > > Besides, *if* this patch causes a change in behaviour of a port, > the most likely change is that a program that now ignores SIGINT > exits on SIGINT afterwards. That may be worth investigating and > making a decision on in each individual case, but it's not a > super-critical change in behaviour that might require testing > in snaps. > > Yours, > Ingo > Your reasoning makes sense to me. Assuming you're confident enough with the applications linked to libedit: OK martijn@
Re: dhcpleased(8): ignore servers / parts of lease
On 2021/08/09 15:03, Andras Vinter wrote: > It's probably an overkill for first implementation, but in the future > I think we should support subnet definitions in CIDR notation (e.x.: > 192.168.0.0/24) and IP ranges for fine control (e.x.: > 192.168.0.100-192.168.0.254). dhclient never needed that.
Re: libedit: stop ignoring SIGINT
Ingo Schwarze wrote: > > So if we decide which of our interpretations should take precedence > > it might be a good idea to put it into snaps for a while. > > I don't think so in this case. Let's not over-use the feature of > putting stuff in snaps. I think that should be reserved for stuff > that is quite important and somewhat urgent and can't easily be > tested in a less disruptive way. But here, testing a program is > quite feasible once you know which program to test. We know what needs testing, because it links against the library. Once the first set of base programs is tested to work well with the change, it is quite likely we have complete confidence it only improves the situation in ports. Snapshot testing is reserved for discovering unknown breakage, quickly. Like "we don't know how or which programs abuse something" or "what weird machines do people have" or "we don't know how our user's fingers work". It's not like our users are going to quickly discover a weird behaviour from ^C, considering I only noticed this decade-old problem in sftp a few days ago.
Re: libedit: stop ignoring SIGINT
Ingo Schwarze wrote: > as mentioned earlier, deraadt@ reported that sftp(1) ignores Ctrl-C. > Fixing that without longjmp(3) requires making editline(3) better > behaved. If a library interface encourages use longjmp(), that library should be thrown into the dustbin of history. Our src tree has very few longjmp these days. Thank you for the effort to discourage addition of more. > The following patch causes el_gets(3) and el_wgets(3) to return > failure when read(2)ing from the terminal is interrupted by a > signal other than SIGCONT or SIGWINCH. That allows the calling > program to decide what to do, usually either exit the program or > provide a fresh prompt to the user. Looks good. > * bc(1) >It behaves well with the patch: Ctrl-C discards the current >input line and starts a new input line. >The reason why this already works even without the patch >is that bc(1) does very scary stuff inside the signal handler: >pass a file-global EditLine pointer on the heap to el_line(3) >and access fields inside the returned struct. Needless to >say that no signal handler should do such things... Otto -- if you are short of time, at minimum mark the variable usage line with /* XXX signal race */ as we have done throughout the tree. I would encourage anyone who sees such problems inside any signal handler to show such comment-adding diffs. If these problems are documented, they can be fixed eventually, usually through event-loop logic, but I'll admit many of the comments are in non-event-loop programs. > * ftp(1) >It behaves well with the patch: Ctrl-C discards the current >input line and provides a fresh prompt. >The reason why it already works without the patch is that ftp(1) >uses setjmp(3)/longjmp(3) to forcefully grab back control >from el_gets(3) without waiting for it to return. Horrible isn't it. > * sftp(1) >Behaviour is improved with the patch: Ctrl-C now exits sftp(1). >If desired, i can supply a very simple follow-up patch to sftp.c >to instead behave like ftp(1) and bc(1), i.e. discard the >current input line and provide a fresh prompt. >Neither doing undue work in the signal handler nor longjmp(3) >will be required for that (if this patch gets committed). I suspect dtucker will want to decide on the interactive behaviour, but what you describe sounds right. > Also note that deraadt@ pointed out in private mail to me that the > fact that read__fixio() clears FIONBIO is probably a symptom of > botched editline(3) API design. That might be worth fixing, too, > as far as that is feasible, but it is unrelated to the sftp(1) > Ctrl-C issue; let's address one topic at a time. I mentioned rarely having seen a good outcome from code mixing any of the 3: FIONBIO, FIONREAD, and select/poll. Often the non-blocking was added to select/poll code to hide some sort of bug, or the select/poll code was added amateurishly to older code without removing the FIONBIO. There are a few situations you need both approaches mixed, but it isn't the general case, and thus FIONBIO has a "polled IO" smell for me.
Re: libedit: stop ignoring SIGINT
Hi Martijn, Martijn van Duren wrote on Mon, Aug 09, 2021 at 02:15:42PM +0200: > If we're stripping down fixio to a single function, why not > inline the whole thing? I deliberately tried to: 1. Keep patches that change behaviour as small as possible to make review as simple as possible for people who fear they might be affected but are not specifically interested in editline(3). 2. Make sure that reorg / cleanup patches do not change behaviour. > Also, as far as my brain explains things to me, it could theoretically > be possible that the signal handler kicks in right after we return a -1 > from read(2), making it possible to get something like an EIO and > entering the sig switch statement. Assuming that a signal handler > doesn't clobber our errno (which libedit doesn't do, but who knows what > bad code is out there) we should probably only check the signal type > when we know we have an EINTR. Yes, that looks like a race condition bug that so far escaped my attention. But it seems unrelated to what should happen with signals in general, so can we keep fixing that bug separate? > Finally, I don't understand why we only have a single retry on EAGAIN? Not having *any* retries inside read_char() would look like a worthy long-term goal to me, but i'm not yet completely sure that can be reached. I would prefer to steer into the direction of fewer magic retries rather than more of them. Either way, EAGAIN seems unrelated to SIGINT, so i'd prefer to keep topics separate. > If the application keeps resetting the FIONBIO in such a furious way > that it happens more then once in a single call (no idea how that could > happen) it might be an uphill battle, but we just burn away cpu cycles. > It is not and should probably not be treated like something fatal. Actually, if the application sets FIONBIO at all (even once), chances are quite low in the first place that stuff works as inteded by the author of the application. So i certainly wouldn't worry about an application setting FIONBIO in a loop, we have significantly worse problems than that. But again, that's a separate topic. > Diff below does this. (minus 27 LoC) I do not in general object to cleaning this code up, and getting rid of read__fixio() indeed seems to be a long-term goal. But i hope we will be able to remove all the (mostly broken) functionality from read__fixio() in a step-by-step manner, and once the function is empty, it will fade away without having to disturb the code in read_char(). Either way - separate topic... The most important short-term goal seems to fix sftp(1), including the steps required to get that done in a clean way. > The following ports use libedit (there might be a better way of > finding them, but this works): Hmm, thanks, that list feels useful. > So if we decide which of our interpretations should take precedence > it might be a good idea to put it into snaps for a while. I don't think so in this case. Let's not over-use the feature of putting stuff in snaps. I think that should be reserved for stuff that is quite important and somewhat urgent and can't easily be tested in a less disruptive way. But here, testing a program is quite feasible once you know which program to test. Besides, *if* this patch causes a change in behaviour of a port, the most likely change is that a program that now ignores SIGINT exits on SIGINT afterwards. That may be worth investigating and making a decision on in each individual case, but it's not a super-critical change in behaviour that might require testing in snaps. Yours, Ingo
Re: dhcpleased(8): ignore servers / parts of lease
It's probably an overkill for first implementation, but in the future
I think we should support subnet definitions in CIDR notation (e.x.:
192.168.0.0/24) and IP ranges for fine control (e.x.:
192.168.0.100-192.168.0.254).
BRs
/Andras
On Mon, Aug 9, 2021 at 2:35 PM Florian Obser wrote:
>
> On 2021-08-08 12:14 -07, patrick keshishian wrote:
> > On Sun, Aug 08, 2021 at 12:37:54PM +0200, Florian Obser wrote:
> >> This implements ignoring of nameservers and / or routes in leases as
> >> well as completely ignoring servers (you cannot block rogue DHCP servers
> >> in pf because bpf sees packets before pf).
> >>
> >> Various people voiced the need for these features.
> >> Tests, OKs?
> >>
> >> diff --git dhcpleased.c dhcpleased.c
> >> index 36a4a21c21a..e005d7de9ae 100644
> >> --- dhcpleased.c
> >> +++ dhcpleased.c
> >> @@ -569,6 +569,20 @@ main_dispatch_engine(int fd, short event, void *bula)
> >> sizeof(imsg_interface.if_index));
> >> break;
> >> }
> >> +case IMSG_WITHDRAW_ROUTES: {
> >> +struct imsg_configure_interface imsg_interface;
> >> +if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_interface))
> >> +fatalx("%s: IMSG_CONFIGURE_INTERFACE wrong "
> >> +"length: %lu", __func__,
> >> +IMSG_DATA_SIZE(imsg));
> >> +memcpy(_interface, imsg.data,
> >> +sizeof(imsg_interface));
> >> +if (imsg_interface.routes_len >= MAX_DHCP_ROUTES)
> >> +fatalx("%s: too many routes in imsg",
> >> __func__);
> >> +if (imsg_interface.routes_len > 0)
> >> +configure_routes(RTM_DELETE, _interface);
> >> +break;
> >> +}
> >> case IMSG_PROPOSE_RDNS: {
> >> struct imsg_propose_rdns rdns;
> >> if (IMSG_DATA_SIZE(imsg) != sizeof(rdns))
> >> diff --git dhcpleased.conf.5 dhcpleased.conf.5
> >> index 9e6846f899e..b856113bac1 100644
> >> --- dhcpleased.conf.5
> >> +++ dhcpleased.conf.5
> >> @@ -57,6 +57,17 @@ A list of interfaces to overwrite defaults:
> >> .Ic interface
> >> options are as follows:
> >> .Bl -tag -width Ds
> >> +.It Ic ignore dns
> >> +Ignore nameservers from leases on this interface.
> >> +The default is to not ignore nameservers.
> >> +.It Ic ignore routes
> >> +Ignore routes from leases on this interface.
> >> +The default is to not ignore routes.
> >> +.It Ic ignore Ar server-ip
> >> +Ignore leases from
> >> +.Ar server-ip .
> >> +This option can be listed multiple times.
> >> +The default is to not ignore servers.
> >> .It Ic send client id Ar client-id
> >> Send the dhcp client identifier option with a value of
> >> .Ar client-id .
> >> diff --git dhcpleased.h dhcpleased.h
> >> index 7f6ec87ad19..1d20b77dbd3 100644
> >> --- dhcpleased.h
> >> +++ dhcpleased.h
> >> @@ -151,6 +151,12 @@
> >> #define DHCPRELEASE 7
> >> #define DHCPINFORM 8
> >>
> >> +/* Ignore parts of DHCP lease */
> >> +#define IGN_ROUTES 1
> >> +#define IGN_DNS 2
> >> +
> >> +#define MAX_SERVERS 16 /* max servers that can be ignores
> >> per if */
> >
> > Typo in comment: ignored (not ignores)
>
> thanks, fixed in my tree.
>
> >
> > Should there be a mention of a limitation in the man page where
> > it states the option can be listed multiple times?
>
> I really do hope that 16 is enough for everybody. If it turns out not
> then we should probably bite the bullet and implement this with a proper
> list. So for now I don't want to document the limitation.
>
> >
> > --patrick
> >
> >
> >> +
> >> #define IMSG_DATA_SIZE(imsg)((imsg).hdr.len - IMSG_HEADER_SIZE)
> >> #define DHCP_SNAME_LEN 64
> >> #define DHCP_FILE_LEN 128
> >> @@ -216,6 +222,7 @@ enum imsg_type {
> >> IMSG_DECONFIGURE_INTERFACE,
> >> IMSG_PROPOSE_RDNS,
> >> IMSG_WITHDRAW_RDNS,
> >> +IMSG_WITHDRAW_ROUTES,
> >> IMSG_REPROPOSE_RDNS,
> >> IMSG_REQUEST_REBOOT,
> >> };
> >> @@ -246,6 +253,9 @@ struct iface_conf {
> >> int vc_id_len;
> >> uint8_t *c_id;
> >> int c_id_len;
> >> +int ignore;
> >> +struct in_addr ignore_servers[MAX_SERVERS];
> >> +int ignore_servers_len;
> >> };
> >>
> >> struct dhcpleased_conf {
> >> @@ -304,6 +314,8 @@ const char *sin_to_str(struct sockaddr_in *);
> >>
> >> /* frontend.c */
> >> struct iface_conf *find_iface_conf(struct iface_conf_head *, char *);
> >> +int *changed_ifaces(struct dhcpleased_conf *, struct
> >> + dhcpleased_conf *);
> >>
> >> /* printconf.c */
> >> void
Re: dhcpleased(8): ignore servers / parts of lease
On 2021-08-08 12:14 -07, patrick keshishian wrote:
> On Sun, Aug 08, 2021 at 12:37:54PM +0200, Florian Obser wrote:
>> This implements ignoring of nameservers and / or routes in leases as
>> well as completely ignoring servers (you cannot block rogue DHCP servers
>> in pf because bpf sees packets before pf).
>>
>> Various people voiced the need for these features.
>> Tests, OKs?
>>
>> diff --git dhcpleased.c dhcpleased.c
>> index 36a4a21c21a..e005d7de9ae 100644
>> --- dhcpleased.c
>> +++ dhcpleased.c
>> @@ -569,6 +569,20 @@ main_dispatch_engine(int fd, short event, void *bula)
>> sizeof(imsg_interface.if_index));
>> break;
>> }
>> +case IMSG_WITHDRAW_ROUTES: {
>> +struct imsg_configure_interface imsg_interface;
>> +if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_interface))
>> +fatalx("%s: IMSG_CONFIGURE_INTERFACE wrong "
>> +"length: %lu", __func__,
>> +IMSG_DATA_SIZE(imsg));
>> +memcpy(_interface, imsg.data,
>> +sizeof(imsg_interface));
>> +if (imsg_interface.routes_len >= MAX_DHCP_ROUTES)
>> +fatalx("%s: too many routes in imsg", __func__);
>> +if (imsg_interface.routes_len > 0)
>> +configure_routes(RTM_DELETE, _interface);
>> +break;
>> +}
>> case IMSG_PROPOSE_RDNS: {
>> struct imsg_propose_rdns rdns;
>> if (IMSG_DATA_SIZE(imsg) != sizeof(rdns))
>> diff --git dhcpleased.conf.5 dhcpleased.conf.5
>> index 9e6846f899e..b856113bac1 100644
>> --- dhcpleased.conf.5
>> +++ dhcpleased.conf.5
>> @@ -57,6 +57,17 @@ A list of interfaces to overwrite defaults:
>> .Ic interface
>> options are as follows:
>> .Bl -tag -width Ds
>> +.It Ic ignore dns
>> +Ignore nameservers from leases on this interface.
>> +The default is to not ignore nameservers.
>> +.It Ic ignore routes
>> +Ignore routes from leases on this interface.
>> +The default is to not ignore routes.
>> +.It Ic ignore Ar server-ip
>> +Ignore leases from
>> +.Ar server-ip .
>> +This option can be listed multiple times.
>> +The default is to not ignore servers.
>> .It Ic send client id Ar client-id
>> Send the dhcp client identifier option with a value of
>> .Ar client-id .
>> diff --git dhcpleased.h dhcpleased.h
>> index 7f6ec87ad19..1d20b77dbd3 100644
>> --- dhcpleased.h
>> +++ dhcpleased.h
>> @@ -151,6 +151,12 @@
>> #define DHCPRELEASE 7
>> #define DHCPINFORM 8
>>
>> +/* Ignore parts of DHCP lease */
>> +#define IGN_ROUTES 1
>> +#define IGN_DNS 2
>> +
>> +#define MAX_SERVERS 16 /* max servers that can be ignores per
>> if */
>
> Typo in comment: ignored (not ignores)
thanks, fixed in my tree.
>
> Should there be a mention of a limitation in the man page where
> it states the option can be listed multiple times?
I really do hope that 16 is enough for everybody. If it turns out not
then we should probably bite the bullet and implement this with a proper
list. So for now I don't want to document the limitation.
>
> --patrick
>
>
>> +
>> #define IMSG_DATA_SIZE(imsg)((imsg).hdr.len - IMSG_HEADER_SIZE)
>> #define DHCP_SNAME_LEN 64
>> #define DHCP_FILE_LEN 128
>> @@ -216,6 +222,7 @@ enum imsg_type {
>> IMSG_DECONFIGURE_INTERFACE,
>> IMSG_PROPOSE_RDNS,
>> IMSG_WITHDRAW_RDNS,
>> +IMSG_WITHDRAW_ROUTES,
>> IMSG_REPROPOSE_RDNS,
>> IMSG_REQUEST_REBOOT,
>> };
>> @@ -246,6 +253,9 @@ struct iface_conf {
>> int vc_id_len;
>> uint8_t *c_id;
>> int c_id_len;
>> +int ignore;
>> +struct in_addr ignore_servers[MAX_SERVERS];
>> +int ignore_servers_len;
>> };
>>
>> struct dhcpleased_conf {
>> @@ -304,6 +314,8 @@ const char *sin_to_str(struct sockaddr_in *);
>>
>> /* frontend.c */
>> struct iface_conf *find_iface_conf(struct iface_conf_head *, char *);
>> +int *changed_ifaces(struct dhcpleased_conf *, struct
>> + dhcpleased_conf *);
>>
>> /* printconf.c */
>> voidprint_config(struct dhcpleased_conf *);
>> diff --git engine.c engine.c
>> index 076a57e9ba6..67693c358ee 100644
>> --- engine.c
>> +++ engine.c
>> @@ -139,6 +139,7 @@ void
>> send_configure_interface(struct dhcpleased_iface *);
>> void send_rdns_proposal(struct dhcpleased_iface *);
>> void send_deconfigure_interface(struct
>> dhcpleased_iface *);
>> void send_rdns_withdraw(struct dhcpleased_iface *);
>> +void
Re: dhcpleased(8): ignore servers / parts of lease
On 2021-08-08 11:52 +01, Jason McIntyre wrote:
> On Sun, Aug 08, 2021 at 12:37:54PM +0200, Florian Obser wrote:
>> This implements ignoring of nameservers and / or routes in leases as
>> well as completely ignoring servers (you cannot block rogue DHCP servers
>> in pf because bpf sees packets before pf).
>>
>> Various people voiced the need for these features.
>> Tests, OKs?
>>
>> diff --git dhcpleased.conf.5 dhcpleased.conf.5
>> index 9e6846f899e..b856113bac1 100644
>> --- dhcpleased.conf.5
>> +++ dhcpleased.conf.5
>> @@ -57,6 +57,17 @@ A list of interfaces to overwrite defaults:
>> .Ic interface
>> options are as follows:
>> .Bl -tag -width Ds
>> +.It Ic ignore dns
>> +Ignore nameservers from leases on this interface.
>> +The default is to not ignore nameservers.
>> +.It Ic ignore routes
>> +Ignore routes from leases on this interface.
>> +The default is to not ignore routes.
>> +.It Ic ignore Ar server-ip
>> +Ignore leases from
>> +.Ar server-ip .
>> +This option can be listed multiple times.
>> +The default is to not ignore servers.
>
> hi.
>
> you probably want
>
> .It Ic ignore Ar server-ip ...
>
> then you can either remove the "multiple times" text to shorten the
> text block, or leave it in to be explicit.
That's actually not implemented, only this works (for now):
ignore 192.0.2.1
ignore 192.0.2.2
This is a syntax error:
ignore 192.0.2.1 192.0.2.2
I should probably implement
ignore { 192.0.2.1 192.0.2.2 }
>
> the diff reads fine.
>
> jmc
>
>> .It Ic send client id Ar client-id
>> Send the dhcp client identifier option with a value of
>> .Ar client-id .
>
--
I'm not entirely sure you are real.
Re: libedit: stop ignoring SIGINT
On Mon, Aug 09, 2021 at 01:19:08PM +0200, Ingo Schwarze wrote: > Hi, > > as mentioned earlier, deraadt@ reported that sftp(1) ignores Ctrl-C. > Fixing that without longjmp(3) requires making editline(3) better > behaved. > > Currently, when read(2) from the terminal gets interrupted by a > signal, editline(3) ignores the (first) signal and unconditionally > calls read(2) a second time. That seems wrong: if the user hits > Ctrl-C, it is sane to assume that they meant it, not that they > want it ignored. > > The following patch causes el_gets(3) and el_wgets(3) to return > failure when read(2)ing from the terminal is interrupted by a > signal other than SIGCONT or SIGWINCH. That allows the calling > program to decide what to do, usually either exit the program or > provide a fresh prompt to the user. > > If i know how to grep(1), the following programs in the base system > use -ledit: > > * bc(1) >It behaves well with the patch: Ctrl-C discards the current >input line and starts a new input line. >The reason why this already works even without the patch >is that bc(1) does very scary stuff inside the signal handler: >pass a file-global EditLine pointer on the heap to el_line(3) >and access fields inside the returned struct. Needless to >say that no signal handler should do such things... > > * cdio(1) >Behaviour is acceptable and unchanged with the patch: >Ctrl-C exits cdio(1). > > * ftp(1) >It behaves well with the patch: Ctrl-C discards the current >input line and provides a fresh prompt. >The reason why it already works without the patch is that ftp(1) >uses setjmp(3)/longjmp(3) to forcefully grab back control >from el_gets(3) without waiting for it to return. > > * lldb(1) >It misbehaves with or without the patch and ignores Ctrl-C. >I freely admit that i don't feel too enthusiastic about >debugging that beast. > > * sftp(1) >Behaviour is improved with the patch: Ctrl-C now exits sftp(1). >If desired, i can supply a very simple follow-up patch to sftp.c >to instead behave like ftp(1) and bc(1), i.e. discard the >current input line and provide a fresh prompt. >Neither doing undue work in the signal handler nor longjmp(3) >will be required for that (if this patch gets committed). > > * bgplgsh(8), fsdb(8) >I have no idea how to test those. Does anyone think that testing >either of them would be required? I had a quick test with bgplgsh(8), hitting Ctrl-C exits bgplgsh. Same before with or without the diff. Not sure if anyone is actually using bgplgsh(8) -- I never used it. -- :wq Claudio
Re: libedit: stop ignoring SIGINT
On Mon, 2021-08-09 at 14:15 +0200, Martijn van Duren wrote: > On Mon, 2021-08-09 at 13:19 +0200, Ingo Schwarze wrote: > > Hi, > > > > as mentioned earlier, deraadt@ reported that sftp(1) ignores Ctrl-C. > > Fixing that without longjmp(3) requires making editline(3) better > > behaved. > > > > Currently, when read(2) from the terminal gets interrupted by a > > signal, editline(3) ignores the (first) signal and unconditionally > > calls read(2) a second time. That seems wrong: if the user hits > > Ctrl-C, it is sane to assume that they meant it, not that they > > want it ignored. > > > > The following patch causes el_gets(3) and el_wgets(3) to return > > failure when read(2)ing from the terminal is interrupted by a > > signal other than SIGCONT or SIGWINCH. That allows the calling > > program to decide what to do, usually either exit the program or > > provide a fresh prompt to the user. > > > > If i know how to grep(1), the following programs in the base system > > use -ledit: > > > > * bc(1) > > It behaves well with the patch: Ctrl-C discards the current > > input line and starts a new input line. > > The reason why this already works even without the patch > > is that bc(1) does very scary stuff inside the signal handler: > > pass a file-global EditLine pointer on the heap to el_line(3) > > and access fields inside the returned struct. Needless to > > say that no signal handler should do such things... > > > > * cdio(1) > > Behaviour is acceptable and unchanged with the patch: > > Ctrl-C exits cdio(1). > > > > * ftp(1) > > It behaves well with the patch: Ctrl-C discards the current > > input line and provides a fresh prompt. > > The reason why it already works without the patch is that ftp(1) > > uses setjmp(3)/longjmp(3) to forcefully grab back control > > from el_gets(3) without waiting for it to return. > > > > * lldb(1) > > It misbehaves with or without the patch and ignores Ctrl-C. > > I freely admit that i don't feel too enthusiastic about > > debugging that beast. > > > > * sftp(1) > > Behaviour is improved with the patch: Ctrl-C now exits sftp(1). > > If desired, i can supply a very simple follow-up patch to sftp.c > > to instead behave like ftp(1) and bc(1), i.e. discard the > > current input line and provide a fresh prompt. > > Neither doing undue work in the signal handler nor longjmp(3) > > will be required for that (if this patch gets committed). > > > > * bgplgsh(8), fsdb(8) > > I have no idea how to test those. Does anyone think that testing > > either of them would be required? > > > > Regarding the patch below, note that differentiating EINTR behaviour > > by signal number is not needed because the calling code in read_char() > > already handles SIGCONT and SIGWINCH, so those two never arrive in > > read__fixio() in the first place. > > > > Also note that deraadt@ pointed out in private mail to me that the > > fact that read__fixio() clears FIONBIO is probably a symptom of > > botched editline(3) API design. That might be worth fixing, too, > > as far as that is feasible, but it is unrelated to the sftp(1) > > Ctrl-C issue; let's address one topic at a time. > > > > Any feedback is welcome. > > > > Yours, > > Ingo > > > > P.S. > > I decided to Cc: tech@ again because this patch might affect > > even people who are not specifically interested in editline(3), > > and i have no intention to cause unpleasant surprises. > > > If we're stripping down fixio to a single function, why not inline the > whole thing? > > Also, as far as my brain explains things to me, it could theoretically > be possible that the signal handler kicks in right after we return a -1 > from read(2), making it possible to get something like an EIO and > entering the sig switch statement. Assuming that a signal handler > doesn't clobber our errno (which libedit doesn't do, but who knows what > bad code is out there) we should probably only check the signal type > when we know we have an EINTR. > > Finally, I don't understand why we only have a single retry on EAGAIN? > If the application keeps resetting the FIONBIO in such a furious way > that it happens more then once in a single call (no idea how that could > happen) it might be an uphill battle, but we just burn away cpu cycles. > It is not and should probably not be treated like something fatal. > > Diff below does this. (minus 27 LoC) > > The following ports use libedit (there might be a better way of finding > them, but this works): > $ find . -name Makefile | xargs grep -F '.include ' | \ > > cut -d: -f1 | while read file; do \ > > (cd $(dirname $file); make show=WANTLIB | \ > > grep -q '\' && echo $(dirname $file)); \> > > done 2> /dev/null > ./devel/clang-tools-extra > ./devel/llvm > ./mail/dcc > ./mail/nmh > ./emulators/gsplus > ./emulators/nono > ./lang/brainfuck > ./lang/eltclsh > ./lang/lua/5.1 > ./lang/lua/5.2 > ./lang/lua/5.3 > ./lang/swi-prolog >
Re: libedit: stop ignoring SIGINT
On Mon, 2021-08-09 at 13:19 +0200, Ingo Schwarze wrote: > Hi, > > as mentioned earlier, deraadt@ reported that sftp(1) ignores Ctrl-C. > Fixing that without longjmp(3) requires making editline(3) better > behaved. > > Currently, when read(2) from the terminal gets interrupted by a > signal, editline(3) ignores the (first) signal and unconditionally > calls read(2) a second time. That seems wrong: if the user hits > Ctrl-C, it is sane to assume that they meant it, not that they > want it ignored. > > The following patch causes el_gets(3) and el_wgets(3) to return > failure when read(2)ing from the terminal is interrupted by a > signal other than SIGCONT or SIGWINCH. That allows the calling > program to decide what to do, usually either exit the program or > provide a fresh prompt to the user. > > If i know how to grep(1), the following programs in the base system > use -ledit: > > * bc(1) > It behaves well with the patch: Ctrl-C discards the current > input line and starts a new input line. > The reason why this already works even without the patch > is that bc(1) does very scary stuff inside the signal handler: > pass a file-global EditLine pointer on the heap to el_line(3) > and access fields inside the returned struct. Needless to > say that no signal handler should do such things... > > * cdio(1) > Behaviour is acceptable and unchanged with the patch: > Ctrl-C exits cdio(1). > > * ftp(1) > It behaves well with the patch: Ctrl-C discards the current > input line and provides a fresh prompt. > The reason why it already works without the patch is that ftp(1) > uses setjmp(3)/longjmp(3) to forcefully grab back control > from el_gets(3) without waiting for it to return. > > * lldb(1) > It misbehaves with or without the patch and ignores Ctrl-C. > I freely admit that i don't feel too enthusiastic about > debugging that beast. > > * sftp(1) > Behaviour is improved with the patch: Ctrl-C now exits sftp(1). > If desired, i can supply a very simple follow-up patch to sftp.c > to instead behave like ftp(1) and bc(1), i.e. discard the > current input line and provide a fresh prompt. > Neither doing undue work in the signal handler nor longjmp(3) > will be required for that (if this patch gets committed). > > * bgplgsh(8), fsdb(8) > I have no idea how to test those. Does anyone think that testing > either of them would be required? > > Regarding the patch below, note that differentiating EINTR behaviour > by signal number is not needed because the calling code in read_char() > already handles SIGCONT and SIGWINCH, so those two never arrive in > read__fixio() in the first place. > > Also note that deraadt@ pointed out in private mail to me that the > fact that read__fixio() clears FIONBIO is probably a symptom of > botched editline(3) API design. That might be worth fixing, too, > as far as that is feasible, but it is unrelated to the sftp(1) > Ctrl-C issue; let's address one topic at a time. > > Any feedback is welcome. > > Yours, > Ingo > > P.S. > I decided to Cc: tech@ again because this patch might affect > even people who are not specifically interested in editline(3), > and i have no intention to cause unpleasant surprises. > If we're stripping down fixio to a single function, why not inline the whole thing? Also, as far as my brain explains things to me, it could theoretically be possible that the signal handler kicks in right after we return a -1 from read(2), making it possible to get something like an EIO and entering the sig switch statement. Assuming that a signal handler doesn't clobber our errno (which libedit doesn't do, but who knows what bad code is out there) we should probably only check the signal type when we know we have an EINTR. Finally, I don't understand why we only have a single retry on EAGAIN? If the application keeps resetting the FIONBIO in such a furious way that it happens more then once in a single call (no idea how that could happen) it might be an uphill battle, but we just burn away cpu cycles. It is not and should probably not be treated like something fatal. Diff below does this. (minus 27 LoC) The following ports use libedit (there might be a better way of finding them, but this works): $ find . -name Makefile | xargs grep -F '.include ' | \ > cut -d: -f1 | while read file; do \ > (cd $(dirname $file); make show=WANTLIB | \ > grep -q '\' && echo $(dirname $file)); \> > done 2> /dev/null ./devel/clang-tools-extra ./devel/llvm ./mail/dcc ./mail/nmh ./emulators/gsplus ./emulators/nono ./lang/brainfuck ./lang/eltclsh ./lang/lua/5.1 ./lang/lua/5.2 ./lang/lua/5.3 ./lang/swi-prolog ./math/gbc ./net/dnsdist ./net/honeyd ./net/icinga/core2 ./net/knot ./net/ntp ./security/kc ./security/pivy ./shells/dash ./shells/nsh ./sysutils/ipmitool So if we decide which of our interpretations should take precedence it might be a good idea to put it into snaps for a while. martijn@ Index:
libedit: stop ignoring SIGINT
Hi,
as mentioned earlier, deraadt@ reported that sftp(1) ignores Ctrl-C.
Fixing that without longjmp(3) requires making editline(3) better
behaved.
Currently, when read(2) from the terminal gets interrupted by a
signal, editline(3) ignores the (first) signal and unconditionally
calls read(2) a second time. That seems wrong: if the user hits
Ctrl-C, it is sane to assume that they meant it, not that they
want it ignored.
The following patch causes el_gets(3) and el_wgets(3) to return
failure when read(2)ing from the terminal is interrupted by a
signal other than SIGCONT or SIGWINCH. That allows the calling
program to decide what to do, usually either exit the program or
provide a fresh prompt to the user.
If i know how to grep(1), the following programs in the base system
use -ledit:
* bc(1)
It behaves well with the patch: Ctrl-C discards the current
input line and starts a new input line.
The reason why this already works even without the patch
is that bc(1) does very scary stuff inside the signal handler:
pass a file-global EditLine pointer on the heap to el_line(3)
and access fields inside the returned struct. Needless to
say that no signal handler should do such things...
* cdio(1)
Behaviour is acceptable and unchanged with the patch:
Ctrl-C exits cdio(1).
* ftp(1)
It behaves well with the patch: Ctrl-C discards the current
input line and provides a fresh prompt.
The reason why it already works without the patch is that ftp(1)
uses setjmp(3)/longjmp(3) to forcefully grab back control
from el_gets(3) without waiting for it to return.
* lldb(1)
It misbehaves with or without the patch and ignores Ctrl-C.
I freely admit that i don't feel too enthusiastic about
debugging that beast.
* sftp(1)
Behaviour is improved with the patch: Ctrl-C now exits sftp(1).
If desired, i can supply a very simple follow-up patch to sftp.c
to instead behave like ftp(1) and bc(1), i.e. discard the
current input line and provide a fresh prompt.
Neither doing undue work in the signal handler nor longjmp(3)
will be required for that (if this patch gets committed).
* bgplgsh(8), fsdb(8)
I have no idea how to test those. Does anyone think that testing
either of them would be required?
Regarding the patch below, note that differentiating EINTR behaviour
by signal number is not needed because the calling code in read_char()
already handles SIGCONT and SIGWINCH, so those two never arrive in
read__fixio() in the first place.
Also note that deraadt@ pointed out in private mail to me that the
fact that read__fixio() clears FIONBIO is probably a symptom of
botched editline(3) API design. That might be worth fixing, too,
as far as that is feasible, but it is unrelated to the sftp(1)
Ctrl-C issue; let's address one topic at a time.
Any feedback is welcome.
Yours,
Ingo
P.S.
I decided to Cc: tech@ again because this patch might affect
even people who are not specifically interested in editline(3),
and i have no intention to cause unpleasant surprises.
Index: read.c
===
RCS file: /cvs/src/lib/libedit/read.c,v
retrieving revision 1.45
diff -u -U15 -r1.45 read.c
--- read.c 9 Aug 2021 09:11:26 - 1.45
+++ read.c 9 Aug 2021 10:00:18 -
@@ -134,22 +134,19 @@
/* read__fixio():
* Try to recover from a read error
*/
static int
read__fixio(int fd, int e)
{
int zero = 0;
switch (e) {
case EAGAIN:
if (ioctl(fd, FIONBIO, ) == -1)
return -1;
return 0;
- case EINTR:
- return 0;
-
default:
return -1;
}
}
Re: [External] : TCP missing window update stalls connection
On Fri, Aug 06, 2021 at 05:22:18PM +0200, Alexandr Nedvedicky wrote: > > Although I did not obverve it, there seems to be the same problem > > for snd_wl1 and rcv_up. For rcv_up I copied the comparison with > > rcv_nxt from urgent processing to keep future urgent data. Over the weekend I thought about the SEQ_GT(tp->rcv_nxt, tp->rcv_up) check. I think FreeBSD is right and we don't need it. In the regular case we may receive a retransmit of an old packet. This check preserves the rcv_up from packets that we received earlier but with higher sequence number. But in the header prediction code we know that TAILQ_EMPTY(>t_segq). So the current packet is the most recent one and we can blindly take its rcv_nxt as urgent pointer. So maybe we want take the simplified diff below. > can you also share some details about testing you have done? > (tool + command line options) That is quite tricky. I do not have a simple test case for that. One of our OpenBSD based product guarantees unidirectional traffic. We habe a userland process that receives the data, sends it to another OpenBSD machine. There it goes to socket splicing and finaly it ends in a Linux FTP server. some magic -> user land sending process --> socket splicing --> Linux FTP I suspect that with all the machines and processes involved, we have strange timing and run into the race. bluhm Index: netinet/tcp_input.c === RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v retrieving revision 1.368 diff -u -p -r1.368 tcp_input.c --- netinet/tcp_input.c 16 Apr 2021 12:08:25 - 1.368 +++ netinet/tcp_input.c 9 Aug 2021 09:41:05 - @@ -966,6 +966,8 @@ findpcb: tp->t_pmtud_mss_acked = acked; tp->snd_una = th->th_ack; + /* Pull snd_wl2 up to prevent seq wrap. */ + tp->snd_wl2 = th->th_ack; /* * We want snd_last to track snd_una so * as to avoid sequence wraparound problems @@ -1015,6 +1017,9 @@ findpcb: tcp_clean_sackreport(tp); tcpstat_inc(tcps_preddat); tp->rcv_nxt += tlen; + /* Pull snd_wl1 and rcv_up up to prevent seq wrap. */ + tp->snd_wl1 = th->th_seq; + tp->rcv_up = tp->rcv_nxt; tcpstat_pkt(tcps_rcvpack, tcps_rcvbyte, tlen); ND6_HINT(tp);
bgpd MRT RFC8050 support (add-path for mrt dumps)
This diff adds the bits needed to support add-path in MRT dumps.
The problem here is that MRT as a stateless protocol has no chance
to know what kind of encoding (add-path or not) is used for the NLRI in
message dumps. And for table dumps there is a need to add an extra field
to the dumps to show the path-id.
There are two issues:
- for message dumps: it is necessary to peek into UPDATE messages to
figure out if that update is using add-path or not. This comes from the
fact that the add-path RFC allows to set the option per AFI/SAFI and
also per direction. This is a major pain in bgpd since UPDATE messages
are actually parsed in the RDE and not in the SE. The SE just does the
basic lenght checks (header size, total length). So this peak into the
packet needs to be done with some care (especialy for MP encoded
UPDATEs).
- for table dumps the RFC did a major fobar and defined a extra special
encoding for non-IPv4/IPv6 prefixes. In the general encoding the path-id
is not part of the rib entries sub-struct but is instead part of the
NLRI. This encoding is to complex to build into the bgpd codebase and it
seems the only other BGP implementation supporting RIB_GENERIC_ADDPATH,
gobgp, is also not implementing it according to the RFC but instead is
using the same encoding as for the other _ADDPATH types. OpenBGPD will
do the same.
This seems to work for me and results in the right output in bgpctl.
Please review mrt_bgp_guess_aid() and check if all checks are correct to
ensure we don't run off the rails.
--
:wq Claudio
Index: mrt.c
===
RCS file: /cvs/src/usr.sbin/bgpd/mrt.c,v
retrieving revision 1.104
diff -u -p -r1.104 mrt.c
--- mrt.c 24 Jun 2021 10:04:05 - 1.104
+++ mrt.c 9 Aug 2021 10:13:43 -
@@ -91,23 +91,128 @@ int mrt_open(struct mrt *, time_t);
x == MRT_TABLE_DUMP_V2) ? RDEIDX : SEIDX\
)
-void
-mrt_dump_bgp_msg(struct mrt *mrt, void *pkg, u_int16_t pkglen,
-struct peer *peer)
+static u_int8_t
+mrt_bgp_guess_aid(u_int8_t *pkg, u_int16_t pkglen)
+{
+ u_int16_t wlen, alen, len, afi;
+ u_int8_t type, aid;
+
+ pkg += MSGSIZE_HEADER;
+ pkglen -= MSGSIZE_HEADER;
+
+ if (pkglen < 4)
+ goto bad;
+
+ memcpy(, pkg, 2);
+ wlen = ntohs(wlen);
+ pkg += 2;
+ pkglen -= 2;
+
+ memcpy(, pkg, 2);
+ alen = ntohs(alen);
+ pkg += 2;
+ pkglen -= 2;
+
+ if (wlen > 0 || alen < pkglen || (wlen == 0 && alen == 0)) {
+ /* UPDATE has withdraw or NLRI prefixes or IPv4 EoR */
+ return AID_INET;
+ }
+
+ /* bad attribute length */
+ if (alen > pkglen)
+ goto bad;
+
+ /* try to extract AFI/SAFI from the MP attributes */
+ while (alen > 0) {
+ if (alen < 3)
+ goto bad;
+ type = pkg[1];
+ if (pkg[0] & ATTR_EXTLEN) {
+ if (alen < 4)
+ goto bad;
+ memcpy(, pkg + 2, 2);
+ len = ntohs(len);
+ pkg += 4;
+ alen -= 4;
+ } else {
+ len = pkg[2];
+ pkg += 3;
+ alen -= 3;
+ }
+ if (len > alen)
+ goto bad;
+
+ if (type == ATTR_MP_REACH_NLRI ||
+ type == ATTR_MP_UNREACH_NLRI) {
+ if (alen < 3)
+ goto bad;
+ memcpy(, pkg, 2);
+ afi = ntohs(afi);
+ if (afi2aid(afi, pkg[2], ) == -1)
+ goto bad;
+ return aid;
+ }
+
+ pkg += len;
+ alen -= len;
+ }
+
+bad:
+ return AID_UNSPEC;
+}
+
+static u_int16_t
+mrt_bgp_msg_subtype(struct mrt *mrt, void *pkg, u_int16_t pkglen,
+struct peer *peer, enum msg_type msgtype, int in)
{
- struct ibuf *buf;
- int incoming = 0;
u_int16_tsubtype = BGP4MP_MESSAGE;
+ u_int8_t aid, mask;
if (peer->capa.neg.as4byte)
subtype = BGP4MP_MESSAGE_AS4;
+ if (msgtype != UPDATE)
+ return subtype;
+
+ /*
+* RFC8050 adjust types for add-path enabled sessions.
+* It is necessary to extract the AID from UPDATES to decide
+* if the add-path types are needed or not. The ADDPATH
+* subtypes only matter for BGP UPDATES.
+*/
+
+ mask = in ? CAPA_AP_RECV : CAPA_AP_SEND;
+ /* only guess if add-path could be active */
+ if (peer->capa.neg.add_path[0] & mask) {
+ aid = mrt_bgp_guess_aid(pkg, pkglen);
+ if (aid != AID_UNSPEC &&
+
snmpd: tweak listen on
On Sun, 2021-08-08 at 14:44 +0100, Stuart Henderson wrote:
> > This is probably is a bad example.
> > Reading it like this: you're correct that we listen on all interfaces
> > by default, but that's not listed in snmpd.conf(5). So that should
> > probably be fixed (including mentioning that setting one "listen on"
> > disables the all interfaces default).
>
> Let's handle that separately. (it would be convenient to support
> "any" to mean any v4+v6 as well).
>
> > Second, your examples enable snmpv2c on all interfaces, while you
> > enable an implicit snmpv3 on 127.0.0.1. This should probably be the
>
> I wasn't intending that they should all be uncommented at once,
> just showing some common options. And actually it seems snmpd
> doesn't allow listening to 0.0.0.0 as well as a specific v4 address
> (and similarly for :: and v6) so while it's a convenient idea to
> allow v2c on localhost for quick testing while using v3 for external
> traffic, it doesn't actually work.
This diff fixes all of the above:
- Allow any to be used resolving to 0.0.0.0 and ::
- Set SO_REUSEADDR on sockets, so we can listen on both any and
localhost
- Document that we listen on any by default
The listen on text is starting to get quite large, so I hope that one of
our man guru's can either confirm that it's still readable enough or
help me to polish it.
martijn@
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/snmpd/parse.y,v
retrieving revision 1.64
diff -u -p -r1.64 parse.y
--- parse.y 20 Jun 2021 19:55:48 - 1.64
+++ parse.y 9 Aug 2021 09:53:08 -
@@ -128,8 +128,9 @@ typedef struct {
%token STRING
%token NUMBER
%typehostcmn
+%typelistenproto listenflag listenflags
%typesrcaddr port
-%typeoptwrite yesno seclevel listenopt listenopts
+%typeoptwrite yesno seclevel
%type objtype cmd
%type oid hostoid trapoid
%type auth
@@ -195,7 +196,7 @@ yesno : STRING {
}
;
-main : LISTEN ON listenproto
+main : LISTEN ON listen_udptcp
| READONLY COMMUNITY STRING {
if (strlcpy(conf->sc_rdcommunity, $3,
sizeof(conf->sc_rdcommunity)) >=
@@ -273,15 +274,16 @@ main : LISTEN ON listenproto
}
;
-listenproto: UDP listen_udp
- | TCP listen_tcp
- | listen_udp
+listenproto: /* empty */ { $$ = SOCK_DGRAM; }
+ | UDP { $$ = SOCK_DGRAM; }
+ | TCP listen_tcp{ $$ = SOCK_STREAM; }
+ ;
-listenopts : /* empty */ { $$ = 0; }
- | listenopts listenopt { $$ |= $2; }
+listenflags: /* empty */ { $$ = 0; }
+ | listenflags listenflag { $$ |= $2; }
;
-listenopt : READ { $$ = ADDRESS_FLAG_READ; }
+listenflag : READ { $$ = ADDRESS_FLAG_READ; }
| WRITE { $$ = ADDRESS_FLAG_WRITE; }
| NOTIFY { $$ = ADDRESS_FLAG_NOTIFY; }
| SNMPV1 { $$ = ADDRESS_FLAG_SNMPV1; }
@@ -289,71 +291,50 @@ listenopt : READ { $$ = ADDRESS_FLAG_REA
| SNMPV3 { $$ = ADDRESS_FLAG_SNMPV3; }
;
-listen_udp : STRING port listenopts{
+listen_udptcp : listenproto STRING port listenflags {
struct sockaddr_storage ss[16];
- int nhosts, i;
- char *port = $2;
+ int nhosts, j;
+ char *address[2], *port = $3;
+ size_t addresslen = 1, i;
if (port == NULL) {
- if (($3 & ADDRESS_FLAG_PERM) ==
+ if (($4 & ADDRESS_FLAG_PERM) ==
ADDRESS_FLAG_NOTIFY)
port = SNMPTRAP_PORT;
else
port = SNMP_PORT;
}
- nhosts = host($1, port, SOCK_DGRAM, ss, nitems(ss));
- if (nhosts < 1) {
- yyerror("invalid address: %s", $1);
- free($1);
- free($2);
- YYERROR;
+ if (strcmp($2, "any") == 0) {
+ addresslen = 2;
+ address[0] = "0.0.0.0";
+ address[1] = "::";
+ } else {
+ addresslen = 1;
+ address[0] = $2;
}
- if (nhosts > (int)nitems(ss))
- log_warn("%s:%s
Re: libedit: read__fixio cleanup
Hi Martijn, Martijn van Duren wrote on Mon, Aug 09, 2021 at 11:04:35AM +0200: > Btw, would there be any benefit to declare zero as const in this > context? I dont think so. At least according to the ioctl(2) manual page, FIONBIO expects an int * argument, not a const int *. Yours, Ingo
Re: libedit: read__fixio cleanup
Btw, would there be any benefit to declare zero as const in this
context?
On Sun, 2021-08-08 at 13:42 +0200, Ingo Schwarze wrote:
> Hi,
>
> deraadt@ recently reported to me that the editline(3) library, while
> line editing is active - for example inside el_gets(3) - ignores
> the first SIGINT it receives, for example the the first Ctrl-C the
> user presses. I consider that a bug in the editline(3) library.
> Some programs, for example our old ftp(1) implementation, work
> around the bug in a horrible way by using setjmp(3)/longjmp(3).
>
> The root cause of the problem is in libedit/read.c, in the interaction
> of the functions read_char() and read__fixio(). Before fixing the bug
> can reasonably be considered, the function read__fixio() direly needs
> cleanup. As it stands, it is utterly unreadable.
>
> So here is a patch to make it clear what the function does, with
> no functional change intended yet (-37 +5 LOC).
>
> There will be one or more follow-up patches. If you want to receive
> them, please reply to me, i won't necessarily send them all to
> tech@.
>
> I see some value in avoiding gratuitious divergence from NetBSD,
> but getting rid of this horrible mess is not gratuitious.
>
> Rationale:
> * Do not mark an argument as unused that is actually used.
> * errno cannot possibly be -1. Even is it were, treating it as
> EAGAIN makes no sense, treating it as the most severe error
> imaginable makes more sense to me.
> * We define EWOULDBLOCK to be the same as EAGAIN, so no need
> to handle it separately.
> * No need to #define TRY_AGAIN to use it just once.
> * Don't do the same thing twice. We do support the FIONBIO ioctl(2),
> so the the indirection using the F_GETFL fcntl(2) can be deleted.
> * No need to play confusing games with the "e" variable.
> Just return -1 for error or 0 for success in a straightforward
> manner.
>
> OK?
> Ingo
>
> P.S.
> I also considered whether this FIONBIO dance should better be done
> at the initialization stage rather than after EAGAIN already happened.
> But maybe not. This is a library. The application program might
> set the fd to non-blocking mode at any time and then call el_gets(3)
> again, in which case the library needs to restore blocking I/O to
> do its work.
>
>
> Index: read.c
> ===
> RCS file: /cvs/src/lib/libedit/read.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 read.c
> --- read.c 25 May 2016 09:36:21 - 1.44
> +++ read.c 8 Aug 2021 10:30:06 -
> @@ -39,9 +39,10 @@
> * read.c: Clean this junk up! This is horrible code.
> * Terminal read functions
> */
> +#include
> +
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -134,55 +135,16 @@ el_read_getfn(struct el_read_t *el_read)
> /* read__fixio():
> * Try to recover from a read error
> */
> -/* ARGSUSED */
> static int
> -read__fixio(int fd __attribute__((__unused__)), int e)
> +read__fixio(int fd, int e)
> {
> + int zero = 0;
>
> switch (e) {
> - case -1:/* Make sure that the code is reachable */
> -
> -#ifdef EWOULDBLOCK
> - case EWOULDBLOCK:
> -#ifndef TRY_AGAIN
> -#define TRY_AGAIN
> -#endif
> -#endif /* EWOULDBLOCK */
> -
> -#if defined(POSIX) && defined(EAGAIN)
> -#if defined(EWOULDBLOCK) && EWOULDBLOCK != EAGAIN
> case EAGAIN:
> -#ifndef TRY_AGAIN
> -#define TRY_AGAIN
> -#endif
> -#endif /* EWOULDBLOCK && EWOULDBLOCK != EAGAIN */
> -#endif /* POSIX && EAGAIN */
> -
> - e = 0;
> -#ifdef TRY_AGAIN
> -#if defined(F_SETFL) && defined(O_NDELAY)
> - if ((e = fcntl(fd, F_GETFL)) == -1)
> + if (ioctl(fd, FIONBIO, ) == -1)
> return -1;
> -
> - if (fcntl(fd, F_SETFL, e & ~O_NDELAY) == -1)
> - return -1;
> - else
> - e = 1;
> -#endif /* F_SETFL && O_NDELAY */
> -
> -#ifdef FIONBIO
> - {
> - int zero = 0;
> -
> - if (ioctl(fd, FIONBIO, ) == -1)
> - return -1;
> - else
> - e = 1;
> - }
> -#endif /* FIONBIO */
> -
> -#endif /* TRY_AGAIN */
> - return e ? 0 : -1;
> + return 0;
>
> case EINTR:
> return 0;
>
Re: bgpd add add-path receive support
On Fri, Aug 06, 2021 at 08:34:18PM +0200, Sebastian Benoit wrote:
> Claudio Jeker(cje...@diehard.n-r-g.com) on 2021.08.04 17:55:45 +0200:
> > On Fri, Jul 30, 2021 at 12:02:12PM +0200, Claudio Jeker wrote:
> > > This diff implements the bit to support the receive side of
> > > RFC7911 - Advertisement of Multiple Paths in BGP.
> > >
> > > I did some basic tests and it works for me. People running route
> > > collectors should give this a try. The interaction of Add-Path and bgpctl
> > > probably needs some work. Also the MRT dumper needs to be updated to
> > > support RFC8050. I have a partial diff for that ready as well.
> > >
> > > Sending out multiple paths will follow in a later step since that is a
> > > bit more complex. I still need to decide how stable I want to make the
> > > assigned path_ids for the multiple paths and then changes to the decision
> > > process and adjrib-out are required to allow multipe paths there.
> >
> > Updated diff that includes some minimal support for bgpctl.
> > This add 'bgpctl show rib nei foo path-id 42' as a way to limit which
> > paths to show. Now the RFC itself is very flexible in how path-ids are
> > assigned (it is possible that different prefixes have different path-ids)
> > but the assumtion is that most systems assign path-id in a stable way and
> > so it makes sense to allow to filter on path-id.
> > Apart from that not much changed.
>
> ok benno@
Thanks a lot :)
> Only one thing, I worry that using this while the sending side is not working
> can lead to
> blackholing of prefixes.
>
Add-path allows for send / recv to be independent and so this is would me
more of a common issue with add-path. In general having more paths to
select from should help to stop oscilation (e.g. with route reflectors)
but I think this is frequently used to collect routes and still get full
views. Not sure if blackholing is possible (in the end there are more
routes to select from available) but route loops could be an issue.
By default add-path is disabled and that will remain. I agree that
operators need to evaluate carefully what add-path will give them.
Also plan is to finish the MRT support for add-path and then work on
add-path send. So this should arrive soon as well :)
--
:wq Claudio
> >
> > --
> > :wq Claudio
> >
> > Index: bgpctl/bgpctl.8
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.8,v
> > retrieving revision 1.99
> > diff -u -p -r1.99 bgpctl.8
> > --- bgpctl/bgpctl.8 16 Jun 2021 16:24:12 - 1.99
> > +++ bgpctl/bgpctl.8 4 Aug 2021 13:15:53 -
> > @@ -348,6 +348,13 @@ Show RIB memory statistics.
> > Show only entries from the specified peer.
> > .It Cm neighbor group Ar description
> > Show only entries from the specified peer group.
> > +.It Cm path-id Ar pathid
> > +Show only entries which match the specified
> > +.Ar pathid .
> > +Must be used together with either
> > +.Cm neighbor
> > +or
> > +.Cm out .
> > .It Cm peer-as Ar as
> > Show all entries with
> > .Ar as
> > Index: bgpctl/bgpctl.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> > retrieving revision 1.272
> > diff -u -p -r1.272 bgpctl.c
> > --- bgpctl/bgpctl.c 2 Aug 2021 16:51:39 - 1.272
> > +++ bgpctl/bgpctl.c 4 Aug 2021 15:54:25 -
> > @@ -249,6 +249,7 @@ main(int argc, char *argv[])
> > ribreq.neighbor = neighbor;
> > strlcpy(ribreq.rib, res->rib, sizeof(ribreq.rib));
> > ribreq.aid = res->aid;
> > + ribreq.path_id = res->pathid;
> > ribreq.flags = res->flags;
> > imsg_compose(ibuf, type, 0, 0, -1, , sizeof(ribreq));
> > break;
> > Index: bgpctl/parser.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
> > retrieving revision 1.106
> > diff -u -p -r1.106 parser.c
> > --- bgpctl/parser.c 16 Feb 2021 08:30:21 - 1.106
> > +++ bgpctl/parser.c 4 Aug 2021 13:08:31 -
> > @@ -61,7 +61,8 @@ enum token_type {
> > RD,
> > FAMILY,
> > RTABLE,
> > - FILENAME
> > + FILENAME,
> > + PATHID,
> > };
> >
> > struct token {
> > @@ -114,6 +115,7 @@ static const struct token t_log[];
> > static const struct token t_fib_table[];
> > static const struct token t_show_fib_table[];
> > static const struct token t_communication[];
> > +static const struct token t_show_rib_path[];
> >
> > static const struct token t_main[] = {
> > { KEYWORD, "reload", RELOAD, t_communication},
> > @@ -178,10 +180,11 @@ static const struct token t_show_rib[] =
> > { FLAG, "in", F_CTL_ADJ_IN, t_show_rib},
> > { FLAG, "out", F_CTL_ADJ_OUT, t_show_rib},
> > { KEYWORD, "neighbor", NONE, t_show_rib_neigh},
> > + { KEYWORD, "ovs", NONE, t_show_ovs},
> > +
Re: date -j and seconds since the Epoch
Hello Ingo, thanks for looking into this. On 8/6/21 8:13 PM, Ingo Schwarze wrote: Hi Gerhard and Bryan, Gerhard Roth wrote on Mon, Aug 02, 2021 at 10:36:05AM +0200: Bryan Vyhmeister found a strange behavior in date(1): # date -f %s -j 1627519989 Thu Jul 29 01:53:09 PDT 2021 # date -u -f %s -j 1627519989 Thu Jul 29 00:53:09 UTC 2021 Looks like PDT is GMT-1, which of course is wrong. The problem arises from the -f option. The argument of date(1) is passed to strptime(3). Normally, this will return a broken down time in the local timezone. This claim confused me somewhat at first. I think a more accurate statement would be that strptime(3) does not use the TZ at all. It merely parses the string and fills the fields in struct tm. The question whether the caller had any particular time zone in mind does not even arise here. Since date(1) says: ENVIRONMENT TZ The time zone to use when parsing or displaying dates. [...] the command $ date -f %H:%M -j 9:00 is correct to essentially just echo "09:00" back at you because date(1) is specified to use the same time zone for parsing and printing. But the '%s' format makes an exception and returns a date in UTC. That is indeed true. So for %s the time zone used for parsing is necessarily different, while the time zone used for printing is still the $TZ specified by the user (or the /etc/localtime specified by the sysadmin). So i think your approach of using timegm(3) for %s and mktime(3) otherwise is essentially correct. However, a format string can contain more characters than just a single conversion specification. For example, somebody might wish to parse an input file containing a line SSE=1627519989 and legitimately say $ date -f SSE=%s -j $(grep SSE= input_file) which still yields the wrong result even with your patch. You're right. I was thinking about this too, but couldn't come up with a sensible example of how to combine '%s' with something else. Doing a strstr(3) instead of strcmp(3) is surely the better solution. Even worse, what are the admittedly weird commands $ date -f %s:%H -j 1627519989:15 $ date -f %H:%s -j 15:1627519989 $ date -f %H:%s:%m -j 15:1627519989:03 supposed to do? Apparently, data from later conversions is supposed to override data from earlier ones, so should the last conversion that is related to the the time zone win, i.e. %s:%H use mktime(3) but %H:%s and %H:%s:%m gmtime(3)? I would argue that is excessive complexity for little benefit, if any. One might also argue that %s:%H 1627519989:09 is more usefully interpreted as "9 o'clock UTC on the day containing 1627519989" than "9 o'clock in the local TZ on the day containing 1627519989". In addition to using a consistent input time zone, the former also avoids fun with crossing the date line that the latter might cause. The former is also easier to implement, see the patch below. What do people think? ok gerhard@ The patch below isn't very beautiful, but fixes the problem: # date -f %s -j 1627519989 Wed Jul 28 17:53:09 PDT 2021 P.S. Your patch was mangled (one missing blank line and spurious spaces inserted before tabs on some lines). Lately, it is becoming annoying that even very experienced developers no longer seem able to reliably send email containing patches that actually apply... :-( Sorry about that. I will write "I shouldn't use thunderbird to submit patches" a hundred times ;) Gerhard Index: date.c === RCS file: /cvs/src/bin/date/date.c,v retrieving revision 1.56 diff -u -p -r1.56 date.c --- date.c 8 Aug 2019 02:17:51 - 1.56 +++ date.c 6 Aug 2021 17:48:01 - @@ -219,7 +219,11 @@ setthetime(char *p, const char *pformat) } /* convert broken-down time to UTC clock time */ - if ((tval = mktime(lt)) == -1) + if (pformat != NULL && strstr(pformat, "%s") != NULL) + tval = timegm(lt); + else + tval = mktime(lt); + if (tval == -1) errx(1, "specified date is outside allowed range"); if (jflag)
