Re: [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites

[Laszlo Ersek <lersek@redhat.com>]

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