Re: ksh(1): implement p_tv() with p_ts()

2023-09-11 Thread Theo Buehler
On Mon, Sep 11, 2023 at 10:10:49PM -0500, Scott Cheloha wrote:
> p_tv() is identical to p_ts() in every way except for the subsecond
> conversion constants.
> 
> Better to write p_ts() once: in p_tv(), convert from timeval to
> timespec and call p_ts().

While this looks like an improvement to me, this uses a new non-portable
construct in ksh. I don't know how much we care.

> 
> Index: c_sh.c
> ===
> RCS file: /cvs/src/bin/ksh/c_sh.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 c_sh.c
> --- c_sh.c22 May 2020 07:50:07 -  1.64
> +++ c_sh.c12 Sep 2023 03:07:16 -
> @@ -680,14 +680,10 @@ static void
>  p_tv(struct shf *shf, int posix, struct timeval *tv, int width, char *prefix,
>  char *suffix)
>  {
> - if (posix)
> - shf_fprintf(shf, "%s%*lld.%02ld%s", prefix ? prefix : "",
> - width, (long long)tv->tv_sec, tv->tv_usec / 1, suffix);
> - else
> - shf_fprintf(shf, "%s%*lldm%02lld.%02lds%s", prefix ? prefix : 
> "",
> - width, (long long)tv->tv_sec / 60,
> - (long long)tv->tv_sec % 60,
> - tv->tv_usec / 1, suffix);
> + struct timespec ts;
> +
> + TIMEVAL_TO_TIMESPEC(tv, );
> + p_ts(shf, posix, , width, prefix, suffix);
>  }
>  
>  static void
> 



Re: rpki-client: ensure X.509 Subject only contains commonName and serialNumber

2023-09-11 Thread Theo Buehler
On Tue, Sep 12, 2023 at 12:03:01AM +, Job Snijders wrote:
> On Mon, Sep 11, 2023 at 09:31:03AM +0200, Theo Buehler wrote:
> > > - * This only parses the RFC 3779 extensions since these are necessary for
> > > - * validation.
> > 
> > Isn't this still true? You don't really parse the subject name.
> 
> I took 'parse' to mean something like 'inspects', and since it also
> inspects the X.509 version, KeyUsage, and soon Subject it seemed a
> misleading comment to me :-)

The new comment is fine with me. I think the point the comment was
trying to make is that it actually parses the two RFC 3779 extensions,
whereas it only does some sanity checks on the others.

> I incorporated your feedback, OK?

yes, just one nit

> +#if 0
> + if (as->type != V_ASN1_PRINTABLESTRING) {

Instead of reaching into the string, I think it'd be nicer to do

if (ASN1_STRING_type(as) != V_ASN1_PRINTABLESTRING) {

> + warnx("%s: RFC 6487 section 4.5: commonName is"
> + " not PrintableString", fn);
> + return 0;
> + }
> +#endif



Re: ps.1/kvm documentation

2023-09-11 Thread Philip Guenther
On Mon, Sep 11, 2023 at 5:29 AM Marc Espie 
wrote:

> On Mon, Sep 11, 2023 at 12:10:17PM +0200, Claudio Jeker wrote:
> > On Mon, Sep 11, 2023 at 11:02:00AM +0200, Marc Espie wrote:
> > > I was reading through ps.1, which has two slightly different options
> > >  -H  Also display information about kernel visible threads.
> > >  -k  Also display information about kernel threads.
> > >
> > > It's not at all obvious what the difference between these options
> might be.
> >
> > kernel threads == kthread(9) created threads
> >
> > Those should have K in STAT and the name is in () like:
> >  3141 ??  RK/1   4057:57.90 (idle1)
>

Fun fact: at the current time, all kernel threads are actually full
processes, though this just wastes some memory and cache.
Part of my (evil) plan in adding thread names was removing an impediment in
changing kthread_create(9) to create just a thread inside process 0.


> kernel visible threads == __tfork_thread(3) created threads for userland
> > applications. For example:
> >
> > 43838  556612 ??  IpU  0:01.58 firefox (firefox/Cache2 I/O)
> > 43838  415551 ??  IpU  0:00.01 firefox (firefox/Cookie)
> > 43838  377915 ??  IpU  0:00.01 firefox (firefox/Worker Launcher)
> >
> > These threads all share the same PID but have different TID.
> >
> > I think the "kernel visible" is there to tell that pure userland threads
> > can not be reported by ps. I think go routines are such an example.
>

Claudio's descriptions are correct.  How to massage those into the ps(1)
manpage is unclear to me.



> Any of you guys willing to provide a patch to ps(1) and kvm_getprocs(3) ?
>

I think I've written a diff for kvm_getprocs(3) at least 3 times and each
previous time the way the API makes me grit my teeth has resulted in
incomplete rewrites of the API and its callers that I haven't been happy
enough with to complete and push through.  Enough stalling on documenting
the current behavior: how about something like the diff below?

Philip

 Index: lib/libkvm/kvm_getprocs.3
===
RCS file: /data/src/openbsd/src/lib/libkvm/kvm_getprocs.3,v
retrieving revision 1.21
diff -u -p -r1.21 kvm_getprocs.3
--- lib/libkvm/kvm_getprocs.3   11 Aug 2019 15:48:08 -  1.21
+++ lib/libkvm/kvm_getprocs.3   12 Sep 2023 04:25:32 -
@@ -91,6 +91,13 @@ processes with real user ID
 .Fa arg
 .El
 .Pp
+In addition, if
+.Dv KERN_PROC_SHOW_THREADS
+is bitwise ORed into
+.Fa op
+then additional entries for matched processes' associated threads
+are returned.
+.Pp
 Only the first
 .Fa elemsize
 bytes of each array entry are returned.


ksh(1): implement p_tv() with p_ts()

2023-09-11 Thread Scott Cheloha
p_tv() is identical to p_ts() in every way except for the subsecond
conversion constants.

Better to write p_ts() once: in p_tv(), convert from timeval to
timespec and call p_ts().

ok?

Index: c_sh.c
===
RCS file: /cvs/src/bin/ksh/c_sh.c,v
retrieving revision 1.64
diff -u -p -r1.64 c_sh.c
--- c_sh.c  22 May 2020 07:50:07 -  1.64
+++ c_sh.c  12 Sep 2023 03:07:16 -
@@ -680,14 +680,10 @@ static void
 p_tv(struct shf *shf, int posix, struct timeval *tv, int width, char *prefix,
 char *suffix)
 {
-   if (posix)
-   shf_fprintf(shf, "%s%*lld.%02ld%s", prefix ? prefix : "",
-   width, (long long)tv->tv_sec, tv->tv_usec / 1, suffix);
-   else
-   shf_fprintf(shf, "%s%*lldm%02lld.%02lds%s", prefix ? prefix : 
"",
-   width, (long long)tv->tv_sec / 60,
-   (long long)tv->tv_sec % 60,
-   tv->tv_usec / 1, suffix);
+   struct timespec ts;
+
+   TIMEVAL_TO_TIMESPEC(tv, );
+   p_ts(shf, posix, , width, prefix, suffix);
 }
 
 static void



Re: simple pledge for xeyes(1)

2023-09-11 Thread Thomas Frohwein
On Fri, Sep 08, 2023 at 08:55:10AM -0300, Lucas de Sena wrote:

[...]

> Quoting from `xenocara/app/xclock/xclock.c`:
> 
> > {
> > /* force reading of XErrorDB into memory to avoid adding "rpath" to 
> >pledge below */
> > char buf[1];
> >
> >(void)XGetErrorDatabaseText(XtDisplay(toplevel), "XProtoError", "0", "", 
> >  buf, 1);
> > }
> > if (pledge("stdio", NULL) == -1)
> >err(1, "pledge");

In xclock.c, this happens right before XtAppMainLoop, and it looks like
we could do the exact same thing for xeyes... Is this better?

Index: xeyes.c
===
RCS file: /cvs/xenocara/app/xeyes/xeyes.c,v
retrieving revision 1.5
diff -u -p -r1.5 xeyes.c
--- xeyes.c 29 Aug 2021 17:50:32 -  1.5
+++ xeyes.c 12 Sep 2023 01:34:03 -
@@ -32,6 +32,10 @@ from the X Consortium.
 # include "config.h"
 #endif
 
+#ifdef HAVE_PLEDGE
+# include 
+# include 
+#endif
 #include 
 #include 
 #include 
@@ -142,6 +146,19 @@ main(int argc, char **argv)
 XtRealizeWidget (toplevel);
 (void) XSetWMProtocols (XtDisplay(toplevel), XtWindow(toplevel),
 _delete_window, 1);
+
+#ifdef HAVE_PLEDGE
+{
+   /* force reading of XErrorDB into memory to avoid adding "rpath" to
+ * pledge below */
+char buf[1];
+
+(void)XGetErrorDatabaseText(XtDisplay(toplevel), "XProtoError", "0", 
"", buf, 1);
+}
+if (pledge("stdio", NULL) == -1)
+   err(1, "pledge");
+#endif
+
 XtAppMainLoop(app_context);
 
 return 0;



Re: rpki-client: ensure X.509 Subject only contains commonName and serialNumber

2023-09-11 Thread Job Snijders
On Mon, Sep 11, 2023 at 09:31:03AM +0200, Theo Buehler wrote:
> > - * This only parses the RFC 3779 extensions since these are necessary for
> > - * validation.
> 
> Isn't this still true? You don't really parse the subject name.

I took 'parse' to mean something like 'inspects', and since it also
inspects the X.509 version, KeyUsage, and soon Subject it seemed a
misleading comment to me :-)

I incorporated your feedback, OK?

Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.114
diff -u -p -r1.114 cert.c
--- cert.c  29 Jun 2023 10:28:25 -  1.114
+++ cert.c  11 Sep 2023 23:44:58 -
@@ -594,9 +594,7 @@ certificate_policies(struct parse *p, X5
 }
 
 /*
- * Lightweight version of cert_parse_pre() for ASPA, ROA, and RSC EE certs.
- * This only parses the RFC 3779 extensions since these are necessary for
- * validation.
+ * Lightweight version of cert_parse_pre() for EE certs.
  * Returns cert on success and NULL on failure.
  */
 struct cert *
@@ -616,6 +614,9 @@ cert_parse_ee_cert(const char *fn, X509 
goto out;
}
 
+   if (!x509_valid_subject(fn, x))
+   goto out;
+
if (X509_get_key_usage(x) != KU_DIGITAL_SIGNATURE) {
warnx("%s: RFC 6487 section 4.8.4: KU must be digitalSignature",
fn);
@@ -726,6 +727,9 @@ cert_parse_pre(const char *fn, const uns
fn);
goto out;
}
+
+   if (!x509_valid_subject(p.fn, x))
+   goto out;
 
/* Look for X509v3 extensions. */
 
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.188
diff -u -p -r1.188 extern.h
--- extern.h29 Jun 2023 14:33:35 -  1.188
+++ extern.h11 Sep 2023 23:44:58 -
@@ -839,6 +839,7 @@ int  x509_location(const char *, const 
GENERAL_NAME *, char **);
 int x509_inherits(X509 *);
 int x509_any_inherits(X509 *);
+int x509_valid_subject(const char *, const X509 *);
 time_t  x509_find_expires(time_t, struct auth *, struct crl_tree *);
 
 /* printers */
Index: x509.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
retrieving revision 1.73
diff -u -p -r1.73 x509.c
--- x509.c  23 Jun 2023 15:32:15 -  1.73
+++ x509.c  11 Sep 2023 23:44:59 -
@@ -861,6 +861,86 @@ x509_location(const char *fn, const char
 }
 
 /*
+ * Check that the subject only contains commonName and serialNumber.
+ * Return 0 on failure.
+ */
+int
+x509_valid_subject(const char *fn, const X509 *x)
+{
+   const X509_NAME *xn;
+   const X509_NAME_ENTRY *ne;
+   const ASN1_OBJECT *ao;
+   const ASN1_STRING *as;
+   int cn = 0, sn = 0;
+   int i, nid;
+
+   if ((xn = X509_get_subject_name(x)) == NULL) {
+   warnx("%s: X509_get_subject_name", fn);
+   return 0;
+   }
+
+   for (i = 0; i < X509_NAME_entry_count(xn); i++) {
+   if ((ne = X509_NAME_get_entry(xn, i)) == NULL) {
+   warnx("%s: X509_NAME_get_entry", fn);
+   return 0;
+   }
+   if ((ao = X509_NAME_ENTRY_get_object(ne)) == NULL) {
+   warnx("%s: X509_NAME_ENTRY_get_object", fn);
+   return 0;
+   }
+
+   nid = OBJ_obj2nid(ao);
+   switch (nid) {
+   case NID_commonName:
+   if (cn++ > 0) {
+   warnx("%s: duplicate commonName in subject",
+   fn);
+   return 0;
+   }
+   if ((as = X509_NAME_ENTRY_get_data(ne)) == NULL) {
+   warnx("%s: X509_NAME_ENTRY_get_data failed",
+   fn);
+   return 0;
+   }
+/*
+ * The following check can be enabled after AFRINIC re-issues CA certs.
+ * https://lists.afrinic.net/pipermail/dbwg/2023-March/000436.html
+ */
+#if 0
+   if (as->type != V_ASN1_PRINTABLESTRING) {
+   warnx("%s: RFC 6487 section 4.5: commonName is"
+   " not PrintableString", fn);
+   return 0;
+   }
+#endif
+   break;
+   case NID_serialNumber:
+   if (sn++ > 0) {
+   warnx("%s: duplicate serialNumber in subject",
+   fn);
+   return 0;
+   }
+   break;
+   case NID_undef:
+   warnx("%s: 

Re: Use counters_read(9) from ddb(4)

2023-09-11 Thread Martin Pieuchot
On 06/09/23(Wed) 23:13, Alexander Bluhm wrote:
> On Wed, Sep 06, 2023 at 12:23:33PM -0500, Scott Cheloha wrote:
> > On Wed, Sep 06, 2023 at 01:04:19PM +0100, Martin Pieuchot wrote:
> > > Debugging OOM is hard.  UVM uses per-CPU counters and sadly
> > > counters_read(9) needs to allocate memory.  This is not acceptable in
> > > ddb(4).  As a result I cannot see the content of UVM counters in OOM
> > > situations.
> > > 
> > > Diff below introduces a *_static() variant of counters_read(9) that
> > > takes a secondary buffer to avoid calling malloc(9).  Is it fine?  Do
> > > you have a better idea?  Should we make it the default or using the
> > > stack might be a problem?
> > 
> > Instead of adding a second interface I think we could get away with
> > just extending counters_read(9) to take a scratch buffer as an optional
> > fourth parameter:
> > 
> > void
> > counters_read(struct cpumem *cm, uint64_t *output, unsigned int n,
> > uint64_t *scratch);
> > 
> > "scratch"?  "temp"?  "tmp"?
> 
> scratch is fine for me

Fine with me.



Re: Dell R7615 kernel protection fault

2023-09-11 Thread Mike Larkin
On Mon, Sep 11, 2023 at 03:23:28PM +0200, Hrvoje Popovski wrote:
> On 11.9.2023. 6:27, Hrvoje Popovski wrote:
> > On 11.9.2023. 2:48, Mike Larkin wrote:
> >> On Sun, Sep 10, 2023 at 01:36:33AM +0200, Hrvoje Popovski wrote:
> >>> Hi all,
> >>>
> >>> I've installed latest snapshot with uefi on Dell R7615 with AMD EPYC
> >>> 9554P, with some NVMe disks on BOSS-N1 adapter and with Samsung NVMe
> >>> disks directly connected to backplane and installation was fast and
> >>> without any problems.
> >>> But after that machine panics with this message
> >>> https://kosjenka.srce.hr/~hrvoje/openbsd/r7615-ddb1.jpg
> >>>
> >>
> >> did it work before on an older snapshot?
> >>
> >
> > this is brand new machine and I installed latest snapshot.
> > will try older snapshot now ...
> >
> >
>
> Hi,
>
> I've tried snapshots from 2023-06-30 and 2023-06-07 and I'm getting same
> kernel protection fault.
>
>
>

hm. I think we'd need to see a backtrace here, and if you can get that far,
a register dump would be useful as well. either %rsi or %rdi here is probably
trash.



Re: Dell R7615 kernel protection fault

2023-09-11 Thread Hrvoje Popovski
On 11.9.2023. 6:27, Hrvoje Popovski wrote:
> On 11.9.2023. 2:48, Mike Larkin wrote:
>> On Sun, Sep 10, 2023 at 01:36:33AM +0200, Hrvoje Popovski wrote:
>>> Hi all,
>>>
>>> I've installed latest snapshot with uefi on Dell R7615 with AMD EPYC
>>> 9554P, with some NVMe disks on BOSS-N1 adapter and with Samsung NVMe
>>> disks directly connected to backplane and installation was fast and
>>> without any problems.
>>> But after that machine panics with this message
>>> https://kosjenka.srce.hr/~hrvoje/openbsd/r7615-ddb1.jpg
>>>
>>
>> did it work before on an older snapshot?
>>
> 
> this is brand new machine and I installed latest snapshot.
> will try older snapshot now ...
> 
> 

Hi,

I've tried snapshots from 2023-06-30 and 2023-06-07 and I'm getting same
kernel protection fault.





Re: ps.1/kvm documentation

2023-09-11 Thread Marc Espie
On Mon, Sep 11, 2023 at 12:10:17PM +0200, Claudio Jeker wrote:
> On Mon, Sep 11, 2023 at 11:02:00AM +0200, Marc Espie wrote:
> > I was reading through ps.1, which has two slightly different options
> >  -H  Also display information about kernel visible threads.
> >  -k  Also display information about kernel threads.
> > 
> > It's not at all obvious what the difference between these options might be.
> > 
>  
> kernel threads == kthread(9) created threads
> 
> Those should have K in STAT and the name is in () like:
>  3141 ??  RK/1   4057:57.90 (idle1)
> 
> kernel visible threads == __tfork_thread(3) created threads for userland
> applications. For example:
> 
> 43838  556612 ??  IpU  0:01.58 firefox (firefox/Cache2 I/O)
> 43838  415551 ??  IpU  0:00.01 firefox (firefox/Cookie)
> 43838  377915 ??  IpU  0:00.01 firefox (firefox/Worker Launcher)
> 
> These threads all share the same PID but have different TID.
> 
> I think the "kernel visible" is there to tell that pure userland threads
> can not be reported by ps. I think go routines are such an example.
> 
> > From the log:
> > 
> > revision 1.77
> > date: 2011/09/25 00:29:59;  author: guenther;  state: Exp;  lines: +5 -3;
> > Add -H option to show rthreads, hiding them by default
> > 
> > Diff from uwe@
> > 
> > so slightly more info.
> > 
> > Looking at the code, now this is KERN_PROC_KTHREAD vs KERN_PROC_SHOW_THREADS
> > in kvm_getprocs(3).
> > 
> > Now KERN_PROC_KTHREAD is documented, but there is nothing about
> > KERN_PROC_SHOW_THREADS.
> > 
> > The code around (dothreads) in kvm* doesn't make things really obvious.

So far I have two explanations, but it's far away from my comfort zone
that I don't know how to make the documentation less crappy.

Any of you guys willing to provide a patch to ps(1) and kvm_getprocs(3) ?



Re: ps.1/kvm documentation

2023-09-11 Thread Claudio Jeker
On Mon, Sep 11, 2023 at 11:02:00AM +0200, Marc Espie wrote:
> I was reading through ps.1, which has two slightly different options
>  -H  Also display information about kernel visible threads.
>  -k  Also display information about kernel threads.
> 
> It's not at all obvious what the difference between these options might be.
> 
 
kernel threads == kthread(9) created threads

Those should have K in STAT and the name is in () like:
 3141 ??  RK/1   4057:57.90 (idle1)

kernel visible threads == __tfork_thread(3) created threads for userland
applications. For example:

43838  556612 ??  IpU  0:01.58 firefox (firefox/Cache2 I/O)
43838  415551 ??  IpU  0:00.01 firefox (firefox/Cookie)
43838  377915 ??  IpU  0:00.01 firefox (firefox/Worker Launcher)

These threads all share the same PID but have different TID.

I think the "kernel visible" is there to tell that pure userland threads
can not be reported by ps. I think go routines are such an example.

> From the log:
> 
> revision 1.77
> date: 2011/09/25 00:29:59;  author: guenther;  state: Exp;  lines: +5 -3;
> Add -H option to show rthreads, hiding them by default
> 
> Diff from uwe@
> 
> so slightly more info.
> 
> Looking at the code, now this is KERN_PROC_KTHREAD vs KERN_PROC_SHOW_THREADS
> in kvm_getprocs(3).
> 
> Now KERN_PROC_KTHREAD is documented, but there is nothing about
> KERN_PROC_SHOW_THREADS.
> 
> The code around (dothreads) in kvm* doesn't make things really obvious.
> 

-- 
:wq Claudio



Re: ps.1/kvm documentation

2023-09-11 Thread Marc Espie
On Mon, Sep 11, 2023 at 11:00:41AM +0100, Stuart Henderson wrote:
> -H is userland threads, -k is kernel threads. I guess "kernel visible" was
> to distinguish between the old uthread where threads were handled in
> userland and not visible to the kernel, and rthread ...
> 
> -- 
>  Sent from a phone, apologies for poor formatting.
> 
> On 11 September 2023 10:02:32 Marc Espie  wrote:
> 
> > I was reading through ps.1, which has two slightly different options
> > -H  Also display information about kernel visible threads.
> > -k  Also display information about kernel threads.
> > 
> > 
> > It's not at all obvious what the difference between these options might be.
> > 
> > From the log:
> > 
> > revision 1.77
> > date: 2011/09/25 00:29:59;  author: guenther;  state: Exp;  lines: +5 -3;
> > Add -H option to show rthreads, hiding them by default
> > 
> > Diff from uwe@
> > 
> > so slightly more info.
> > 
> > Looking at the code, now this is KERN_PROC_KTHREAD vs KERN_PROC_SHOW_THREADS
> > in kvm_getprocs(3).
> > 
> > Now KERN_PROC_KTHREAD is documented, but there is nothing about
> > KERN_PROC_SHOW_THREADS.
> > 
> > The code around (dothreads) in kvm* doesn't make things really obvious.
> 
We can probably just say "userland threads"/"kernel threads" these days ?

I expect documenting KERN_PROC_SHOW_THREADS  would be a good idea /



Re: ps.1/kvm documentation

2023-09-11 Thread Stuart Henderson
-H is userland threads, -k is kernel threads. I guess "kernel visible" was 
to distinguish between the old uthread where threads were handled in 
userland and not visible to the kernel, and rthread ...


--
 Sent from a phone, apologies for poor formatting.

On 11 September 2023 10:02:32 Marc Espie  wrote:


I was reading through ps.1, which has two slightly different options
-H  Also display information about kernel visible threads.
-k  Also display information about kernel threads.


It's not at all obvious what the difference between these options might be.

From the log:

revision 1.77
date: 2011/09/25 00:29:59;  author: guenther;  state: Exp;  lines: +5 -3;
Add -H option to show rthreads, hiding them by default

Diff from uwe@

so slightly more info.

Looking at the code, now this is KERN_PROC_KTHREAD vs KERN_PROC_SHOW_THREADS
in kvm_getprocs(3).

Now KERN_PROC_KTHREAD is documented, but there is nothing about
KERN_PROC_SHOW_THREADS.

The code around (dothreads) in kvm* doesn't make things really obvious.




ps.1/kvm documentation

2023-09-11 Thread Marc Espie
I was reading through ps.1, which has two slightly different options
 -H  Also display information about kernel visible threads.
 -k  Also display information about kernel threads.


It's not at all obvious what the difference between these options might be.

>From the log:

revision 1.77
date: 2011/09/25 00:29:59;  author: guenther;  state: Exp;  lines: +5 -3;
Add -H option to show rthreads, hiding them by default

Diff from uwe@

so slightly more info.

Looking at the code, now this is KERN_PROC_KTHREAD vs KERN_PROC_SHOW_THREADS
in kvm_getprocs(3).

Now KERN_PROC_KTHREAD is documented, but there is nothing about
KERN_PROC_SHOW_THREADS.

The code around (dothreads) in kvm* doesn't make things really obvious.



path: speed-up pkg-config

2023-09-11 Thread Marc Espie
Not to pkgconf levels, but still way faster than what we had

Updated patch from what I've shown to people, turns out the second grep
wasn't quite working.

This does cache the set_variables_from_env shennanigans, speeding up large
processing of recursive files by a large factor (since we keep a cache
of relevant env variables, and don't bother setting the same value twice)


The optimisation in PkgConfig.pm is sligtly less powerful: we got a marker
for variable expansions straight up when we parse a pkgconfig file, before
even splitting into lists, so instead of "raw" lists, 
tag them as NoExpand/ToExpand classes, so  that we can forego variable
expansion altogether.

Please test, this appears to pass regress, and I've just put this into
a partial bulk.

Index: pkg-config
===
RCS file: /cvs/src/usr.bin/pkg-config/pkg-config,v
retrieving revision 1.96
diff -u -p -r1.96 pkg-config
--- pkg-config  8 Jun 2023 08:55:27 -   1.96
+++ pkg-config  11 Sep 2023 07:11:54 -
@@ -279,6 +279,25 @@ if ($mode{cflags} || $mode{libs} || $mod
 exit $rc;
 
 ###
+sub set_variables_from_env($file)
+{
+   state (%done, @l);
+
+   if (!defined $done{$file}) {
+   my $pkg = $file;
+
+   $pkg =~ s/(^.*\/)?(.*?)\.pc$/$2/g;
+   $pkg = uc($pkg);
+   if (!@l) {
+   @l = grep {/PKG_CONFIG_/} keys %ENV;
+   }
+   for my $k (grep {/PKG_CONFIG_${pkg}_(\w+)/} @l) {
+   $variables->{lc($1)} = $ENV{$k};
+   }
+   $done{$file} = 1;
+   }
+
+}
 
 sub handle_config($p, $op, $v, $list)
 {
@@ -300,22 +319,7 @@ sub handle_config($p, $op, $v, $list)
}
 
my $get_props = sub($property) {
-   my $pkg;
-
-   # See if there's anything in the environment that we need to
-   # take into account.
-   ($pkg = $p) =~ s/(^.*\/)?(.*?)\.pc$/$2/g;
-   $pkg = uc($pkg);
-
-   if (grep {/PKG_CONFIG_${pkg}.*/} keys %ENV) {
-   # Now that we know we have something to look for, do
-   # the inefficient iteration.
-   while (my ($k, $v) = each %ENV) {
-   if ($k =~ /^PKG_CONFIG_${pkg}_(\w+)/) {
-   $variables->{lc($1)} = $v;
-   }
-   }
-   }
+   set_variables_from_env($p);
 
my $deps = $cfg->get_property($property, $variables);
return unless defined $deps;
Index: OpenBSD/PkgConfig.pm
===
RCS file: /cvs/src/usr.bin/pkg-config/OpenBSD/PkgConfig.pm,v
retrieving revision 1.10
diff -u -p -r1.10 PkgConfig.pm
--- OpenBSD/PkgConfig.pm8 Jun 2023 08:55:27 -   1.10
+++ OpenBSD/PkgConfig.pm11 Sep 2023 07:11:54 -
@@ -16,6 +16,7 @@
 
 use v5.36;
 
+
 # interface to the *.pc file format of pkg-config.
 package OpenBSD::PkgConfig;
 
@@ -72,10 +73,14 @@ sub add_variable($self, $name, $value)
 
 sub parse_value($self, $name, $value)
 {
+   my $class = "OpenBSD::PkgConfig::NoExpand";
+   if ($value =~ m/\$\{.*\}/) {
+   $class = "OpenBSD::PkgConfig::ToExpand";
+   }
if (defined $parse->{$name}) {
-   return $parse->{$name}($value);
+   return bless $parse->{$name}($value), $class;
} else {
-   return [split /(?parse_value($name, $value);
} else {
-   $v = [];
+   $v = bless [], "OpenBSD::PkgConfig::NoExpand";
}
$self->{properties}{$name} = $v;
 }
@@ -121,8 +126,9 @@ sub read_fh($class, $fh, $name = '')
}
}
if (defined $cfg->{properties}{Libs}) {
-   $cfg->{properties}{Libs} =
-   $cfg->compress_list($cfg->{properties}{Libs});
+   $cfg->{properties}{Libs} = bless
+   $cfg->compress_list($cfg->{properties}{Libs}),
+   ref($cfg->{properties}{Libs});
}
return $cfg;
 }
@@ -220,6 +226,9 @@ sub get_property($self, $k, $extra = {})
if (!defined $l) {
return undef;
}
+   if ($l->noexpand) {
+   return [@$l];
+   }
my $r = [];
for my $v (@$l) {
my $w = $self->expanded($v, $extra);
@@ -263,4 +272,17 @@ sub add_bases($self, $extra)
}
 }
 
+package OpenBSD::PkgConfig::NoExpand;
+our @ISA = qw(OpenBSD::PkgConfig);
+sub noexpand($)
+{
+   1
+}
+
+package OpenBSD::PkgConfig::ToExpand;
+our @ISA = qw(OpenBSD::PkgConfig);
+sub noexpand($)
+{
+   0
+}
 1;



Re: rpki-client: ensure X.509 Subject only contains commonName and serialNumber

2023-09-11 Thread Theo Buehler
On Mon, Sep 11, 2023 at 01:42:03AM +, Job Snijders wrote:
> This adds another compliance check for the X.509 subject name.
> 
> Only commonName, and optionally serialNumber, are permitted in the
> certificate subject name. See RFC 6487 section 4.4 and 4.5.
> 
> It seems the one CA who was not compliant with this requirement got
> their act together, so now is an opportune time to get this in.

I wish there was a better place for this, but due to the unfortunate
parse/validate split, there doesn't seem to be.

> OK?

Some comments below.

> Index: cert.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.114
> diff -u -p -r1.114 cert.c
> --- cert.c29 Jun 2023 10:28:25 -  1.114
> +++ cert.c9 Jul 2023 13:38:02 -
> @@ -595,14 +595,13 @@ certificate_policies(struct parse *p, X5
>  
>  /*
>   * Lightweight version of cert_parse_pre() for ASPA, ROA, and RSC EE certs.

"ASPA, ROA, and RSC" is a lie, isn't it.

> - * This only parses the RFC 3779 extensions since these are necessary for
> - * validation.

Isn't this still true? You don't really parse the subject name.

>   * Returns cert on success and NULL on failure.
>   */
>  struct cert *
>  cert_parse_ee_cert(const char *fn, X509 *x)
>  {
>   struct parse p;
> + X509_NAME   *xn;
>   X509_EXTENSION  *ext;
>   int  index;
>  
> @@ -616,6 +615,13 @@ cert_parse_ee_cert(const char *fn, X509 
>   goto out;
>   }
>  
> + if ((xn = X509_get_subject_name(x)) == NULL) {
> + warnx("%s: X509_get_subject_name", fn);
> + goto out;
> + }
> + if (!x509_valid_subject(fn, xn))
> + goto out;
> +
>   if (X509_get_key_usage(x) != KU_DIGITAL_SIGNATURE) {
>   warnx("%s: RFC 6487 section 4.8.4: KU must be digitalSignature",
>   fn);
> @@ -669,6 +675,7 @@ cert_parse_pre(const char *fn, const uns
>   const X509_ALGOR*palg;
>   const ASN1_BIT_STRING   *piuid = NULL, *psuid = NULL;
>   const ASN1_OBJECT   *cobj;
> + const X509_NAME *xn;
>   ASN1_OBJECT *obj;
>   EVP_PKEY*pkey;
>   struct parse p;
> @@ -726,6 +733,13 @@ cert_parse_pre(const char *fn, const uns
>   fn);
>   goto out;
>   }
> +
> + if ((xn = X509_get_subject_name(x)) == NULL) {
> + warnx("%s: X509_get_subject_name", p.fn);
> + goto out;
> + }
> + if (!x509_valid_subject(p.fn, xn))
> + goto out;
>  
>   /* Look for X509v3 extensions. */
>  
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.188
> diff -u -p -r1.188 extern.h
> --- extern.h  29 Jun 2023 14:33:35 -  1.188
> +++ extern.h  9 Jul 2023 13:38:02 -
> @@ -839,6 +839,7 @@ intx509_location(const char *, const 
>   GENERAL_NAME *, char **);
>  int   x509_inherits(X509 *);
>  int   x509_any_inherits(X509 *);
> +int   x509_valid_subject(const char *, const X509_NAME *);
>  time_tx509_find_expires(time_t, struct auth *, struct 
> crl_tree *);
>  
>  /* printers */
> Index: x509.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
> retrieving revision 1.73
> diff -u -p -r1.73 x509.c
> --- x509.c23 Jun 2023 15:32:15 -  1.73
> +++ x509.c9 Jul 2023 13:38:02 -
> @@ -861,6 +861,80 @@ x509_location(const char *fn, const char
>  }
>  
>  /*
> + * Check that the subject only contains commonName and serialNumber.
> + * Return 0 on failure.
> + */
> +int
> +x509_valid_subject(const char *fn, const X509_NAME *xn)

I would pass in the X509 itself rather than extracting the name in the
caller. This adds

> +{
> + X509_NAME_ENTRY *ne;
> + ASN1_OBJECT *ao;
> + ASN1_STRING *as;
> + int cn = 0, sn = 0;
> + int i, nid;

And get the subject name here. Then you only have to do this once and
the caller only grows by three lines and without additional variable.

> +
> + for (i = 0; i < X509_NAME_entry_count(xn); i++) {
> + if ((ne = X509_NAME_get_entry(xn, i)) == NULL) {
> + warnx("%s: X509_NAME_get_entry", fn);
> + return 0;
> + }
> + if ((ao = X509_NAME_ENTRY_get_object(ne)) == NULL) {
> + warnx("%s: X509_NAME_ENTRY_get_object", fn);
> + return 0;
> + }
> +
> + nid = OBJ_obj2nid(ao);
> + switch (nid) {
> + case NID_commonName:
> + if (cn++ > 0) {
> + warnx("%s: duplicate commonName in subject",
> +