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);
}
