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

> #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

  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

  05 CryptoPkg/TlsLib: replace TlsGetCipherString() with

  06 CryptoPkg/TlsLib: use binary search in the TlsGetCipherMapping()

  07 CryptoPkg/TlsLib: pre-compute OpensslCipherLength in

(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?

edk2-devel mailing list

Reply via email to