Re: OpenSMTPD docs: forward.5

2021-02-13 Thread Larry Hynes
Jason McIntyre  wrote:
> On Fri, Feb 12, 2021 at 03:15:47PM +, Larry Hynes wrote:
> > 
> > Index: forward.5
> > ===
> > RCS file: /cvs/src/usr.sbin/smtpd/forward.5,v
> > retrieving revision 1.9
> > diff -u -p -r1.9 forward.5
> > --- forward.5   13 Mar 2015 22:41:54 -  1.9
> > +++ forward.5   12 Feb 2021 15:14:44 -
> > @@ -49,10 +49,10 @@ group or world-writable;
> >  if the home directory is group writeable;
> >  or if the file is not owned by the user.
> >  .Pp
> > -Users should avoid editing directly the
> > +Users should avoid editing the
> >  .Nm .forward
> > -file to prevent delivery failures from occurring if a message
> > -arrives while the file is not fully written.
> > +file directly, to prevent delivery failures from occurring if
> > +a message arrives while the file is not fully written.
> >  The best option is to use a temporary file and use the
> >  .Xr mv 1
> >  command to atomically overwrite the former
> > 
> 
> fixed, thanks.
> jmc

I meant to note that 'mandoc -Tlint' gives the following gripe about
forward.5:

forward.5:40:13: STYLE: no blank before trailing delimiter:
Pq :include:

It can be "fixed" by escaping the closing ':' at EOL with '\&' but I
don't know if that's correct (or worth it).



Re: Secure by default

2021-02-13 Thread Mohamed Aslan
How's is this related to tech@?

On Sun, Feb 14, 2021 at 12:44:00AM +0530, sivasubramanian muthusamy wrote:
> Hello,
> 
> I am an ordinary computer user, installed 6.8 without connecting to
> the Internet yet, (a friend and a technical expert recently advised me
> in a different context: do not expose your machine to the Internet-
> don't know what that means)
> 
> OpenBSD intro says OpenBSD is secure by default. How is it secure by
> default for an average user who does not get to ssh, does not use his
> computer as a web-server or as a VM host, who does not have to share
> screen etc? What ports are open by default and what applications start
> by default?
> 
> Before connecting the computer to the Internet, what other steps
> should a very ordinary user take? Block a few more ports? Which ones?
> 
> Thank you.
> 
> Sivasubramanian M
> 



Re: OpenSMTPD docs: forward.5

2021-02-13 Thread Jason McIntyre
On Sat, Feb 13, 2021 at 09:47:41PM +, Larry Hynes wrote:
> Jason McIntyre  wrote:
> > On Fri, Feb 12, 2021 at 03:15:47PM +, Larry Hynes wrote:
> > > 
> > > Index: forward.5
> > > ===
> > > RCS file: /cvs/src/usr.sbin/smtpd/forward.5,v
> > > retrieving revision 1.9
> > > diff -u -p -r1.9 forward.5
> > > --- forward.5 13 Mar 2015 22:41:54 -  1.9
> > > +++ forward.5 12 Feb 2021 15:14:44 -
> > > @@ -49,10 +49,10 @@ group or world-writable;
> > >  if the home directory is group writeable;
> > >  or if the file is not owned by the user.
> > >  .Pp
> > > -Users should avoid editing directly the
> > > +Users should avoid editing the
> > >  .Nm .forward
> > > -file to prevent delivery failures from occurring if a message
> > > -arrives while the file is not fully written.
> > > +file directly, to prevent delivery failures from occurring if
> > > +a message arrives while the file is not fully written.
> > >  The best option is to use a temporary file and use the
> > >  .Xr mv 1
> > >  command to atomically overwrite the former
> > > 
> > 
> > fixed, thanks.
> > jmc
> 
> I meant to note that 'mandoc -Tlint' gives the following gripe about
> forward.5:
> 
>   forward.5:40:13: STYLE: no blank before trailing delimiter:
>   Pq :include:
> 
> It can be "fixed" by escaping the closing ':' at EOL with '\&' but I
> don't know if that's correct (or worth it).
> 

the idea is to catch punctuation not separated by whitespace, such as:

Such as
.Ar foo:

but in this case the argument to Pq is correct.

it's better to have it as a false positive (i think) than to mangle the
source to avoid the warning.

jmc



Re: Secure by default

2021-02-13 Thread flint pyrite
I am not sure about your use case but to myself, my computer is useless
these days without a connection.


On Sat, Feb 13, 2021 at 1:15 PM sivasubramanian muthusamy <
6.inter...@gmail.com> wrote:

> Hello,
>
> I am an ordinary computer user, installed 6.8 without connecting to
> the Internet yet, (a friend and a technical expert recently advised me
> in a different context: do not expose your machine to the Internet-
> don't know what that means)
>
> OpenBSD intro says OpenBSD is secure by default. How is it secure by
> default for an average user who does not get to ssh, does not use his
> computer as a web-server or as a VM host, who does not have to share
> screen etc? What ports are open by default and what applications start
> by default?
>
> Before connecting the computer to the Internet, what other steps
> should a very ordinary user take? Block a few more ports? Which ones?
>
> Thank you.
>
> Sivasubramanian M
>
>


Re: video(4) multiple opens

2021-02-13 Thread Marcus Glocker
On Sat, Feb 13, 2021 at 10:45:17AM +0100, Claudio Jeker wrote:

> On Sat, Feb 13, 2021 at 10:26:48AM +0100, Marcus Glocker wrote:
> > On Sat, Feb 13, 2021 at 08:30:04AM +0100, Claudio Jeker wrote:
> > 
> > > On Fri, Feb 12, 2021 at 10:59:05PM +0100, Jeremie Courreges-Anglas wrote:
> > > > On Wed, Feb 10 2021, Martin Pieuchot  wrote:
> > > > 
> > > > [...]
> > > > 
> > > > > Which fields is the new lock protecting?  Why isn't the KERNEL_LOCK()
> > > > > enough?
> > > > 
> > > > When I mentioned this potential lack of locking to Marcus, I was
> > > > mistaken.  Some of the functions in video.c are called from syscalls
> > > > that are marked NOLOCK.  But now that I have added some printf
> > > > debugging, I can see that the kernel lock is held in places where
> > > > I didn't expect it to be held (videoioctl, videoread, videoclose...).
> > > > So there's probably a layer of locking that I am missing.
> > > > 
> > > > Marcus, sorry for my misleading diff. O:)
> > > > 
> > > > *crawls back under his rock*
> > > 
> > > Just because a syscall is marked NOLOCK does not mean that all of it will
> > > run without the KERNEL_LOCK. For example read and write are marked NOLOCK
> > > but the KERNEL_LOCK is grabbed later for all non-socket filedescriptors.
> > > This is why video(4) still runs with the KERNEL_LOCK.
> > > The NOLOCK in syscalls.master just tell the kernel to not grab the
> > > KERNEL_LOCK right at the start of the syscall.
> > 
> > OK, that wasn't clear to me either!  If Jeremie has some space left
> > under his rock, I would join :-)
> > 
> > That basically means we need no extra locking in video(4) for this
> > implementation, right?  For a test I replaced the rw_enter_write()
> > with KERNEL_ASSERT_LOCKED(), running a stream and multiple concurrent
> > ioctl programs, and all went fine.
> > 
> > I would come up with a revised diff then for the device owner part.
> > Just doing some polishing together with Anton on that.
> 
> For now this is OK. I have some diffs around which will allow unlocked
> read and write down to individual devices (I was experimenting with tun(4)
> and tap(4)). For ioctl() this will not happen any time soon. ioctl() is a
> world of pain. Maybe for devices like video(4) the call path will be
> manageable but for network devices it is just mess. The main issue is that
> ioctls work on multiple layers and sometimes the calls have layer
> violations and often multiple sleep points built into them.

Got it.  Thanks for the explanation.
Here the adapted diff, including a lot of input from anton@.
I sprinkled KERNEL_ASSERT_LOCKED() in those functions where we require
locking.  Not sure if we should keep them in?


Index: sys/dev/video.c
===
RCS file: /cvs/src/sys/dev/video.c,v
retrieving revision 1.48
diff -u -p -u -p -r1.48 video.c
--- sys/dev/video.c 31 Jan 2021 19:32:01 -  1.48
+++ sys/dev/video.c 13 Feb 2021 09:49:45 -
@@ -47,7 +47,8 @@ struct video_softc {
struct device   *sc_dev;/* hardware device struct */
struct video_hw_if  *hw_if; /* hardware interface */
char sc_dying;  /* device detached */
-   struct process   *sc_owner; /* owner process */
+   struct process  *sc_owner;  /* owner process */
+   uint8_t  sc_open;   /* device opened */
 
int  sc_fsize;
uint8_t *sc_fbuffer;
@@ -69,6 +70,8 @@ int   videoactivate(struct device *, int);
 intvideoprint(void *, const char *);
 
 void   video_intr(void *);
+intvideo_stop(struct video_softc *);
+intvideo_claim(struct video_softc *, struct process *);
 
 struct cfattach video_ca = {
sizeof(struct video_softc), videoprobe, videoattach,
@@ -122,6 +125,9 @@ videoopen(dev_t dev, int flags, int fmt,
 {
int unit;
struct video_softc *sc;
+   int error = 0;
+
+   KERNEL_ASSERT_LOCKED();
 
unit = VIDEOUNIT(dev);
if (unit >= video_cd.cd_ndevs ||
@@ -129,38 +135,40 @@ videoopen(dev_t dev, int flags, int fmt,
 sc->hw_if == NULL)
return (ENXIO);
 
-   if (sc->sc_owner != NULL) {
-   if (sc->sc_owner == p->p_p)
-   return (0);
-   else
-   return (EBUSY);
-   } else
-   sc->sc_owner = p->p_p;
+   if (sc->sc_open) {
+   DPRINTF(("%s: device already open\n", __func__));
+   return (0);
+   } else {
+   sc->sc_open = 1;
+   DPRINTF(("%s: set device to open\n", __func__));
+   }
 
sc->sc_vidmode = VIDMODE_NONE;
sc->sc_frames_ready = 0;
 
if (sc->hw_if->open != NULL)
-   return (sc->hw_if->open(sc->hw_hdl, flags, >sc_fsize,
-   sc->sc_fbuffer, video_intr, sc));
-   else
-   return (0);
+ 

Secure by default

2021-02-13 Thread sivasubramanian muthusamy
Hello,

I am an ordinary computer user, installed 6.8 without connecting to
the Internet yet, (a friend and a technical expert recently advised me
in a different context: do not expose your machine to the Internet-
don't know what that means)

OpenBSD intro says OpenBSD is secure by default. How is it secure by
default for an average user who does not get to ssh, does not use his
computer as a web-server or as a VM host, who does not have to share
screen etc? What ports are open by default and what applications start
by default?

Before connecting the computer to the Internet, what other steps
should a very ordinary user take? Block a few more ports? Which ones?

Thank you.

Sivasubramanian M



Re: smtpd: use libtls

2021-02-13 Thread Eric Faurot
Hi.

The diff seems to work for the few people who tested it (thanks).
Anyone wants to ok this?

Eric.

Index: ca.c
===
RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v
retrieving revision 1.37
diff -u -p -r1.37 ca.c
--- ca.c31 Dec 2020 08:27:15 -  1.37
+++ ca.c19 Jan 2021 11:09:54 -
@@ -69,6 +69,7 @@ static int ecdsae_do_verify(const unsign
 EC_KEY *);
 
 
+static struct dict pkeys;
 static uint64_t reqid = 0;
 
 static void
@@ -132,26 +133,29 @@ ca_init(void)
struct pki  *pki;
const char  *k;
void*iter_dict;
+   char*hash;
 
log_debug("debug: init private ssl-tree");
+   dict_init();
iter_dict = NULL;
while (dict_iter(env->sc_pki_dict, _dict, , (void **))) {
if (pki->pki_key == NULL)
continue;
 
-   if ((in = BIO_new_mem_buf(pki->pki_key,
-   pki->pki_key_len)) == NULL)
-   fatalx("ca_launch: key");
-
-   if ((pkey = PEM_read_bio_PrivateKey(in,
-   NULL, NULL, NULL)) == NULL)
-   fatalx("ca_launch: PEM");
+   in = BIO_new_mem_buf(pki->pki_key, pki->pki_key_len);
+   if (in == NULL)
+   fatalx("ca_init: key");
+   pkey = PEM_read_bio_PrivateKey(in, NULL, NULL, NULL);
+   if (pkey == NULL)
+   fatalx("ca_init: PEM");
BIO_free(in);
 
-   pki->pki_pkey = pkey;
-
-   freezero(pki->pki_key, pki->pki_key_len);
-   pki->pki_key = NULL;
+   hash = ssl_pubkey_hash(pki->pki_cert, pki->pki_cert_len);
+   if (dict_check(, hash))
+   EVP_PKEY_free(pkey);
+   else
+   dict_xset(, hash, pkey);
+   free(hash);
}
 }
 
@@ -223,15 +227,15 @@ end:
 void
 ca_imsg(struct mproc *p, struct imsg *imsg)
 {
+   EVP_PKEY*pkey;
RSA *rsa = NULL;
EC_KEY  *ecdsa = NULL;
const void  *from = NULL;
unsigned char   *to = NULL;
struct msg   m;
-   const char  *pkiname;
+   const char  *hash;
size_t   flen, tlen, padding;
int  buf_len;
-   struct pki  *pki;
int  ret = 0;
uint64_t id;
int  v;
@@ -267,16 +271,15 @@ ca_imsg(struct mproc *p, struct imsg *im
case IMSG_CA_RSA_PRIVDEC:
m_msg(, imsg);
m_get_id(, );
-   m_get_string(, );
+   m_get_string(, );
m_get_data(, , );
m_get_size(, );
m_get_size(, );
m_end();
 
-   pki = dict_get(env->sc_pki_dict, pkiname);
-   if (pki == NULL || pki->pki_pkey == NULL ||
-   (rsa = EVP_PKEY_get1_RSA(pki->pki_pkey)) == NULL)
-   fatalx("ca_imsg: invalid pki");
+   pkey = dict_get(, hash);
+   if (pkey == NULL || (rsa = EVP_PKEY_get1_RSA(pkey)) == NULL)
+   fatalx("ca_imsg: invalid pkey hash");
 
if ((to = calloc(1, tlen)) == NULL)
fatalx("ca_imsg: calloc");
@@ -306,14 +309,14 @@ ca_imsg(struct mproc *p, struct imsg *im
case IMSG_CA_ECDSA_SIGN:
m_msg(, imsg);
m_get_id(, );
-   m_get_string(, );
+   m_get_string(, );
m_get_data(, , );
m_end();
 
-   pki = dict_get(env->sc_pki_dict, pkiname);
-   if (pki == NULL || pki->pki_pkey == NULL ||
-   (ecdsa = EVP_PKEY_get1_EC_KEY(pki->pki_pkey)) == NULL)
-   fatalx("ca_imsg: invalid pki");
+   pkey = dict_get(, hash);
+   if (pkey == NULL ||
+   (ecdsa = EVP_PKEY_get1_EC_KEY(pkey)) == NULL)
+   fatalx("ca_imsg: invalid pkey hash");
 
buf_len = ECDSA_size(ecdsa);
if ((to = calloc(1, buf_len)) == NULL)
@@ -350,12 +353,12 @@ rsae_send_imsg(int flen, const unsigned 
struct imsg  imsg;
int  n, done = 0;
const void  *toptr;
-   char*pkiname;
+   char*hash;
size_t   tlen;
struct msg   m;
uint64_t id;
 
-   if ((pkiname = RSA_get_ex_data(rsa, 0)) == NULL)
+   if ((hash = RSA_get_ex_data(rsa, 0)) == NULL)
return (0);
 
/*
@@ -365,7 +368,7 @@ rsae_send_imsg(int flen, const unsigned 
m_create(p_ca, cmd, 0, 0, -1);
reqid++;
m_add_id(p_ca, reqid);
- 

LibreSSL regressions

2021-02-13 Thread Jan Klemkow
Hi,

A coworker of mine has made tests with LibreSSL [1] and found some
regressions.  I took his test descriptions and created the following
automated regression test.  In the repository he described his findings
in detail.  I kept the numbers of the files and subtests in the target
names for now.  So, its easier to match it with his files.

I don't know how to handle the result of "test-01-ssl".  Thats why its
just a comment.  Someone may have an idea to handle this properly.

Any comments, wishes or OK's?

bye,
Jan

[1]: https://github.com/noxxi/libressl-tests

Index: regress/lib/libssl/Makefile
===
RCS file: /cvs/src/regress/lib/libssl/Makefile,v
retrieving revision 1.42
diff -u -p -r1.42 Makefile
--- regress/lib/libssl/Makefile 14 Oct 2020 15:53:22 -  1.42
+++ regress/lib/libssl/Makefile 12 Feb 2021 19:42:56 -
@@ -16,6 +16,7 @@ SUBDIR += tlsext
 SUBDIR += tlslegacy
 SUBDIR += key_schedule
 SUBDIR += unit
+SUBDIR += validate
 
 # Things that take a long time should go below here. 
 SUBDIR += tlsfuzzer
Index: regress/lib/libssl/validate/Makefile
===
RCS file: regress/lib/libssl/validate/Makefile
diff -N regress/lib/libssl/validate/Makefile
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/lib/libssl/validate/Makefile13 Feb 2021 10:50:30 -
@@ -0,0 +1,104 @@
+# Tests from: https://github.com/noxxi/libressl-tests
+
+PERL=perl
+
+REGRESS_TARGETS =  test-00-01-ssl
+REGRESS_TARGETS += test-00-02-ssl
+REGRESS_TARGETS += test-00-03-ssl
+REGRESS_TARGETS += test-00-04-ssl
+REGRESS_TARGETS += test-00-05-ssl
+REGRESS_TARGETS += test-00-06-ssl
+REGRESS_TARGETS += test-01-ssl
+REGRESS_TARGETS += test-02-ssl
+REGRESS_ROOT_TARGETS = ${REGRESS_TARGETS}
+REGRESS_CLEANUP =  cleanup-ssl
+REGRESS_SETUP =create-libressl-test-certs
+
+create-libressl-test-certs: create-libressl-test-certs.pl
+   ${PERL} ${.CURDIR}/$@.pl
+
+cleanup-ssl:
+   pkill openssl || true
+   rm *.pem *.key
+
+test-00-01-ssl:
+   # unusual wildcard cert, no CA given to client
+   # cleanup
+   pkill openssl || true
+   sleep 2
+   # start client
+   ${KTRACE} openssl s_server -cert server-unusual-wildcard.pem \
+   -key server-unusual-wildcard.pem -www & \
+   timeout=$$(($$(date +%s) + 5)); \
+   while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \
+   do test $$(date +%s) -lt $$timeout || exit 1; done
+   # start client
+   echo "data" | openssl s_client -verify_return_error -connect 
127.0.0.1:4433 \
+   | grep "Verify return code: 21"
+
+test-00-02-ssl:
+   # unusual wildcard cert, CA given to client
+   # cleanup
+   pkill openssl || true
+   sleep 2
+   # start server
+   ${KTRACE} openssl s_server -cert server-unusual-wildcard.pem \
+   -key server-unusual-wildcard.pem -www & \
+   timeout=$$(($$(date +%s) + 5)); \
+   while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \
+   do test $$(date +%s) -lt $$timeout || exit 1; done
+   # start client
+   echo "data" | openssl s_client -connect 127.0.0.1:4433 -CAfile caR.pem \
+   | grep "Verify return code: 0"
+
+test-00-03-ssl:
+   # common wildcard cert, no CA given to client
+   # cleanup
+   pkill openssl || true
+   sleep 2
+   # start server
+   ${KTRACE} openssl s_server -cert server-common-wildcard.pem \
+   -key server-common-wildcard.pem -www & \
+   timeout=$$(($$(date +%s) + 5)); \
+   while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \
+   do test $$(date +%s) -lt $$timeout || exit 1; done
+   # start client
+   echo "data" | openssl s_client -connect 127.0.0.1:4433 \
+   | grep "Verify return code: 21"
+
+test-00-04-ssl:
+   # common wildcard cert, CA given to client
+   # cleanup
+   pkill openssl || true
+   sleep 2
+   # start server
+   ${KTRACE} openssl s_server -cert server-unusual-wildcard.pem \
+   -key server-unusual-wildcard.pem -www & \
+   timeout=$$(($$(date +%s) + 5)); \
+   while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \
+   do test $$(date +%s) -lt $$timeout || exit 1; done
+   # start client
+   echo "data" | openssl s_client -connect 127.0.0.1:4433 -CAfile caR.pem \
+   | grep "Verify return code: 21"
+
+test-00-05-ssl:
+   # openssl verify, unusual wildcard cert
+   openssl verify -CAfile caR.pem server-unusual-wildcard.pem \
+   | grep "server-unusual-wildcard.pem: OK"
+
+test-00-06-ssl:
+   # openssl verify, common wildcard cert
+   openssl verify -CAfile caR.pem server-common-wildcard.pem \
+   | grep "server-common-wildcard.pem: OK"
+
+test-01-ssl:
+   # Not all chain certificates are sent in s_server
+   # 

Re: LibreSSL regressions

2021-02-13 Thread Theo Buehler
On Sat, Feb 13, 2021 at 11:58:04AM +0100, Jan Klemkow wrote:
> Hi,
> 
> A coworker of mine has made tests with LibreSSL [1] and found some
> regressions.  I took his test descriptions and created the following
> automated regression test.  In the repository he described his findings
> in detail.  I kept the numbers of the files and subtests in the target
> names for now.  So, its easier to match it with his files.
> 
> I don't know how to handle the result of "test-01-ssl".  Thats why its
> just a comment.  Someone may have an idea to handle this properly.
> 
> Any comments, wishes or OK's?

First of all thanks for the effort!

The perl script and probably also the Makefile should have a license.

Please add a check that tests whether the required perl modules are
installed (p5-IO-Socket-SSL and p5-Net-SSLeay) and otherwise prints
SKIPPED and their names, so I can install them if they're not present.
I never remember their exact capitalization and hyphenation...


Various comments inline, and a patch for openssl(1) at the end that may
simplify some things.

> [1]: https://github.com/noxxi/libressl-tests
> 
> Index: regress/lib/libssl/Makefile
> ===
> RCS file: /cvs/src/regress/lib/libssl/Makefile,v
> retrieving revision 1.42
> diff -u -p -r1.42 Makefile
> --- regress/lib/libssl/Makefile   14 Oct 2020 15:53:22 -  1.42
> +++ regress/lib/libssl/Makefile   12 Feb 2021 19:42:56 -
> @@ -16,6 +16,7 @@ SUBDIR += tlsext
>  SUBDIR += tlslegacy
>  SUBDIR += key_schedule
>  SUBDIR += unit
> +SUBDIR += validate

I'm not too keen on hooking up a test that fails. It breaks my workflow.
I use REGRESS_FAIL_EARLY in my environment to spot unexpected failures.

Could you add the currently failing tests to REGRESS_EXPECTED_FAILURES?

I also don't think it's a libssl regress test. It's either for
lib/libcrypto or usr.bin/openssl, probably the latter.

>  # Things that take a long time should go below here. 
>  SUBDIR += tlsfuzzer
> Index: regress/lib/libssl/validate/Makefile
> ===
> RCS file: regress/lib/libssl/validate/Makefile
> diff -N regress/lib/libssl/validate/Makefile
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ regress/lib/libssl/validate/Makefile  13 Feb 2021 10:50:30 -
> @@ -0,0 +1,104 @@
> +# Tests from: https://github.com/noxxi/libressl-tests
> +
> +PERL=perl

Please use consistent spacing.

PERL = perl

Could you add

OPENSSL ?= openssl

and use ${OPENSSL} instead of plain openssl throughout the Makefile?
This way we can compare with OpenSSL by doing 'make OPENSSL=eopenssl11'.

> +
> +REGRESS_TARGETS =test-00-01-ssl
> +REGRESS_TARGETS +=   test-00-02-ssl
> +REGRESS_TARGETS +=   test-00-03-ssl
> +REGRESS_TARGETS +=   test-00-04-ssl
> +REGRESS_TARGETS +=   test-00-05-ssl
> +REGRESS_TARGETS +=   test-00-06-ssl
> +REGRESS_TARGETS +=   test-01-ssl
> +REGRESS_TARGETS +=   test-02-ssl
> +REGRESS_ROOT_TARGETS =   ${REGRESS_TARGETS}

Please drop this line. These tests do not require root.

> +REGRESS_CLEANUP =cleanup-ssl
> +REGRESS_SETUP =  create-libressl-test-certs

I think this should be REGRESS_SETUP_ONCE. No need to regen the certs
for each and every test.

> +
> +create-libressl-test-certs: create-libressl-test-certs.pl
> + ${PERL} ${.CURDIR}/$@.pl
> +
> +cleanup-ssl:
> + pkill openssl || true
> + rm *.pem *.key
> +
> +test-00-01-ssl:
> + # unusual wildcard cert, no CA given to client
> + # cleanup
> + pkill openssl || true
> + sleep 2

pkill + sleep 2 is a bit icky. It's not the first time I wanted to have
OpenSSL's -naccept option available for testing. See patch below.

> + # start client

# start server

> + ${KTRACE} openssl s_server -cert server-unusual-wildcard.pem \
> + -key server-unusual-wildcard.pem -www & \

I'd suggest replacing '-www' with '-naccept 1' and...

> + timeout=$$(($$(date +%s) + 5)); \
> + while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \
> + do test $$(date +%s) -lt $$timeout || exit 1; done
> + # start client
> + echo "data" | openssl s_client -verify_return_error -connect 
> 127.0.0.1:4433 \

... do 'echo "Q"' here.

Unless you want to use a random port (which may be more appropriate than
killing all openssl processes and assuming that 4433 is free), I would
just drop '-connect 127.0.0.1:4433'.

> + | grep "Verify return code: 21"
> +
> +test-00-02-ssl:
> + # unusual wildcard cert, CA given to client
> + # cleanup
> + pkill openssl || true
> + sleep 2
> + # start server
> + ${KTRACE} openssl s_server -cert server-unusual-wildcard.pem \
> + -key server-unusual-wildcard.pem -www & \
> + timeout=$$(($$(date +%s) + 5)); \
> + while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \
> + do test $$(date +%s) -lt $$timeout || exit 1; done
> + # start client
> + 

Re: video(4) multiple opens

2021-02-13 Thread Claudio Jeker
On Sat, Feb 13, 2021 at 10:26:48AM +0100, Marcus Glocker wrote:
> On Sat, Feb 13, 2021 at 08:30:04AM +0100, Claudio Jeker wrote:
> 
> > On Fri, Feb 12, 2021 at 10:59:05PM +0100, Jeremie Courreges-Anglas wrote:
> > > On Wed, Feb 10 2021, Martin Pieuchot  wrote:
> > > 
> > > [...]
> > > 
> > > > Which fields is the new lock protecting?  Why isn't the KERNEL_LOCK()
> > > > enough?
> > > 
> > > When I mentioned this potential lack of locking to Marcus, I was
> > > mistaken.  Some of the functions in video.c are called from syscalls
> > > that are marked NOLOCK.  But now that I have added some printf
> > > debugging, I can see that the kernel lock is held in places where
> > > I didn't expect it to be held (videoioctl, videoread, videoclose...).
> > > So there's probably a layer of locking that I am missing.
> > > 
> > > Marcus, sorry for my misleading diff. O:)
> > > 
> > > *crawls back under his rock*
> > 
> > Just because a syscall is marked NOLOCK does not mean that all of it will
> > run without the KERNEL_LOCK. For example read and write are marked NOLOCK
> > but the KERNEL_LOCK is grabbed later for all non-socket filedescriptors.
> > This is why video(4) still runs with the KERNEL_LOCK.
> > The NOLOCK in syscalls.master just tell the kernel to not grab the
> > KERNEL_LOCK right at the start of the syscall.
> 
> OK, that wasn't clear to me either!  If Jeremie has some space left
> under his rock, I would join :-)
> 
> That basically means we need no extra locking in video(4) for this
> implementation, right?  For a test I replaced the rw_enter_write()
> with KERNEL_ASSERT_LOCKED(), running a stream and multiple concurrent
> ioctl programs, and all went fine.
> 
> I would come up with a revised diff then for the device owner part.
> Just doing some polishing together with Anton on that.

For now this is OK. I have some diffs around which will allow unlocked
read and write down to individual devices (I was experimenting with tun(4)
and tap(4)). For ioctl() this will not happen any time soon. ioctl() is a
world of pain. Maybe for devices like video(4) the call path will be
manageable but for network devices it is just mess. The main issue is that
ioctls work on multiple layers and sometimes the calls have layer
violations and often multiple sleep points built into them.

-- 
:wq Claudio



Re: video(4) multiple opens

2021-02-13 Thread Marcus Glocker
On Sat, Feb 13, 2021 at 08:30:04AM +0100, Claudio Jeker wrote:

> On Fri, Feb 12, 2021 at 10:59:05PM +0100, Jeremie Courreges-Anglas wrote:
> > On Wed, Feb 10 2021, Martin Pieuchot  wrote:
> > 
> > [...]
> > 
> > > Which fields is the new lock protecting?  Why isn't the KERNEL_LOCK()
> > > enough?
> > 
> > When I mentioned this potential lack of locking to Marcus, I was
> > mistaken.  Some of the functions in video.c are called from syscalls
> > that are marked NOLOCK.  But now that I have added some printf
> > debugging, I can see that the kernel lock is held in places where
> > I didn't expect it to be held (videoioctl, videoread, videoclose...).
> > So there's probably a layer of locking that I am missing.
> > 
> > Marcus, sorry for my misleading diff. O:)
> > 
> > *crawls back under his rock*
> 
> Just because a syscall is marked NOLOCK does not mean that all of it will
> run without the KERNEL_LOCK. For example read and write are marked NOLOCK
> but the KERNEL_LOCK is grabbed later for all non-socket filedescriptors.
> This is why video(4) still runs with the KERNEL_LOCK.
> The NOLOCK in syscalls.master just tell the kernel to not grab the
> KERNEL_LOCK right at the start of the syscall.

OK, that wasn't clear to me either!  If Jeremie has some space left
under his rock, I would join :-)

That basically means we need no extra locking in video(4) for this
implementation, right?  For a test I replaced the rw_enter_write()
with KERNEL_ASSERT_LOCKED(), running a stream and multiple concurrent
ioctl programs, and all went fine.

I would come up with a revised diff then for the device owner part.
Just doing some polishing together with Anton on that.



Re: OpenSMTPD docs: smtp.1

2021-02-13 Thread Jason McIntyre
On Fri, Feb 12, 2021 at 05:35:07PM +, Larry Hynes wrote:
> Note: the command 'smtp -h' only returns usage. Usage exits with '1'
> (I assume this is to satisfy the default case in switch (ch) in
> main()), even when called from the correct use of the '-h' flag,
> which I don't think is correct.
> 
> Index: smtp.1
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtp.1,v
> retrieving revision 1.8
> diff -u -p -r1.8 smtp.1
> --- smtp.121 Dec 2020 11:48:38 -  1.8
> +++ smtp.112 Feb 2021 17:30:55 -
> @@ -58,7 +58,7 @@ Default to the current username.
>  .It Fl H Ar helo
>  Define the hostname to advertise (HELO) when establishing the SMTP session.
>  .It Fl h
> -Display version and usage.
> +Display usage.
>  .It Fl n
>  Do not actually execute a transaction,
>  just try to establish an SMTP session and quit.
> 

taken.

i think that's the last of your current diffs - thanks for the
submissions.

jmc



Re: OpenSMTPD docs: table.5

2021-02-13 Thread Jason McIntyre
On Fri, Feb 12, 2021 at 03:30:11PM +, Larry Hynes wrote:
> 
> Index: table.5
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/table.5,v
> retrieving revision 1.11
> diff -u -p -r1.11 table.5
> --- table.5   11 Aug 2019 13:00:57 -  1.11
> +++ table.5   12 Feb 2021 15:29:25 -
> @@ -52,7 +52,7 @@ value3
>  .Ed
>  .Pp
>  A mapping will be written with each key and value on a line,
> -whitespaces separating both columns:
> +whitespace separating both columns:
>  .Bd -literal -offset indent
>  key1 value1
>  key2 value2
> @@ -90,7 +90,7 @@ user3   otheru...@example.com
>  .Ed
>  .Pp
>  In a virtual domain context, the key is either a user part, a full email
> -address or a catch all, following selection rules described in
> +address or a catch-all, following selection rules described in

taken, since makemap.8 uses the same spelling

>  .Xr smtpd.conf 5 ,
>  and the value is one or many recipients as described in
>  .Xr aliases 5 :
> @@ -164,7 +164,7 @@ They can only be used in the following c
>  When used as a "from source", the address of a client is compared to the list
>  of addresses in the table until a match is found.
>  .Pp
> -A netaddr table can contain exact addresses or netmasks, and looks as follow:
> +A netaddr table can contain exact addresses or netmasks, as follows:
>  .Bd -literal -offset indent
>  192.168.1.1
>  ::1
> @@ -172,7 +172,7 @@ ipv6:::1
>  192.168.1.0/24
>  .Ed
>  .Ss Userinfo tables
> -User info tables are used in rule context to specify an alternate user base,
> +Userinfo tables are used in a rule context to specify an alternate userbase,

taken

>  mapping virtual users to local system users by UID, GID and home directory.
>  .Pp
>  .D1 Ic action Ar name method Cm userbase Pf < Ar table Ns >
> @@ -234,15 +234,15 @@ user@*.domain
>  .Ed
>  .Ss Addrname tables
>  Addrname tables are used to map IP addresses to hostnames.
> -They can be used in both listen context and relay context:
> +They can be used in both listen and relay contexts:
>  .Bd -unfilled -offset indent
>  .Ic listen on Ar interface Cm hostnames Pf < Ar table Ns >
>  .Ic action Ar name Cm relay helo\-src Pf < Ar table Ns >
>  .Ed
>  .Pp
> -In listen context, the table is used to look up the server name to advertise
> +In a listen context, the table is used to look up the server name to 
> advertise
>  depending on the local address of the socket on which a connection is 
> accepted.
> -In relay context, the table is used to determine the hostname for the HELO
> +In a relay context, the table is used to determine the hostname for the HELO
>  sequence of the SMTP protocol, depending on the local address used for the
>  outgoing connection.
>  .Pp
> 

changes not commented on not committed.
jmc



Re: OpenSMTPD docs: smtpd.conf.5

2021-02-13 Thread Jason McIntyre
On Fri, Feb 12, 2021 at 03:29:02PM +, Larry Hynes wrote:
> 
> Index: smtpd.conf.5
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
> retrieving revision 1.256
> diff -u -p -r1.256 smtpd.conf.5
> --- smtpd.conf.5  27 Jan 2021 14:59:10 -  1.256
> +++ smtpd.conf.5  12 Feb 2021 15:28:04 -
> @@ -184,7 +184,7 @@ Specify how long a message may remain in
>  .It Cm user Ar username
>  Specify the
>  .Ar username
> -for performing the delivery, to be looked up with
> +to perform the delivery, looked up with
>  .Xr getpwnam 3 .
>  .Pp
>  This is used for virtual hosting where a single username
> @@ -202,7 +202,7 @@ function.
>  .Pp
>  The
>  .Cm userbase
> -does not apply for the
> +does not apply to the

taken

>  .Cm user
>  option.
>  .It Cm virtual Pf < Ar table Ns >
> @@ -322,13 +322,13 @@ If the list contains more than one addre
>  in such a way that traffic is routed as efficiently as possible.
>  .El
>  .It Ic admd Ar authservid
> -The Administrative Management Domain this mailserver belongs to.
> +The Administrative Management Domain this mail server belongs to.

i committed this since every other instance in this page is two words,
not one

>  The authservid will be forwarded to filters using it to identify or mark
>  authentication-results headers.
>  If omitted it defaults to the server name.
>  .It Ic bounce Cm warn-interval Ar delay Op , Ar delay ...
>  Send warning messages to the envelope sender when temporary delivery
> -failures cause a message to remain on the queue for longer than
> +failures cause a message to remain in the queue for longer than

i think it's probably ok to say "on the queue" but i agree "in the
queue" is probably more usual, so i took this

>  .Ar delay .
>  Each
>  .Ar delay
> @@ -359,11 +359,11 @@ directive.
>  .It Ic filter Ar chain-name Ic chain Brq Ar filter-name Op , Ar ...
>  Register a chain of filters
>  .Ar chain-name ,
> -consisting of the filters listed from
> +consisting of the filters listed in

taken

>  .Ar filter-name .
> -Filters part of a filter chain are executed in order of declaration for
> -each phase that they are registered for.
> -A filter chain may be used in place of a filter for any directive but
> +Filters in a filter chain are executed in order of declaration
> +for each phase that they are registered for.
> +A filter chain may be used in place of a filter for any directive except
>  filter chains themselves.

taken

>  .It Ic filter Ar filter-name Ic phase Ar phase-name Ic match Ar conditions 
> decision
>  Register a filter
> @@ -372,8 +372,9 @@ A
>  .Ar decision
>  about what to do with the mail is taken at phase
>  .Ar phase-name
> -when matching
> -.Ar conditions .
> +when
> +.Ar conditions
> +are matched.
>  Phases, matching conditions, and decisions are described in
>  .Sx MAIL FILTERING ,
>  below.
> @@ -387,15 +388,15 @@ backed by the
>  process.
>  .It Ic filter Ar filter-name Ic proc-exec Ar command
>  Register and execute
> -.Qq proc
> +.Ar command
> +as
> +.Qq proc-exec
>  filter
> -.Ar filter-name
> -from
> -.Ar command .
> +.Ar filter-name .

i don;t understand enough about what is happening here. can you resubmit
an updated diff once i have committed your changes to this file,
preferrably with an example, and i will try and get some feedback for
it.

>  If
>  .Ar command
>  starts with a slash it is executed with an absolute path,
> -else it will be run from
> +otherwise it will be run from

taken

>  .Dq /usr/local/libexec/smtpd/ .
>  .It Ic include Qq Ar pathname
>  Replace this directive with the content of the additional configuration
> @@ -404,7 +405,7 @@ file at the absolute
>  .It Ic listen on Ar interface Oo Ar family Oc Op Ar options
>  Listen on the
>  .Ar interface
> -for incoming connections, using the same syntax as for
> +for incoming connections, using the same syntax as

taken

>  .Xr ifconfig 8 .
>  The
>  .Ar interface
> @@ -461,7 +462,7 @@ Override the server name for specific ad
>  The
>  .Ar names
>  table contains a mapping of IP addresses to hostnames.
> -If the address on which the connection arrives appears in the mapping,
> +If the address on which the connection is made appears in the mapping,
>  the associated hostname is used.
>  .It Cm mask-src
>  Omit the
> @@ -485,7 +486,7 @@ Listen on the given
>  instead of the default port 25.
>  .It Cm proxy-v2
>  Support the PROXYv2 protocol,
> -rewriting appropriately source address received from proxy.
> +appropriately rewriting the source address passed from the proxy.

taken

>  .It Cm received-auth
>  In
>  .Dq Received
> @@ -555,7 +556,8 @@ matching envelope, and atomically save t
>  spool for later processing by the respective dispatcher
>  .Ar name .
>  .Pp
> -The following matching options are supported and can all be negated:
> +The following session matching options are supported and can all
> +be negated:
>  .Bl -tag -width Ds
>  .It Xo
>  .Op Ic \&!
> @@