Hi Laszlo 

Appreciate your contribution. I have reviewed the series patches you attached 
here. First, I assume you have verified the patches on OVMF and the 
functionality works well, then, below are my comments:  

1. The patches for OvmfPkg/NetworkPkg (0001-0004) are good to me.

2. For CryptoPkg, the major viewpoint is also related to the 
TlsCipherMappingTable. For this table, only include the supported ciphersuites 
looks more reasonable. After talked with Qin, I know the unsupported cipher 
suites won't be rejected/filtrated by the OpenSSL cipher list setting, if so, 
the cipher suite list that passed from the client to the server in the 
ClientHello message might also include such unsupported cipher suites. In such 
a case, the failure will happen once the server select the unsupported cipher 
suite. From the handshake process view, it's unreasonable since the client sent 
the desired cipher suites, then the server selected one of them but still met 
the error. Anyway, filtrating the unsupported cipher suites as early as 
possible is a wise choice. So, TlsCipherMappingTable should only include the 
supported cipher suites by reference the security requirement of CryptoPkg. 

3. For patch 0006, it's good to me to optimize the searching algorithm since 
the table is larger than before.

4. Can we combined some patches together to make the things simple? e.g. 
Patches 0005/0007/0010/0011/0012/0013. Those patches are the same purpose to 
fix the issues in 0013.

5. For patch 0008, I think it's unnecessary to provide such script. I prefer to 
maintain the TlsCipherMappingTable more statical since it's the internal 
mapping table. How about we keep it as internal assistant tool?

6. For patch 0009 to extend the TlsCipherMappingTable, I think Qin can help us 
to provide the supported cipher suites by reference the security requirement of 
CryptoPkg.

Thanks,
Jiaxin




> -----Original Message-----
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, April 3, 2018 10:52 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>; Gary Ching-Pang Lin
> <g...@suse.com>; Wu, Jiaxin <jiaxin...@intel.com>; Justen, Jordan L
> <jordan.l.jus...@intel.com>; Gao, Liming <liming....@intel.com>; Kinney,
> Michael D <michael.d.kin...@intel.com>; Long, Qin <qin.l...@intel.com>;
> Fu, Siyuan <siyuan...@intel.com>; Ye, Ting <ting...@intel.com>
> Subject: [PATCH 00/13] {Ovmf,Mde,Network,Crypto}Pkg: fixes+features for
> setting HTTPS cipher suites
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: tls_ciphers
> 
> Earlier I posted two patch sets for better platform control of the CA
> certificates used in HTTPS booting (and for putting that control to use
> in OVMF):
> 
>   [edk2] [PATCH 0/5] NetworkPkg: HTTP and TLS updates
> 
>   [edk2] [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list
>                      for HTTPS boot
> 
> These series have been committed; thank you everyone that helped with
> review and testing.
> 
> My next goal is better platform control of the TLS cipher suites that
> are used in HTTPS booting (and similarly, putting that control to use in
> OVMF). That's what this series is about.
> 
> You'll see references to TianoCore BZ#915 in the commit messages. The BZ
> is not public just yet, because I originally thought that I found
> security issues. It turns out that's not the case, so the BZ should be
> opened up soon. Either way, the commit messages contain enough
> information about the code changes.
> 
> I'm aware some of my reviewers are currently traveling for business --
> please take your time and feel free to review the patches whenever it
> best suits you.
> 
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Cc: Gary Ching-Pang Lin <g...@suse.com>
> Cc: Jiaxin Wu <jiaxin...@intel.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Liming Gao <liming....@intel.com>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Cc: Qin Long <qin.l...@intel.com>
> Cc: Siyuan Fu <siyuan...@intel.com>
> Cc: Ting Ye <ting...@intel.com>
> 
> Thanks!
> Laszlo
> 
> Laszlo Ersek (13):
>   OvmfPkg/TlsAuthConfigLib: configure trusted cipher suites for HTTPS
>     boot
>   MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC
>   NetworkPkg/TlsDxe: verify DataSize for EfiTlsCipherList
>   NetworkPkg/TlsDxe: clean up byte order conversion for EfiTlsCipherList
>   CryptoPkg/TlsLib: replace TlsGetCipherString() with
>     TlsGetCipherMapping()
>   CryptoPkg/TlsLib: use binary search in the TlsGetCipherMapping()
>     function
>   CryptoPkg/TlsLib: pre-compute OpensslCipherLength in
>     TlsCipherMappingTable
>   CryptoPkg/TlsLib: add the "TlsMappingTable.sh" POSIX shell script
>   CryptoPkg/TlsLib: extend "TlsCipherMappingTable"
>   CryptoPkg/TlsLib: sort [LibraryClasses] section in the INF file
>   CryptoPkg/TlsLib: sanitize lib classes in internal header and INF
>   CryptoPkg/TlsLib: clean up leading comment for TlsSetCipherList()
>   CryptoPkg/TlsLib: rewrite TlsSetCipherList()
> 
>  CryptoPkg/Include/Library/TlsLib.h                    |   9 +-
>  CryptoPkg/Library/TlsLib/InternalTlsLib.h             |   4 +
>  CryptoPkg/Library/TlsLib/TlsConfig.c                  | 448 
> +++++++++++++++++---
>  CryptoPkg/Library/TlsLib/TlsLib.inf                   |  11 +-
>  CryptoPkg/Library/TlsLib/TlsMappingTable.sh           | 140 ++++++
>  MdePkg/Include/Protocol/Tls.h                         |  10 +
>  NetworkPkg/TlsDxe/TlsProtocol.c                       |  17 +-
>  OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c   |  98 +++++
>  OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf |   3 +-
>  9 files changed, 664 insertions(+), 76 deletions(-)
>  create mode 100644 CryptoPkg/Library/TlsLib/TlsMappingTable.sh
> 
> --
> 2.14.1.3.gb7cf6e02401b

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to