Re: [staging/HTTPS-TLS][PATCH 0/4] Replace the TLS definitions with the standardized one

["Palmer, Thomas" <thomas.palmer@hpe.com>]

Jiaxin / Qin,


	I'm unaware of what criteria is required for a cipher to be in this TlsCipherMappingTable.  I had presumed that it would be b/c UEFI supported the cipher for TLS operation.  If unsupported ciphers are allowed ... then logically wouldn't we need to add all ciphers?  What advantage do we gain by having an entry in this table but not actually use the cipher in communication?
	
	Currently TlsGetCipherString is the only means we have to validate the cipher string.  If a cipher is in the table but not in OpenSSL lib, then we will provide imperfect feedback if the unsupported cipher is buried in a list of supported ciphers.  OpenSSL will simply use only the ciphers it supports and quietly drop the unsupported cipher.  A user that inspects the list of set ciphers would be curious why a certain cipher was being "dropped" / "filtered".   However, if TlsGetCipherString sees that the cipher is not in our mapping table the TlsSetCipherList function will return immediate feedback.

	I'm not enthralled with supporting weak/idea ciphers either.  I would vouch for us removing those ciphers from TlsCipherMappingTable.  It is not our responsibility to document the IANA/Description string description in code.

	This document (http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-52r1.pdf) would be a good list for initial cipher support.  We have some of the ciphers on the list already.  Here is the sorted list:

TLS_DH_DSS_WITH_3DES_EDE_CBC_SHA
TLS_DH_DSS_WITH_AES_128_CBC_SHA
TLS_DH_DSS_WITH_AES_128_CBC_SHA256
TLS_DH_DSS_WITH_AES_128_GCM_SHA256
TLS_DH_DSS_WITH_AES_256_CBC_SHA
TLS_DH_DSS_WITH_AES_256_CBC_SHA256
TLS_DH_DSS_WITH_AES_256_GCM_SHA384
TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA
TLS_DHE_DSS_WITH_AES_128_CBC_SHA
TLS_DHE_DSS_WITH_AES_128_CBC_SHA256
TLS_DHE_DSS_WITH_AES_128_GCM_SHA256
TLS_DHE_DSS_WITH_AES_256_CBC_SHA
TLS_DHE_DSS_WITH_AES_256_CBC_SHA256
TLS_DHE_DSS_WITH_AES_256_GCM_SHA384
TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA
TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA
TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA256
TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256
TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA
TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA384
TLS_ECDH_ECDSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384
TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
TLS_RSA_WITH_3DES_EDE_CBC_SHA
TLS_RSA_WITH_AES_128_CBC_SHA
TLS_RSA_WITH_AES_128_CBC_SHA256
TLS_RSA_WITH_AES_128_CCM17
TLS_RSA_WITH_AES_128_GCM_SHA256
TLS_RSA_WITH_AES_256_CBC_SHA
TLS_RSA_WITH_AES_256_CBC_SHA256
TLS_RSA_WITH_AES_256_CCM
TLS_RSA_WITH_AES_256_GCM_SHA384

Thomas

-----Original Message-----
From: Long, Qin [mailto:qin.long@intel.com] 
Sent: Sunday, July 31, 2016 8:48 PM
To: Wu, Jiaxin <jiaxin.wu@intel.com>; Palmer, Thomas <thomas.palmer@hpe.com>; edk2-devel@lists.01.org
Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: RE: [staging/HTTPS-TLS][PATCH 0/4] Replace the TLS definitions with the standardized one

I personally prefer to keep the current supported cipher suite for our UEFI-TLS enabling. We can have the full RFC definitions, and platform specific cipher sets for validation now. It's better to maintain one minimal scope in this phase.

"enable-weak-ssl-ciphers" looks odd. Disabling weak ciphers is the recommendation for hardening SSL communications.
For other ciphers (idea, dsa, etc), we can enable them step-by-step depending on the real requirements. 


Best Regards & Thanks,
LONG, Qin

> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Monday, August 01, 2016 9:23 AM
> To: Palmer, Thomas; Long, Qin; edk2-devel@lists.01.org
> Cc: Ye, Ting; Fu, Siyuan; Gao, Liming
> Subject: RE: [staging/HTTPS-TLS][PATCH 0/4] Replace the TLS 
> definitions with the standardized one
> 
> Thomas,
> I agree some of them are not supported due to the UEFI OpenSSL 
> configuration, but it doesn't affect those mapping relationship added 
> in the patch. So, I have no strong opinion whether to support it by 
> modifying the current OpenSSL configuration. Since Qin is the OpenSSL 
> expert, I'd like to hear his views.
> 
> Qin,
> What's your opinion?
> 
> Thanks.
> Jiaxin
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf 
> > Of Palmer, Thomas
> > Sent: Saturday, July 30, 2016 6:03 AM
> > To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> > Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; 
> > Gao, Liming <liming.gao@intel.com>; Long, Qin <qin.long@intel.com>
> > Subject: Re: [edk2] [staging/HTTPS-TLS][PATCH 0/4] Replace the TLS 
> > definitions with the standardized one
> >
> > Jiaxin,
> >
> > 	UEFI's OpenSSL library does not support all the ciphers that were 
> > added in your patch due to the UEFI configuration.  We need to 
> > remove
> > "no- idea" and "no-dsa" from the process_files.sh and add
> > "enable-weak-ssl- ciphers"
> >
> > 	While we are modifying process_files.sh, we can remove "no-
> pqueue"
> > from process_files.sh so that OpensslLib.inf is in sync.
> >
> > 	I can send out a patch to do so if you wish.
> >
> > Thomas
> >
> > -----Original Message-----
> > From: Jiaxin Wu [mailto:jiaxin.wu@intel.com]
> > Sent: Thursday, July 14, 2016 12:51 AM
> > To: edk2-devel@lists.01.org
> > Cc: Liming Gao <liming.gao@intel.com>; Palmer, Thomas 
> > <thomas.palmer@hpe.com>; Long Qin <qin.long@intel.com>; Ye Ting 
> > <ting.ye@intel.com>; Fu Siyuan <siyuan.fu@intel.com>; Wu Jiaxin 
> > <jiaxin.wu@intel.com>
> > Subject: [staging/HTTPS-TLS][PATCH 0/4] Replace the TLS definitions 
> > with the standardized one
> >
> > The series patches are used to replace the TLS definitions with the 
> > standardized one. In addition, more TLS cipher suite mapping between 
> > Cipher Suite definitions and OpenSSL-used Cipher Suite name are added.
> >
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Palmer Thomas <thomas.palmer@hpe.com>
> > Cc: Long Qin <qin.long@intel.com>
> > Cc: Ye Ting <ting.ye@intel.com>
> > Cc: Fu Siyuan <siyuan.fu@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> > Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> >
> > Jiaxin Wu (4):
> >   MdePkg: Add a header to standardize TLS definitions
> >   CryptoPkg: Add more TLS cipher suite mapping
> >   NetworkPkg/TlsDxe: Replace the definitions with the standardized one
> >   NetworkPkg/HttpDxe: Replace the definitions with the standardized 
> > one
> >
> >  CryptoPkg/Library/TlsLib/TlsLib.c      | 3585 ++++++++++++++++--------------
> --
> >  MdePkg/Include/IndustryStandard/Tls1.h |   93 +
> >  NetworkPkg/HttpDxe/HttpDriver.h        |    2 +
> >  NetworkPkg/HttpDxe/HttpProto.c         |   12 +-
> >  NetworkPkg/HttpDxe/HttpsSupport.c      |   22 +-
> >  NetworkPkg/HttpDxe/HttpsSupport.h      |   44 -
> >  NetworkPkg/TlsDxe/TlsImpl.c            |   56 +-
> >  NetworkPkg/TlsDxe/TlsImpl.h            |   30 +-
> >  NetworkPkg/TlsDxe/TlsProtocol.c        |    2 +-
> >  9 files changed, 1945 insertions(+), 1901 deletions(-)  create mode
> > 100644 MdePkg/Include/IndustryStandard/Tls1.h
> >
> > --
> > 1.9.5.msysgit.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel