Re: Remove ENGINE use from relayd
On Thu, Jul 13, 2023 at 05:44:03AM +0200, Theo Buehler wrote: > This is analogous to the change that op committed to smtpd a few days > ago. Instead of using ENGINE to make RSA use privsep via imsg, create > an RSA method that has custom priv_enc/priv_dec methods, replace the > default RSA method. Ditch numerous wrappers that extract the default > methods on the fly only to add a log call. > > This removes a lot of boilerplate and shows more clearly where the > actual magic happens. Regress exercises this code and passes. Nice, that is a lot of boilerplate. ok tobhe@ > > Index: ca.c > === > RCS file: /cvs/src/usr.sbin/relayd/ca.c,v > retrieving revision 1.42 > diff -u -p -r1.42 ca.c > --- ca.c 11 Jun 2023 10:30:26 - 1.42 > +++ ca.c 11 Jul 2023 18:21:47 - > @@ -41,20 +41,8 @@ voidca_launch(void); > int ca_dispatch_parent(int, struct privsep_proc *, struct imsg *); > int ca_dispatch_relay(int, struct privsep_proc *, struct imsg *); > > -int rsae_pub_enc(int, const u_char *, u_char *, RSA *, int); > -int rsae_pub_dec(int,const u_char *, u_char *, RSA *, int); > int rsae_priv_enc(int, const u_char *, u_char *, RSA *, int); > int rsae_priv_dec(int, const u_char *, u_char *, RSA *, int); > -int rsae_mod_exp(BIGNUM *, const BIGNUM *, RSA *, BN_CTX *); > -int rsae_bn_mod_exp(BIGNUM *, const BIGNUM *, const BIGNUM *, > - const BIGNUM *, BN_CTX *, BN_MONT_CTX *); > -int rsae_init(RSA *); > -int rsae_finish(RSA *); > -int rsae_sign(int, const u_char *, u_int, u_char *, u_int *, > - const RSA *); > -int rsae_verify(int dtype, const u_char *m, u_int, const u_char *, > - u_int, const RSA *); > -int rsae_keygen(RSA *, int, BIGNUM *, BN_GENCB *); > > static struct relayd *env = NULL; > > @@ -301,7 +289,7 @@ ca_dispatch_relay(int fd, struct privsep > * RSA privsep engine (called from unprivileged processes) > */ > > -const RSA_METHOD *rsa_default = NULL; > +static const RSA_METHOD *rsa_default; > static RSA_METHOD *rsae_method; > > static int > @@ -417,20 +405,6 @@ rsae_send_imsg(int flen, const u_char *f > } > > int > -rsae_pub_enc(int flen,const u_char *from, u_char *to, RSA *rsa,int padding) > -{ > - DPRINTF("%s:%d", __func__, __LINE__); > - return RSA_meth_get_pub_enc(rsa_default)(flen, from, to, rsa, padding); > -} > - > -int > -rsae_pub_dec(int flen,const u_char *from, u_char *to, RSA *rsa,int padding) > -{ > - DPRINTF("%s:%d", __func__, __LINE__); > - return RSA_meth_get_pub_dec(rsa_default)(flen, from, to, rsa, padding); > -} > - > -int > rsae_priv_enc(int flen, const u_char *from, u_char *to, RSA *rsa, int > padding) > { > DPRINTF("%s:%d", __func__, __LINE__); > @@ -444,69 +418,10 @@ rsae_priv_dec(int flen, const u_char *fr > return rsae_send_imsg(flen, from, to, rsa, padding, IMSG_CA_PRIVDEC); > } > > -int > -rsae_mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) > -{ > - DPRINTF("%s:%d", __func__, __LINE__); > - return RSA_meth_get_mod_exp(rsa_default)(r0, I, rsa, ctx); > -} > - > -int > -rsae_bn_mod_exp(BIGNUM *r, const BIGNUM *a, const BIGNUM *p, > -const BIGNUM *m, BN_CTX *ctx, BN_MONT_CTX *m_ctx) > -{ > - DPRINTF("%s:%d", __func__, __LINE__); > - return RSA_meth_get_bn_mod_exp(rsa_default)(r, a, p, m, ctx, m_ctx); > -} > - > -int > -rsae_init(RSA *rsa) > -{ > - DPRINTF("%s:%d", __func__, __LINE__); > - if (RSA_meth_get_init(rsa_default) == NULL) > - return 1; > - return RSA_meth_get_init(rsa_default)(rsa); > -} > - > -int > -rsae_finish(RSA *rsa) > -{ > - DPRINTF("%s:%d", __func__, __LINE__); > - if (RSA_meth_get_finish(rsa_default) == NULL) > - return 1; > - return RSA_meth_get_finish(rsa_default)(rsa); > -} > - > -int > -rsae_sign(int type, const u_char *m, u_int m_length, u_char *sigret, > -u_int *siglen, const RSA *rsa) > -{ > - DPRINTF("%s:%d", __func__, __LINE__); > - return RSA_meth_get_sign(rsa_default)(type, m, m_length, > - sigret, siglen, rsa); > -} > - > -int > -rsae_verify(int dtype, const u_char *m, u_int m_length, const u_char *sigbuf, > -u_int siglen, const RSA *rsa) > -{ > - DPRINTF("%s:%d", __func__, __LINE__); > - return RSA_meth_get_verify(rsa_default)(dtype, m, m_length, > - sigbuf, siglen, rsa); > -} > - > -int > -rsae_keygen(RSA *rsa, int bits, BIGNUM *e, BN_GENCB *cb) > -{ > - DPRINTF("%s:%d", __func__, __LINE__); > - return RSA_meth_get_keygen(rsa_default)(rsa, bits, e, cb); > -} > - > void > ca_engine_init(struct relayd *x_env) > { > - ENGINE *e = NULL; > - const char *errstr, *name; > + const char *errstr; > > if (env == NULL) > env = x_env; > @@ -514,68 +429,25 @@ ca_engine_init(struct relayd *x_env) > if (rsa_default != NULL) > return; > > - if ((rsae_method = RSA_meth_ne
bgpd: cleanup mrt.c
This is a follow-up to use more of the new ibuf API to write the mrt message. This removes all of the DUMP_XYZ macros and replaces them with ibuf_add_nX() calls. Also unify the error handling by using goto fail; in all cases and use a more generic log_warn() there once. -- :wq Claudio Index: mrt.c === RCS file: /cvs/src/usr.sbin/bgpd/mrt.c,v retrieving revision 1.115 diff -u -p -r1.115 mrt.c --- mrt.c 12 Jul 2023 14:45:42 - 1.115 +++ mrt.c 13 Jul 2023 07:57:23 - @@ -46,44 +46,6 @@ int mrt_dump_hdr_se(struct ibuf **, stru int mrt_dump_hdr_rde(struct ibuf **, uint16_t type, uint16_t, uint32_t); int mrt_open(struct mrt *, time_t); -#define DUMP_BYTE(x, b) \ - do {\ - u_char t = (b);\ - if (ibuf_add((x), &t, sizeof(t)) == -1) { \ - log_warn("mrt_dump1: ibuf_add error"); \ - goto fail; \ - } \ - } while (0) - -#define DUMP_SHORT(x, s) \ - do {\ - uint16_tt; \ - t = htons((s)); \ - if (ibuf_add((x), &t, sizeof(t)) == -1) { \ - log_warn("mrt_dump2: ibuf_add error"); \ - goto fail; \ - } \ - } while (0) - -#define DUMP_LONG(x, l) \ - do {\ - uint32_tt; \ - t = htonl((l)); \ - if (ibuf_add((x), &t, sizeof(t)) == -1) { \ - log_warn("mrt_dump3: ibuf_add error"); \ - goto fail; \ - } \ - } while (0) - -#define DUMP_NLONG(x, l) \ - do {\ - uint32_tt = (l);\ - if (ibuf_add((x), &t, sizeof(t)) == -1) { \ - log_warn("mrt_dump4: ibuf_add error"); \ - goto fail; \ - } \ - } while (0) - #define RDEIDX 0 #define SEIDX 1 #define TYPE2IDX(x)((x == MRT_TABLE_DUMP ||\ @@ -248,13 +210,16 @@ mrt_dump_state(struct mrt *mrt, uint16_t 2 * sizeof(short), 0) == -1) return; - DUMP_SHORT(buf, old_state); - DUMP_SHORT(buf, new_state); + if (ibuf_add_n16(buf, old_state) == -1) + goto fail; + if (ibuf_add_n16(buf, new_state) == -1) + goto fail; ibuf_close(&mrt->wbuf, buf); return; fail: + log_warn("%s: ibuf error", __func__); ibuf_free(buf); } @@ -330,39 +295,48 @@ mrt_attr_dump(struct ibuf *buf, struct r return (-1); if (!v2) { if (aid2afi(nexthop->aid, &afi, &safi)) - return (-1); - DUMP_SHORT(nhbuf, afi); - DUMP_BYTE(nhbuf, safi); + goto fail; + if (ibuf_add_n16(nhbuf, afi) == -1) + goto fail; + if (ibuf_add_n8(nhbuf, safi) == -1) + goto fail; } switch (nexthop->aid) { case AID_INET6: - DUMP_BYTE(nhbuf, sizeof(struct in6_addr)); + if (ibuf_add_n8(nhbuf, sizeof(struct in6_addr)) == -1) + goto fail; if (ibuf_add(nhbuf, &nexthop->v6, sizeof(struct in6_addr)) == -1) goto fail; break; case AID_VPN_IPv4: - DUMP_BYTE(nhbuf, sizeof(uint64_t) + - sizeof(struct in_addr)); - DUMP_NLONG(nhbuf, 0); /* set RD to 0 */ - DUMP_NLONG(nhbuf, 0); -
Re: Remove ENGINE use from relayd
On 2023/07/13 05:44:03 +0200, Theo Buehler wrote: > This is analogous to the change that op committed to smtpd a few days > ago. Instead of using ENGINE to make RSA use privsep via imsg, create > an RSA method that has custom priv_enc/priv_dec methods, replace the > default RSA method. Ditch numerous wrappers that extract the default > methods on the fly only to add a log call. > > This removes a lot of boilerplate and shows more clearly where the > actual magic happens. Regress exercises this code and passes. nice to see all that redundant code go; ok op
Re: Remove ENGINE use from relayd
I for one welcome our new relayd maintainer!
Re: bgpd: cleanup mrt.c
On Thu, Jul 13, 2023 at 10:04:33AM +0200, Claudio Jeker wrote: > This is a follow-up to use more of the new ibuf API to write the mrt message. > > This removes all of the DUMP_XYZ macros and replaces them with > ibuf_add_nX() calls. Also unify the error handling by using > goto fail; in all cases and use a more generic log_warn() there once. The conversions all look correct, so that's ok. There are a few silent failures and a few double logs. The former should be avoided and I think this should be done in this diff. I'm not sure how much effort should be invested in fully avoiding the latter. It's a bit messy: The only caller mrt_dump_entry_v2_rib() already logs an ibuf failure on error, so there's no need to add a log to mrt_dump_entry_v2_rib()'s fail label. In mrt_dump_hdr_se() there is no log after the fail: label. I think it needs one, otherwise mrt_dump_{bgp_msg,state}() could fail silently. Most callers of mrt_dump_hdr_rde() will log an ibuf error on failure, so you probably want to remove the log in the early return after ibuf_dynamic. Also, add a log (or goto fail) on mrt_dump_hdr_rde() failure in mrt_dump_entry(). In particular, there is often a double log for the 'unsupported type' case in mrt_dump_hdr_rde()...
Re: Improve error message in rcctl(8) again
On Mon, Jul 10, 2023 at 04:46:28PM -0400, Anthony Coulter wrote: > Seven years ago I tried to restart a configuration file and it didn't > work out: https://marc.info/?l=openbsd-tech&m=147318006722787&w=2 > > This morning I tried to disable a different configuration file and got > similar results. Maybe my hands get too used to typing ".conf" when I > am fiddling with config files and restarting services. Or maybe I have > the mind of a LISP programmer, to whom code and data are the same > thing. But if I have not stopped passing config files to rcctl after > seven years I will probably never stop, so we should fix the error > message again. > > Test cases: > > # rcctl disable foo.bar > # rcctl set foo.bar status off > > Compare the error messages before and after the patch. > > > diff --git usr.sbin/rcctl/rcctl.sh usr.sbin/rcctl/rcctl.sh > index fb87943ba00..489c0217c45 100644 > --- usr.sbin/rcctl/rcctl.sh > +++ usr.sbin/rcctl/rcctl.sh > @@ -541,6 +541,12 @@ case ${action} in > svc_is_avail ${svc} || \ > rcctl_err "service ${svc} does not > exist" 2 > done > + else > + # But you still have to catch invalid characters > + for svc in ${svcs}; do > + _rc_check_name "${svc}" || \ > + rcctl_err "service ${svc} does not > exist" 2 > + done > fi > ;; > get|getdef) > @@ -572,6 +578,9 @@ case ${action} in > if [ "${action} ${var} ${args}" != "set status off" ]; then > svc_is_avail ${svc} || \ > rcctl_err "service ${svc} does not exist" 2 > + else > + _rc_check_name "${svc}" || \ > + rcctl_err "service ${svc} does not exist" 2 > fi > [[ ${var} != > @(class|execdir|flags|logger|rtable|status|timeout|user) ]] && usage > svc_is_meta ${svc} && [ "${var}" != "status" ] && \ > Hi Anthony. Thanks for the patch. Slightly modified version but it should do the same. Does this work for you? Index: rcctl.sh === RCS file: /cvs/src/usr.sbin/rcctl/rcctl.sh,v retrieving revision 1.116 diff -u -p -r1.116 rcctl.sh --- rcctl.sh24 Apr 2023 14:31:15 - 1.116 +++ rcctl.sh13 Jul 2023 09:58:39 - @@ -535,13 +535,17 @@ case ${action} in shift 1 svcs="$*" [ -z "${svcs}" ] && usage - # it's ok to disable a non-existing daemon - if [ "${action}" != "disable" ]; then - for svc in ${svcs}; do + for svc in ${svcs}; do + # it's ok to disable a non-existing daemon + if [ "${action}" != "disable" ]; then svc_is_avail ${svc} || \ rcctl_err "service ${svc} does not exist" 2 - done - fi + # but still check for bad input + else + _rc_check_name "${svc}" || \ + rcctl_err "service ${svc} does not exist" 2 + fi + done ;; get|getdef) svc=$2 @@ -571,6 +575,10 @@ case ${action} in # it's ok to disable a non-existing daemon if [ "${action} ${var} ${args}" != "set status off" ]; then svc_is_avail ${svc} || \ + rcctl_err "service ${svc} does not exist" 2 + # but still check for bad input + else + _rc_check_name "${svc}" || \ rcctl_err "service ${svc} does not exist" 2 fi [[ ${var} != @(class|execdir|flags|logger|rtable|status|timeout|user) ]] && usage
Re: Improve error message in rcctl(8) again
> Hi Anthony. > > Thanks for the patch. > Slightly modified version but it should do the same. > Does this work for you? > > Index: rcctl.sh > === > RCS file: /cvs/src/usr.sbin/rcctl/rcctl.sh,v > retrieving revision 1.116 > diff -u -p -r1.116 rcctl.sh > --- rcctl.sh 24 Apr 2023 14:31:15 - 1.116 > +++ rcctl.sh 13 Jul 2023 09:58:39 - > @@ -535,13 +535,17 @@ case ${action} in > shift 1 > svcs="$*" > [ -z "${svcs}" ] && usage > - # it's ok to disable a non-existing daemon > - if [ "${action}" != "disable" ]; then > - for svc in ${svcs}; do > + for svc in ${svcs}; do > + # it's ok to disable a non-existing daemon > + if [ "${action}" != "disable" ]; then > svc_is_avail ${svc} || \ > rcctl_err "service ${svc} does not > exist" 2 >- done >- fi > + # but still check for bad input > + else > + _rc_check_name "${svc}" || \ > + rcctl_err "service ${svc} does not > exist" 2 > + fi <<< TRAILING WHITESPACE >>> > + done > [...snip...] That works, but note the trailing whitespace after the "fi" in the second-last line of the code snippet above. Thanks, Anthony
Fix dhcrelay6 on carp
This patch fixes dhcrelay on carp. Without it, the AF_LINK entry (the only one containing the interface index and rdomain of the carp interface) of carp interfaces was ignored. When doing the IPV6_JOIN_GROUP, ip6_setmoptions() would see an zero interface index and picked an arbitrary, "appropriate one" instead of the carp interface itself. Similary code is found in the IPv4 version of dhcrelay. Index: usr.sbin/dhcrelay6/dispatch.c === RCS file: /cvs/src/usr.sbin/dhcrelay6/dispatch.c,v retrieving revision 1.2 diff -u -p -u -p -r1.2 dispatch.c --- usr.sbin/dhcrelay6/dispatch.c 17 Jan 2021 13:41:24 - 1.2 +++ usr.sbin/dhcrelay6/dispatch.c 13 Jul 2023 13:38:38 - @@ -168,7 +168,8 @@ setup_iflist(void) /* Skip non ethernet interfaces. */ if (ifi->ifi_type != IFT_ETHER && - ifi->ifi_type != IFT_ENC) { + ifi->ifi_type != IFT_ENC && + ifi->ifi_type != IFT_CARP) { TAILQ_REMOVE(&intflist, intf, entry); free(intf); continue; smime.p7s Description: S/MIME cryptographic signature
rtwn: R92C_RXDW0_OWN -> R92C_TXDW0_OWN
In rtwn_tx(), check if the OWN bit of Tx instead of Rx is set. Luckily, definitions of R92C_TXDW0_OWN and R92C_RXDW0_OWN are the same. ok? Index: sys/dev/pci/if_rtwn.c === RCS file: /cvs/src/sys/dev/pci/if_rtwn.c,v retrieving revision 1.40 diff -u -p -u -p -r1.40 if_rtwn.c --- sys/dev/pci/if_rtwn.c 21 Apr 2022 21:03:03 - 1.40 +++ sys/dev/pci/if_rtwn.c 14 Jul 2023 06:43:06 - @@ -1022,7 +1022,7 @@ rtwn_tx(void *cookie, struct mbuf *m, st /* Fill Tx descriptor. */ txd = &tx_ring->desc[tx_ring->cur]; - if (htole32(txd->txdw0) & R92C_RXDW0_OWN) { + if (htole32(txd->txdw0) & R92C_TXDW0_OWN) { m_freem(m); return (ENOBUFS); }