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 06:09, Wu, Jiaxin wrote:
> 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,

That's right; I tested cipher suite negotiation failures and successes.

For example, I configured apache to "Disable All SSL and TLS Protocols
Except TLS 1 and Up"
<https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/system_administrators_guide/ch-web_servers#s2-apache-mod_ssl-enabling>,
and then I verified that HTTPS boot would succeed vs. fail dependent on
whether I passed strong ciphers too, or only weak ones, to OVMF.

> then, below are my comments:  
> 
> 1. The patches for OvmfPkg/NetworkPkg (0001-0004) are good to me.

Thanks. For patch #2, "MdePkg/Include/Protocol/Tls.h: pack structures
from the TLS RFC", we exchanged some points with Liming earlier:

http://mid.mail-archive.com/4A89E2EF3DFEDB4C8BFDE51014F606A14E1F1B10@SHSMSX104.ccr.corp.intel.com

(Please see also my response to that.)

I see that both you and Siyuan are OK with patch #2, i.e. with the
separate #pragma directives. I'd also like Liming to confirm that he
accepts the patch as-is.

> 
> 2. For CryptoPkg, the major viewpoint is also related to the TlsCipherMappingTable. For this table, only include the supported ciphersuites looks more reasonable.

OK.

I think this means that I should preserve patches #5 through #7, and
drop patches #8 ('CryptoPkg/TlsLib: add the "TlsMappingTable.sh" POSIX
shell script') and #9 ('CryptoPkg/TlsLib: extend "TlsCipherMappingTable"').

Is that correct?

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

Oh! You are totally right. I apologize for missing this -- I didn't
realize this from Qin's comments on TianoCore #915.

In other words, it is actually *important* that "TlsCipherMappingTable"
match the cipher suites that we build into edk2. I understand now. Thanks!

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

Yes.

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

I'm not against squashing these patches together, but separating patch
#6 (the binary search) out of the middle is not possible without a
rewrite of that patch, because it has context dependencies on patch #5.

Do you want me to do that? I.e., first implement the binary search for
TlsGetCipherString() -- without changing the interface --, and *then*
switch it over to TlsGetCipherMapping(), as part of the large squashed
patch?

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

Sure, given that TlsCipherMappingTable *must* match the ciphers that we
build into OpensslLib, updating that table semi-automatically is out of
question (it can only be done manually, in synch with OpensslLib.inf
changes). Therefore the shell script is useless. I'll just drop the patch.

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

Right -- I think I'll simply drop patch #9 from the v2 series, and I'll
let Qin post a separate patch later, so that TlsCipherMappingTable
matches the ciphers we build in 100%.

Qin, do you prefer that I open a separate TianoCore BZ for this? Also,
would you like to post the patch for TlsCipherMappingTable before or
after my v2?

Thanks!
Laszlo