From owner-svn-src-head@freebsd.org Wed May 13 02:01:45 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 701EE2E42E2; Wed, 13 May 2020 02:01:45 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 49MHwT2KJrz4MKN; Wed, 13 May 2020 02:01:45 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from mail-qk1-f181.google.com (mail-qk1-f181.google.com [209.85.222.181]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) (Authenticated sender: kevans) by smtp.freebsd.org (Postfix) with ESMTPSA id 3EE9B2CD97; Wed, 13 May 2020 02:01:45 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: by mail-qk1-f181.google.com with SMTP id s186so14030912qkd.4; Tue, 12 May 2020 19:01:45 -0700 (PDT) X-Gm-Message-State: AGi0PuY4Q8PnFfceu8I/0M1BwYg0ZTRqeXA/tzoFDCquAIkC9zsEOm67 eh0aC2fFBsZO1Pt8wLtQCrDqthkYAtyZammq1Sk= X-Google-Smtp-Source: APiQypIWhNvpyOSpd+c2OiSLNe2+pxjbNJxX+nShO6w9swqF5gQ0oqG+US9PczP9in9JmwG0v7ZGEQ56cioxvpTcFYI= X-Received: by 2002:a37:6658:: with SMTP id a85mr23655178qkc.493.1589335304304; Tue, 12 May 2020 19:01:44 -0700 (PDT) MIME-Version: 1.0 References: <202005090201.04921Tpf028388@repo.freebsd.org> <20200511181027.GA60902@spindle.one-eyed-alien.net> <20200512221603.GB60902@spindle.one-eyed-alien.net> In-Reply-To: <20200512221603.GB60902@spindle.one-eyed-alien.net> From: Kyle Evans Date: Tue, 12 May 2020 21:01:33 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r360833 - head To: Brooks Davis Cc: src-committers , svn-src-all , svn-src-head Content-Type: text/plain; charset="UTF-8" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 May 2020 02:01:45 -0000 On Tue, May 12, 2020 at 5:16 PM Brooks Davis wrote: > > On Mon, May 11, 2020 at 01:45:14PM -0500, Kyle Evans wrote: > > On Mon, May 11, 2020 at 1:10 PM Brooks Davis wrote: > > > > > > On Sat, May 09, 2020 at 02:01:29AM +0000, Kyle Evans wrote: > > > > Author: kevans > > > > Date: Sat May 9 02:01:29 2020 > > > > New Revision: 360833 > > > > URL: https://svnweb.freebsd.org/changeset/base/360833 > > > > > > > > Log: > > > > installworld: attempt a certctl rehash at the tail end > > > > > > > > This can be run as root or normal user with no problem; if they hadn't > > > > twisted the WITHOUT_CAROOT knob, we'll attempt to use the host certctl to > > > > rehash the DESTDIR. This would allow one to build systems WITHOUT_OPENSSL + > > > > WITH_CAROOT with a populated /etc/ssl that they can then use with an > > > > appropriate *ssl from somewhere else. > > > > > > > > Cross-builds are fine because this will always use the host certctl, or just > > > > nag if it's missing and it wasn't a WITHOUT_CAROOT build. > > > > > > > > MFC after: 1 week > > > > Differential Revision: https://reviews.freebsd.org/D24641 > > > > > > > > Modified: > > > > head/Makefile.inc1 > > > > > > > > Modified: head/Makefile.inc1 > > > > ============================================================================== > > > > --- head/Makefile.inc1 Sat May 9 01:48:08 2020 (r360832) > > > > +++ head/Makefile.inc1 Sat May 9 02:01:29 2020 (r360833) > > > > @@ -1403,6 +1403,16 @@ distributeworld installworld stageworld: _installcheck > > > > ${DESTDIR}/${DISTDIR}/${dist}.debug.meta > > > > .endfor > > > > .endif > > > > +.elif make(installworld) && ${MK_CAROOT} != "no" > > > > + # We could make certctl a bootstrap tool, but it requires OpenSSL and > > > > + # friends, which we likely don't want. We'll rehash on a best-effort > > > > + # basis, otherwise we'll just mention that we're not doing it to raise > > > > + # awareness. > > > > + @if which certctl>/dev/null; then \ > > > > + certctl rehash \ > > > > > > Does this update METALOG with the added links? > > > > > > It seems a little weird to rely on DESTDIR from the environment. > > > > > > In general I'm not enthusiastic about additions to installworld that do > > > anything other than copying files, creating links, etc in simple ways. > > > > I will happily back this out if I can get some qualified eyes to > > review/improve it. It does not update METALOG, and it probably should. > > Agreed on DESTDIR. As for point #3, I guess we can continue spreading > > `certctl rehash` all over the tree in various points that may need it; > > the release(7) scripts will need to be done if we don't do it here at > > a minimum, and I haven't put much thought into it beyond that. > > I'm not in a rush to back this out given that it's solving a real > problem, but lets talk about improvements. > > I kind of feel like this belongs in distribution (which I think would > deal with release scripts) or in etcupdate/mergemaster, but I'm not > sure either of those are correct. I'd be happy to review changes to > update the METALOG (I guess we'd extend certctl with an option to do > that?) I think that's the most important things because we really > should be routinely validating that DESTDIR only contains things in the > METALOG. A quick and dirty fix for the DESTDIR weirdness might be adding > "env DESTDIR=${DESTDIR}" so it's explicit. > Got it- not an egregious enough violation to promptly back out, but we will work towards a better solution. For the larger picture, distribution is probably the correct spot for this; we currently rehash at the following points: - mergemaster/etcupdate - freebsd-update - post-install of the caroot pkg (pkgbase) This leaves two primary final blind spots, which this commit was attempting to resolve: 1.) Install media (including VM images) 2.) Initial installs (e.g. from bsdinstall) #1 could be solved by directly adding it to the release(7) scripts and #2 could be solved by having bsdinstall do it at config time, but there seem to be multiple potential points that could instead hit both (with a single stone) while also making covering the array of other images that re@ produces (e.g. AWS images) and perhaps other points that we've not immediately considered. To this end, distribution or installworld are probably equally sufficient, but I'm incredibly unfamiliar with the former. My understanding from glancing at it over the years is that it may be executed into a tempdir and merged with mergemaster/etcupdate, or directly into the new root if we're looking at a new install that doesn't have potential local changes to merge in. Part of the problem that I see in my head is probably solved by just having a var that mergemaster/etcupdate can pass in to distribution to not bother with the rehash. We want to keep the mergemaster/etcupdate rehash because if they're running then the target's certainly not been rehashed. OTOH, we probably don't want to rehash multiple times in that pipeline- from a certctl standpoint, it's completely harmless, but it's not the most effective use of system resources and probably looks a bit bad. > > The close-to-infuriating part of the caroot project has been that it's > > incredibly hard to get almost anyone else (with some obvious > > exceptions) to do more than informal discussion on the matter; actual > > review, in particular, is difficult to obtain. > > Thanks for doing all this work! I'm afraid the whole thing gives me > third-rail vibes (not the idea or the implementation, but the potential > for things to go wrong) so I've veered away from the larger reviews. :( > Thanks! I'm effectively diffused now... apologies for the suboptimal tone of my previous message, I do want to work towards a solution that we can all be happy with. Thanks, Kyle Evans