Re: libcrypto: wrapper for internal x509v3_cache_extensions()
On Fri, Jan 20, 2023 at 09:13:04PM +, Job Snijders wrote:
> On Fri, Jan 20, 2023 at 09:35:08PM +0100, Theo Buehler wrote:
> > On Fri, Jan 20, 2023 at 08:06:00PM +, Job Snijders wrote:
> > > While studying why X509_check_ca() is the ugly thing it is, tb@
> > > suggested x509v3_cache_extensions() might benefit from a wrapper to
> > > avoid duplication of locking and checking the stupid EXFLAG_INVALID
> > > flag. x509v3_cache_extensions() isn't a public function anyway.
> > >
> > > Passes regress & rpki-client.
> > >
> > > OK?
> >
> > Cool, thanks. I think it's better to pull the EXFLAG_SET check into the
> > new function like beck has done in x509_verify_cert_cache_extensions()
> > which then becomes a simple wrapper (it's nice to keep the 'namespacing'
> > clean in x509_verify.c).
> >
> > Details inline.
>
> Thanks for the feedback!
>
> I think all your points have been addressed in the below changeset.
Yes that's great, thank you.
ok tb
with the two small nits below.
> +/*
> + * 1 is success, 0 is failure.
> + */
Please remove this comment. This is the default in libcrypto.
> +int
> +x509v3_cache_extensions(X509 *x)
> +{
> + if (!(x->ex_flags & EXFLAG_SET)) {
This is not really a Boolean, so please check explicitly against 0 like
a few lines down.
if ((x->ex_flags & EXFLAG_SET) == 0) {
> + CRYPTO_w_lock(CRYPTO_LOCK_X509);
> + x509v3_cache_extensions_internal(x);
> + CRYPTO_w_unlock(CRYPTO_LOCK_X509);
> + }
> +
> + return (x->ex_flags & EXFLAG_INVALID) == 0;
> +}
Re: mem.4: be more accurate about securelevel
On Fri, Jan 20, 2023 at 01:15:29PM -0700, Theo de Raadt wrote:
> Todd C. Miller wrote:
> > I wonder if it makes sense to have a version of sysctl.conf that
> > only gets used for the next reboot and then is removed, kind of
> > like /etc/rc.firsttime. Maybe call it /etc/sysctl.once.
>
> Well you are shown the change at boot, and it is visible in dmesg -s,
> which should be good enough.
Otherwise, something like this might be useful for test machines:
Index: wsemul_vt100.c
===
RCS file: /cvs/src/sys/dev/wscons/wsemul_vt100.c,v
retrieving revision 1.42
diff -u -p -r1.42 wsemul_vt100.c
--- wsemul_vt100.c 12 Jan 2023 20:39:37 - 1.42
+++ wsemul_vt100.c 20 Jan 2023 21:00:08 -
@@ -170,6 +170,12 @@ wsemul_vt100_cnattach(const struct wsscr
#ifndef WS_KERNEL_BG
#define WS_KERNEL_BG WSCOL_BLUE
#endif
+#ifndef WS_INSEC_BG
+#define WS_INSEC_BG WSCOL_RED
+#endif
+#ifndef WS_INSEC_FG
+#define WS_INSEC_FG WSCOL_WHITE
+#endif
#ifndef WS_KERNEL_COLATTR
#define WS_KERNEL_COLATTR 0
#endif
@@ -186,6 +192,16 @@ wsemul_vt100_cnattach(const struct wsscr
if (res)
edp->kernattr = defattr;
+ if (type->capabilities & WSSCREEN_WSCOLORS)
+ res = (*edp->emulops->pack_attr)(cookie,
+ WS_INSEC_FG, WS_INSEC_BG,
+ WS_KERNEL_COLATTR | WSATTR_WSCOLORS, &edp->insecattr);
+ else
+ res = (*edp->emulops->pack_attr)(cookie, 0, 0,
+ WS_KERNEL_MONOATTR, &edp->insecattr);
+ if (res)
+ edp->insecattr = defattr;
+
edp->tabs = NULL;
#ifdef HAVE_DOUBLE_WIDTH_HEIGHT
edp->dblwid = NULL;
@@ -387,15 +403,15 @@ wsemul_vt100_output_normal(struct wsemul
return rc;
}
}
-
+#define KERN_ATTR (securelevel < 1 ? edp->insecattr : edp->kernattr)
#ifdef HAVE_DOUBLE_WIDTH_HEIGHT
WSEMULOP(rc, edp, &edp->abortstate, putchar,
(edp->emulcookie, edp->crow, edp->ccol << edp->dw, dc,
-kernel ? edp->kernattr : edp->curattr));
+kernel ? KERN_ATTR : edp->curattr));
#else
WSEMULOP(rc, edp, &edp->abortstate, putchar,
(edp->emulcookie, edp->crow, edp->ccol, dc,
-kernel ? edp->kernattr : edp->curattr));
+kernel ? KERN_ATTR : edp->curattr));
#endif
if (rc != 0) {
/* undo potential sschartab update */
Index: wsemul_vt100var.h
===
RCS file: /cvs/src/sys/dev/wscons/wsemul_vt100var.h,v
retrieving revision 1.12
diff -u -p -r1.12 wsemul_vt100var.h
--- wsemul_vt100var.h 12 Jan 2023 20:39:37 - 1.12
+++ wsemul_vt100var.h 20 Jan 2023 21:00:08 -
@@ -38,6 +38,7 @@ struct wsemul_vt100_emuldata {
uint32_t defattr; /* default attribute */
uint32_t kernattr; /* attribute for kernel output */
+ uint32_t insecattr; /* ^^^ for securelevel < 1 ^^^ */
void *cbcookie;
#ifdef DIAGNOSTIC
int console;
Re: libcrypto: wrapper for internal x509v3_cache_extensions()
On Fri, Jan 20, 2023 at 09:35:08PM +0100, Theo Buehler wrote:
> On Fri, Jan 20, 2023 at 08:06:00PM +, Job Snijders wrote:
> > While studying why X509_check_ca() is the ugly thing it is, tb@
> > suggested x509v3_cache_extensions() might benefit from a wrapper to
> > avoid duplication of locking and checking the stupid EXFLAG_INVALID
> > flag. x509v3_cache_extensions() isn't a public function anyway.
> >
> > Passes regress & rpki-client.
> >
> > OK?
>
> Cool, thanks. I think it's better to pull the EXFLAG_SET check into the
> new function like beck has done in x509_verify_cert_cache_extensions()
> which then becomes a simple wrapper (it's nice to keep the 'namespacing'
> clean in x509_verify.c).
>
> Details inline.
Thanks for the feedback!
I think all your points have been addressed in the below changeset.
Kind regards,
Job
Index: x509/x509_internal.h
===
RCS file: /cvs/src/lib/libcrypto/x509/x509_internal.h,v
retrieving revision 1.23
diff -u -p -r1.23 x509_internal.h
--- x509/x509_internal.h26 Nov 2022 16:08:54 - 1.23
+++ x509/x509_internal.h20 Jan 2023 21:09:59 -
@@ -94,7 +94,7 @@ int x509_vfy_check_policy(X509_STORE_CTX
int x509_vfy_check_trust(X509_STORE_CTX *ctx);
int x509_vfy_check_chain_extensions(X509_STORE_CTX *ctx);
int x509_vfy_callback_indicate_completion(X509_STORE_CTX *ctx);
-void x509v3_cache_extensions(X509 *x);
+int x509v3_cache_extensions(X509 *x);
X509 *x509_vfy_lookup_cert_match(X509_STORE_CTX *ctx, X509 *x);
time_t x509_verify_asn1_time_to_time_t(const ASN1_TIME *atime, int notafter);
Index: x509/x509_purp.c
===
RCS file: /cvs/src/lib/libcrypto/x509/x509_purp.c,v
retrieving revision 1.18
diff -u -p -r1.18 x509_purp.c
--- x509/x509_purp.c26 Nov 2022 16:08:55 - 1.18
+++ x509/x509_purp.c20 Jan 2023 21:09:59 -
@@ -76,8 +76,6 @@
#define ns_reject(x, usage) \
(((x)->ex_flags & EXFLAG_NSCERT) && !((x)->ex_nscert & (usage)))
-void x509v3_cache_extensions(X509 *x);
-
static int check_ssl_ca(const X509 *x);
static int check_purpose_ssl_client(const X509_PURPOSE *xp, const X509 *x,
int ca);
@@ -131,13 +129,9 @@ X509_check_purpose(X509 *x, int id, int
int idx;
const X509_PURPOSE *pt;
- if (!(x->ex_flags & EXFLAG_SET)) {
- CRYPTO_w_lock(CRYPTO_LOCK_X509);
- x509v3_cache_extensions(x);
- CRYPTO_w_unlock(CRYPTO_LOCK_X509);
- if (x->ex_flags & EXFLAG_INVALID)
- return -1;
- }
+ if (!x509v3_cache_extensions(x))
+ return -1;
+
if (id == -1)
return 1;
idx = X509_PURPOSE_get_by_id(id);
@@ -449,8 +443,8 @@ setup_crldp(X509 *x)
setup_dp(x, sk_DIST_POINT_value(x->crldp, i));
}
-void
-x509v3_cache_extensions(X509 *x)
+static void
+x509v3_cache_extensions_internal(X509 *x)
{
BASIC_CONSTRAINTS *bs;
PROXY_CERT_INFO_EXTENSION *pci;
@@ -640,6 +634,21 @@ x509v3_cache_extensions(X509 *x)
x->ex_flags |= EXFLAG_SET;
}
+/*
+ * 1 is success, 0 is failure.
+ */
+int
+x509v3_cache_extensions(X509 *x)
+{
+ if (!(x->ex_flags & EXFLAG_SET)) {
+ CRYPTO_w_lock(CRYPTO_LOCK_X509);
+ x509v3_cache_extensions_internal(x);
+ CRYPTO_w_unlock(CRYPTO_LOCK_X509);
+ }
+
+ return (x->ex_flags & EXFLAG_INVALID) == 0;
+}
+
/* CA checks common to all purposes
* return codes:
* 0 not a CA
@@ -680,11 +689,7 @@ check_ca(const X509 *x)
int
X509_check_ca(X509 *x)
{
- if (!(x->ex_flags & EXFLAG_SET)) {
- CRYPTO_w_lock(CRYPTO_LOCK_X509);
- x509v3_cache_extensions(x);
- CRYPTO_w_unlock(CRYPTO_LOCK_X509);
- }
+ x509v3_cache_extensions(x);
return check_ca(x);
}
@@ -895,19 +900,10 @@ X509_check_issued(X509 *issuer, X509 *su
if (X509_NAME_cmp(X509_get_subject_name(issuer),
X509_get_issuer_name(subject)))
return X509_V_ERR_SUBJECT_ISSUER_MISMATCH;
- if (!(issuer->ex_flags & EXFLAG_SET)) {
- CRYPTO_w_lock(CRYPTO_LOCK_X509);
- x509v3_cache_extensions(issuer);
- CRYPTO_w_unlock(CRYPTO_LOCK_X509);
- }
- if (issuer->ex_flags & EXFLAG_INVALID)
+
+ if (!x509v3_cache_extensions(issuer))
return X509_V_ERR_UNSPECIFIED;
- if (!(subject->ex_flags & EXFLAG_SET)) {
- CRYPTO_w_lock(CRYPTO_LOCK_X509);
- x509v3_cache_extensions(subject);
- CRYPTO_w_unlock(CRYPTO_LOCK_X509);
- }
- if (subject->ex_flags & EXFLAG_INVALID)
+ if (!x509v3_cache_extensions(subject))
return X509_V_ERR_UNSPECIFIED;
if (subject->akid) {
Index: x509/x509_verify.c
===
RCS file: /c
Re: libcrypto: wrapper for internal x509v3_cache_extensions()
On Fri, Jan 20, 2023 at 08:06:00PM +, Job Snijders wrote:
> While studying why X509_check_ca() is the ugly thing it is, tb@
> suggested x509v3_cache_extensions() might benefit from a wrapper to
> avoid duplication of locking and checking the stupid EXFLAG_INVALID
> flag. x509v3_cache_extensions() isn't a public function anyway.
>
> Passes regress & rpki-client.
>
> OK?
Cool, thanks. I think it's better to pull the EXFLAG_SET check into the
new function like beck has done in x509_verify_cert_cache_extensions()
which then becomes a simple wrapper (it's nice to keep the 'namespacing'
clean in x509_verify.c).
Details inline.
>
> Kind regards,
>
> Job
>
> Index: x509/x509_internal.h
> ===
> RCS file: /cvs/src/lib/libcrypto/x509/x509_internal.h,v
> retrieving revision 1.23
> diff -u -p -r1.23 x509_internal.h
> --- x509/x509_internal.h 26 Nov 2022 16:08:54 - 1.23
> +++ x509/x509_internal.h 20 Jan 2023 19:59:56 -
> @@ -94,7 +94,8 @@ int x509_vfy_check_policy(X509_STORE_CTX
> int x509_vfy_check_trust(X509_STORE_CTX *ctx);
> int x509_vfy_check_chain_extensions(X509_STORE_CTX *ctx);
> int x509_vfy_callback_indicate_completion(X509_STORE_CTX *ctx);
> -void x509v3_cache_extensions(X509 *x);
> +void x509v3_cache_extensions_internal(X509 *x);
remove the internal function here.
> +int x509v3_cache_extensions(X509 *x);
> X509 *x509_vfy_lookup_cert_match(X509_STORE_CTX *ctx, X509 *x);
>
> time_t x509_verify_asn1_time_to_time_t(const ASN1_TIME *atime, int notafter);
> Index: x509/x509_purp.c
> ===
> RCS file: /cvs/src/lib/libcrypto/x509/x509_purp.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 x509_purp.c
> --- x509/x509_purp.c 26 Nov 2022 16:08:55 - 1.18
> +++ x509/x509_purp.c 20 Jan 2023 19:59:57 -
> @@ -76,7 +76,8 @@
> #define ns_reject(x, usage) \
> (((x)->ex_flags & EXFLAG_NSCERT) && !((x)->ex_nscert & (usage)))
>
> -void x509v3_cache_extensions(X509 *x);
> +void x509v3_cache_extensions_internal(X509 *x);
> +int x509v3_cache_extensions(X509 *x);
drop these two prototypes
>
> static int check_ssl_ca(const X509 *x);
> static int check_purpose_ssl_client(const X509_PURPOSE *xp, const X509 *x,
> @@ -132,12 +133,10 @@ X509_check_purpose(X509 *x, int id, int
> const X509_PURPOSE *pt;
>
> if (!(x->ex_flags & EXFLAG_SET)) {
> - CRYPTO_w_lock(CRYPTO_LOCK_X509);
> - x509v3_cache_extensions(x);
> - CRYPTO_w_unlock(CRYPTO_LOCK_X509);
> - if (x->ex_flags & EXFLAG_INVALID)
> + if (!x509v3_cache_extensions(x))
> return -1;
> }
This becomes
if (!x509v3_cache_extensions(x))
return -1;
> +
> if (id == -1)
> return 1;
> idx = X509_PURPOSE_get_by_id(id);
> @@ -450,7 +449,7 @@ setup_crldp(X509 *x)
> }
>
> void
static void
> -x509v3_cache_extensions(X509 *x)
> +x509v3_cache_extensions_internal(X509 *x)
> {
> BASIC_CONSTRAINTS *bs;
> PROXY_CERT_INFO_EXTENSION *pci;
> @@ -640,6 +639,19 @@ x509v3_cache_extensions(X509 *x)
> x->ex_flags |= EXFLAG_SET;
> }
>
> +/*
> + * 1 is success, 0 is failure.
> + */
> +int
> +x509v3_cache_extensions(X509 *x)
> +{
if ((x->ex_flags & EXFLAG_SET) == 0) {
CRYPTO_w_lock(CRYPTO_LOCK_X509);
x509v3_cache_extensions_internal(x);
CRYPTO_w_unlock(CRYPTO_LOCK_X509);
}
> + CRYPTO_w_lock(CRYPTO_LOCK_X509);
> + x509v3_cache_extensions_internal(x);
> + CRYPTO_w_unlock(CRYPTO_LOCK_X509);
> +
> + return ((x->ex_flags & EXFLAG_INVALID) == 0);
Drop outer parentheses.
> +}
> +
> /* CA checks common to all purposes
> * return codes:
> * 0 not a CA
> @@ -680,11 +692,8 @@ check_ca(const X509 *x)
> int
> X509_check_ca(X509 *x)
> {
> - if (!(x->ex_flags & EXFLAG_SET)) {
> - CRYPTO_w_lock(CRYPTO_LOCK_X509);
> + if (!(x->ex_flags & EXFLAG_SET))
> x509v3_cache_extensions(x);
> - CRYPTO_w_unlock(CRYPTO_LOCK_X509);
> - }
x509v3_cache_extensions(x)
>
> return check_ca(x);
> }
> @@ -896,19 +905,13 @@ X509_check_issued(X509 *issuer, X509 *su
> X509_get_issuer_name(subject)))
> return X509_V_ERR_SUBJECT_ISSUER_MISMATCH;
> if (!(issuer->ex_flags & EXFLAG_SET)) {
> - CRYPTO_w_lock(CRYPTO_LOCK_X509);
> - x509v3_cache_extensions(issuer);
> - CRYPTO_w_unlock(CRYPTO_LOCK_X509);
> + if (!x509v3_cache_extensions(issuer))
> + return X509_V_ERR_UNSPECIFIED;
> }
> - if (issuer->ex_flags & EXFLAG_INVALID)
> - return X509_V_ERR_UNSPECIFIED;
> if (!(subject->ex_flags & EXFLAG_SET)) {
> - CRYPTO_w_lock(CRYPTO_LOCK_X509);
> - x509v3_cache_extensions(subject
Re: mem.4: be more accurate about securelevel
Todd C. Miller wrote: > On Fri, 20 Jan 2023 11:29:15 -0700, "Theo de Raadt" wrote: > > > During this mimmmutable and xonly work, I keep finding test machines where > > I enabled kern.allowkmem, and have to disable it. Sometimes weeks later. > > Both kern.allowkmem and securelevel disabling are dangerous, especially in > > our world where so much other dangerous stuff has been stopped. > > I wonder if it makes sense to have a version of sysctl.conf that > only gets used for the next reboot and then is removed, kind of > like /etc/rc.firsttime. Maybe call it /etc/sysctl.once. Well you are shown the change at boot, and it is visible in dmesg -s, which should be good enough. I guess I'm saying if I am sloppy, others will also be sloppy.
Re: mem.4: be more accurate about securelevel
On Fri, 20 Jan 2023 11:29:15 -0700, "Theo de Raadt" wrote: > During this mimmmutable and xonly work, I keep finding test machines where > I enabled kern.allowkmem, and have to disable it. Sometimes weeks later. > Both kern.allowkmem and securelevel disabling are dangerous, especially in > our world where so much other dangerous stuff has been stopped. I wonder if it makes sense to have a version of sysctl.conf that only gets used for the next reboot and then is removed, kind of like /etc/rc.firsttime. Maybe call it /etc/sysctl.once. - todd
libcrypto: wrapper for internal x509v3_cache_extensions()
While studying why X509_check_ca() is the ugly thing it is, tb@
suggested x509v3_cache_extensions() might benefit from a wrapper to
avoid duplication of locking and checking the stupid EXFLAG_INVALID
flag. x509v3_cache_extensions() isn't a public function anyway.
Passes regress & rpki-client.
OK?
Kind regards,
Job
Index: x509/x509_internal.h
===
RCS file: /cvs/src/lib/libcrypto/x509/x509_internal.h,v
retrieving revision 1.23
diff -u -p -r1.23 x509_internal.h
--- x509/x509_internal.h26 Nov 2022 16:08:54 - 1.23
+++ x509/x509_internal.h20 Jan 2023 19:59:56 -
@@ -94,7 +94,8 @@ int x509_vfy_check_policy(X509_STORE_CTX
int x509_vfy_check_trust(X509_STORE_CTX *ctx);
int x509_vfy_check_chain_extensions(X509_STORE_CTX *ctx);
int x509_vfy_callback_indicate_completion(X509_STORE_CTX *ctx);
-void x509v3_cache_extensions(X509 *x);
+void x509v3_cache_extensions_internal(X509 *x);
+int x509v3_cache_extensions(X509 *x);
X509 *x509_vfy_lookup_cert_match(X509_STORE_CTX *ctx, X509 *x);
time_t x509_verify_asn1_time_to_time_t(const ASN1_TIME *atime, int notafter);
Index: x509/x509_purp.c
===
RCS file: /cvs/src/lib/libcrypto/x509/x509_purp.c,v
retrieving revision 1.18
diff -u -p -r1.18 x509_purp.c
--- x509/x509_purp.c26 Nov 2022 16:08:55 - 1.18
+++ x509/x509_purp.c20 Jan 2023 19:59:57 -
@@ -76,7 +76,8 @@
#define ns_reject(x, usage) \
(((x)->ex_flags & EXFLAG_NSCERT) && !((x)->ex_nscert & (usage)))
-void x509v3_cache_extensions(X509 *x);
+void x509v3_cache_extensions_internal(X509 *x);
+int x509v3_cache_extensions(X509 *x);
static int check_ssl_ca(const X509 *x);
static int check_purpose_ssl_client(const X509_PURPOSE *xp, const X509 *x,
@@ -132,12 +133,10 @@ X509_check_purpose(X509 *x, int id, int
const X509_PURPOSE *pt;
if (!(x->ex_flags & EXFLAG_SET)) {
- CRYPTO_w_lock(CRYPTO_LOCK_X509);
- x509v3_cache_extensions(x);
- CRYPTO_w_unlock(CRYPTO_LOCK_X509);
- if (x->ex_flags & EXFLAG_INVALID)
+ if (!x509v3_cache_extensions(x))
return -1;
}
+
if (id == -1)
return 1;
idx = X509_PURPOSE_get_by_id(id);
@@ -450,7 +449,7 @@ setup_crldp(X509 *x)
}
void
-x509v3_cache_extensions(X509 *x)
+x509v3_cache_extensions_internal(X509 *x)
{
BASIC_CONSTRAINTS *bs;
PROXY_CERT_INFO_EXTENSION *pci;
@@ -640,6 +639,19 @@ x509v3_cache_extensions(X509 *x)
x->ex_flags |= EXFLAG_SET;
}
+/*
+ * 1 is success, 0 is failure.
+ */
+int
+x509v3_cache_extensions(X509 *x)
+{
+ CRYPTO_w_lock(CRYPTO_LOCK_X509);
+ x509v3_cache_extensions_internal(x);
+ CRYPTO_w_unlock(CRYPTO_LOCK_X509);
+
+ return ((x->ex_flags & EXFLAG_INVALID) == 0);
+}
+
/* CA checks common to all purposes
* return codes:
* 0 not a CA
@@ -680,11 +692,8 @@ check_ca(const X509 *x)
int
X509_check_ca(X509 *x)
{
- if (!(x->ex_flags & EXFLAG_SET)) {
- CRYPTO_w_lock(CRYPTO_LOCK_X509);
+ if (!(x->ex_flags & EXFLAG_SET))
x509v3_cache_extensions(x);
- CRYPTO_w_unlock(CRYPTO_LOCK_X509);
- }
return check_ca(x);
}
@@ -896,19 +905,13 @@ X509_check_issued(X509 *issuer, X509 *su
X509_get_issuer_name(subject)))
return X509_V_ERR_SUBJECT_ISSUER_MISMATCH;
if (!(issuer->ex_flags & EXFLAG_SET)) {
- CRYPTO_w_lock(CRYPTO_LOCK_X509);
- x509v3_cache_extensions(issuer);
- CRYPTO_w_unlock(CRYPTO_LOCK_X509);
+ if (!x509v3_cache_extensions(issuer))
+ return X509_V_ERR_UNSPECIFIED;
}
- if (issuer->ex_flags & EXFLAG_INVALID)
- return X509_V_ERR_UNSPECIFIED;
if (!(subject->ex_flags & EXFLAG_SET)) {
- CRYPTO_w_lock(CRYPTO_LOCK_X509);
- x509v3_cache_extensions(subject);
- CRYPTO_w_unlock(CRYPTO_LOCK_X509);
+ if (!x509v3_cache_extensions(subject))
+ return X509_V_ERR_UNSPECIFIED;
}
- if (subject->ex_flags & EXFLAG_INVALID)
- return X509_V_ERR_UNSPECIFIED;
if (subject->akid) {
int ret = X509_check_akid(issuer, subject->akid);
Index: x509/x509_verify.c
===
RCS file: /cvs/src/lib/libcrypto/x509/x509_verify.c,v
retrieving revision 1.62
diff -u -p -r1.62 x509_verify.c
--- x509/x509_verify.c 17 Jan 2023 23:49:28 - 1.62
+++ x509/x509_verify.c 20 Jan 2023 19:59:57 -
@@ -242,12 +242,9 @@ static int
x509_verify_cert_cache_extensions(X509 *cert)
{
if (!(cert->ex_flags & EXFLAG_SET)) {
- CRYPTO_w_lock(CRYPTO_LOCK_X509);
- x509v3_cache_extensions(cert);
-
Re: mem.4: be more accurate about securelevel
I don't argue for it to be undefined behaviour. It just is a bad idea to put ideas into people's minds. In this case the idea vaguely is 'oh i should disable securelevel, i can do more with my machine'. During this mimmmutable and xonly work, I keep finding test machines where I enabled kern.allowkmem, and have to disable it. Sometimes weeks later. Both kern.allowkmem and securelevel disabling are dangerous, especially in our world where so much other dangerous stuff has been stopped.
Re: mem.4: be more accurate about securelevel
Hi Stuart, Stuart Henderson wrote on Fri, Jan 20, 2023 at 08:50:48AM +: > On 2023/01/18 12:46, Theo de Raadt wrote: >> But you should not start a sentence with also. >> Also you should not start a sentence with but. >> >> Not the best english. jmc can weight in perhaps. >> Jan Klemkow wrote: >>> .Pp >>> Even with sufficient file system permissions, >>> these devices can only be opened when the >>> -.Xr securelevel 7 >>> -is insecure or when the >>> .Va kern.allowkmem >>> .Xr sysctl 2 >>> variable is set. >>> +Also the >>> +.Xr securelevel 7 >>> +insecure is needed, to open the device writable. > This is all that's needed isn't it? > > Even with sufficient file system permissions, > these devices can only be opened when the > .Xr securelevel 7 > -is insecure or when the > -is insecure and the > .Va kern.allowkmem > .Xr sysctl 2 > variable is set. I believe that is not what we want to say: deraadt@ argues that - nobody should run with insecure securelevel, not even for offline debugging - and it is not needed for read access to /dev/mem (The discussion has in part drifted off list.) If we want a complete description (including the strongly discouraged way to get write access), the following floating diff is the best i'm aware of: Even with sufficient file system permissions, these devices can only be opened when the -.Xr securelevel 7 -is insecure or when the .Va kern.allowkmem .Xr sysctl 2 variable is set. +Write access additionally requires an insecure +.Xr securelevel 7 . If we want to discourage this even more, we could say something like this: .Pp Even with sufficient file system permissions, -these devices can only be opened when the +these devices can only be opened for reading and only when the -.Xr securelevel 7 -is insecure or when the .Va kern.allowkmem .Xr sysctl 2 variable is set. .Sh FILES That would make write behaviour undefined, such that it could be removed with no further documentation fuss once write access is indeed removed in the future. I would be fine with either direction. Yours, Ingo
Re: Inconsistent isdigit(3) man page
Hi Todd, hi Bob, Todd C. Miller wrote on Fri, Jan 20, 2023 at 09:59:20AM -0700: > On Fri, 20 Jan 2023 09:32:38 -0700, Bob Beck wrote: >> So isdigit(3) says in the first paragraph that >> 'The complete list of decimal digits is 0 and 1-9, in any locale.' The intended meaning of this sentence was "on OpenBSD". We often have the DESCRIPTION state OpenBSD behaviour, then relegate portability information to other sections. I see how this practice could cause confusion in the case at hand. >> Later on it says: >> >> 'On systems supporting non-ASCII single-byte character encodings, >> different c arguments may correspond to the digits, and the results of >> isdigit() may depend on the LC_CTYPE locale(1).' >> >> Argubly worring about non ASCII single byte character encodings >> doesn't make sense for an OpenBSD man page, but setting that aside for >> a minute it's the second part that is of concern.. "It may depend on >> the LC_CTYPE locale()" - Ok, well does it or doesn't it? > On OpenBSD isdigit() never uses the locale, but you already knew > that. >> Various spec docs seem all over the place on this, so I am also >> paging Dr. Posix in this email... Hi Philip! :) Is isdigit() >> safe from being screwed up by locale or not? > POSIX says that isdigit() may be influenced by the locale. Since > LC_CTYPE defines the different character classes, of which "digit" > is one, I think that part of the manual is correct as-is. > > However, perhaps we should make isdigit(3) match isalpha(3) like > so: I agree with Todd. OK schwarze@ Ingo > Index: lib/libc/gen/isdigit.3 > === > RCS file: /cvs/src/lib/libc/gen/isdigit.3,v > retrieving revision 1.13 > diff -u -p -u -r1.13 isdigit.3 > --- lib/libc/gen/isdigit.311 Sep 2022 06:38:10 - 1.13 > +++ lib/libc/gen/isdigit.320 Jan 2023 16:58:20 - > @@ -51,7 +51,12 @@ The > and > .Fn isdigit_l > functions test for any decimal-digit character. > -The complete list of decimal digits is 0 and 1\(en9, in any locale. > +In the C locale, the complete list of decimal digits is 0 and 1\(en9. > +.Ox > +always uses the C locale for these functions, > +ignoring the global locale, the thread-specific locale, and the > +.Fa locale > +argument. > .Sh RETURN VALUES > These functions return zero if the character tests false or > non-zero if the character tests true.
Re: rdsetroot.8: sync synopsis with usage, improve wording
On Fri, Jan 20, 2023 at 05:00:37PM +, Klemens Nanni wrote: > Alright, sorry for the noise. > > Is this minimal sync plus stdout mention fine? > > Index: rdsetroot.8 > === > RCS file: /cvs/src/usr.sbin/rdsetroot/rdsetroot.8,v > retrieving revision 1.2 > diff -u -p -r1.2 rdsetroot.8 > --- rdsetroot.8 5 Apr 2019 21:44:32 - 1.2 > +++ rdsetroot.8 20 Jan 2023 16:58:56 - > @@ -45,6 +45,11 @@ Debug. > Rather than inserting, extract the > .Ar disk.fs > image. > +If > +.Ar disk.fs > +is not speficied, s/speficied/specified otherwise ok.
Re: rdsetroot.8: sync synopsis with usage, improve wording
ok by me.
jmc
On 20 January 2023 17:00:37 GMT, Klemens Nanni wrote:
>Alright, sorry for the noise.
>
>Is this minimal sync plus stdout mention fine?
>
>Index: rdsetroot.8
>===
>RCS file: /cvs/src/usr.sbin/rdsetroot/rdsetroot.8,v
>retrieving revision 1.2
>diff -u -p -r1.2 rdsetroot.8
>--- rdsetroot.85 Apr 2019 21:44:32 - 1.2
>+++ rdsetroot.820 Jan 2023 16:58:56 -
>@@ -45,6 +45,11 @@ Debug.
> Rather than inserting, extract the
> .Ar disk.fs
> image.
>+If
>+.Ar disk.fs
>+is not speficied,
>+.Nm
>+writes to standard output.
> The disk can be made accessible using
> .Xr vnconfig 8 ,
> filesystems can be manipulated, and finally re-inserted into the RAMDISK
> kernel.
>Index: rdsetroot.c
>===
>RCS file: /cvs/src/usr.sbin/rdsetroot/rdsetroot.c,v
>retrieving revision 1.3
>diff -u -p -r1.3 rdsetroot.c
>--- rdsetroot.c24 Oct 2021 21:24:19 - 1.3
>+++ rdsetroot.c20 Jan 2023 16:58:58 -
>@@ -294,6 +294,6 @@ usage(void)
> {
> extern char *__progname;
>
>- fprintf(stderr, "usage: %s [-dx] bsd [fs]\n", __progname);
>+ fprintf(stderr, "usage: %s [-dx] kernel [disk.fs]\n", __progname);
> exit(1);
> }
>
Re: rdsetroot.8: sync synopsis with usage, improve wording
Alright, sorry for the noise.
Is this minimal sync plus stdout mention fine?
Index: rdsetroot.8
===
RCS file: /cvs/src/usr.sbin/rdsetroot/rdsetroot.8,v
retrieving revision 1.2
diff -u -p -r1.2 rdsetroot.8
--- rdsetroot.8 5 Apr 2019 21:44:32 - 1.2
+++ rdsetroot.8 20 Jan 2023 16:58:56 -
@@ -45,6 +45,11 @@ Debug.
Rather than inserting, extract the
.Ar disk.fs
image.
+If
+.Ar disk.fs
+is not speficied,
+.Nm
+writes to standard output.
The disk can be made accessible using
.Xr vnconfig 8 ,
filesystems can be manipulated, and finally re-inserted into the RAMDISK
kernel.
Index: rdsetroot.c
===
RCS file: /cvs/src/usr.sbin/rdsetroot/rdsetroot.c,v
retrieving revision 1.3
diff -u -p -r1.3 rdsetroot.c
--- rdsetroot.c 24 Oct 2021 21:24:19 - 1.3
+++ rdsetroot.c 20 Jan 2023 16:58:58 -
@@ -294,6 +294,6 @@ usage(void)
{
extern char *__progname;
- fprintf(stderr, "usage: %s [-dx] bsd [fs]\n", __progname);
+ fprintf(stderr, "usage: %s [-dx] kernel [disk.fs]\n", __progname);
exit(1);
}
Re: Inconsistent isdigit(3) man page
On Fri, 20 Jan 2023 09:32:38 -0700, Bob Beck wrote: > So isdigit(3) says in the first paragraph that > > 'The complete list of decimal digits is 0 and 1-9, in any locale.' > > Later on it says: > > 'On systems supporting non-ASCII single-byte character encodings, > different c arguments may correspond to the digits, and the results of > isdigit() may depend on the LC_CTYPE locale(1).' > > Argubly worring about non ASCII single byte character encodings > doesn't make sense for an OpenBSD man page, but setting that aside for > a minute it's the second part that is of concern.. "It may depend on > the LC_CTYPE locale()" - Ok, well does it or doesn't it? On OpenBSD isdigit() never uses the locale, but you already knew that. > Various spec docs seem all over the place on this, so I am also > paging Dr. Posix in this email... Hi Philip! :) Is isdigit() > safe from being screwed up by locale or not? POSIX says that isdigit() may be influenced by the locale. Since LC_CTYPE defines the different character classes, of which "digit" is one, I think that part of the manual is correct as-is. However, perhaps we should make isdigit(3) match isalpha(3) like so: Index: lib/libc/gen/isdigit.3 === RCS file: /cvs/src/lib/libc/gen/isdigit.3,v retrieving revision 1.13 diff -u -p -u -r1.13 isdigit.3 --- lib/libc/gen/isdigit.3 11 Sep 2022 06:38:10 - 1.13 +++ lib/libc/gen/isdigit.3 20 Jan 2023 16:58:20 - @@ -51,7 +51,12 @@ The and .Fn isdigit_l functions test for any decimal-digit character. -The complete list of decimal digits is 0 and 1\(en9, in any locale. +In the C locale, the complete list of decimal digits is 0 and 1\(en9. +.Ox +always uses the C locale for these functions, +ignoring the global locale, the thread-specific locale, and the +.Fa locale +argument. .Sh RETURN VALUES These functions return zero if the character tests false or non-zero if the character tests true.
Inconsistent isdigit(3) man page
So isdigit(3) says in the first paragraph that 'The complete list of decimal digits is 0 and 1-9, in any locale.' Later on it says: 'On systems supporting non-ASCII single-byte character encodings, different c arguments may correspond to the digits, and the results of isdigit() may depend on the LC_CTYPE locale(1).' Argubly worring about non ASCII single byte character encodings doesn't make sense for an OpenBSD man page, but setting that aside for a minute it's the second part that is of concern.. "It may depend on the LC_CTYPE locale()" - Ok, well does it or doesn't it? Various spec docs seem all over the place on this, so I am also paging Dr. Posix in this email... Hi Philip! :) Is isdigit() safe from being screwed up by locale or not? Should it be as below? Thoughts? Index: isdigit.3 === RCS file: /cvs/src/lib/libc/gen/isdigit.3,v retrieving revision 1.13 diff -u -p -u -p -r1.13 isdigit.3 --- isdigit.3 11 Sep 2022 06:38:10 - 1.13 +++ isdigit.3 20 Jan 2023 16:23:08 - @@ -59,11 +59,7 @@ non-zero if the character tests true. On systems supporting non-ASCII single-byte character encodings, different .Fa c -arguments may correspond to the digits, and the results of -.Fn isdigit -may depend on the -.Ev LC_CTYPE -.Xr locale 1 . +arguments may correspond to the digits. .Sh SEE ALSO .Xr isalnum 3 , .Xr isalpha 3 ,
Re: adjust bgpd aspa-set format
On Fri, Jan 20, 2023 at 03:38:45PM +0100, Claudio Jeker wrote:
> This diff removes the extra "allow" from the aspa-set provider-set element
> spec. The allow is not needed and confuses more than it helps.
>
> This change adjusts the parser, printconf, rpki-client and the regress
> tests. Job and I decided that the filters will use avs (ASPA validation
> state) as keyword, so adjust that as well.
>
> Also try to document the aspa-set in bgpd.conf.5
ok
> Index: usr.sbin/bgpd/bgpd.conf.5
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.conf.5,v
> retrieving revision 1.228
> diff -u -p -r1.228 bgpd.conf.5
> --- usr.sbin/bgpd/bgpd.conf.5 4 Jan 2023 14:33:30 - 1.228
> +++ usr.sbin/bgpd/bgpd.conf.5 20 Jan 2023 14:30:27 -
> @@ -426,12 +426,16 @@ may be defined, against which
> will validate the origin of each prefix.
> The
> .Ic roa-set
> -is merged with the tables received via
> +and
I would use "and the"
> +.Ic aspa-set
> +are merged with the corresponding tables received via
> .Ic rtr
> sessions.
> .Pp
> A set definition can span multiple lines, and an optional comma is allowed
> between elements.
> +The same set can be defined more than once, in this case the definitions are
> +merged into one common set.
> .Pp
> .Bl -tag -width Ds -compact
> .It Xo
> @@ -443,6 +447,30 @@ An
> stores AS numbers, and can be used with the AS specific parameter in
> .Sx FILTER
> rules.
> +.Pp
> +.It Xo
> +.Ic aspa-set
> +.Ic { Ic customer-as Ar as-number
> +.Op Ic expires Ar seconds
> +.Ic provider-as Ic { Ar as-number
> +.Op Ic inet Ns | Ns Ic inet6
> +.Ic ... Ic } ... Ic }
> +.Xc
> +The
> +.Ic aspa-set
> +holds a collection of
> +.Em Validated ASPA Payloads Pq VAPs .
> +Each as AS_PATH received from an eBGP peer is checked against the
> +.Ic aspa-set ,
> +and the ASAP Validation State (AVS) is set.
ASPA
> +.Ic expires
> +can be set to the seconds since Epoch until when this VAP is valid.
> +.Bd -literal -offset indent
> +roa-set {
> + customer-as 64511 provider-as { 64496 65496 }
> + customer-as 64496 provider-as { 65496 64544 }
> +}
> +.Ed
> .Pp
> .It Xo
> .Ic origin-set Ar name
Re: rdsetroot.8: sync synopsis with usage, improve wording
On Fri, Jan 20, 2023 at 02:51:31PM +, Jason McIntyre wrote: > On Fri, Jan 20, 2023 at 12:35:05PM +, Klemens Nanni wrote: > > 19.01.2023 19:11, Jason McIntyre ??: > > > On Thu, Jan 19, 2023 at 06:50:14PM +, Klemens Nanni wrote: > > >> $ man -h rdsetroot > > >> rdsetroot [-dx] kernel [disk.fs] > > >> vs. > > >> $ rdsetroot > > >> usage: rdsetroot [-dx] bsd [fs] > > >> > > > > > > i have to say i think the man page has better argument names, but i've > > > nothing against your changes. just as long as they match. > > > > Either way is fine, just let them match. > > > > >> Clarify that -x uses stdout (could be a default file like ktrace(1) does) > > >> and switch 'standard in/output' to more common '.Va stdin/out' like the > > >> related vnconfig(8) does. > > >> > > > > > > i'm fine with your diff, but i think that "reads from standard input" is > > > much easier to understand than the change you propose. > > > > Sure, new version with both points incorporated. > > > > Feedback? OK? > > > > > > > > jmc > > > > > >> Explaining how disk images can round trip through rdsetroot and vnconfig > > >> is vaguely useful, imho, even less so in the description of -x, so > > >> briefly > > >> mention the relation and let readers follow the reference for more. > > > > > > Index: rdsetroot.8 > > === > > RCS file: /cvs/src/usr.sbin/rdsetroot/rdsetroot.8,v > > retrieving revision 1.2 > > diff -u -p -r1.2 rdsetroot.8 > > --- rdsetroot.8 5 Apr 2019 21:44:32 - 1.2 > > +++ rdsetroot.8 20 Jan 2023 12:33:31 - > > @@ -24,30 +24,29 @@ > > .Nm rdsetroot > > .Op Fl dx > > .Ar kernel > > -.Op Ar disk.fs > > +.Op Ar disk > > well, disk.fs and your original proposal (fs) both hinted that you want > a filesystem. i'm less sure about "disk". you aren;t concerned about > that? i really don;t see an issue with how it is now. I agree. Just "disk" isn't very clear that it's intended to be a filesystem image. > > > .Sh DESCRIPTION > > The > > .Nm > > -utility inserts the file > > -.Ar disk.fs > > -into the reserved space inside a RAMDISK kernel. > > +utility inserts the disk image > > +.Ar disk > > +into the reserved space inside the RAMDISK kernel > > +.Ar bsd . > > if you stick with "kernel" you'll need to make that change here too. It reads better as "a RAMDISK kernel", rather than, "the RAMDISK kernel", unless you also change it to "the specified RAMDISK kernel". Overall, I think that this diff reduces the readability of the manual page rather than improves it. The useful parts are to make the argument names match between manual and command, and to clarify that -x writes to standard output if no output file is specified. The rest is just churn. I'm doubtful that it's even useful to explicitly state that the image can be manipulated and then re-inserted. It's fairly obvious.
Re: rdsetroot.8: sync synopsis with usage, improve wording
On Fri, Jan 20, 2023 at 12:35:05PM +, Klemens Nanni wrote:
> 19.01.2023 19:11, Jason McIntyre ??:
> > On Thu, Jan 19, 2023 at 06:50:14PM +, Klemens Nanni wrote:
> >>$ man -h rdsetroot
> >>rdsetroot [-dx] kernel [disk.fs]
> >> vs.
> >>$ rdsetroot
> >>usage: rdsetroot [-dx] bsd [fs]
> >>
> >
> > i have to say i think the man page has better argument names, but i've
> > nothing against your changes. just as long as they match.
>
> Either way is fine, just let them match.
>
> >> Clarify that -x uses stdout (could be a default file like ktrace(1) does)
> >> and switch 'standard in/output' to more common '.Va stdin/out' like the
> >> related vnconfig(8) does.
> >>
> >
> > i'm fine with your diff, but i think that "reads from standard input" is
> > much easier to understand than the change you propose.
>
> Sure, new version with both points incorporated.
>
> Feedback? OK?
>
> >
> > jmc
> >
> >> Explaining how disk images can round trip through rdsetroot and vnconfig
> >> is vaguely useful, imho, even less so in the description of -x, so briefly
> >> mention the relation and let readers follow the reference for more.
>
>
> Index: rdsetroot.8
> ===
> RCS file: /cvs/src/usr.sbin/rdsetroot/rdsetroot.8,v
> retrieving revision 1.2
> diff -u -p -r1.2 rdsetroot.8
> --- rdsetroot.8 5 Apr 2019 21:44:32 - 1.2
> +++ rdsetroot.8 20 Jan 2023 12:33:31 -
> @@ -24,30 +24,29 @@
> .Nm rdsetroot
> .Op Fl dx
> .Ar kernel
> -.Op Ar disk.fs
> +.Op Ar disk
well, disk.fs and your original proposal (fs) both hinted that you want
a filesystem. i'm less sure about "disk". you aren;t concerned about
that? i really don;t see an issue with how it is now.
> .Sh DESCRIPTION
> The
> .Nm
> -utility inserts the file
> -.Ar disk.fs
> -into the reserved space inside a RAMDISK kernel.
> +utility inserts the disk image
> +.Ar disk
> +into the reserved space inside the RAMDISK kernel
> +.Ar bsd .
if you stick with "kernel" you'll need to make that change here too.
jmc
> If
> -.Ar disk.fs
> -is not specified,
> -.Nm
> -reads from standard input.
> +.Ar disk
> +is not specified, standard input is used.
> +Disk images can be used with
> +.Xr vnconfig 8 .
> .Pp
> The options are as follows:
> .Bl -tag -width Ds
> .It Fl d
> Debug.
> .It Fl x
> -Rather than inserting, extract the
> -.Ar disk.fs
> -image.
> -The disk can be made accessible using
> -.Xr vnconfig 8 ,
> -filesystems can be manipulated, and finally re-inserted into the RAMDISK
> kernel.
> +Extract the disk image.
> +If
> +.Ar disk
> +is not specified, standard output is used.
> .El
> .Sh SEE ALSO
> .Xr config 8 ,
> Index: rdsetroot.c
> ===
> RCS file: /cvs/src/usr.sbin/rdsetroot/rdsetroot.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 rdsetroot.c
> --- rdsetroot.c 24 Oct 2021 21:24:19 - 1.3
> +++ rdsetroot.c 20 Jan 2023 12:33:33 -
> @@ -294,6 +294,6 @@ usage(void)
> {
> extern char *__progname;
>
> - fprintf(stderr, "usage: %s [-dx] bsd [fs]\n", __progname);
> + fprintf(stderr, "usage: %s [-dx] kernel [disk]\n", __progname);
> exit(1);
> }
>
adjust bgpd aspa-set format
This diff removes the extra "allow" from the aspa-set provider-set element
spec. The allow is not needed and confuses more than it helps.
This change adjusts the parser, printconf, rpki-client and the regress
tests. Job and I decided that the filters will use avs (ASPA validation
state) as keyword, so adjust that as well.
Also try to document the aspa-set in bgpd.conf.5
--
:wq Claudio
? BUILDTIME
Index: regress/usr.sbin/bgpd/config/bgpd.conf.14.in
===
RCS file: /cvs/src/regress/usr.sbin/bgpd/config/bgpd.conf.14.in,v
retrieving revision 1.1
diff -u -p -r1.1 bgpd.conf.14.in
--- regress/usr.sbin/bgpd/config/bgpd.conf.14.in18 Nov 2022 10:26:04
- 1.1
+++ regress/usr.sbin/bgpd/config/bgpd.conf.14.in20 Jan 2023 14:30:25
-
@@ -27,10 +27,10 @@ aspa-set {
aspa-set {
customer-as 3 provider-as { 5 }
customer-as 2 expires 1668181648 provider-as { 3 4 }
- customer-as 5 provider-as { 1 2 allow inet 7 allow inet6 }
+ customer-as 5 provider-as { 1, 2 inet, 7 inet6 }
}
-#match from any aspa unknown
-#match from any aspa invalid
-#match from any aspa valid
+#match from any avs unknown
+#match from any avs invalid
+#match from any avs valid
Index: regress/usr.sbin/bgpd/config/bgpd.conf.14.ok
===
RCS file: /cvs/src/regress/usr.sbin/bgpd/config/bgpd.conf.14.ok,v
retrieving revision 1.1
diff -u -p -r1.1 bgpd.conf.14.ok
--- regress/usr.sbin/bgpd/config/bgpd.conf.14.ok18 Nov 2022 10:26:04
- 1.1
+++ regress/usr.sbin/bgpd/config/bgpd.conf.14.ok20 Jan 2023 14:30:25
-
@@ -12,7 +12,7 @@ aspa-set {
customer-as 1 provider-as { 2 3 4 5 6 }
customer-as 2 expires 1668181648 provider-as { 3 4 }
customer-as 3 provider-as { 5 }
- customer-as 5 provider-as { 1 2 allow inet 7 allow inet6 }
+ customer-as 5 provider-as { 1 2 inet 7 inet6 }
customer-as 17 provider-as { 12 }
customer-as 41 provider-as { 2 }
customer-as 42 expires 12345 provider-as { 3 4 }
Index: usr.sbin/bgpd/bgpd.conf.5
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.conf.5,v
retrieving revision 1.228
diff -u -p -r1.228 bgpd.conf.5
--- usr.sbin/bgpd/bgpd.conf.5 4 Jan 2023 14:33:30 - 1.228
+++ usr.sbin/bgpd/bgpd.conf.5 20 Jan 2023 14:30:27 -
@@ -426,12 +426,16 @@ may be defined, against which
will validate the origin of each prefix.
The
.Ic roa-set
-is merged with the tables received via
+and
+.Ic aspa-set
+are merged with the corresponding tables received via
.Ic rtr
sessions.
.Pp
A set definition can span multiple lines, and an optional comma is allowed
between elements.
+The same set can be defined more than once, in this case the definitions are
+merged into one common set.
.Pp
.Bl -tag -width Ds -compact
.It Xo
@@ -443,6 +447,30 @@ An
stores AS numbers, and can be used with the AS specific parameter in
.Sx FILTER
rules.
+.Pp
+.It Xo
+.Ic aspa-set
+.Ic { Ic customer-as Ar as-number
+.Op Ic expires Ar seconds
+.Ic provider-as Ic { Ar as-number
+.Op Ic inet Ns | Ns Ic inet6
+.Ic ... Ic } ... Ic }
+.Xc
+The
+.Ic aspa-set
+holds a collection of
+.Em Validated ASPA Payloads Pq VAPs .
+Each as AS_PATH received from an eBGP peer is checked against the
+.Ic aspa-set ,
+and the ASAP Validation State (AVS) is set.
+.Ic expires
+can be set to the seconds since Epoch until when this VAP is valid.
+.Bd -literal -offset indent
+roa-set {
+ customer-as 64511 provider-as { 64496 65496 }
+ customer-as 64496 provider-as { 65496 64544 }
+}
+.Ed
.Pp
.It Xo
.Ic origin-set Ar name
Index: usr.sbin/bgpd/parse.y
===
RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
retrieving revision 1.438
diff -u -p -r1.438 parse.y
--- usr.sbin/bgpd/parse.y 4 Jan 2023 14:33:30 - 1.438
+++ usr.sbin/bgpd/parse.y 20 Jan 2023 14:30:27 -
@@ -636,11 +636,11 @@ aspa_tas : as4number_any {
$$->aid = AID_UNSPEC;
$$->num = 1;
}
- | as4number_any ALLOW family {
+ | as4number_any family {
if (($$ = calloc(1, sizeof(*$$))) == NULL)
fatal(NULL);
$$->as = $1;
- $$->aid = $3;
+ $$->aid = $2;
$$->num = 1;
}
;
Index: usr.sbin/bgpd/printconf.c
===
RCS file: /cvs/src/usr.sbin/bgpd/printconf.c,v
retrieving revision 1.161
diff -u -p -r1.161 printconf.c
--- usr.sbin/bgpd/printconf.c 4 Jan 2023 14:33:30 - 1.161
+++ usr.sbin/bgpd/printconf.c 20 Jan 2023 14:30:27 -
@@ -609,9 +609,8 @@ print_aspa(struct aspa_tree *a)
ipsec(4): remove unused `ipsec_policy_head' all policies list
We link all policies to this list, but don't use it for any purpose.
Index: sys/net/pfkeyv2.c
===
RCS file: /cvs/src/sys/net/pfkeyv2.c,v
retrieving revision 1.255
diff -u -p -r1.255 pfkeyv2.c
--- sys/net/pfkeyv2.c 8 Jan 2023 10:26:36 - 1.255
+++ sys/net/pfkeyv2.c 20 Jan 2023 13:39:02 -
@@ -2012,7 +2012,6 @@ pfkeyv2_dosend(struct socket *so, void *
NET_UNLOCK();
goto ret;
}
- TAILQ_INSERT_HEAD(&ipsec_policy_head, ipo, ipo_list);
ipsec_in_use++;
} else {
ipo->ipo_last_searched = ipo->ipo_flags = 0;
Index: sys/netinet/ip_ipsp.c
===
RCS file: /cvs/src/sys/netinet/ip_ipsp.c,v
retrieving revision 1.275
diff -u -p -r1.275 ip_ipsp.c
--- sys/netinet/ip_ipsp.c 11 Nov 2022 18:09:58 - 1.275
+++ sys/netinet/ip_ipsp.c 20 Jan 2023 13:39:02 -
@@ -111,8 +111,6 @@ struct pool tdb_pool;
u_int32_t ipsec_ids_next_flow = 1; /* [F] may not be zero */
struct ipsec_ids_tree ipsec_ids_tree; /* [F] */
struct ipsec_ids_flows ipsec_ids_flows;/* [F] */
-struct ipsec_policy_head ipsec_policy_head =
-TAILQ_HEAD_INITIALIZER(ipsec_policy_head);
void ipsp_ids_gc(void *);
Index: sys/netinet/ip_ipsp.h
===
RCS file: /cvs/src/sys/netinet/ip_ipsp.h,v
retrieving revision 1.240
diff -u -p -r1.240 ip_ipsp.h
--- sys/netinet/ip_ipsp.h 14 Jul 2022 13:52:10 - 1.240
+++ sys/netinet/ip_ipsp.h 20 Jan 2023 13:39:02 -
@@ -293,7 +293,6 @@ struct ipsec_policy {
struct ipsec_acquire_head ipo_acquires; /* [A] List of acquires */
TAILQ_ENTRY(ipsec_policy) ipo_tdb_next; /* [P] List TDB policies */
- TAILQ_ENTRY(ipsec_policy) ipo_list; /* List of all policies */
};
#defineIPSP_POLICY_NONE0x /* No flags set */
@@ -565,8 +564,6 @@ extern int ipsec_exp_first_use; /* seco
extern char ipsec_def_enc[];
extern char ipsec_def_auth[];
extern char ipsec_def_comp[];
-
-extern TAILQ_HEAD(ipsec_policy_head, ipsec_policy) ipsec_policy_head;
extern struct mutex tdb_sadb_mtx;
extern struct mutex ipo_tdb_mtx;
Index: sys/netinet/ip_spd.c
===
RCS file: /cvs/src/sys/netinet/ip_spd.c,v
retrieving revision 1.117
diff -u -p -r1.117 ip_spd.c
--- sys/netinet/ip_spd.c17 Jun 2022 13:40:21 - 1.117
+++ sys/netinet/ip_spd.c20 Jan 2023 13:39:02 -
@@ -697,8 +697,6 @@ ipsec_delete_policy(struct ipsec_policy
ipsp_delete_acquire_locked(ipa);
mtx_leave(&ipsec_acquire_mtx);
- TAILQ_REMOVE(&ipsec_policy_head, ipo, ipo_list);
-
if (ipo->ipo_ids)
ipsp_ids_free(ipo->ipo_ids);
Re: bgpd validate ASPATH with ASPA
On Fri, Jan 20, 2023 at 12:21:14PM +0100, Claudio Jeker wrote:
> This diff adds the reload logic and rewrites larger parts of what was
> already there to have ASPA validation in the RDE.
>
> The main reason this diff is so large is that the ASPA state cache on
> struct rde_aspath needs to be afi/aid and role independent. So I changed
> the aspa functions to be role and aid independent which results in a lot
> of churn.
>
> The code now uses rde_aspa_validity() with the cache in rde_aspath to
> figure out if a prefix is ASPA valid, invalid or unknown.
> rde_aspa_validity() is cheap since it just checks various bits to decide.
> The cache is updated by checking a generation counter that is increased
> during reload. This is done since the tables are walked by prefix and not
> by ASPATH.
>
> There is still no filter syntax available to deny aspa invalid routes but
> that will follow soon.
>
> The diff includes bgpd, bgpctl and regress test changes. There is a lot of
> churn in regress test because of bgpctl output changes.
I missed a small bit in the diff. In rde_filter_match() the state->vstate
needs to be masked with the ROA_MASK else the ovs validity will not match.
I added this to the big diff but just included the delta here.
--
:wq Claudio
Index: rde_filter.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde_filter.c,v
retrieving revision 1.131
diff -u -p -r1.131 rde_filter.c
--- rde_filter.c12 Jan 2023 17:35:51 - 1.131
+++ rde_filter.c20 Jan 2023 11:37:27 -
@@ -223,7 +223,7 @@ rde_filter_match(struct filter_rule *f,
return (0);
if (f->match.ovs.is_set) {
- if (state->vstate != f->match.ovs.validity)
+ if ((state->vstate & ROA_MASK) != f->match.ovs.validity)
return (0);
}
Re: rdsetroot.8: sync synopsis with usage, improve wording
19.01.2023 19:11, Jason McIntyre пишет:
> On Thu, Jan 19, 2023 at 06:50:14PM +, Klemens Nanni wrote:
>> $ man -h rdsetroot
>> rdsetroot [-dx] kernel [disk.fs]
>> vs.
>> $ rdsetroot
>> usage: rdsetroot [-dx] bsd [fs]
>>
>
> i have to say i think the man page has better argument names, but i've
> nothing against your changes. just as long as they match.
Either way is fine, just let them match.
>> Clarify that -x uses stdout (could be a default file like ktrace(1) does)
>> and switch 'standard in/output' to more common '.Va stdin/out' like the
>> related vnconfig(8) does.
>>
>
> i'm fine with your diff, but i think that "reads from standard input" is
> much easier to understand than the change you propose.
Sure, new version with both points incorporated.
Feedback? OK?
>
> jmc
>
>> Explaining how disk images can round trip through rdsetroot and vnconfig
>> is vaguely useful, imho, even less so in the description of -x, so briefly
>> mention the relation and let readers follow the reference for more.
Index: rdsetroot.8
===
RCS file: /cvs/src/usr.sbin/rdsetroot/rdsetroot.8,v
retrieving revision 1.2
diff -u -p -r1.2 rdsetroot.8
--- rdsetroot.8 5 Apr 2019 21:44:32 - 1.2
+++ rdsetroot.8 20 Jan 2023 12:33:31 -
@@ -24,30 +24,29 @@
.Nm rdsetroot
.Op Fl dx
.Ar kernel
-.Op Ar disk.fs
+.Op Ar disk
.Sh DESCRIPTION
The
.Nm
-utility inserts the file
-.Ar disk.fs
-into the reserved space inside a RAMDISK kernel.
+utility inserts the disk image
+.Ar disk
+into the reserved space inside the RAMDISK kernel
+.Ar bsd .
If
-.Ar disk.fs
-is not specified,
-.Nm
-reads from standard input.
+.Ar disk
+is not specified, standard input is used.
+Disk images can be used with
+.Xr vnconfig 8 .
.Pp
The options are as follows:
.Bl -tag -width Ds
.It Fl d
Debug.
.It Fl x
-Rather than inserting, extract the
-.Ar disk.fs
-image.
-The disk can be made accessible using
-.Xr vnconfig 8 ,
-filesystems can be manipulated, and finally re-inserted into the RAMDISK
kernel.
+Extract the disk image.
+If
+.Ar disk
+is not specified, standard output is used.
.El
.Sh SEE ALSO
.Xr config 8 ,
Index: rdsetroot.c
===
RCS file: /cvs/src/usr.sbin/rdsetroot/rdsetroot.c,v
retrieving revision 1.3
diff -u -p -r1.3 rdsetroot.c
--- rdsetroot.c 24 Oct 2021 21:24:19 - 1.3
+++ rdsetroot.c 20 Jan 2023 12:33:33 -
@@ -294,6 +294,6 @@ usage(void)
{
extern char *__progname;
- fprintf(stderr, "usage: %s [-dx] bsd [fs]\n", __progname);
+ fprintf(stderr, "usage: %s [-dx] kernel [disk]\n", __progname);
exit(1);
}
bgpd validate ASPATH with ASPA
This diff adds the reload logic and rewrites larger parts of what was
already there to have ASPA validation in the RDE.
The main reason this diff is so large is that the ASPA state cache on
struct rde_aspath needs to be afi/aid and role independent. So I changed
the aspa functions to be role and aid independent which results in a lot
of churn.
The code now uses rde_aspa_validity() with the cache in rde_aspath to
figure out if a prefix is ASPA valid, invalid or unknown.
rde_aspa_validity() is cheap since it just checks various bits to decide.
The cache is updated by checking a generation counter that is increased
during reload. This is done since the tables are walked by prefix and not
by ASPATH.
There is still no filter syntax available to deny aspa invalid routes but
that will follow soon.
The diff includes bgpd, bgpctl and regress test changes. There is a lot of
churn in regress test because of bgpctl output changes.
--
:wq Claudio
Index: usr.sbin/bgpd/bgpd.h
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
retrieving revision 1.458
diff -u -p -r1.458 bgpd.h
--- usr.sbin/bgpd/bgpd.h17 Jan 2023 16:09:01 - 1.458
+++ usr.sbin/bgpd/bgpd.h20 Jan 2023 08:18:06 -
@@ -840,7 +840,8 @@ struct ctl_show_rib {
uint32_tflags;
uint8_t prefixlen;
uint8_t origin;
- uint8_t validation_state;
+ uint8_t roa_validation_state;
+ uint8_t aspa_validation_state;
int8_t dmetric;
/* plus an aspath */
};
Index: usr.sbin/bgpd/rde.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.589
diff -u -p -r1.589 rde.c
--- usr.sbin/bgpd/rde.c 18 Jan 2023 17:40:17 - 1.589
+++ usr.sbin/bgpd/rde.c 20 Jan 2023 10:56:39 -
@@ -61,7 +61,7 @@ uint8_trde_attr_missing(struct rde_as
int rde_get_mp_nexthop(u_char *, uint16_t, uint8_t,
struct filterstate *);
voidrde_as4byte_fixup(struct rde_peer *, struct rde_aspath *);
-uint8_t rde_aspa_validation(struct rde_peer *, struct
rde_aspath *,
+uint8_t rde_aspa_validity(struct rde_peer *, struct rde_aspath
*,
uint8_t);
voidrde_reflector(struct rde_peer *, struct rde_aspath *);
@@ -82,8 +82,9 @@ static voidrde_softreconfig_in(struct
static void rde_softreconfig_sync_reeval(struct rib_entry *, void *);
static void rde_softreconfig_sync_fib(struct rib_entry *, void *);
static void rde_softreconfig_sync_done(void *, uint8_t);
-static void rde_roa_reload(void);
-static void rde_aspa_reload(void);
+static void rde_rpki_reload(void);
+static int rde_roa_reload(void);
+static int rde_aspa_reload(void);
int rde_update_queue_pending(void);
voidrde_update_queue_runner(void);
voidrde_update6_queue_runner(uint8_t);
@@ -111,6 +112,7 @@ static struct imsgbuf *ibuf_main;
static struct bgpd_config *conf, *nconf;
static struct rde_prefixset rde_roa, roa_new;
static struct rde_aspa *rde_aspa, *aspa_new;
+static uint8_t rde_aspa_generation;
volatile sig_atomic_t rde_quit = 0;
struct filter_head *out_rules, *out_rules_tmp;
@@ -1162,8 +1164,8 @@ rde_dispatch_imsg_rtr(struct imsgbuf *ib
break;
case IMSG_RECONF_DONE:
/* end of update */
- rde_roa_reload();
- rde_aspa_reload();
+ if (rde_roa_reload() + rde_aspa_reload() != 0)
+ rde_rpki_reload();
break;
}
imsg_free(&imsg);
@@ -1364,6 +1366,14 @@ rde_update_dispatch(struct rde_peer *pee
state.aspath.flags |= F_ATTR_LOOP;
rde_reflector(peer, &state.aspath);
+
+ /* Cache aspa lookup for all updates from ebgp sessions. */
+ if (state.aspath.flags & F_ATTR_ASPATH &&
+ peer->conf.ebgp) {
+ aspa_validation(rde_aspa, state.aspath.aspath,
+ &state.aspath.aspa_state);
+ state.aspath.aspa_generation = rde_aspa_generation;
+ }
}
p = imsg->data;
@@ -1525,10 +1535,6 @@ rde_update_dispatch(struct rde_peer *pee
NULL, 0);
goto done;
}
-#if NOTYET
- state.aspath.aspa_state = rde_aspa_validation(peer,
- &state.aspath, AID_INET);
-#endif
}
while (nlri_len > 0) {
if (peer_has_add_path(peer, AID_INET, CAPA_AP_RECV)) {
@@ -1601,10 +1607,6 @@ rde_update_dispa
Re: mem.4: be more accurate about securelevel
On 2023/01/18 12:46, Theo de Raadt wrote: > But you should not start a sentence with also. > Also you should not start a sentence with but. > > Not the best english. jmc can weight in perhaps. > > Jan Klemkow wrote: > > .Pp > > Even with sufficient file system permissions, > > these devices can only be opened when the > > -.Xr securelevel 7 > > -is insecure or when the > > .Va kern.allowkmem > > .Xr sysctl 2 > > variable is set. > > +Also the > > +.Xr securelevel 7 > > +insecure is needed, to open the device writable. This is all that's needed isn't it? Even with sufficient file system permissions, these devices can only be opened when the .Xr securelevel 7 -is insecure or when the -is insecure and the .Va kern.allowkmem .Xr sysctl 2 variable is set.
