Re: vmd: remove the user quota tracking
On Wed, Oct 05, 2022 at 05:03:16PM -0400, Dave Voutila wrote:
> Matthew Martin recently presented a patch on tech@ [1] fixing some missed
> scaling from when I converted vmd(8) to use bytes instead of megabytes
> everywhere. I finally found time to wade through the code it touches and
> am proposing we simply "tedu" the incomplete feature.
>
> Does anyone use this? (And if so, how?)
>
> I don't see much value in this framework and it only adds additional
> state to track. Users can be confined by limits associated in
> login.conf(5) for the most part. There are more interesting things to
> work on, so unless anyone speaks up I'll look for an OK to remove it.
>
> -dv
>
> [1] https://marc.info/?l=openbsd-tech=166346196317673=2
For what it's worth this works for me (I can use double-p's packer
builder with the diff). Thanks
> diff refs/heads/master refs/heads/vmd-user
> commit - bfe2092d87b190d9f89c4a6f2728a539b7f88233
> commit + e84ff2c7628a811e00044a447ad906d6e24beac0
> blob - 374d7de6629e072065b5c0232536c23c1e5bbbe0
> blob + a192223cf118e2a8764b24f965a15acbf8ae506f
> --- usr.sbin/vmd/config.c
> +++ usr.sbin/vmd/config.c
> @@ -98,12 +98,6 @@ config_init(struct vmd *env)
> return (-1);
> TAILQ_INIT(env->vmd_switches);
> }
> - if (what & CONFIG_USERS) {
> - if ((env->vmd_users = calloc(1,
> - sizeof(*env->vmd_users))) == NULL)
> - return (-1);
> - TAILQ_INIT(env->vmd_users);
> - }
>
> return (0);
> }
> @@ -238,13 +232,6 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, ui
> return (EALREADY);
> }
>
> - /* increase the user reference counter and check user limits */
> - if (vm->vm_user != NULL && user_get(vm->vm_user->usr_id.uid) != NULL) {
> - user_inc(vcp, vm->vm_user, 1);
> - if (user_checklimit(vm->vm_user, vcp) == -1)
> - return (EPERM);
> - }
> -
> /*
>* Rate-limit the VM so that it cannot restart in a loop:
>* if the VM restarts after less than VM_START_RATE_SEC seconds,
> blob - 2f3ac1a76f2c3e458919eca85c238a668c10422a
> blob + 755cbedb6a18502a87724502ec86e9e426961701
> --- usr.sbin/vmd/vmd.c
> +++ usr.sbin/vmd/vmd.c
> @@ -1188,9 +1188,6 @@ vm_stop(struct vmd_vm *vm, int keeptty, const char *ca
> vm->vm_state &= ~(VM_STATE_RECEIVED | VM_STATE_RUNNING
> | VM_STATE_SHUTDOWN);
>
> - user_inc(>vm_params.vmc_params, vm->vm_user, 0);
> - user_put(vm->vm_user);
> -
> if (vm->vm_iev.ibuf.fd != -1) {
> event_del(>vm_iev.ev);
> close(vm->vm_iev.ibuf.fd);
> @@ -1243,7 +1240,6 @@ vm_remove(struct vmd_vm *vm, const char *caller)
>
> TAILQ_REMOVE(env->vmd_vms, vm, vm_entry);
>
> - user_put(vm->vm_user);
> vm_stop(vm, 0, caller);
> free(vm);
> }
> @@ -1286,7 +1282,6 @@ vm_register(struct privsep *ps, struct vmop_create_par
> struct vmd_vm *vm = NULL, *vm_parent = NULL;
> struct vm_create_params *vcp = >vmc_params;
> struct vmop_owner *vmo = NULL;
> - struct vmd_user *usr = NULL;
> uint32_t nid, rng;
> unsigned int i, j;
> struct vmd_switch *sw;
> @@ -1362,13 +1357,6 @@ vm_register(struct privsep *ps, struct vmop_create_par
> }
> }
>
> - /* track active users */
> - if (uid != 0 && env->vmd_users != NULL &&
> - (usr = user_get(uid)) == NULL) {
> - log_warnx("could not add user");
> - goto fail;
> - }
> -
> if ((vm = calloc(1, sizeof(*vm))) == NULL)
> goto fail;
>
> @@ -1379,7 +1367,6 @@ vm_register(struct privsep *ps, struct vmop_create_par
> vm->vm_tty = -1;
> vm->vm_receive_fd = -1;
> vm->vm_state &= ~VM_STATE_PAUSED;
> - vm->vm_user = usr;
>
> for (i = 0; i < VMM_MAX_DISKS_PER_VM; i++)
> for (j = 0; j < VM_MAX_BASE_PER_DISK; j++)
> @@ -1903,104 +1890,6 @@ struct vmd_user *
> return (NULL);
> }
>
> -struct vmd_user *
> -user_get(uid_t uid)
> -{
> - struct vmd_user *usr;
> -
> - if (uid == 0)
> - return (NULL);
> -
> - /* first try to find an existing user */
> - TAILQ_FOREACH(usr, env->vmd_users, usr_entry) {
> - if (usr->usr_id.uid == uid)
> - goto done;
> - }
> -
> - if ((usr = calloc(1, sizeof(*usr))) == NULL) {
> - log_warn("could not allocate user");
> - return (NULL);
> - }
> -
> - usr->usr_id.uid = uid;
> - usr->usr_id.gid = -1;
> - TAILQ_INSERT_TAIL(env->vmd_users, usr, usr_entry);
> -
> - done:
> - DPRINTF("%s: uid %d #%d +",
> - __func__, usr->usr_id.uid, usr->usr_refcnt + 1);
> - usr->usr_refcnt++;
> -
> - return (usr);
> -}
> -
> -void
> -user_put(struct vmd_user *usr)
> -{
> - if (usr == NULL)
> - return;
> -
> -
Re: vmd: remove the user quota tracking
On Wed, Oct 05, 2022 at 05:03:16PM -0400, Dave Voutila wrote:
> Matthew Martin recently presented a patch on tech@ [1] fixing some missed
> scaling from when I converted vmd(8) to use bytes instead of megabytes
> everywhere. I finally found time to wade through the code it touches and
> am proposing we simply "tedu" the incomplete feature.
>
> Does anyone use this? (And if so, how?)
>
> I don't see much value in this framework and it only adds additional
> state to track. Users can be confined by limits associated in
> login.conf(5) for the most part. There are more interesting things to
> work on, so unless anyone speaks up I'll look for an OK to remove it.
>
> -dv
>
> [1] https://marc.info/?l=openbsd-tech=166346196317673=2
>
I'd wait for someone to speak up and become the owner of this part of vmd and
if nobody does, ok mlarkin to nuke it.
-ml
>
> diff refs/heads/master refs/heads/vmd-user
> commit - bfe2092d87b190d9f89c4a6f2728a539b7f88233
> commit + e84ff2c7628a811e00044a447ad906d6e24beac0
> blob - 374d7de6629e072065b5c0232536c23c1e5bbbe0
> blob + a192223cf118e2a8764b24f965a15acbf8ae506f
> --- usr.sbin/vmd/config.c
> +++ usr.sbin/vmd/config.c
> @@ -98,12 +98,6 @@ config_init(struct vmd *env)
> return (-1);
> TAILQ_INIT(env->vmd_switches);
> }
> - if (what & CONFIG_USERS) {
> - if ((env->vmd_users = calloc(1,
> - sizeof(*env->vmd_users))) == NULL)
> - return (-1);
> - TAILQ_INIT(env->vmd_users);
> - }
>
> return (0);
> }
> @@ -238,13 +232,6 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, ui
> return (EALREADY);
> }
>
> - /* increase the user reference counter and check user limits */
> - if (vm->vm_user != NULL && user_get(vm->vm_user->usr_id.uid) != NULL) {
> - user_inc(vcp, vm->vm_user, 1);
> - if (user_checklimit(vm->vm_user, vcp) == -1)
> - return (EPERM);
> - }
> -
> /*
>* Rate-limit the VM so that it cannot restart in a loop:
>* if the VM restarts after less than VM_START_RATE_SEC seconds,
> blob - 2f3ac1a76f2c3e458919eca85c238a668c10422a
> blob + 755cbedb6a18502a87724502ec86e9e426961701
> --- usr.sbin/vmd/vmd.c
> +++ usr.sbin/vmd/vmd.c
> @@ -1188,9 +1188,6 @@ vm_stop(struct vmd_vm *vm, int keeptty, const char *ca
> vm->vm_state &= ~(VM_STATE_RECEIVED | VM_STATE_RUNNING
> | VM_STATE_SHUTDOWN);
>
> - user_inc(>vm_params.vmc_params, vm->vm_user, 0);
> - user_put(vm->vm_user);
> -
> if (vm->vm_iev.ibuf.fd != -1) {
> event_del(>vm_iev.ev);
> close(vm->vm_iev.ibuf.fd);
> @@ -1243,7 +1240,6 @@ vm_remove(struct vmd_vm *vm, const char *caller)
>
> TAILQ_REMOVE(env->vmd_vms, vm, vm_entry);
>
> - user_put(vm->vm_user);
> vm_stop(vm, 0, caller);
> free(vm);
> }
> @@ -1286,7 +1282,6 @@ vm_register(struct privsep *ps, struct vmop_create_par
> struct vmd_vm *vm = NULL, *vm_parent = NULL;
> struct vm_create_params *vcp = >vmc_params;
> struct vmop_owner *vmo = NULL;
> - struct vmd_user *usr = NULL;
> uint32_t nid, rng;
> unsigned int i, j;
> struct vmd_switch *sw;
> @@ -1362,13 +1357,6 @@ vm_register(struct privsep *ps, struct vmop_create_par
> }
> }
>
> - /* track active users */
> - if (uid != 0 && env->vmd_users != NULL &&
> - (usr = user_get(uid)) == NULL) {
> - log_warnx("could not add user");
> - goto fail;
> - }
> -
> if ((vm = calloc(1, sizeof(*vm))) == NULL)
> goto fail;
>
> @@ -1379,7 +1367,6 @@ vm_register(struct privsep *ps, struct vmop_create_par
> vm->vm_tty = -1;
> vm->vm_receive_fd = -1;
> vm->vm_state &= ~VM_STATE_PAUSED;
> - vm->vm_user = usr;
>
> for (i = 0; i < VMM_MAX_DISKS_PER_VM; i++)
> for (j = 0; j < VM_MAX_BASE_PER_DISK; j++)
> @@ -1903,104 +1890,6 @@ struct vmd_user *
> return (NULL);
> }
>
> -struct vmd_user *
> -user_get(uid_t uid)
> -{
> - struct vmd_user *usr;
> -
> - if (uid == 0)
> - return (NULL);
> -
> - /* first try to find an existing user */
> - TAILQ_FOREACH(usr, env->vmd_users, usr_entry) {
> - if (usr->usr_id.uid == uid)
> - goto done;
> - }
> -
> - if ((usr = calloc(1, sizeof(*usr))) == NULL) {
> - log_warn("could not allocate user");
> - return (NULL);
> - }
> -
> - usr->usr_id.uid = uid;
> - usr->usr_id.gid = -1;
> - TAILQ_INSERT_TAIL(env->vmd_users, usr, usr_entry);
> -
> - done:
> - DPRINTF("%s: uid %d #%d +",
> - __func__, usr->usr_id.uid, usr->usr_refcnt + 1);
> - usr->usr_refcnt++;
> -
> - return (usr);
> -}
> -
> -void
> -user_put(struct vmd_user *usr)
> -{
> - if (usr == NULL)
> -
vmd: remove the user quota tracking
Matthew Martin recently presented a patch on tech@ [1] fixing some missed
scaling from when I converted vmd(8) to use bytes instead of megabytes
everywhere. I finally found time to wade through the code it touches and
am proposing we simply "tedu" the incomplete feature.
Does anyone use this? (And if so, how?)
I don't see much value in this framework and it only adds additional
state to track. Users can be confined by limits associated in
login.conf(5) for the most part. There are more interesting things to
work on, so unless anyone speaks up I'll look for an OK to remove it.
-dv
[1] https://marc.info/?l=openbsd-tech=166346196317673=2
diff refs/heads/master refs/heads/vmd-user
commit - bfe2092d87b190d9f89c4a6f2728a539b7f88233
commit + e84ff2c7628a811e00044a447ad906d6e24beac0
blob - 374d7de6629e072065b5c0232536c23c1e5bbbe0
blob + a192223cf118e2a8764b24f965a15acbf8ae506f
--- usr.sbin/vmd/config.c
+++ usr.sbin/vmd/config.c
@@ -98,12 +98,6 @@ config_init(struct vmd *env)
return (-1);
TAILQ_INIT(env->vmd_switches);
}
- if (what & CONFIG_USERS) {
- if ((env->vmd_users = calloc(1,
- sizeof(*env->vmd_users))) == NULL)
- return (-1);
- TAILQ_INIT(env->vmd_users);
- }
return (0);
}
@@ -238,13 +232,6 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, ui
return (EALREADY);
}
- /* increase the user reference counter and check user limits */
- if (vm->vm_user != NULL && user_get(vm->vm_user->usr_id.uid) != NULL) {
- user_inc(vcp, vm->vm_user, 1);
- if (user_checklimit(vm->vm_user, vcp) == -1)
- return (EPERM);
- }
-
/*
* Rate-limit the VM so that it cannot restart in a loop:
* if the VM restarts after less than VM_START_RATE_SEC seconds,
blob - 2f3ac1a76f2c3e458919eca85c238a668c10422a
blob + 755cbedb6a18502a87724502ec86e9e426961701
--- usr.sbin/vmd/vmd.c
+++ usr.sbin/vmd/vmd.c
@@ -1188,9 +1188,6 @@ vm_stop(struct vmd_vm *vm, int keeptty, const char *ca
vm->vm_state &= ~(VM_STATE_RECEIVED | VM_STATE_RUNNING
| VM_STATE_SHUTDOWN);
- user_inc(>vm_params.vmc_params, vm->vm_user, 0);
- user_put(vm->vm_user);
-
if (vm->vm_iev.ibuf.fd != -1) {
event_del(>vm_iev.ev);
close(vm->vm_iev.ibuf.fd);
@@ -1243,7 +1240,6 @@ vm_remove(struct vmd_vm *vm, const char *caller)
TAILQ_REMOVE(env->vmd_vms, vm, vm_entry);
- user_put(vm->vm_user);
vm_stop(vm, 0, caller);
free(vm);
}
@@ -1286,7 +1282,6 @@ vm_register(struct privsep *ps, struct vmop_create_par
struct vmd_vm *vm = NULL, *vm_parent = NULL;
struct vm_create_params *vcp = >vmc_params;
struct vmop_owner *vmo = NULL;
- struct vmd_user *usr = NULL;
uint32_t nid, rng;
unsigned int i, j;
struct vmd_switch *sw;
@@ -1362,13 +1357,6 @@ vm_register(struct privsep *ps, struct vmop_create_par
}
}
- /* track active users */
- if (uid != 0 && env->vmd_users != NULL &&
- (usr = user_get(uid)) == NULL) {
- log_warnx("could not add user");
- goto fail;
- }
-
if ((vm = calloc(1, sizeof(*vm))) == NULL)
goto fail;
@@ -1379,7 +1367,6 @@ vm_register(struct privsep *ps, struct vmop_create_par
vm->vm_tty = -1;
vm->vm_receive_fd = -1;
vm->vm_state &= ~VM_STATE_PAUSED;
- vm->vm_user = usr;
for (i = 0; i < VMM_MAX_DISKS_PER_VM; i++)
for (j = 0; j < VM_MAX_BASE_PER_DISK; j++)
@@ -1903,104 +1890,6 @@ struct vmd_user *
return (NULL);
}
-struct vmd_user *
-user_get(uid_t uid)
-{
- struct vmd_user *usr;
-
- if (uid == 0)
- return (NULL);
-
- /* first try to find an existing user */
- TAILQ_FOREACH(usr, env->vmd_users, usr_entry) {
- if (usr->usr_id.uid == uid)
- goto done;
- }
-
- if ((usr = calloc(1, sizeof(*usr))) == NULL) {
- log_warn("could not allocate user");
- return (NULL);
- }
-
- usr->usr_id.uid = uid;
- usr->usr_id.gid = -1;
- TAILQ_INSERT_TAIL(env->vmd_users, usr, usr_entry);
-
- done:
- DPRINTF("%s: uid %d #%d +",
- __func__, usr->usr_id.uid, usr->usr_refcnt + 1);
- usr->usr_refcnt++;
-
- return (usr);
-}
-
-void
-user_put(struct vmd_user *usr)
-{
- if (usr == NULL)
- return;
-
- DPRINTF("%s: uid %d #%d -",
- __func__, usr->usr_id.uid, usr->usr_refcnt - 1);
-
- if (--usr->usr_refcnt > 0)
- return;
-
- TAILQ_REMOVE(env->vmd_users, usr, usr_entry);
- free(usr);
-}
-
-void
-user_inc(struct vm_create_params *vcp, struct
Re: install.sub: Get rid of useless/confusing subshell
On October 4, 2022 10:11:46 PM GMT+02:00, Klemens Nanni
wrote:
>This function's style is a bit off: it wraps the body in a subshell to
>discard all stdout/err at once, but a still uses return inside it.
>
>1. A command list (using {}) would be enough here as it groups like a
> subshell but avoids spawning another shell;
>2. discarding stdout/err at the end of an if block works the same
> (effecting both condition and body) and saves one level of indent;
>3. return inside a subshell inside a function does NOT return from the
> function but merely exits the subshell; this is easily misread.
>
>Saving a fork and indent and improving readability boils down to this
>(cvs diff -wU1):
>
> |@@ -3320,3 +3317,2 @@ check_unattendedupgrade() {
> | _d=${_d%% *}
> |- (
> | if [[ -n $_d ]]; then
> |@@ -3331,5 +3327,5 @@ check_unattendedupgrade() {
> | rm -f /dev/{r,}$_d?
> |- fi
> |+ fi >/dev/null 2>&1
> |+
> | return $_rc
> |- ) > /dev/null 2>&1
> | }
>
>Feedback? OK?
While I dislike the ">/dev/null 2>&1" sledgehammer, this is in the right
direction.
ok halex@
>
>
>Index: install.sub
>===
>RCS file: /cvs/src/distrib/miniroot/install.sub,v
>retrieving revision 1.1209
>diff -u -p -r1.1209 install.sub
>--- install.sub4 Oct 2022 19:59:10 - 1.1209
>+++ install.sub4 Oct 2022 20:00:54 -
>@@ -3315,20 +3315,19 @@ check_unattendedupgrade() {
> local _d=$(get_dkdevs_root) _rc=1
>
> _d=${_d%% *}
>- (
>- if [[ -n $_d ]]; then
>- make_dev $_d
>- if mount -t ffs -r /dev/${_d}a /mnt; then
>- [[ -f /mnt/bsd.upgrade && -f
>/mnt/auto_upgrade.conf ]]
>- _rc=$?
>- ((_rc == 0)) && cp /mnt/auto_upgrade.conf /
>- echo "Which disk is the root disk = ${_d}" >>
>/auto_upgrade.conf
>- umount /mnt
>- fi
>- rm -f /dev/{r,}$_d?
>+ if [[ -n $_d ]]; then
>+ make_dev $_d
>+ if mount -t ffs -r /dev/${_d}a /mnt; then
>+ [[ -f /mnt/bsd.upgrade && -f /mnt/auto_upgrade.conf ]]
>+ _rc=$?
>+ ((_rc == 0)) && cp /mnt/auto_upgrade.conf /
>+ echo "Which disk is the root disk = ${_d}" >>
>/auto_upgrade.conf
>+ umount /mnt
> fi
>- return $_rc
>- ) > /dev/null 2>&1
>+ rm -f /dev/{r,}$_d?
>+ fi >/dev/null 2>&1
>+
>+ return $_rc
> }
>
> WATCHDOG_PERIOD_SEC=$((30 * 60))
>
Re: rc: do not clear mfs /tmp
On October 5, 2022 12:57:44 AM GMT+02:00, Klemens Nanni wrote: >There is no problem to fix, but every boot I read "/clearing /tmp" and >know it is a useless step since my /tmp live on volatile RAM anyway. > >Other steps in rc(8) also check and print/log conditionally, so this >can do as well, saving yet another line. I don't think the gain outweighs the (albeit minor) bloating. I'd be ok with exactly this part of the diff though, for the same reason: >-# (not needed with mfs /tmp, but doesn't hurt there...). :-) /Alexander
Re: malloc: prep for immutable pages
Marc Espie wrote: > On Tue, Oct 04, 2022 at 10:15:51AM -0600, Theo de Raadt wrote: > > A note on why this chance is coming. > > > > malloc.c (as it is today), does mprotects back and forth between RW and > > R, to protect an internal object. This object is in bss, it is not > > allocated with mmap. With the upcoming mimmutable change, the bss will > > become immutable by default, at program load time. mimmutable even prevents > > changing a RW object to R. > > I'm probably missing something here, but for me, traditionally, > BSS is the "set to 0" section of global variables of a program... which are > usually going to be changed to some other value. > > Or are we talking at cross purposes ? If you read the mimmutable diff, it has a manual page, and the answer is in there.
Re: malloc: prep for immutable pages
On Wed, Oct 05, 2022 at 02:47:19PM +0200, Marc Espie wrote: > On Tue, Oct 04, 2022 at 10:15:51AM -0600, Theo de Raadt wrote: > > A note on why this chance is coming. > > > > malloc.c (as it is today), does mprotects back and forth between RW and > > R, to protect an internal object. This object is in bss, it is not > > allocated with mmap. With the upcoming mimmutable change, the bss will > > become immutable by default, at program load time. mimmutable even prevents > > changing a RW object to R. > > I'm probably missing something here, but for me, traditionally, > BSS is the "set to 0" section of global variables of a program... which are > usually going to be changed to some other value. > > Or are we talking at cross purposes ? > malloc sets up a few values in a bss struct and then sets the page its in to r/o. But when switching to multi-threaded mode a few variables in the struct are changed (in current), so the page has to be made r/w again, modified and then back to r/o. The diff changes things so that the pages can be set to r/o and immutable after malloc init. -Otto
Re: malloc: prep for immutable pages
On Tue, Oct 04, 2022 at 10:15:51AM -0600, Theo de Raadt wrote: > A note on why this chance is coming. > > malloc.c (as it is today), does mprotects back and forth between RW and > R, to protect an internal object. This object is in bss, it is not > allocated with mmap. With the upcoming mimmutable change, the bss will > become immutable by default, at program load time. mimmutable even prevents > changing a RW object to R. I'm probably missing something here, but for me, traditionally, BSS is the "set to 0" section of global variables of a program... which are usually going to be changed to some other value. Or are we talking at cross purposes ?
Re: installboot: merge duplicate code into sr_open_chunk()
On Sat, Sep 10, 2022 at 02:10:22AM +, Klemens Nanni wrote:
> It does not have the prettiest signature, but nicely folds identical
> copies into MI softraid.c, which then allows us to
> - avoid further diverging MD code
> - implement the keydisk fix on tech@ once instead of thrice
> - reuse sr_open_chunk() in an upcoming diff to make -p softraid aware
>
> The last point is especially useful, otherwise I'd have to copy what
> sr_open_chunk() does yet another three times (each *_softraid.c file),
> turning it a much simpler and smaller diff.
>
> This conflicts with the keydisk fix on tech@
> "Softraid crypto with keydisk and installboot, skip on the same disk".
The keydisk fix just landed.
> I've successfully tested the diff below together with the keydisk fix
> as well as the upcoming -p/softraid diff on amd64, arm64 and sparc64.
mlarking reported success installing root on softraid on arm64 without
manual intervention using the combined diff posted earlier.
> Feedback? OK?
Rebased unification diff below without functional changes.
Index: efi_softraid.c
===
RCS file: /cvs/src/usr.sbin/installboot/efi_softraid.c,v
retrieving revision 1.3
diff -u -p -r1.3 efi_softraid.c
--- efi_softraid.c 5 Oct 2022 09:58:43 - 1.3
+++ efi_softraid.c 5 Oct 2022 10:24:34 -
@@ -16,18 +16,9 @@
* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/
-#include
-#include
-#include
-
#include
-#include
-#include
-#include
#include
-#include
-#include
#include
#include "installboot.h"
@@ -40,38 +31,9 @@ sr_install_bootblk(int devfd, int vol, i
int diskfd;
char part;
- /* Get device name for this disk/chunk. */
- memset(, 0, sizeof(bd));
- bd.bd_volid = vol;
- bd.bd_diskid = disk;
- if (ioctl(devfd, BIOCDISK, ) == -1)
- err(1, "BIOCDISK");
-
- /* Check disk status. */
- if (bd.bd_status != BIOC_SDONLINE && bd.bd_status != BIOC_SDREBUILD) {
- fprintf(stderr, "softraid chunk %u not online - skipping...\n",
- disk);
- return;
- }
-
- /* Keydisks always have a size of zero. */
- if (bd.bd_size == 0) {
- fprintf(stderr, "softraid chunk %u is keydisk - skipping...\n",
- disk);
+ diskfd = sr_open_chunk(devfd, vol, disk, , , );
+ if (diskfd == -1)
return;
- }
-
- if (strlen(bd.bd_vendor) < 1)
- errx(1, "invalid disk name");
- part = bd.bd_vendor[strlen(bd.bd_vendor) - 1];
- if (part < 'a' || part >= 'a' + MAXPARTITIONS)
- errx(1, "invalid partition %c\n", part);
- bd.bd_vendor[strlen(bd.bd_vendor) - 1] = '\0';
-
- /* Open device. */
- if ((diskfd = opendev(bd.bd_vendor, (nowrite ? O_RDONLY : O_RDWR),
- OPENDEV_PART, )) == -1)
- err(1, "open: %s", realdev);
if (verbose)
fprintf(stderr, "%s%c: %s boot blocks on %s\n", bd.bd_vendor,
Index: i386_softraid.c
===
RCS file: /cvs/src/usr.sbin/installboot/i386_softraid.c,v
retrieving revision 1.20
diff -u -p -r1.20 i386_softraid.c
--- i386_softraid.c 5 Oct 2022 09:58:43 - 1.20
+++ i386_softraid.c 5 Oct 2022 10:24:34 -
@@ -32,7 +32,6 @@
#include
#include
#include
-#include
#include "installboot.h"
#include "i386_installboot.h"
@@ -51,38 +50,9 @@ sr_install_bootblk(int devfd, int vol, i
char part, efipart;
int diskfd;
- /* Get device name for this disk/chunk. */
- memset(, 0, sizeof(bd));
- bd.bd_volid = vol;
- bd.bd_diskid = disk;
- if (ioctl(devfd, BIOCDISK, ) == -1)
- err(1, "BIOCDISK");
-
- /* Check disk status. */
- if (bd.bd_status != BIOC_SDONLINE && bd.bd_status != BIOC_SDREBUILD) {
- fprintf(stderr, "softraid chunk %u not online - skipping...\n",
- disk);
- return;
- }
-
- /* Keydisks always have a size of zero. */
- if (bd.bd_size == 0) {
- fprintf(stderr, "softraid chunk %u is keydisk - skipping...\n",
- disk);
+ diskfd = sr_open_chunk(devfd, vol, disk, , , );
+ if (diskfd == -1)
return;
- }
-
- if (strlen(bd.bd_vendor) < 1)
- errx(1, "invalid disk name");
- part = bd.bd_vendor[strlen(bd.bd_vendor) - 1];
- if (part < 'a' || part >= 'a' + MAXPARTITIONS)
- errx(1, "invalid partition %c\n", part);
- bd.bd_vendor[strlen(bd.bd_vendor) - 1] = '\0';
-
- /* Open this device and check its disklabel. */
- if ((diskfd = opendev(bd.bd_vendor, (nowrite? O_RDONLY:O_RDWR),
- OPENDEV_PART, )) == -1)
- err(1, "open: %s", dev);
/* Get and check disklabel. */
if
Re: ypldap client cert authentication
On Mon, Sep 19, 2022 at 09:54:56PM +1000, Jonathan Matthew wrote:
> This adds client certificate authentication to ypldap(8). libtls makes the
> actual certificate part of this straightforward (I would still like it
> reviewed, though), but there are some LDAP complications.
>
> Depending on your LDAP server and how you connect to it (LDAPS on port 636
> or LDAP+TLS on port 389), a client presenting a certificate might
> automatically be bound as the subject of the certificate, or it might not.
> If it's not, the client can do an LDAP bind operation using the SASL
> EXTERNAL mechanism to bind as the cert subject, and it can optionally specify
> an identity, which means the bind will fail if the cert subject doesn't match
> that identity. If the client didn't present a certificate, the bind will
> also fail (one would hope).
>
> For reference, with Active Directory, SASL EXTERNAL bind is required when
> using LDAP+TLS, but not when using LDAPS, and the client identity can be
> specified in the form of "dn:" followed by the expected cert subject DN.
> OpenLDAP doesn't seem to do automatic bind at all, so SASL EXTERNAL would
> always be required there, and it doesn't appear to support specifying the
> expected identity with the bind.
>
> The diff adds 'certfile' and 'keyfile' config directives for specifying
> the certificate to use, and a 'bindext' directive for enabling SASL EXTERNAL
> bind, optionally including the identity string. SASL EXTERNAL bind doesn't
> get enabled implicitly when you configure a client cert, because ypldap
> can't tell if it's required or supported by the server. It's also not an
> error to enable SASL EXTERNAL bind without a client cert, since you could be
> connecting through stunnel or something.
>
> To configure this in ypldap.conf, you'd do something like this:
>
> directory "ldap.example.com" tls {
> bindext "dn:CN=ypldap,OU,Accounts,DC=example,DC=com"
> certfile "/etc/ssl/ypldap-cert.pem"
> keyfile "/etc/ssl/private/ypldap-key.pem"
>
> ...
> }
>
> ok?
The libtls parts of this are of course fine. I do not have a way to test
this, but your explanations and the code make sense.
ok tb
Two minor comments below.
>
> Index: aldap.c
> ===
> RCS file: /cvs/src/usr.sbin/ypldap/aldap.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 aldap.c
> --- aldap.c 31 Mar 2022 09:06:55 - 1.48
> +++ aldap.c 19 Sep 2022 11:47:13 -
> @@ -220,6 +220,40 @@ fail:
> }
>
> int
> +aldap_bind_sasl_external(struct aldap *ldap, char *bindid)
> +{
> + struct ber_element *root = NULL, *elm;
> +
> + if ((root = ober_add_sequence(NULL)) == NULL)
> + goto fail;
> +
> + elm = ober_printf_elements(root, "d{tds{ts", ++ldap->msgid,
> + BER_CLASS_APP, LDAP_REQ_BIND, VERSION, "",
> + BER_CLASS_CONTEXT, LDAP_AUTH_SASL, LDAP_SASL_MECH_EXTERNAL);
I think a NULL check for elm would be appropriate: while it's unlikely,
the subsequent ober_add_*() calls might hide an error in
ober_printf_elements().
> + if (bindid == NULL)
> + elm = ober_add_null(elm);
> + else
> + elm = ober_add_string(elm, bindid);
> +
> + if (elm == NULL)
> + goto fail;
> +
> + LDAP_DEBUG("aldap_bind_sasl_external", root);
> +
> + if (aldap_send(ldap, root) == -1) {
> + root = NULL;
> + goto fail;
> + }
> + return (ldap->msgid);
> +fail:
> + if (root != NULL)
Since ber.c r1.32, this NULL check is no longer necessary. Perhaps all
such checks could be garbage collected in another diff.
> + ober_free_elements(root);
> +
> + ldap->err = ALDAP_ERR_OPERATION_FAILED;
> + return (-1);
> +}
> +
> +int
> aldap_unbind(struct aldap *ldap)
> {
> struct ber_element *root = NULL, *elm;
> Index: aldap.h
> ===
> RCS file: /cvs/src/usr.sbin/ypldap/aldap.h,v
> retrieving revision 1.14
> diff -u -p -r1.14 aldap.h
> --- aldap.h 11 May 2019 17:46:02 - 1.14
> +++ aldap.h 19 Sep 2022 11:47:13 -
> @@ -32,6 +32,8 @@
> #define LDAP_PAGED_OID "1.2.840.113556.1.4.319"
> #define LDAP_STARTTLS_OID"1.3.6.1.4.1.1466.20037"
>
> +#define LDAP_SASL_MECH_EXTERNAL "EXTERNAL"
> +
> struct aldap {
> #define ALDAP_ERR_SUCCESS0
> #define ALDAP_ERR_PARSER_ERROR 1
> @@ -137,6 +139,7 @@ enum deref_aliases {
>
> enum authentication_choice {
> LDAP_AUTH_SIMPLE= 0,
> + LDAP_AUTH_SASL = 3,
> };
>
> enum scope {
> @@ -222,6 +225,7 @@ void aldap_freemsg(struct
> aldap_messa
> int aldap_req_starttls(struct aldap *);
>
> int aldap_bind(struct aldap *, char *, char *);
> +int aldap_bind_sasl_external(struct aldap *, char *);
> int aldap_unbind(struct aldap *);
> int aldap_search(struct aldap *, char *,
Re: acme-client: allow newlines in alternative names
Makes sense to me, OK florian
Please wait a day or two in case there are objections.
On 2022-10-05 09:28 +02, Omar Polo wrote:
> just a small scratch to itch; i'd prefer if i could split the
> alternative names in multiple lines without using \
>
> so, now one should be able to write
>
> domain example.com {
> alternative names {
> some-subdomain.example.com
> another-subdomain.example.com
> }
> }
>
> not sure if the manpage needs an update then, now it reads:
>
> alternative names {...}
> A list of alternative names, comma or space separated, for which
> the certificate will be valid.
>
> but in other places we don't specify that newlines are accepted within
> curly braces blocks.
>
> diff /home/op/w/acme-client
> commit - 0b11e45035f5839d5e3ce9e86d234a5c7eb8f919
> path + /home/op/w/acme-client
> blob - e09c2283ad4b05452a6305be2d296cbb48496f2f
> file + parse.y
> --- parse.y
> +++ parse.y
> @@ -177,8 +177,8 @@ comma : ','
> nl : '\n' optnl/* one newline or more */
> ;
>
> -comma: ','
> - | /*empty*/
> +optcommanl : ',' optnl
> + | optnl
> ;
>
> authority: AUTHORITY STRING {
> @@ -287,7 +287,7 @@ domainoptsl : ALTERNATIVE NAMES '{' altname_l '}'
> | domainoptsl optnl
> ;
>
> -domainoptsl : ALTERNATIVE NAMES '{' altname_l '}'
> +domainoptsl : ALTERNATIVE NAMES '{' optnl altname_l '}'
> | DOMAIN NAME STRING {
> char *s;
> if (domain->domain != NULL) {
> @@ -395,8 +395,8 @@ altname_l : altname comma altname_l
> }
> ;
>
> -altname_l: altname comma altname_l
> - | altname
> +altname_l: altname optcommanl altname_l
> + | altname optnl
> ;
>
> altname : STRING {
>
--
I'm not entirely sure you are real.
acme-client: allow newlines in alternative names
just a small scratch to itch; i'd prefer if i could split the
alternative names in multiple lines without using \
so, now one should be able to write
domain example.com {
alternative names {
some-subdomain.example.com
another-subdomain.example.com
}
}
not sure if the manpage needs an update then, now it reads:
alternative names {...}
A list of alternative names, comma or space separated, for which
the certificate will be valid.
but in other places we don't specify that newlines are accepted within
curly braces blocks.
diff /home/op/w/acme-client
commit - 0b11e45035f5839d5e3ce9e86d234a5c7eb8f919
path + /home/op/w/acme-client
blob - e09c2283ad4b05452a6305be2d296cbb48496f2f
file + parse.y
--- parse.y
+++ parse.y
@@ -177,8 +177,8 @@ comma : ','
nl : '\n' optnl/* one newline or more */
;
-comma : ','
- | /*empty*/
+optcommanl : ',' optnl
+ | optnl
;
authority : AUTHORITY STRING {
@@ -287,7 +287,7 @@ domainoptsl : ALTERNATIVE NAMES '{' altname_l '}'
| domainoptsl optnl
;
-domainoptsl: ALTERNATIVE NAMES '{' altname_l '}'
+domainoptsl: ALTERNATIVE NAMES '{' optnl altname_l '}'
| DOMAIN NAME STRING {
char *s;
if (domain->domain != NULL) {
@@ -395,8 +395,8 @@ altname_l : altname comma altname_l
}
;
-altname_l : altname comma altname_l
- | altname
+altname_l : altname optcommanl altname_l
+ | altname optnl
;
altname: STRING {
