> On Jan 16, 2022, at 1:10 PM, Jarkko Sakkinen <jarkko@xxxxxxxxxx> wrote: > > On Sat, Jan 15, 2022 at 09:55:47PM -0500, Mimi Zohar wrote: >> On Sat, 2022-01-15 at 21:15 +0200, Jarkko Sakkinen wrote: >>> On Sat, Jan 15, 2022 at 09:14:45PM +0200, Jarkko Sakkinen wrote: >>>> On Sat, Jan 15, 2022 at 07:12:35PM +0000, Eric Snowberg wrote: >>>>> >>>>> >>>>>> On Jan 15, 2022, at 10:11 AM, Jarkko Sakkinen <jarkko@xxxxxxxxxx> wrote: >>>>>> >>>>>> On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote: >>>>>>> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote: >>>>>>>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote: >>>>>>>>> >>>>>>>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote: >>>>>>>>>> >>>>>>>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote: >>>>>>>>>>>> Jarkko, my concern is that once this version of the patch set is >>>>>>>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine >>>>>>>>>>>> keyring be considered a regression? >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Currently certificates built into the kernel do not have a CA restriction on them. >>>>>>>>>>> IMA will trust anything in this keyring even if the CA bit is not set. While it would >>>>>>>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. >>>>>>>>>>> >>>>>>>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig. >>>>>>>>>>> This Kconfig would do the CA enforcement on the machine keyring. However if the >>>>>>>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, >>>>>>>>>>> plus it would allow IMA to work with non-CA keys. This would be done by removing >>>>>>>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would >>>>>>>>>>> be an appropriate solution. I believe this would get around what you are identifying as >>>>>>>>>>> a possible regression. >>>>>>>>>> >>>>>>>>>> True the problem currently exists with the builtin keys, but there's a >>>>>>>>>> major difference between trusting the builtin keys and those being >>>>>>>>>> loading via MOK. This is an integrity gap that needs to be closed and >>>>>>>>>> shouldn't be expanded to keys on the .machine keyring. >>>>>>>>>> >>>>>>>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable. >>>>>>>>> >>>>>>>>> Ok, I’ll leave that part out. Could you clarify the wording I should include in the future >>>>>>>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to >>>>>>>>> make this decision? >>>>>>>> >>>>>>>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY >>>>>>>> "help" is very clear: >>>>>>> >>>>>>> [Reposting the text due to email formatting issues.] >>>>>>> >>>>>>> help >>>>>>> Keys may be added to the IMA or IMA blacklist keyrings, if the >>>>>>> key is validly signed by a CA cert in the system built-in or >>>>>>> secondary trusted keyrings. >>>>>>> >>>>>>> Intermediate keys between those the kernel has compiled in and the >>>>>>> IMA keys to be added may be added to the system secondary keyring, >>>>>>> provided they are validly signed by a key already resident in the >>>>>>> built-in or secondary trusted keyrings. >>>>>>> >>>>>>> >>>>>>> The first paragraph requires "validly signed by a CA cert in the system >>>>>>> built-in or secondary trusted keyrings" for keys to be loaded onto the >>>>>>> IMA keyring. This Kconfig is limited to just the builtin and secondary >>>>>>> keyrings. Changing this silently to include the ".machine" keyring >>>>>>> introduces integrity risks that previously did not exist. A new IMA >>>>>>> Kconfig needs to be defined to allow all three keyrings - builtin, >>>>>>> machine, and secondary. >>>>>>> >>>>>>> The second paragraph implies that only CA and intermediate CA keys are >>>>>>> on secondary keyring, or as in our case the ".machine" keyring linked >>>>>>> to the secondary keyring. >>>>>>> >>>>>>> Mimi >>>>>>> >>>>>> I have also now test environment for this patch set but if there are >>>>>> any possible changes, I'm waiting for a new version, as it is anyway >>>>>> for 5.18 cycle earliest. >>>>> >>>>> Other than the two sentence changes, I have not seen anything identified >>>>> code wise requiring a change. If you’d like me to respin a v10 with the sentence >>>>> changes let me know. Or if you want to remove the ima reference, that works >>>>> too. Just let me know how you want to handle this. Thanks. >>>> >>>> I'm basically waiting also Mimi to test this as I do not have IMA test >>>> environment. >>>> >>>> From my side: >>>> >>>> Tested-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx> >>> >>> I can pick the whole thing at the time when I get green light. >> >> The MOK keys are not loaded onto the .machine keyring if >> CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY is enabled. >> From an IMA perspective nothing has changed. >> >> After the IMA references in the patch descriptions are removed, feel >> free to add Tested-by: Mimi Zohar <zohar@xxxxxxxxxxxxx> on patches 1 - >> 5. >> >> thanks, >> >> Mimi > > Eric, for me it would be at least a convenience, and overally it would > make sure that I pick the right thing if you would fix the typos (and > you can add all the tested-by tags of course as no functional changes). > > There's been times when I've manually "just fixed typos", and failed in a > way or another because of human error. Just want to make sure that we > have exactly the right content applied, I hope you understand my point > of view. And we are early for the 5.18 release cycle anyway. No problem, I’ll put together a v10 with the changes. Thanks for your review.
