Hi, Laszlo,

I prefer to open a separate BZ for this TlsCipherMappingTable update.
Current list was produced by some rough collections from Jiaxin and me, which 
meet the basic cipher requirement for TLS(v1.0/1.1/1.2) to set up one 
successful connection.

Will re-sorted this table based on IANA & IETF-RFCs & EDKII-openssl build 
options.


Best Regards & Thanks,
LONG, Qin

From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
Ersek
Sent: Tuesday, April 10, 2018 5:48 PM
To: Wu, Jiaxin <jiaxin...@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>; Ye, Ting <ting...@intel.com>; 
Justen, Jordan L <jordan.l.jus...@intel.com>; Gao, Liming 
<liming....@intel.com>; Gary Ching-Pang Lin <g...@suse.com>; Long, Qin 
<qin.l...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>; Fu, 
Siyuan <siyuan...@intel.com>
Subject: Re: [edk2] [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: 
fixes+features for setting HTTPS cipher suites

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:

4A89E2EF3DFEDB4C8BFDE51014F606A14E1F1B10@SHSMSX104.ccr.corp.intel.com">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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to