From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BEAF7225FA062 for ; Tue, 10 Apr 2018 03:02:30 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EC08640201A1; Tue, 10 Apr 2018 10:02:29 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-125.rdu2.redhat.com [10.10.120.125]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3F2742166BAD; Tue, 10 Apr 2018 10:02:28 +0000 (UTC) To: "Long, Qin" Cc: "Wu, Jiaxin" , edk2-devel-01 , Ard Biesheuvel , Gary Ching-Pang Lin , "Justen, Jordan L" , "Gao, Liming" , "Kinney, Michael D" , "Fu, Siyuan" , "Ye, Ting" References: <20180403145149.8925-1-lersek@redhat.com> <895558F6EA4E3B41AC93A00D163B7274163B458F@SHSMSX103.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <475313c2-8a3a-4be4-483c-b15b4d1cbfa6@redhat.com> Date: Tue, 10 Apr 2018 12:02:27 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 10 Apr 2018 10:02:30 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 10 Apr 2018 10:02:30 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Apr 2018 10:02:31 -0000 Content-Type: text/plain; charset=UTF-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 04/10/18 09:40, Long, Qin wrote: > Hi, Laszlo, > > Some comments / discussions were added in https://bugzilla.tianocore.org/show_bug.cgi?id=915 > with comment 09 & 11. Right. There I missed your main point about "TlsCipherMappingTable"; I'm sorry about that. With Jiaxin's more detailed explanation, I get it now. > Back to the patch review. Some comments were appended: > > #0001, #0003, #0004，#0010，#0011: > Looks good to me. Thanks -- are you OK if I squash 10 and 11, like Jiaxin suggests? > #0002 - I personally think in general we should reduce using "#pragma pack", except that these > data really have serialization requirement (e.g. variable) to match extra data layout. > Here we just use these structures for setting / getting data, instead of direct data > transport. I am thinking if it's better to update the implementation part. > But too many sizeof() were used, and Ovmf part may also need to store preferred > CipherList data. So it's still good to me to pack something. Well, I think the *set* of structures that should be packed is clear -- given that we make explicit references to the TLS RFC, I believe we *must* pack those structures. The remaining question I see (with reference to Liming's suggestion earlier) is whether we should use separate #pragma directives for the individual struct types, or if we should move those structs to a common section of the header file called "structures from the TLS RFC" or something like that, and pack them with a single #pragma. > #0008, #0009: > - As the BZ comments. The TlsCipherMappingTable extension and generation with script looks > incorrect. This table should include all available / supported ciphers, which was actually > platform / configuration dependent. > I prefer to maintain one static / limited table for edk2 tls implementation. Any new cipher > requirement can be evaluated & enabled, and then added into this table. Yup, I'm ready to drop patches #8 and #9. Thanks for your patience explaining. > #0005, #0006, #0007, #0012， #0013: > These implementation looks good to me. > But some of updates were based on the assumption of #0008-0009. I have no strong opinion > if some original light implementation are good enough currently. Thanks. My personal preference would be the following -- making sure Jiaxin is CC'd... yes, he is: (a) clarify how exactly we want to pack the structures in patch #2, and rework patch #2 according to agreement (b) keep *each* of the following patches separate: 01 OvmfPkg/TlsAuthConfigLib: configure trusted cipher suites for HTTPS boot 02 MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC 03 NetworkPkg/TlsDxe: verify DataSize for EfiTlsCipherList 04 NetworkPkg/TlsDxe: clean up byte order conversion for EfiTlsCipherList 05 CryptoPkg/TlsLib: replace TlsGetCipherString() with TlsGetCipherMapping() 06 CryptoPkg/TlsLib: use binary search in the TlsGetCipherMapping() function 07 CryptoPkg/TlsLib: pre-compute OpensslCipherLength in TlsCipherMappingTable (c) drop the following patches: 08 CryptoPkg/TlsLib: add the "TlsMappingTable.sh" POSIX shell script 09 CryptoPkg/TlsLib: extend "TlsCipherMappingTable" (d) squash the following patches together (into one patch): 10 CryptoPkg/TlsLib: sort [LibraryClasses] section in the INF file 11 CryptoPkg/TlsLib: sanitize lib classes in internal header and INF (e) squash the following patches together (into another patch): 12 CryptoPkg/TlsLib: clean up leading comment for TlsSetCipherList() 13 CryptoPkg/TlsLib: rewrite TlsSetCipherList() Jiaxin, does this look OK to you? Thanks! Laszlo