From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1nKp7M-0008Cn-IP for mharc-grub-devel@gnu.org; Thu, 17 Feb 2022 17:19:16 -0500 Received: from eggs.gnu.org ([209.51.188.92]:35754) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nKp7L-0008AT-56 for grub-devel@gnu.org; Thu, 17 Feb 2022 17:19:15 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:29160) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nKp7I-0006s4-J5 for grub-devel@gnu.org; Thu, 17 Feb 2022 17:19:14 -0500 Received: from pps.filterd (m0187473.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 21HLCea0013117; Thu, 17 Feb 2022 22:18:56 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : subject : from : reply-to : to : cc : date : in-reply-to : references : content-type : mime-version : content-transfer-encoding; s=pp1; bh=l3y47XV+QSRnaqLyZJ+KhgtvYCLox+0C36uV7DJ9dDI=; b=O9hcb0q0MbhxPbYeJ9LIZ72jCH/2GE3YB+M4ou9+1Y+cMOicrRS+I+ioaeNDluoxE/Vr 2nLYeo7HrVV9TYRFDkdp+OmcU4uDEBS2+3L4Y7oZkNlGL70/3ZUFrZdkI8EvvnUudvo0 FRqzk/ZV5es0CiX9ZF8fkomaBtdQ+CBne5/fVZMjzV5ejUwjenApeTr711L1izBESCHD b9A2LLK2YiYjr7ksO9sYgnjkcV42GlLSjJI3YsLV+gHk2zqx9G5ErFi9aK7sqE756D/D uhMPf+Vbgih+6V2WZk0rEvLn5jozLWnNjdAbr9rOHU1H7fIoKqTJka2dPU9he14PG92Q sA== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 3e9ugfd7p8-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 17 Feb 2022 22:18:56 +0000 Received: from m0187473.ppops.net (m0187473.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 21HMD1I0017660; Thu, 17 Feb 2022 22:18:55 GMT Received: from ppma01dal.us.ibm.com (83.d6.3fa9.ip4.static.sl-reverse.com [169.63.214.131]) by mx0a-001b2d01.pphosted.com with ESMTP id 3e9ugfd7ny-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 17 Feb 2022 22:18:55 +0000 Received: from pps.filterd (ppma01dal.us.ibm.com [127.0.0.1]) by ppma01dal.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 21HM9YDB005345; Thu, 17 Feb 2022 22:18:54 GMT Received: from b03cxnp08025.gho.boulder.ibm.com (b03cxnp08025.gho.boulder.ibm.com [9.17.130.17]) by ppma01dal.us.ibm.com with ESMTP id 3e91f7h3f0-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 17 Feb 2022 22:18:54 +0000 Received: from b03ledav004.gho.boulder.ibm.com (b03ledav004.gho.boulder.ibm.com [9.17.130.235]) by b03cxnp08025.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 21HMIoGD24641802 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 17 Feb 2022 22:18:50 GMT Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id BB1F378064; Thu, 17 Feb 2022 22:18:50 +0000 (GMT) Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id E60C37805F; Thu, 17 Feb 2022 22:18:48 +0000 (GMT) Received: from jarvis.int.hansenpartnership.com (unknown [9.163.8.111]) by b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP; Thu, 17 Feb 2022 22:18:48 +0000 (GMT) Message-ID: Subject: Re: [PATCH v4 1/2] cryptodisk: add OS provided secret support From: James Bottomley Reply-To: jejb@linux.ibm.com To: The development of GNU GRUB Cc: thomas.lendacky@amd.com, ashish.kalra@amd.com, brijesh.singh@amd.com, david.kaplan@amd.com, jon.grimm@amd.com, tobin@ibm.com, frankeh@us.ibm.com, Dr David Alan Gilbert , dovmurik@linux.vnet.ibm.com, Dov.Murik1@il.ibm.com, Javier Martinez Canillas , GNUtoo@cyberdimension.org, ps@pks.im, Daniel Kiper Date: Thu, 17 Feb 2022 17:18:47 -0500 In-Reply-To: <20220214161812.5677138c@crass-HP-ZBook-15-G2> References: <20220207152944.27183-1-jejb@linux.ibm.com> <20220207152944.27183-2-jejb@linux.ibm.com> <20220214161812.5677138c@crass-HP-ZBook-15-G2> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.34.4 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: AMxgpbRvDSfoub5VO5Vb3hpTCdebPr5T X-Proofpoint-ORIG-GUID: ZL1SSJFNPVwZugeEKrmlL-PW88YZcEk3 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.816,Hydra:6.0.425,FMLib:17.11.62.513 definitions=2022-02-17_09,2022-02-17_01,2021-12-02_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 suspectscore=0 phishscore=0 impostorscore=0 priorityscore=1501 adultscore=0 malwarescore=0 lowpriorityscore=0 mlxlogscore=999 clxscore=1015 bulkscore=0 mlxscore=0 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000 definitions=main-2202170102 Received-SPF: pass client-ip=148.163.156.1; envelope-from=jejb@linux.ibm.com; helo=mx0a-001b2d01.pphosted.com X-Spam_score_int: -19 X-Spam_score: -2.0 X-Spam_bar: -- X-Spam_report: (-2.0 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_MSPIKE_H5=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Feb 2022 22:19:15 -0000 On Mon, 2022-02-14 at 16:18 -0600, Glenn Washburn wrote: > On Mon, 7 Feb 2022 10:29:43 -0500 > James Bottomley wrote: > > > Make use of the new OS provided secrets API so that if the new '-s' > > option is passed in we try to extract the secret from the API > > rather than prompting for it. > > > > The primary consumer of this is AMD SEV, which has been programmed > > to provide an injectable secret to the encrypted virtual > > machine. OVMF provides the secret area and passes it into the EFI > > Configuration Tables. The grub EFI layer pulls the secret out and > > primes the secrets API with it. The upshot of all of this is that > > a SEV protected VM can do an encrypted boot with a protected boot > > secret. > > I think I prefer the key protector framework proposed in the first > patch of Hernan Gatta's Automatic TPM Disk Unlock lock series. It > feels like a more generic mechanism (though admittedly very similar > to yours) because its not tied to cryptodisk. It's not really an either/or, though is it? It looks to me like one can trivially evolve into the other. > I'm not sure where we'd want to use the secrets/protectors framework > outside of cryptodisk, but it seems like it could be useful. The > advantage of this patch is that it allows for the clearing of key > data from memory. So if you evolve one into the other the put API would naturally remain even if the tpm2 protector didn't want it. > I don't think we should have both a secrets and a key protectors > framework, as its seems they are aiming to accomplish basically the > same thing. Well, I don't think we would either. Looking at the protectors API, it would become an incremental patch to this. Where I come from, this is how we do API development: you merge an implementation and an API that's as minimal as possible. You try to make sure the API can be extended if necessary, but you don't extend it yourself until another implementation comes along that needs the extension. This way you don't need to bikeshed about future implementations because the perfect is the enemy of the good. > The name "secrets" seems a bit more generic than "protectors" > because, as in this case, the secret is not protected. There is > something I don't like about the word "secrets", but I don't have a > better suggestion, so this might be what sticks. One thing going for > "secrets" over "protectors", is that the cryptomount option "-s" > works better than "-p" for protectors because that's already taken by > the password option. [...] > + else if (state[4].set) /* secret module */ > > + { > > + grub_err_t rc; > > + > > + if (argc < 1) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "secret module must > > be specified"); > > +#ifndef GRUB_UTIL > > + grub_dl_load (args[0]); > > +#endif > > + se = grub_named_list_find (GRUB_AS_NAMED_LIST > > (secret_providers), args[0]); > > + if (se == NULL) > > + return grub_error (GRUB_ERR_INVALID_COMMAND, "No secret > > provider is found"); > > + > > + rc = se->get (args[1], &cargs.key_data); > > + if (rc) > > + return rc; > > + cargs.key_len = grub_strlen((char *) cargs.key_data); > > It seems better to me to send a pointer to cargs.key_len to se->get() > because it already knows the length without having to do a strlen. > And this will allow NULLs in the key data. The original implementation was based on the grub password limitations in that it had to be a zero terminated ASCII string. While the zero termination could now be relaxed, the ASCII requirement remains, because the LUKS tools don't like passphrases with NULLS in them. It does simplify the code to pass the length pointer into the get API, so I'll do that. [...] > > @@ -1385,7 +1437,7 @@ GRUB_MOD_INIT (cryptodisk) > > { > > grub_disk_dev_register (&grub_cryptodisk_dev); > > cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, > > 0, > > - N_("[-p password] > b>"), > > + N_("[-p password] > s MOD [ID]>"), > > Seems like this should be: > "[-p password|-s MOD [ID]] diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h > > index c6524c9ea..60249f1fc 100644 > > --- a/include/grub/cryptodisk.h > > +++ b/include/grub/cryptodisk.h > > @@ -183,4 +183,18 @@ grub_util_get_geli_uuid (const char *dev); > > grub_cryptodisk_t grub_cryptodisk_get_by_uuid (const char *uuid); > > grub_cryptodisk_t grub_cryptodisk_get_by_source_disk (grub_disk_t > > disk); > > > > +struct grub_secret_entry { > > s/grub_secret_entry/grub_secret_provider/ OK > > + /* as named list */ > > + struct grub_secret_entry *next; > > + struct grub_secret_entry **prev; > > + const char *name; > > + > > + /* additional entries */ > > + grub_err_t (*get) (const char *arg, grub_uint8_t **secret); > > I like having this first arg to pass some data to the secret > provider. I'm guessing this is to accomodate a keyfiles secrets > provider. It seems like it could also be used to differentiate > between different elements using the same secrets provider. > Hypotheticaly say one had a USB device with a bunch of key slots, a > numerical value could be passed to choose which slot to use. In light > of that, perhaps the arg should be a void *, to be more generic. It's always going to be a pointer into the command line arguments, so the type char * is the correct one for that. It could become char **arg and be passed &arg[1] so the provider could take multiple optional arguments, but since the first implementation doesn't consume this, I think that should be for future users to sort out. > > + grub_err_t (*put) (const char *arg, int have_it, grub_uint8_t > > **secret); > > I don't really like the names get and put. Something more descriptive > would be nice. I like "recover_key" or perhaps "recover_secret" for > "get", and "dispose" or "release" for "put". I'm open to other names > also. get_secret; release_secret? James