Re: pf(4) drops valid IGMP/MLD messages

2023-03-02 Thread Luca Di Gregorio
Hi, just another bit of info about this issue.

I've installed from github the newest version of mrouted on a Linux machine.

Just like the built-in OpenBSD's version of mrouted, this github version
sends DVMRP Prune messages
with IP Destination Address = Unicast Address of the adjacent router, and
TTL=255.

Here you can find a tcpdump:
08:49:19.363829 IP (tos 0xc0, ttl 255, id 52275, offset 0, flags [none],
proto IGMP (2), length 44, options (RA))
10.0.12.101 > 10.0.12.1: igmp dvmrp Prune src 10.254.2.0 grp
239.255.255.42 timer 1h47m2s

Best regards,
Luca

Il giorno mar 28 feb 2023 alle ore 15:39 Alexandr Nedvedicky <
sas...@fastmail.net> ha scritto:

> Hello Matthieu,
>
> 
>
> On Tue, Feb 28, 2023 at 02:05:50PM +0100, Matthieu Herrb wrote:
> > > --- a/sys/net/pf.c
> > > +++ b/sys/net/pf.c
> > > @@ -6846,8 +6846,12 @@ pf_walk_header(struct pf_pdesc *pd, struct ip
> *h, u_short *reason)
> > > pd->proto = h->ip_p;
> > > /* IGMP packets have router alert options, allow them */
> > > if (pd->proto == IPPROTO_IGMP) {
> > > -   /* According to RFC 1112 ttl must be set to 1. */
> > > -   if ((h->ip_ttl != 1) || !IN_MULTICAST(h->ip_dst.s_addr)) {
> > > +   /*
> > > +* According to RFC 1112 ttl must be set to 1 in all IGMP
> > > +* packets sent do 224.0.0.1
> > > +*/
> > > +   if ((h->ip_ttl != 1) &&
> > > +   (h->ip_dst.s_addr == INADDR_ALLHOSTS_GROUP)) {
> > > DPFPRINTF(LOG_NOTICE, "Invalid IGMP");
> > > REASON_SET(reason, PFRES_IPOPTIONS);
> > > return (PF_DROP);
> > >
> >
> >
> > Hi,
> >
> > The expression look wrong to me again.
> > I read in RFC1112 that correct packets should have:
> >
> >   (h->ip_ttl == 1 && h->ip.ip_dst.s_addr == INADDR_ALLHOST_GROUP)
>
> the expression above is true for for Query/Report messages
> which are a sub-set of IGMP protocol. See Luca's emails with
> references to RFCs which further extend IGMP protocol.
>
> here in this particular place (pf_walk_header()) I think we really
> want to simply discard all IGMP datagrams sent to 224.0.0.1 with TTL
> different to 1.  anything else should be passed further up and let
> pf(4)
> rules to make a decision on those cases.
>
> thanks and
> regards
> sashan
>


Re: /dev/full

2023-03-02 Thread Theo de Raadt
What is with the long mail?

Does anyone give a shit, besides you?

No.

Noone has any space for this bullshit, or this long explanation.

Crystal Kolipe  wrote:

> On Thu, Mar 02, 2023 at 04:57:10PM -0500, Daniel Dickman wrote:
> > I don???t see the point of implementing /dev/full. The python regress test
> > is the only time I???ve personally run into this. And I think the issue was
> > that python???s test suite made wrong assumptions about what devices exist
> > on a particular system. Therefore the fix needed to be on the python side.
> 
> I mentioned the python test in passing.  I only found out about it after I
> wrote the /dev/full code and did a web search to see if anybody had ever
> mentioned it in conjunction with OpenBSD before.
> 
> > Out of interest, what is the use case you had in mind for such a device?
> 
> I didn't have a specific use case in mind - /dev/full exists on all other
> modern unix-like systems that matter, and people use it, amoungst other
> things, as an easy way to test how shell scripts react when they hit an
> unexpected ENOSPC error.
> 
> I don't particularly use it, (although I might start to now, since it's in
> our set of local patches and therefore available to me on all of our OpenBSD
> machines).
> 
> I only wrote the /dev/full code as an afterthought whilst working on something
> that was related - a new device called /dev/fill, that could be used to test
> Stuart's claim that MFS is slower than some SSDs and eliminate the possible
> slew of the results due to SSDs treating 0x00 differently, (since previous
> tests seem to have mostly used /dev/zero as input).  If you're interested in
> that, please look at the article I linked in the first email.
> 
> I didn't ever bother to mention the /dev/fill stuff on the list, (except as
> a link to the article), because I knew it wouldn't be popular, and anyone who
> wants it knows where to find it and can complile their own kernel.
> 
> But the four line diff to add /dev/full was so trivial that it seemed useful
> to post it for comments.
> 
> Which I have now received.
> 
> Hopefully the above email answers everybodys' questions about my four-line
> diff to add a feature that every other OS has, and yet nobody here wants, and
> the whole issue can be put to rest and forgotten about.
> 



Re: /dev/full

2023-03-02 Thread Crystal Kolipe
On Thu, Mar 02, 2023 at 04:57:10PM -0500, Daniel Dickman wrote:
> I don???t see the point of implementing /dev/full. The python regress test
> is the only time I???ve personally run into this. And I think the issue was
> that python???s test suite made wrong assumptions about what devices exist
> on a particular system. Therefore the fix needed to be on the python side.

I mentioned the python test in passing.  I only found out about it after I
wrote the /dev/full code and did a web search to see if anybody had ever
mentioned it in conjunction with OpenBSD before.

> Out of interest, what is the use case you had in mind for such a device?

I didn't have a specific use case in mind - /dev/full exists on all other
modern unix-like systems that matter, and people use it, amoungst other
things, as an easy way to test how shell scripts react when they hit an
unexpected ENOSPC error.

I don't particularly use it, (although I might start to now, since it's in
our set of local patches and therefore available to me on all of our OpenBSD
machines).

I only wrote the /dev/full code as an afterthought whilst working on something
that was related - a new device called /dev/fill, that could be used to test
Stuart's claim that MFS is slower than some SSDs and eliminate the possible
slew of the results due to SSDs treating 0x00 differently, (since previous
tests seem to have mostly used /dev/zero as input).  If you're interested in
that, please look at the article I linked in the first email.

I didn't ever bother to mention the /dev/fill stuff on the list, (except as
a link to the article), because I knew it wouldn't be popular, and anyone who
wants it knows where to find it and can complile their own kernel.

But the four line diff to add /dev/full was so trivial that it seemed useful
to post it for comments.

Which I have now received.

Hopefully the above email answers everybodys' questions about my four-line
diff to add a feature that every other OS has, and yet nobody here wants, and
the whole issue can be put to rest and forgotten about.



Re: /dev/full

2023-03-02 Thread Daniel Dickman
I don’t see the point of implementing /dev/full. The python regress test is the 
only time I’ve personally run into this. And I think the issue was that 
python’s test suite made wrong assumptions about what devices exist on a 
particular system. Therefore the fix needed to be on the python side.

I considered the idea of /dev/full in base. But didn’t really see the point.

If you are going to do fault injection testing (a very worthy goal) then I 
think you’d want to go a different route than a device node for every scenario.

Out of interest, what is the use case you had in mind for such a device?


> On Mar 2, 2023, at 9:47 AM, Crystal Kolipe  wrote:
> We currently don't implement the /dev/full device, which is present in
> NetBSD, FreeBSD, and Linux.
> 
> For those who haven't heard of it, it's basically the same as /dev/zero, but
> writes to it always return ENOSPC.
> 
> The lack of /dev/full on OpenBSD has previously caused minor issues with
> third party software, for example:
> 
> https://bugs.python.org/issue21934
> 
> Adding support for /dev/full is trivial.  I've attached a patch for amd64,
> if there is interest I can easily produce a set of patches for other archs.
> 
> To create the device file, you'll need to do:
> 
> # mknod -m 666 /dev/full c 2 5
> 
> For those who are interested, I've written a more in-depth discussion about
> memory special devices, including a proposal for another such new device,
> /dev/fill:
> 
> https://research.exoticsilicon.com/articles/memory_special_devices
> 
> --- arch/amd64/amd64/mem.c.distWed Mar 24 11:26:39 2021
> +++ arch/amd64/amd64/mem.cThu Mar  2 11:10:30 2023
> @@ -88,6 +88,7 @@
>break;
>return (EPERM);
>case 2:
> +case 5:
>case 12:
>break;
> #ifdef APERTURE
> @@ -165,9 +166,13 @@
>uio->uio_resid = 0;
>return (0);
> 
> +/* minor device 5 is /dev/full */
>/* minor device 12 is /dev/zero */
> +case 5:
>case 12:
>if (uio->uio_rw == UIO_WRITE) {
> +if (minor(dev)==5)
> +return (ENOSPC);
>c = iov->iov_len;
>break;
>}



Re: Authentication in OpenIKED

2023-03-02 Thread A Tammy



On 3/2/23 10:44, Stuart Henderson wrote:
> On 2023/03/01 22:15, A Tammy wrote:
>>>
>>> -# Configuration for clients connecting with EAP authentication.
>>> +# Configuration for clients connecting with EAP authentication
>>> +# and sending all traffic over the IKEv2 tunnel.
>>>  # Remember to set up a PKI, see ikectl(8) for more information.
>> Is a PKI still needed in this example config? The comment seems to imply
>> that I need one even with PSK auth.
>> Like PKI is an alternative, so maybe something like - Setting up a PKI
>> is an alternative to using a PSK, see ikectl(8) for more information.
> 
> That block is for EAP and yes that needs some form of PKI (either a
> local CA, or at least a server certificate signed by another CA - but
> for the latter you have some awkward handling to assemble the files
> with the intermediate cert in the right place; iked has non-standard
> requirements).
> 
> Could add a couple more lines to make that more clear though,
> and give some hints for people who don't know what PKI is - see below.
> 
> On 2023/03/02 05:35, Crystal Kolipe wrote:
>> On Wed, Mar 01, 2023 at 04:53:00PM +, Stuart Henderson wrote:
>>> How about this? Show a strong psk in the example
>>
>> ...
>>
>>> -#  psk "you-should-not-use-psk-authentication!"
>>> +#  psk "tyBNv13zuo3rg1WVXlaI1g1tTYNzwk962mMUYIvaLh2x8vvvyA"
>>
>> I strongly disagree with this change.
>>
>> Not only are you removing a note that psk authentication is a poor choice,
>> but you're also providing a _specific_ and otherwise strong key in the 
>> example
>> which new or unfamiliar users could _easily_ use believing that it was a good
>> choice.
> 
> Could do something like this .. I do think it's important that if
> there's any example at all that it does give an indication that people
> might like to use a psk with a decent amount of entropy.
> 
> (And that is exactly one of the reasons why my first thought was to
> delete the file..)
> 
> Index: iked.conf
> ===
> RCS file: /cvs/src/etc/examples/iked.conf,v
> retrieving revision 1.2
> diff -u -p -r1.2 iked.conf
> --- iked.conf 1 Mar 2023 22:45:25 -   1.2
> +++ iked.conf 2 Mar 2023 15:20:47 -
> @@ -8,7 +8,10 @@
> 
>  # Configuration for clients connecting with EAP authentication
>  # and sending all traffic over the IKEv2 tunnel.
> -# Remember to set up a PKI, see ikectl(8) for more information.
> +#
> +# EAP requires a server certificate; see ikectl(8) for more details
> +# on generating this with an iked-specific local CA.
> +#
>  #ikev2 "eapclient" passive esp \
>  #from any to dynamic \
>  #local any peer any \
> @@ -17,10 +20,16 @@
>  #config name-server 10.1.0.2 \
>  #tag "$name-$id"
> 
> -# Configuration for a client authenticating with a pre-shared key.
> +# Configuration for a client authenticating with a pre-shared key,
> +# mostly useful for LAN-to-LAN tunnels between static IP endpoints.
> +#
> +# For iked->iked tunnels you can use a simple config using RSA keys
> +# instead - omit psk and copy /etc/iked/local.pub on each side to
> +# /etc/iked/pubkeys/ipv4/ on the other.
> +#
>  #ikev2 esp \
>  #from 10.3.0.0/24 to 10.1.0.0/24 \
>  #from 10.5.0.0/24 to 10.1.0.0/24 \
>  #from 10.5.0.0/24 to 172.16.1.0/24 \
>  #local 192.168.1.1 peer 192.168.2.1 \
> -#psk "tyBNv13zuo3rg1WVXlaI1g1tTYNzwk962mMUYIvaLh2x8vvvyA"
> +#psk "tyBNv13zuo3rg1WVXlaI1g1tTYNzwk962mMUYIvaLh2x8vvvyA-replace-me"
> 

Thanks a lot Stuart, OK aisha



Re: mountd: no need for critical sections

2023-03-02 Thread Alexander Bluhm
On Wed, Mar 01, 2023 at 04:31:07PM -0700, Todd C. Miller wrote:
> The SIGHUP handler only sets a flag these days, there is no longer
> any need to block it while using the exports list.
> 
> OK?

OK bluhm@

In the previous version of the diff you also eliminated the useless
newline in new_exportlist().

> Index: sbin/mountd/mountd.c
> ===
> RCS file: /cvs/src/sbin/mountd/mountd.c,v
> retrieving revision 1.90
> diff -u -p -u -r1.90 mountd.c
> --- sbin/mountd/mountd.c  1 Mar 2023 23:27:46 -   1.90
> +++ sbin/mountd/mountd.c  1 Mar 2023 23:29:18 -
> @@ -736,7 +736,6 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *t
>   char rpcpath[RPCMNT_PATHLEN+1], dirpath[PATH_MAX];
>   struct hostent *hp = NULL;
>   struct exportlist *ep;
> - sigset_t sighup_mask;
>   int defset, hostset;
>   struct fhreturn fhr;
>   struct dirlist *dp;
> @@ -746,8 +745,6 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *t
>   u_short sport;
>   long bad = 0;
>  
> - sigemptyset(&sighup_mask);
> - sigaddset(&sighup_mask, SIGHUP);
>   saddr = transp->xp_raddr.sin_addr.s_addr;
>   sport = ntohs(transp->xp_raddr.sin_port);
>   switch (rqstp->rq_proc) {
> @@ -792,7 +789,6 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *t
>   }
>  
>   /* Check in the exports list */
> - sigprocmask(SIG_BLOCK, &sighup_mask, NULL);
>   ep = bad ? NULL : ex_search(&fsb.f_fsid);
>   hostset = defset = 0;
>   if (ep && (chk_host(ep->ex_defdir, saddr, &defset, &hostset) ||
> @@ -804,7 +800,6 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *t
>   if (!svc_sendreply(transp, xdr_long,
>   (caddr_t)&bad))
>   syslog(LOG_ERR, "Can't send reply");
> - sigprocmask(SIG_UNBLOCK, &sighup_mask, NULL);
>   return;
>   }
>   if (hostset & DP_HOSTSET)
> @@ -820,7 +815,6 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *t
>   if (!svc_sendreply(transp, xdr_long,
>   (caddr_t)&bad))
>   syslog(LOG_ERR, "Can't send reply");
> - sigprocmask(SIG_UNBLOCK, &sighup_mask, NULL);
>   return;
>   }
>   if (!svc_sendreply(transp, xdr_fhs, (caddr_t)&fhr))
> @@ -844,7 +838,6 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *t
>  
>   if (bad && !svc_sendreply(transp, xdr_long, (caddr_t)&bad))
>   syslog(LOG_ERR, "Can't send reply");
> - sigprocmask(SIG_UNBLOCK, &sighup_mask, NULL);
>   return;
>   case RPCMNT_DUMP:
>   if (!svc_sendreply(transp, xdr_mlist, NULL))
> @@ -958,11 +951,7 @@ xdr_explist(XDR *xdrsp, caddr_t cp)
>  {
>   struct exportlist *ep;
>   int false = 0, putdef;
> - sigset_t sighup_mask;
>  
> - sigemptyset(&sighup_mask);
> - sigaddset(&sighup_mask, SIGHUP);
> - sigprocmask(SIG_BLOCK, &sighup_mask, NULL);
>   ep = exphead;
>   while (ep) {
>   putdef = 0;
> @@ -973,12 +962,10 @@ xdr_explist(XDR *xdrsp, caddr_t cp)
>   goto errout;
>   ep = ep->ex_next;
>   }
> - sigprocmask(SIG_UNBLOCK, &sighup_mask, NULL);
>   if (!xdr_bool(xdrsp, &false))
>   return (0);
>   return (1);
>  errout:
> - sigprocmask(SIG_UNBLOCK, &sighup_mask, NULL);
>   return (0);
>  }
>  



Re: Nuke remnants of /dev/io

2023-03-02 Thread Crystal Kolipe
Ping?

On Thu, Feb 23, 2023 at 08:05:07PM -0800, Greg Steuck wrote:
> Thanks Crystal. If somebody wants to commit this, it is OK gnezdo@
> 
> Crystal Kolipe  writes:
> 
> > The iskmemdev function checks for minor number 14 in addition to 0 and 1 on
> > the following archs:
> >
> > amd64, arm64, i386, and riscv64
> >
> > Device 2, 14 was traditionally /dev/io, which we don't support and so 
> > opening
> > it will always return ENXIO from mmopen anyway.
> >
> > We only use iskmemdev in one place in the tree, to return EPERM when trying
> > to access /dev/kmem or /dev/mem when securelevel >= 1.
> >
> > This patch removes the check for minor(dev) == 14 on the four above 
> > mentioned
> > architectures.
> >
> > --- sys/arch/amd64/amd64/conf.c.distMon Feb 20 18:17:44 2023
> > +++ sys/arch/amd64/amd64/conf.c Mon Feb 20 18:29:28 2023
> > @@ -313,7 +313,7 @@
> >  iskmemdev(dev_t dev)
> >  {
> >  
> > -   return (major(dev) == mem_no && (minor(dev) < 2 || minor(dev) == 14));
> > +   return (major(dev) == mem_no && minor(dev) < 2);
> >  }
> >  
> >  /*
> > --- sys/arch/arm64/arm64/conf.c.distMon Feb 20 18:18:20 2023
> > +++ sys/arch/arm64/arm64/conf.c Mon Feb 20 18:29:14 2023
> > @@ -255,7 +255,7 @@
> >  iskmemdev(dev_t dev)
> >  {
> >  
> > -   return (major(dev) == CMAJ_MM && (minor(dev) < 2 || minor(dev) == 14));
> > +   return (major(dev) == CMAJ_MM && minor(dev) < 2);
> >  }
> >  
> >  /*
> > --- sys/arch/i386/i386/conf.c.dist  Mon Feb 20 18:18:35 2023
> > +++ sys/arch/i386/i386/conf.c   Mon Feb 20 18:28:51 2023
> > @@ -309,7 +309,7 @@
> >  int
> >  iskmemdev(dev_t dev)
> >  {
> > -   return (major(dev) == mem_no && (minor(dev) < 2 || minor(dev) == 14));
> > +   return (major(dev) == mem_no && minor(dev) < 2);
> >  }
> >  
> >  /*
> > --- sys/arch/riscv64/riscv64/conf.c.distMon Feb 20 18:18:48 2023
> > +++ sys/arch/riscv64/riscv64/conf.c Mon Feb 20 18:28:35 2023
> > @@ -253,7 +253,7 @@
> >  iskmemdev(dev_t dev)
> >  {
> >  
> > -   return (major(dev) == mem_no && (minor(dev) < 2 || minor(dev) == 14));
> > +   return (major(dev) == mem_no && minor(dev) < 2);
> >  }
> >  
> >  /*
> 



Re: /dev/full

2023-03-02 Thread Crystal Kolipe
Hi Theo,

On Thu, Mar 02, 2023 at 09:06:17AM -0700, Theo de Raadt wrote:
> While at it, maybe we need a /dev/bullshit linked to /dev/random??
> 
> This extra node looks like bullshit to me.
> 
> It is encouraging a large application to do a round-trip through the
> kernel, for a rare occurance.

The python case is a bad use of /dev/full.

It's intended more for providing a convenient way of testing shell scripts
against an ENOSPC condition than use by 'large applications'.

But your point highlighting the potential for abuse of it is valid, and I
can see why we shouldn't encourage that.  If anybody ever wants it, the
patch is in the archives.

> What next? /dev/multiply -- you write two 32-bit values and then you can
> read a new 32 bit value?

Actually, that sounds quite cool :-).



Re: /dev/full

2023-03-02 Thread Stuart Henderson
On 2023/03/02 10:50, Dave Voutila wrote:
> Is this really a problem with ports? That Python issue was related to a
> Python 2.7 unit test and from 2014.

not really, no. there has been the odd patch over the years but nothing
current (no more than 4 including the Python one).



Re: /dev/full

2023-03-02 Thread Theo de Raadt
While at it, maybe we need a /dev/bullshit linked to /dev/random??

This extra node looks like bullshit to me.

It is encouraging a large application to do a round-trip through the
kernel, for a rare occurance.

It should simply skip doing the round-trip through kernel.

What next? /dev/multiply -- you write two 32-bit values and then you can
read a new 32 bit value?

I see absolutely no point in wasting bytes on the root partition, or bytes
in the kernel, for this stupid idea.


Dave Voutila  wrote:

> 
> 
> Crystal Kolipe  writes:
> 
> > We currently don't implement the /dev/full device, which is present in
> > NetBSD, FreeBSD, and Linux.
> >
> > For those who haven't heard of it, it's basically the same as /dev/zero, but
> > writes to it always return ENOSPC.
> >
> > The lack of /dev/full on OpenBSD has previously caused minor issues with
> > third party software, for example:
> >
> > https://bugs.python.org/issue21934
> >
> 
> Is this really a problem with ports? That Python issue was related to a
> Python 2.7 unit test and from 2014.
> 
> > Adding support for /dev/full is trivial.  I've attached a patch for amd64,
> > if there is interest I can easily produce a set of patches for other archs.
> >
> > To create the device file, you'll need to do:
> >
> > # mknod -m 666 /dev/full c 2 5
> >
> > For those who are interested, I've written a more in-depth discussion about
> > memory special devices, including a proposal for another such new device,
> > /dev/fill:
> >
> > https://research.exoticsilicon.com/articles/memory_special_devices
> >
> > --- arch/amd64/amd64/mem.c.dist Wed Mar 24 11:26:39 2021
> > +++ arch/amd64/amd64/mem.c  Thu Mar  2 11:10:30 2023
> > @@ -88,6 +88,7 @@
> > break;
> > return (EPERM);
> > case 2:
> > +   case 5:
> > case 12:
> > break;
> >  #ifdef APERTURE
> > @@ -165,9 +166,13 @@
> > uio->uio_resid = 0;
> > return (0);
> >
> > +   /* minor device 5 is /dev/full */
> > /* minor device 12 is /dev/zero */
> > +   case 5:
> > case 12:
> > if (uio->uio_rw == UIO_WRITE) {
> > +   if (minor(dev)==5)
> > +   return (ENOSPC);
> > c = iov->iov_len;
> > break;
> > }
> 



Re: /dev/full

2023-03-02 Thread Crystal Kolipe
On Thu, Mar 02, 2023 at 10:50:08AM -0500, Dave Voutila wrote:
> 
> Crystal Kolipe  writes:
> 
> > We currently don't implement the /dev/full device, which is present in
> > NetBSD, FreeBSD, and Linux.
> >
> > For those who haven't heard of it, it's basically the same as /dev/zero, but
> > writes to it always return ENOSPC.
> >
> > The lack of /dev/full on OpenBSD has previously caused minor issues with
> > third party software, for example:
> >
> > https://bugs.python.org/issue21934
> >
> 
> Is this really a problem with ports? That Python issue was related to a
> Python 2.7 unit test and from 2014.

I don't think it's a particular problem with ports, although I don't work in
the ports tree enough to give a reliable answer to that question.

I still think it's useful, though.



Re: Authentication in OpenIKED

2023-03-02 Thread Landry Breuil
Le Thu, Mar 02, 2023 at 03:44:35PM +, Stuart Henderson a écrit :
> On 2023/03/01 22:15, A Tammy wrote:
> > >
> > > -# Configuration for clients connecting with EAP authentication.
> > > +# Configuration for clients connecting with EAP authentication
> > > +# and sending all traffic over the IKEv2 tunnel.
> > >  # Remember to set up a PKI, see ikectl(8) for more information.
> -# Configuration for a client authenticating with a pre-shared key.
> +# Configuration for a client authenticating with a pre-shared key,
> +# mostly useful for LAN-to-LAN tunnels between static IP endpoints.
> +#
> +# For iked->iked tunnels you can use a simple config using RSA keys
> +# instead - omit psk and copy /etc/iked/local.pub on each side to
> +# /etc/iked/pubkeys/ipv4/ on the other.
^^

That part is definitely the most important for ppl building OpenBSD/iked
site-to-site vpns, as its dummy-proof and just works.



Re: Authentication in OpenIKED

2023-03-02 Thread Crystal Kolipe
On Thu, Mar 02, 2023 at 03:44:35PM +, Stuart Henderson wrote:
> Could add a couple more lines to make that more clear though,
> and give some hints for people who don't know what PKI is - see below.
> 
> On 2023/03/02 05:35, Crystal Kolipe wrote:

Well done for the, (possibly unintentional), subliminal hint that _I_
don't know what a PKI is ;-).

(Of course, I do)

> -#psk "tyBNv13zuo3rg1WVXlaI1g1tTYNzwk962mMUYIvaLh2x8vvvyA"
> +#psk "tyBNv13zuo3rg1WVXlaI1g1tTYNzwk962mMUYIvaLh2x8vvvyA-replace-me"

Yes, that pretty much solves the issue I was thinking of.

However, I would like to add for the record that I still think that
it's an advantage to avoid using static PSKs even for simple
applications.  It's easier to set up in the beginning, but it makes
more admin work in the long term, because if you ever want or need
to change either one of the keys, then you need to change it on
both machines.  With certificates you can avoid that.

Setting up iked propperly, (with certs), is not particularly difficult.

But anyway, OK by me this version.



Re: /dev/full

2023-03-02 Thread Dave Voutila


Crystal Kolipe  writes:

> We currently don't implement the /dev/full device, which is present in
> NetBSD, FreeBSD, and Linux.
>
> For those who haven't heard of it, it's basically the same as /dev/zero, but
> writes to it always return ENOSPC.
>
> The lack of /dev/full on OpenBSD has previously caused minor issues with
> third party software, for example:
>
> https://bugs.python.org/issue21934
>

Is this really a problem with ports? That Python issue was related to a
Python 2.7 unit test and from 2014.

> Adding support for /dev/full is trivial.  I've attached a patch for amd64,
> if there is interest I can easily produce a set of patches for other archs.
>
> To create the device file, you'll need to do:
>
> # mknod -m 666 /dev/full c 2 5
>
> For those who are interested, I've written a more in-depth discussion about
> memory special devices, including a proposal for another such new device,
> /dev/fill:
>
> https://research.exoticsilicon.com/articles/memory_special_devices
>
> --- arch/amd64/amd64/mem.c.dist   Wed Mar 24 11:26:39 2021
> +++ arch/amd64/amd64/mem.cThu Mar  2 11:10:30 2023
> @@ -88,6 +88,7 @@
>   break;
>   return (EPERM);
>   case 2:
> + case 5:
>   case 12:
>   break;
>  #ifdef APERTURE
> @@ -165,9 +166,13 @@
>   uio->uio_resid = 0;
>   return (0);
>
> + /* minor device 5 is /dev/full */
>   /* minor device 12 is /dev/zero */
> + case 5:
>   case 12:
>   if (uio->uio_rw == UIO_WRITE) {
> + if (minor(dev)==5)
> + return (ENOSPC);
>   c = iov->iov_len;
>   break;
>   }



Re: Authentication in OpenIKED

2023-03-02 Thread Stuart Henderson
On 2023/03/01 22:15, A Tammy wrote:
> >
> > -# Configuration for clients connecting with EAP authentication.
> > +# Configuration for clients connecting with EAP authentication
> > +# and sending all traffic over the IKEv2 tunnel.
> >  # Remember to set up a PKI, see ikectl(8) for more information.
> Is a PKI still needed in this example config? The comment seems to imply
> that I need one even with PSK auth.
> Like PKI is an alternative, so maybe something like - Setting up a PKI
> is an alternative to using a PSK, see ikectl(8) for more information.

That block is for EAP and yes that needs some form of PKI (either a
local CA, or at least a server certificate signed by another CA - but
for the latter you have some awkward handling to assemble the files
with the intermediate cert in the right place; iked has non-standard
requirements).

Could add a couple more lines to make that more clear though,
and give some hints for people who don't know what PKI is - see below.

On 2023/03/02 05:35, Crystal Kolipe wrote:
> On Wed, Mar 01, 2023 at 04:53:00PM +, Stuart Henderson wrote:
> > How about this? Show a strong psk in the example
> 
> ...
> 
> > -#  psk "you-should-not-use-psk-authentication!"
> > +#  psk "tyBNv13zuo3rg1WVXlaI1g1tTYNzwk962mMUYIvaLh2x8vvvyA"
> 
> I strongly disagree with this change.
> 
> Not only are you removing a note that psk authentication is a poor choice,
> but you're also providing a _specific_ and otherwise strong key in the example
> which new or unfamiliar users could _easily_ use believing that it was a good
> choice.

Could do something like this .. I do think it's important that if
there's any example at all that it does give an indication that people
might like to use a psk with a decent amount of entropy.

(And that is exactly one of the reasons why my first thought was to
delete the file..)

Index: iked.conf
===
RCS file: /cvs/src/etc/examples/iked.conf,v
retrieving revision 1.2
diff -u -p -r1.2 iked.conf
--- iked.conf   1 Mar 2023 22:45:25 -   1.2
+++ iked.conf   2 Mar 2023 15:20:47 -
@@ -8,7 +8,10 @@

 # Configuration for clients connecting with EAP authentication
 # and sending all traffic over the IKEv2 tunnel.
-# Remember to set up a PKI, see ikectl(8) for more information.
+#
+# EAP requires a server certificate; see ikectl(8) for more details
+# on generating this with an iked-specific local CA.
+#
 #ikev2 "eapclient" passive esp \
 #  from any to dynamic \
 #  local any peer any \
@@ -17,10 +20,16 @@
 #  config name-server 10.1.0.2 \
 #  tag "$name-$id"

-# Configuration for a client authenticating with a pre-shared key.
+# Configuration for a client authenticating with a pre-shared key,
+# mostly useful for LAN-to-LAN tunnels between static IP endpoints.
+#
+# For iked->iked tunnels you can use a simple config using RSA keys
+# instead - omit psk and copy /etc/iked/local.pub on each side to
+# /etc/iked/pubkeys/ipv4/ on the other.
+#
 #ikev2 esp \
 #  from 10.3.0.0/24 to 10.1.0.0/24 \
 #  from 10.5.0.0/24 to 10.1.0.0/24 \
 #  from 10.5.0.0/24 to 172.16.1.0/24 \
 #  local 192.168.1.1 peer 192.168.2.1 \
-#  psk "tyBNv13zuo3rg1WVXlaI1g1tTYNzwk962mMUYIvaLh2x8vvvyA"
+#  psk "tyBNv13zuo3rg1WVXlaI1g1tTYNzwk962mMUYIvaLh2x8vvvyA-replace-me"



/dev/full

2023-03-02 Thread Crystal Kolipe
We currently don't implement the /dev/full device, which is present in
NetBSD, FreeBSD, and Linux.

For those who haven't heard of it, it's basically the same as /dev/zero, but
writes to it always return ENOSPC.

The lack of /dev/full on OpenBSD has previously caused minor issues with
third party software, for example:

https://bugs.python.org/issue21934

Adding support for /dev/full is trivial.  I've attached a patch for amd64,
if there is interest I can easily produce a set of patches for other archs.

To create the device file, you'll need to do:

# mknod -m 666 /dev/full c 2 5

For those who are interested, I've written a more in-depth discussion about
memory special devices, including a proposal for another such new device,
/dev/fill:

https://research.exoticsilicon.com/articles/memory_special_devices

--- arch/amd64/amd64/mem.c.dist Wed Mar 24 11:26:39 2021
+++ arch/amd64/amd64/mem.c  Thu Mar  2 11:10:30 2023
@@ -88,6 +88,7 @@
break;
return (EPERM);
case 2:
+   case 5:
case 12:
break;
 #ifdef APERTURE
@@ -165,9 +166,13 @@
uio->uio_resid = 0;
return (0);
 
+   /* minor device 5 is /dev/full */
/* minor device 12 is /dev/zero */
+   case 5:
case 12:
if (uio->uio_rw == UIO_WRITE) {
+   if (minor(dev)==5)
+   return (ENOSPC);
c = iov->iov_len;
break;
}



Re: mountd: no need for critical sections

2023-03-02 Thread Todd C . Miller
On Thu, 02 Mar 2023 07:25:17 +, Klemens Nanni wrote:

> The TERM handler also just sets a flag today, but etc/rc.d/mountd still
> has `rc_stop=NO' since 2013
>
> Do not allow stopping/restarting mountd using the rc.d(8) framework;
> if there is need to send a SIGTERM to mountd(8), it should be done
> manually as there is too much involved with RPC daemons to make it
> automagic.
>
> Can this be flipped so `rcctl stop mountd' works again?

I don't think anything has changed in that respect.  However, I'm
not sure why this was disabled in the first place.

 - todd



Re: freeradius denies to authentocate with eap-tls

2023-03-02 Thread Mikhael Lialin

Hello and good day.

Finally found the actual reason.

The outer client is failed eap tls because of packet fragmentation. on 
interface mtu is set as 1500, and packet is 1514.


from tshark:

RADIUS 1514 Access-Request id=4[BoundErrorUnreassembled Packet]
RADIUS 1514 Access-Request id=4, Duplicate 
Request[BoundErrorUnreassembled Packet]
RADIUS 1514 Access-Request id=4, Duplicate 
Request[BoundErrorUnreassembled Packet]
RADIUS 1514 Access-Request id=4, Duplicate 
Request[BoundErrorUnreassembled Packet]


if set fragment_size to wpa_supplicant.conf to a little below value, it 
helps and eap_tls is successful.


It's good for configurable client, however how about phones where all 
parameters are default ?


# fragment_size: Maximum EAP fragment size in bytes (default 1398).
#   This value limits the fragment size for EAP methods that support
#   fragmentation (e.g., EAP-TLS and EAP-PEAP). This value should be set
#   small enough to make the EAP messages fit in MTU of the network
#   interface used for EAPOL. The default value is suitable for most
#   cases.

any idea why this happen ?

Thank you.

On 2/27/23 13:56, Stuart Henderson wrote:

(moving to ports#, reply-to is set, although this is unlikely to be
OpenBSD-specific)

On 2023/02/25 02:18, Mikhael Lialin wrote:

Trying to setup witi with radius eap-tls authentication.

And getting time out while authenticated.

Tried with custome setup, and default setup with generated
certificates within installation.

in ktrace of rediusd something waiting:

28664 radiusd  RET   wait4 -1 errno 10 No child processes

all configuration of freeradius are default after
installation, nothing were modified.

Please help.

Debug ant ktrace session attached.

ktrace is too low-level to be useful here.

freeradius won't work directly with default setup, you at least need to
configure shared secrets between the APs and freeradius (in clients.conf
and on the AP), and tell freeradius how to decide whether a user is
allowed to authenticate.

You say EAP-TLS, this uses certificates for authentication on bith the
server *and* the client, so for that you'll also need to figure out how
to get client certificates signed, etc. I strongly recommend ignoring
this until you have the basics working with password based auth.

Followhttps://wiki.freeradius.org/guide/Basic-configuration-HOWTO
first and make sure it works with radtest on the local machine.
(Note if running it manually in debug mode as suggested in that guide,
you will need the full path /usr/local/sbin/radiusd, there is a minimal
radius daemon from the base OS in /usr/sbin/radiusd which does not
support EAP/PEAP).

If that fails, it needs fixing first before moving onto one of the
EAP methods that you need for WPA-Enterprise (either on an AP directly
or you can try eapol_test running on the freeradius server as shown in
http://deployingradius.com/scripts/eapol_test/  - skip the "building
eapol_test" section and pkg_add wpa_supplicant instead).



Re: arp: initialise global list

2023-03-02 Thread Klemens Nanni
On Thu, Mar 02, 2023 at 01:53:34PM +0300, Vitaliy Makkoveev wrote:
> I like to use LIST_HEAD_INITIALIZER(9) for consistency with other global
> list initializations in netinet/. ok mvs@ with this. 

Needs a little spacing and commment wording wrangling to stay < 80 chars.

OK?

Index: netinet/if_ether.c
===
RCS file: /cvs/src/sys/netinet/if_ether.c,v
retrieving revision 1.256
diff -u -p -r1.256 if_ether.c
--- netinet/if_ether.c  31 Jan 2023 13:41:54 -  1.256
+++ netinet/if_ether.c  2 Mar 2023 11:18:07 -
@@ -105,7 +105,8 @@ struct niqueue arpinq = NIQUEUE_INITIALI
 /* llinfo_arp live time, rt_llinfo and RTF_LLINFO are protected by arp_mtx */
 struct mutex arp_mtx = MUTEX_INITIALIZER(IPL_SOFTNET);
 
-LIST_HEAD(, llinfo_arp) arp_list; /* [mN] list of all llinfo_arp structures */
+LIST_HEAD(, llinfo_arp) arp_list =
+LIST_HEAD_INITIALIZER(arp_list);   /* [mN] list of llinfo_arp structures */
 struct pool arp_pool;  /* [I] pool for llinfo_arp structures */
 intarp_maxtries = 5;   /* [I] arp requests before set to rejected */
 intla_hold_total;  /* [a] packets currently in the arp queue */



Re: arp: initialise global list

2023-03-02 Thread Vitaliy Makkoveev
On Thu, Mar 02, 2023 at 09:36:52AM +, Klemens Nanni wrote:
> Used but not initialised:
> 
>   $ grep arp_list if_ether.c
>   LIST_ENTRY(llinfo_arp)   la_list;   /* [mN] global arp_list 
> */
>   LIST_HEAD(, llinfo_arp) arp_list; /* [mN] list of all llinfo_arp 
> structures */
>   /* Net lock is exclusive, no arp mutex needed for arp_list 
> here. */
>   LIST_FOREACH_SAFE(la, &arp_list, la_list, nla) {
>   LIST_INSERT_HEAD(&arp_list, la, la_list);
> 
> It only works because arp_list is global and thus zero initialised, so
> 
>   #define LIST_INIT(head) do {
> \
>   LIST_FIRST(head) = LIST_END(head);  
> \
>   } while (0)
> 
> is practically a NOOP, but we must not rely on that.
> Use proper queue(9) init (in analogy to nd6_init() and nd6_list).
> 
> OK?

I like to use LIST_HEAD_INITIALIZER(9) for consistency with other global
list initializations in netinet/. ok mvs@ with this. 

> 
> Index: netinet/if_ether.c
> ===
> RCS file: /cvs/src/sys/netinet/if_ether.c,v
> retrieving revision 1.256
> diff -u -p -r1.256 if_ether.c
> --- netinet/if_ether.c31 Jan 2023 13:41:54 -  1.256
> +++ netinet/if_ether.c2 Mar 2023 09:19:53 -
> @@ -146,6 +146,7 @@ arpinit(void)
>  {
>   static struct timeout arptimer_to;
>  
> + LIST_INIT(&arp_list);
>   pool_init(&arp_pool, sizeof(struct llinfo_arp), 0,
>   IPL_SOFTNET, 0, "arp", NULL);
>  
> 



Re: Disabling MULTICAST flag on an interface. Force outgoing multicast traffic to a specific interface

2023-03-02 Thread Luca Di Gregorio
Hi Philip, thanks

1) ok, I’ll take it into account for my configurations

2) adding the route for 224.0.0.0/4 to the specific interface works

Thanks again
Luca

Il giorno mer 1 mar 2023 alle 22:47 Philip Guenther  ha
scritto:

> On Wed, Mar 1, 2023 at 9:58 AM Luca Di Gregorio  wrote:
>
>> 1) does anyone know if there is a way to disable MULTICAST on a single
>> interface?
>> I don't see any option in ifconfig to do this.
>>
>
> There is not.
>
>
>> 2) Can outgoing multicast traffic be routed to a specific interface? I
>> don't see any option in route. It seems that the first interface, by id,
>> is
>> chosen.
>>
>
> Unless you enable a multicast routing daemon, it just follows normal IP
> routing rules and will follow the default route if there's no better
> match.  If you want to force it to a specific interface, just add a route
> for 224.0.0.0/4 pointing to that interface.  (Of course, if you haven't
> set multicast=YES in /etc/rc.conf.local then /etc/netstart will create one
> of those routes itself with the 'reject' flag set to block all multicast,
> but presumably you've already set that correctly.)
>
>
> Philip Guenther
>
>
>


arp: initialise global list

2023-03-02 Thread Klemens Nanni
Used but not initialised:

$ grep arp_list if_ether.c
LIST_ENTRY(llinfo_arp)   la_list;   /* [mN] global arp_list 
*/
LIST_HEAD(, llinfo_arp) arp_list; /* [mN] list of all llinfo_arp 
structures */
/* Net lock is exclusive, no arp mutex needed for arp_list 
here. */
LIST_FOREACH_SAFE(la, &arp_list, la_list, nla) {
LIST_INSERT_HEAD(&arp_list, la, la_list);

It only works because arp_list is global and thus zero initialised, so

#define LIST_INIT(head) do {
\
LIST_FIRST(head) = LIST_END(head);  
\
} while (0)

is practically a NOOP, but we must not rely on that.
Use proper queue(9) init (in analogy to nd6_init() and nd6_list).

OK?

Index: netinet/if_ether.c
===
RCS file: /cvs/src/sys/netinet/if_ether.c,v
retrieving revision 1.256
diff -u -p -r1.256 if_ether.c
--- netinet/if_ether.c  31 Jan 2023 13:41:54 -  1.256
+++ netinet/if_ether.c  2 Mar 2023 09:19:53 -
@@ -146,6 +146,7 @@ arpinit(void)
 {
static struct timeout arptimer_to;
 
+   LIST_INIT(&arp_list);
pool_init(&arp_pool, sizeof(struct llinfo_arp), 0,
IPL_SOFTNET, 0, "arp", NULL);
 



Re: iwx(4) -77 firmware diff for testing

2023-03-02 Thread Stefan Sperling
On Wed, Feb 22, 2023 at 03:31:28PM +0100, Stefan Sperling wrote:
> Below is my work-in-progress diff to update iwx(4) to latest firmware.
> Every system tracking -current should already have the new -77 firmware 
> images.
> 
> The new images contain security fixes of (to me) unknown severity.
> Unfortunately there have been quite a number of firmware API changes since
> our last upgrade and it took me quite some time to get all the required new
> bits in place and arrive at an operational state.
> 
> While testing please enable additional debug output with:
>   ifconfig iwx0 debug
> To activate it at boot time: echo debug >> /etc/hostname.iwx0
> 
> There are some known issue with occasional firmware errors.
> My devices eventually manage to connect and work regardless. If you see a
> firmware error in dmesg please include the extra information shown in dmesg
> after enabling the debugging mode as above. This information is hidden by
> default and the driver will only print "fatal firmware error" to dmesg
> without more context, but the extra context is needed for debugging.
> 
> If you hit an error which looks like this:
> 
>iwx0: firmware parse error 22, section type 19
>iwx0: failed to load init firmware
> 
> Then you will need to increase this constant in if_iwxvar.h until you
> get past the error:
> 
> #define IWX_UCODE_SECT_MAX 56

This new version of the patch fixes two issues:

IWX_UCODE_SECT_MAX was bumped to 57 to accommodate some AX211 devices.

Fixed iwx0: 0x211A | ADVANCED_SYSASSERT
with helpful hints from Johannes Berg at Linux/Intel.
This was an annoying issue since it made connecting to an access point
fail quite often. Root cause were changes in the mac context command,
where the firmware now expects beacon-related info to be initialized
early, already before we attempt to associate.

The only remaining known issue is:
iwx0: 0x20002806 | ADVANCED_SYSASSERT, as seen by jmc@
This seems related to background scans. I could not yet reproduce the error,
roaming seems to work as expected for me, but this might depend on the RF
environment. Since roaming is evidently not completely broken by this I will
not treat this as a blocker for moving development into the tree.

Patch test reports are still welcome but please be fast if there is an issue
you are seeing that I am not aware of. I will start splitting/committing this
soon.

diff refs/heads/master refs/heads/iwxfw
commit - 50b5a752f56b56e236653f0da2264f2ec83f9fde
commit + 0e6ef712fc262423d483ba4cdeab9f957d41e71e
blob - a91a0d85b3877e13d7798e7f90f21fd601eac1e5
blob + b4f90c6d74e76f1b5a4658cde4af19f2fe76a42a
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -235,7 +235,6 @@ int iwx_is_mimo_mcs(int);
 uint8_tiwx_lookup_cmd_ver(struct iwx_softc *, uint8_t, uint8_t);
 uint8_tiwx_lookup_notif_ver(struct iwx_softc *, uint8_t, uint8_t);
 intiwx_is_mimo_ht_plcp(uint8_t);
-intiwx_is_mimo_mcs(int);
 intiwx_store_cscheme(struct iwx_softc *, uint8_t *, size_t);
 intiwx_alloc_fw_monitor_block(struct iwx_softc *, uint8_t, uint8_t);
 intiwx_alloc_fw_monitor(struct iwx_softc *, uint8_t);
@@ -380,10 +379,10 @@ int   iwx_phy_ctxt_cmd_uhb_v3(struct iwx_softc *, 
struct
struct iwx_rx_data *);
 intiwx_binding_cmd(struct iwx_softc *, struct iwx_node *, uint32_t);
 uint8_tiwx_get_vht_ctrl_pos(struct ieee80211com *, struct 
ieee80211_channel *);
-intiwx_phy_ctxt_cmd_uhb_v3(struct iwx_softc *, struct iwx_phy_ctxt *, 
uint8_t,
-   uint8_t, uint32_t, uint8_t, uint8_t);
-intiwx_phy_ctxt_cmd_v3(struct iwx_softc *, struct iwx_phy_ctxt *, uint8_t,
-   uint8_t, uint32_t, uint8_t, uint8_t);
+intiwx_phy_ctxt_cmd_uhb_v3_v4(struct iwx_softc *, struct iwx_phy_ctxt *,
+   uint8_t, uint8_t, uint32_t, uint8_t, uint8_t, int);
+intiwx_phy_ctxt_cmd_v3_v4(struct iwx_softc *, struct iwx_phy_ctxt *,
+   uint8_t, uint8_t, uint32_t, uint8_t, uint8_t, int);
 intiwx_phy_ctxt_cmd(struct iwx_softc *, struct iwx_phy_ctxt *, uint8_t,
uint8_t, uint32_t, uint32_t, uint8_t, uint8_t);
 intiwx_send_cmd(struct iwx_softc *, struct iwx_host_cmd *);
@@ -395,6 +394,8 @@ const struct iwx_rate *iwx_tx_fill_cmd(struct iwx_soft
const void *, uint32_t *);
 void   iwx_free_resp(struct iwx_softc *, struct iwx_host_cmd *);
 void   iwx_cmd_done(struct iwx_softc *, int, int, int);
+uint32_t iwx_fw_rateidx_ofdm(uint8_t);
+uint32_t iwx_fw_rateidx_cck(uint8_t);
 const struct iwx_rate *iwx_tx_fill_cmd(struct iwx_softc *, struct iwx_node *,
struct ieee80211_frame *, uint16_t *, uint32_t *);
 void   iwx_tx_update_byte_tbl(struct iwx_softc *, struct iwx_tx_ring *, int,
@@ -446,11 +447,16 @@ int   iwx_rs_rval2idx(uint8_t);
 intiwx_umac_scan_abort(struct iwx_softc *);
 intiwx_scan_abort(struct iwx_softc *);
 intiwx_enable_mgmt_queue(struct iwx_softc *);
+intiwx_disable_mgmt_queue(struct iwx_softc *);
 intiwx_rs_rval2idx(uint8_t);
 uin

Re: strptime.c

2023-03-02 Thread Theo Buehler
On Sun, Jan 29, 2023 at 08:16:06AM -0700, Todd C. Miller wrote:
> Unfortunately we cannot use strtonum(3) here since there may be
> non-digit characters following the number.  So, strtoll(3)
> it is then.

Since strptime's %s is supposed to be the inverse of strftime's %s,
which is produced with mktime(3), surely this is the way to go. I think
this should go in. I also agree with enh's reading of the FreeBSD code.

ok tb

PS: It would be nice if our regress tests exercised these interfaces a
bit better...



Re: Authentication in OpenIKED

2023-03-02 Thread Crystal Kolipe
On Wed, Mar 01, 2023 at 04:53:00PM +, Stuart Henderson wrote:
> How about this? Show a strong psk in the example

...

> -#psk "you-should-not-use-psk-authentication!"
> +#psk "tyBNv13zuo3rg1WVXlaI1g1tTYNzwk962mMUYIvaLh2x8vvvyA"

I strongly disagree with this change.

Not only are you removing a note that psk authentication is a poor choice,
but you're also providing a _specific_ and otherwise strong key in the example
which new or unfamiliar users could _easily_ use believing that it was a good
choice.

(Which of course it would be, if it wasn't potentially being used by a large
 number of other OpenBSD installs in the wild.)

This is one step away from adding a back-door.

Nobody in their right mind is going to use a literal string:

"you-should-not-use-psk-authentication!"

as their key.  But it's entirely plausible that someone would copy:

"tyBNv13zuo3rg1WVXlaI1g1tTYNzwk962mMUYIvaLh2x8vvvyA"

to their real config.

Maybe they do it with the intention of changing it later, just to see iked
working, but then forget to change it.

If the key itself says, "you-should-not-use-psk-authentication!", then
forgetting to change it is an order of magnitude more difficult.  And
anybody taking over administration of a system and seeing it would know to
change it.  Seeing your example gives no clue that it's a demo key rather
than a locally generated one.

Please, do not commit this part of the change.  Otherwise we will start to
see scanning attempts on the internet using the key you've provided.