(D)TLS: Refactoring around CertificateType support

10 files changed, 201 insertions, 64 deletions

diff --git a/crypto/src/tls/AbstractTlsClient.cs b/crypto/src/tls/AbstractTlsClient.cs
index 77f30bb40..5645e1c13 100644
--- a/crypto/src/tls/AbstractTlsClient.cs
+++ b/crypto/src/tls/AbstractTlsClient.cs
@@ -357,31 +357,30 @@ namespace Org.BouncyCastle.Tls
                 }
             }
 
-            /*
-             * RFC 7250 4.1:
-             *
-             * If the client has no remaining certificate types to send in
-             * the client hello, other than the default X.509 type, it MUST omit the
-             * client_certificate_type extension in the client hello.
-             */
-            short[] clientCertTypes = GetAllowedClientCertificateTypes();
-            if (clientCertTypes != null && (clientCertTypes.Length > 1 || clientCertTypes[0] != CertificateType.X509))
             {
-                TlsExtensionsUtilities.AddClientCertificateTypeExtensionClient(clientExtensions, clientCertTypes);
+                /*
+                 * RFC 7250 4.1. If the client has no remaining certificate types to send in the client hello, other
+                 * than the default X.509 type, it MUST omit the client_certificate_type extension [..].
+                 */
+                short[] clientCertTypes = GetAllowedClientCertificateTypes();
+                if (clientCertTypes != null &&
+                    TlsUtilities.ContainsNot(clientCertTypes, 0, clientCertTypes.Length, CertificateType.X509))
+                {
+                    TlsExtensionsUtilities.AddClientCertificateTypeExtensionClient(clientExtensions, clientCertTypes);
+                }
             }
 
-            /*
-             * RFC 7250 4.1:
-             *
-             * If the client has no remaining certificate types to send in
-             * the client hello, other than the default X.509 certificate type, it
-             * MUST omit the entire server_certificate_type extension from the
-             * client hello.
-             */
-            short[] serverCertTypes = GetAllowedServerCertificateTypes();
-            if (serverCertTypes != null && (serverCertTypes.Length > 1 || serverCertTypes[0] != CertificateType.X509))
             {
-                TlsExtensionsUtilities.AddServerCertificateTypeExtensionClient(clientExtensions, serverCertTypes);
+                /*
+                 * RFC 7250 4.1. If the client has no remaining certificate types to send in the client hello, other than
+                 * the default X.509 certificate type, it MUST omit the entire server_certificate_type extension [..].
+                 */
+                short[] serverCertTypes = GetAllowedServerCertificateTypes();
+                if (serverCertTypes != null &&
+                    TlsUtilities.ContainsNot(serverCertTypes, 0, serverCertTypes.Length, CertificateType.X509))
+                {
+                    TlsExtensionsUtilities.AddServerCertificateTypeExtensionClient(clientExtensions, serverCertTypes);
+                }
             }
 
             if (offeringDtlsV12)
diff --git a/crypto/src/tls/AbstractTlsServer.cs b/crypto/src/tls/AbstractTlsServer.cs
index d986add52..f445ac18d 100644
--- a/crypto/src/tls/AbstractTlsServer.cs
+++ b/crypto/src/tls/AbstractTlsServer.cs
@@ -522,18 +522,30 @@ namespace Org.BouncyCastle.Tls
             if (serverCertTypes != null)
             {
                 TlsCredentials credentials = GetCredentials();
-
-                if (credentials == null || !Arrays.Contains(serverCertTypes, credentials.Certificate.CertificateType))
+                if (credentials != null)
                 {
-                    // outcome 2: we support the extension but have no common types
-                    throw new TlsFatalAlert(AlertDescription.unsupported_certificate);
-                }
+                    // TODO An X509 certificate should still be usable as RawPublicKey
+                    short serverCertificateType = credentials.Certificate.CertificateType;
 
-                // outcome 3: we support the extension and have a common type
-                TlsExtensionsUtilities.AddServerCertificateTypeExtensionServer(m_serverExtensions,
-                    credentials.Certificate.CertificateType);
+                    if (CertificateType.OpenPGP == serverCertificateType && isTlsV13)
+                    {
+                        throw new TlsFatalAlert(AlertDescription.internal_error,
+                            "The OpenPGP certificate type MUST NOT be used with TLS 1.3");
+                    }
+
+                    if (!Arrays.Contains(serverCertTypes, serverCertificateType))
+                    {
+                        // outcome 2: we support the extension but have no common types
+                        throw new TlsFatalAlert(AlertDescription.unsupported_certificate);
+                    }
+
+                    // outcome 3: we support the extension and have a common type
+                    TlsExtensionsUtilities.AddServerCertificateTypeExtensionServer(m_serverExtensions,
+                        serverCertificateType);
+                }
             }
 
+            // TODO If we won't be sending a CertificateRequest, this extension can be ignored
             // RFC 7250 4.2 for client_certificate_type
             short[] remoteClientCertTypes = TlsExtensionsUtilities.GetClientCertificateTypeExtensionClient(
                 m_clientExtensions);
@@ -558,13 +570,18 @@ namespace Org.BouncyCastle.Tls
                     short selectedType = -1;
                     for (int i = 0; i < preferredTypes.Length; i++)
                     {
-                        if (Arrays.Contains(nonPreferredTypes, preferredTypes[i]))
+                        short preferredType = preferredTypes[i];
+                        if (CertificateType.OpenPGP == preferredType && isTlsV13)
+                            continue;
+
+                        if (Arrays.Contains(nonPreferredTypes, preferredType))
                         {
-                            selectedType = preferredTypes[i];
+                            selectedType = preferredType;
                             break;
                         }
                     }
 
+                    // TODO Shouldn't be an error unless we REQUIRE client authentication
                     if (selectedType == -1)
                     {
                         // outcome 2: we support the extension but have no common types
diff --git a/crypto/src/tls/CertificateType.cs b/crypto/src/tls/CertificateType.cs
index eed302416..ae83df7c9 100644
--- a/crypto/src/tls/CertificateType.cs
+++ b/crypto/src/tls/CertificateType.cs
@@ -12,5 +12,10 @@ namespace Org.BouncyCastle.Tls
          * RFC 7250
          */
         public const short RawPublicKey = 2;
+
+        public static bool IsValid(short certificateType)
+        {
+            return certificateType >= X509 && certificateType <= RawPublicKey;
+        }
     }
 }
diff --git a/crypto/src/tls/DtlsClientProtocol.cs b/crypto/src/tls/DtlsClientProtocol.cs
index 88ebbb636..2b132f564 100644
--- a/crypto/src/tls/DtlsClientProtocol.cs
+++ b/crypto/src/tls/DtlsClientProtocol.cs
@@ -653,19 +653,13 @@ namespace Org.BouncyCastle.Tls
                 throw new TlsFatalAlert(AlertDescription.handshake_failure);
             }
 
-            TlsClientContextImpl clientContext = state.clientContext;
-            SecurityParameters securityParameters = clientContext.SecurityParameters;
-
             MemoryStream buf = new MemoryStream(body, false);
 
-            CertificateRequest certificateRequest = CertificateRequest.Parse(clientContext, buf);
+            CertificateRequest certificateRequest = CertificateRequest.Parse(state.clientContext, buf);
 
             TlsProtocol.AssertEmpty(buf);
 
             state.certificateRequest = TlsUtilities.ValidateCertificateRequest(certificateRequest, state.keyExchange);
-
-            securityParameters.m_clientCertificateType = TlsExtensionsUtilities.GetClientCertificateTypeExtensionServer(
-                state.serverExtensions, CertificateType.X509);
         }
 
         /// <exception cref="IOException"/>
@@ -1036,6 +1030,11 @@ namespace Org.BouncyCastle.Tls
                         securityParameters.m_statusRequestVersion = 1;
                     }
 
+                    securityParameters.m_clientCertificateType = TlsUtilities.ProcessClientCertificateTypeExtension(
+                        sessionClientExtensions, sessionServerExtensions, AlertDescription.illegal_parameter);
+                    securityParameters.m_serverCertificateType = TlsUtilities.ProcessServerCertificateTypeExtension(
+                        sessionClientExtensions, sessionServerExtensions, AlertDescription.illegal_parameter);
+
                     state.expectSessionTicket = TlsUtilities.HasExpectedEmptyExtensionData(sessionServerExtensions,
                         ExtensionType.session_ticket, AlertDescription.illegal_parameter);
                 }
diff --git a/crypto/src/tls/DtlsServerProtocol.cs b/crypto/src/tls/DtlsServerProtocol.cs
index 30d491990..a3d04f01e 100644
--- a/crypto/src/tls/DtlsServerProtocol.cs
+++ b/crypto/src/tls/DtlsServerProtocol.cs
@@ -684,6 +684,11 @@ namespace Org.BouncyCastle.Tls
                         securityParameters.m_statusRequestVersion = 1;
                     }
 
+                    securityParameters.m_clientCertificateType = TlsUtilities.ProcessClientCertificateTypeExtension(
+                        clientHelloExtensions, state.serverExtensions, AlertDescription.internal_error);
+                    securityParameters.m_serverCertificateType = TlsUtilities.ProcessServerCertificateTypeExtension(
+                        clientHelloExtensions, state.serverExtensions, AlertDescription.internal_error);
+
                     state.expectSessionTicket = TlsUtilities.HasExpectedEmptyExtensionData(state.serverExtensions,
                         ExtensionType.session_ticket, AlertDescription.internal_error);
                 }
@@ -781,8 +786,7 @@ namespace Org.BouncyCastle.Tls
 
             Certificate.ParseOptions options = new Certificate.ParseOptions()
             {
-                CertificateType = TlsExtensionsUtilities.GetClientCertificateTypeExtensionServer(
-                    state.serverExtensions, CertificateType.X509),
+                CertificateType = state.serverContext.SecurityParameters.ClientCertificateType,
                 MaxChainLength = state.server.GetMaxCertificateChainLength(),
             };
 
diff --git a/crypto/src/tls/SecurityParameters.cs b/crypto/src/tls/SecurityParameters.cs
index 7deeeb72b..56c9e1074 100644
--- a/crypto/src/tls/SecurityParameters.cs
+++ b/crypto/src/tls/SecurityParameters.cs
@@ -51,7 +51,8 @@ namespace Org.BouncyCastle.Tls
         internal Certificate m_peerCertificate = null;
         internal ProtocolVersion m_negotiatedVersion = null;
         internal int m_statusRequestVersion = 0;
-        internal short m_clientCertificateType = -1;
+        internal short m_clientCertificateType = CertificateType.X509;
+        internal short m_serverCertificateType = CertificateType.X509;
 
         // TODO[tls-ops] Investigate whether we can handle verify data using TlsSecret
         internal byte[] m_localVerifyData = null;
@@ -264,6 +265,11 @@ namespace Org.BouncyCastle.Tls
             get { return m_pskIdentity; }
         }
 
+        public short ServerCertificateType
+        {
+            get { return m_serverCertificateType; }
+        }
+
         public byte[] ServerRandom
         {
             get { return m_serverRandom; }
diff --git a/crypto/src/tls/TlsClientProtocol.cs b/crypto/src/tls/TlsClientProtocol.cs
index 6968e5e55..5f60c784c 100644
--- a/crypto/src/tls/TlsClientProtocol.cs
+++ b/crypto/src/tls/TlsClientProtocol.cs
@@ -1321,6 +1321,11 @@ namespace Org.BouncyCastle.Tls
                         securityParameters.m_statusRequestVersion = 1;
                     }
 
+                    securityParameters.m_clientCertificateType = TlsUtilities.ProcessClientCertificateTypeExtension(
+                        sessionClientExtensions, sessionServerExtensions, AlertDescription.illegal_parameter);
+                    securityParameters.m_serverCertificateType = TlsUtilities.ProcessServerCertificateTypeExtension(
+                        sessionClientExtensions, sessionServerExtensions, AlertDescription.illegal_parameter);
+
                     this.m_expectSessionTicket = TlsUtilities.HasExpectedEmptyExtensionData(sessionServerExtensions,
                         ExtensionType.session_ticket, AlertDescription.illegal_parameter);
                 }
@@ -1369,10 +1374,6 @@ namespace Org.BouncyCastle.Tls
 
             this.m_certificateRequest = certificateRequest;
 
-            m_tlsClientContext.SecurityParameters.m_clientCertificateType =
-                TlsExtensionsUtilities.GetClientCertificateTypeExtensionServer(m_serverExtensions,
-                    CertificateType.X509);
-
             TlsUtilities.EstablishServerSigAlgs(m_tlsClientContext.SecurityParameters, certificateRequest);
         }
 
@@ -1423,13 +1424,21 @@ namespace Org.BouncyCastle.Tls
             securityParameters.m_encryptThenMac = false;
             securityParameters.m_truncatedHmac = false;
 
-            /*
-             * TODO[tls13] RFC 8446 4.4.2.1. OCSP Status and SCT Extensions.
-             * 
-             * OCSP information is carried in an extension for a CertificateEntry.
-             */
-            securityParameters.m_statusRequestVersion =
-                m_clientExtensions.ContainsKey(ExtensionType.status_request) ? 1 : 0;
+            if (!securityParameters.IsResumedSession)
+            {
+                /*
+                 * TODO[tls13] RFC 8446 4.4.2.1. OCSP Status and SCT Extensions.
+                 * 
+                 * OCSP information is carried in an extension for a CertificateEntry.
+                 */
+                securityParameters.m_statusRequestVersion = m_clientExtensions.ContainsKey(ExtensionType.status_request)
+                    ? 1 : 0;
+
+                securityParameters.m_clientCertificateType = TlsUtilities.ProcessClientCertificateTypeExtension13(
+                    sessionClientExtensions, sessionServerExtensions, AlertDescription.illegal_parameter);
+                securityParameters.m_serverCertificateType = TlsUtilities.ProcessServerCertificateTypeExtension13(
+                    sessionClientExtensions, sessionServerExtensions, AlertDescription.illegal_parameter);
+            }
 
             this.m_expectSessionTicket = false;
 
@@ -1515,10 +1524,6 @@ namespace Org.BouncyCastle.Tls
             AssertEmpty(buf);
 
             m_certificateRequest = TlsUtilities.ValidateCertificateRequest(certificateRequest, m_keyExchange);
-
-            m_tlsClientContext.SecurityParameters.m_clientCertificateType =
-                TlsExtensionsUtilities.GetClientCertificateTypeExtensionServer(m_serverExtensions,
-                    CertificateType.X509);
         }
 
         /// <exception cref="IOException"/>
diff --git a/crypto/src/tls/TlsExtensionsUtilities.cs b/crypto/src/tls/TlsExtensionsUtilities.cs
index bc4ca2714..ffc1c23c1 100644
--- a/crypto/src/tls/TlsExtensionsUtilities.cs
+++ b/crypto/src/tls/TlsExtensionsUtilities.cs
@@ -308,6 +308,14 @@ namespace Org.BouncyCastle.Tls
         }
 
         /// <exception cref="IOException"/>
+        public static short GetClientCertificateTypeExtensionServer(IDictionary<int, byte[]> extensions)
+        {
+            byte[] extensionData = TlsUtilities.GetExtensionData(extensions, ExtensionType.client_certificate_type);
+            return extensionData == null ? (short)-1 : ReadCertificateTypeExtensionServer(extensionData);
+        }
+
+        /// <exception cref="IOException"/>
+        [Obsolete("Use version without 'defaultValue' instead")]
         public static short GetClientCertificateTypeExtensionServer(IDictionary<int, byte[]> extensions,
             short defaultValue)
         {
@@ -429,6 +437,14 @@ namespace Org.BouncyCastle.Tls
         }
 
         /// <exception cref="IOException"/>
+        public static short GetServerCertificateTypeExtensionServer(IDictionary<int, byte[]> extensions)
+        {
+            byte[] extensionData = TlsUtilities.GetExtensionData(extensions, ExtensionType.server_certificate_type);
+            return extensionData == null ? (short)-1 : ReadCertificateTypeExtensionServer(extensionData);
+        }
+
+        /// <exception cref="IOException"/>
+        [Obsolete("Use version without 'defaultValue' instead")]
         public static short GetServerCertificateTypeExtensionServer(IDictionary<int, byte[]> extensions,
             short defaultValue)
         {
diff --git a/crypto/src/tls/TlsServerProtocol.cs b/crypto/src/tls/TlsServerProtocol.cs
index efe055f1b..528440272 100644
--- a/crypto/src/tls/TlsServerProtocol.cs
+++ b/crypto/src/tls/TlsServerProtocol.cs
@@ -318,6 +318,14 @@ namespace Org.BouncyCastle.Tls
                 securityParameters.m_maxFragmentLength = TlsUtilities.ProcessMaxFragmentLengthExtension(
                     securityParameters.IsResumedSession ? null : clientHelloExtensions, serverEncryptedExtensions,
                     AlertDescription.internal_error);
+
+                if (!securityParameters.IsResumedSession)
+                {
+                    securityParameters.m_clientCertificateType = TlsUtilities.ProcessClientCertificateTypeExtension13(
+                        clientHelloExtensions, serverEncryptedExtensions, AlertDescription.internal_error);
+                    securityParameters.m_serverCertificateType = TlsUtilities.ProcessServerCertificateTypeExtension13(
+                        clientHelloExtensions, serverEncryptedExtensions, AlertDescription.internal_error);
+                }
             }
 
             securityParameters.m_encryptThenMac = false;
@@ -711,6 +719,11 @@ namespace Org.BouncyCastle.Tls
                         securityParameters.m_statusRequestVersion = 1;
                     }
 
+                    securityParameters.m_clientCertificateType = TlsUtilities.ProcessClientCertificateTypeExtension(
+                        m_clientExtensions, m_serverExtensions, AlertDescription.internal_error);
+                    securityParameters.m_serverCertificateType = TlsUtilities.ProcessServerCertificateTypeExtension(
+                        m_clientExtensions, m_serverExtensions, AlertDescription.internal_error);
+
                     this.m_expectSessionTicket = TlsUtilities.HasExpectedEmptyExtensionData(m_serverExtensions,
                         ExtensionType.session_ticket, AlertDescription.internal_error);
                 }
@@ -1311,8 +1324,7 @@ namespace Org.BouncyCastle.Tls
 
             Certificate.ParseOptions options = new Certificate.ParseOptions()
             {
-                CertificateType = TlsExtensionsUtilities.GetClientCertificateTypeExtensionServer(m_serverExtensions,
-                    CertificateType.X509),
+                CertificateType = m_tlsServerContext.SecurityParameters.ClientCertificateType,
                 MaxChainLength = m_tlsServer.GetMaxCertificateChainLength(),
             };
 
@@ -1351,8 +1363,7 @@ namespace Org.BouncyCastle.Tls
 
             Certificate.ParseOptions options = new Certificate.ParseOptions()
             {
-                CertificateType = TlsExtensionsUtilities.GetClientCertificateTypeExtensionServer(m_serverExtensions,
-                    CertificateType.X509),
+                CertificateType = m_tlsServerContext.SecurityParameters.ClientCertificateType,
                 MaxChainLength = m_tlsServer.GetMaxCertificateChainLength(),
             };
 
diff --git a/crypto/src/tls/TlsUtilities.cs b/crypto/src/tls/TlsUtilities.cs
index a9c7629d6..2887b0df1 100644
--- a/crypto/src/tls/TlsUtilities.cs
+++ b/crypto/src/tls/TlsUtilities.cs
@@ -585,6 +585,8 @@ namespace Org.BouncyCastle.Tls
         {
             if (buf == null)
                 throw new ArgumentNullException("buf");
+            if (buf.Length < 1)
+                throw new TlsFatalAlert(AlertDescription.decode_error);
 
             int count = ReadUint8(buf, 0);
             if (buf.Length != (count + 1))
@@ -4711,6 +4713,16 @@ namespace Org.BouncyCastle.Tls
             return true;
         }
 
+        internal static bool ContainsNot(short[] buf, int off, int len, short value)
+        {
+            for (int i = 0; i < len; ++i)
+            {
+                if (value != buf[off + i])
+                    return true;
+            }
+            return false;
+        }
+
         internal static short[] RetainAll(short[] retainer, short[] elements)
         {
             short[] retained = new short[System.Math.Min(retainer.Length, elements.Length)];
@@ -4841,8 +4853,7 @@ namespace Org.BouncyCastle.Tls
 
             Certificate.ParseOptions options = new Certificate.ParseOptions()
             {
-                CertificateType = TlsExtensionsUtilities.GetServerCertificateTypeExtensionServer(serverExtensions,
-                    CertificateType.X509),
+                CertificateType = securityParameters.ServerCertificateType,
                 MaxChainLength = client.GetMaxCertificateChainLength(),
             };
 
@@ -4872,8 +4883,7 @@ namespace Org.BouncyCastle.Tls
 
             Certificate.ParseOptions options = new Certificate.ParseOptions()
             {
-                CertificateType = TlsExtensionsUtilities.GetServerCertificateTypeExtensionServer(serverExtensions,
-                    CertificateType.X509),
+                CertificateType = securityParameters.ServerCertificateType,
                 MaxChainLength = client.GetMaxCertificateChainLength(),
             };
 
@@ -5748,6 +5758,71 @@ namespace Org.BouncyCastle.Tls
             return maxFragmentLength;
         }
 
+        /// <exception cref="IOException"/>
+        internal static short ProcessClientCertificateTypeExtension(IDictionary<int, byte[]> clientExtensions,
+            IDictionary<int, byte[]> serverExtensions, short alertDescription)
+        {
+            short serverValue = TlsExtensionsUtilities.GetClientCertificateTypeExtensionServer(serverExtensions);
+            if (serverValue < 0)
+                return CertificateType.X509;
+
+            if (!CertificateType.IsValid(serverValue))
+                throw new TlsFatalAlert(alertDescription, "Unknown value for client_certificate_type");
+
+            short[] clientValues = TlsExtensionsUtilities.GetClientCertificateTypeExtensionClient(clientExtensions);
+            if (clientValues == null || !Arrays.Contains(clientValues, serverValue))
+                throw new TlsFatalAlert(alertDescription, "Invalid selection for client_certificate_type");
+
+            return serverValue;
+        }
+
+        /// <exception cref="IOException"/>
+        internal static short ProcessClientCertificateTypeExtension13(IDictionary<int, byte[]> clientExtensions,
+            IDictionary<int, byte[]> serverExtensions, short alertDescription)
+        {
+            short certificateType = ProcessClientCertificateTypeExtension(clientExtensions, serverExtensions,
+                alertDescription);
+
+            return ValidateCertificateType13(certificateType, alertDescription);
+        }
+
+        /// <exception cref="IOException"/>
+        internal static short ProcessServerCertificateTypeExtension(IDictionary<int, byte[]> clientExtensions,
+            IDictionary<int, byte[]> serverExtensions, short alertDescription)
+        {
+            short serverValue = TlsExtensionsUtilities.GetServerCertificateTypeExtensionServer(serverExtensions);
+            if (serverValue < 0)
+                return CertificateType.X509;
+
+            if (!CertificateType.IsValid(serverValue))
+                throw new TlsFatalAlert(alertDescription, "Unknown value for server_certificate_type");
+
+            short[] clientValues = TlsExtensionsUtilities.GetServerCertificateTypeExtensionClient(clientExtensions);
+            if (clientValues == null || !Arrays.Contains(clientValues, serverValue))
+                throw new TlsFatalAlert(alertDescription, "Invalid selection for server_certificate_type");
+
+            return serverValue;
+        }
+
+        /// <exception cref="IOException"/>
+        internal static short ProcessServerCertificateTypeExtension13(IDictionary<int, byte[]> clientExtensions,
+            IDictionary<int, byte[]> serverExtensions, short alertDescription)
+        {
+            short certificateType = ProcessServerCertificateTypeExtension(clientExtensions, serverExtensions,
+                alertDescription);
+
+            return ValidateCertificateType13(certificateType, alertDescription);
+        }
+
+        /// <exception cref="IOException"/>
+        private static short ValidateCertificateType13(short certificateType, short alertDescription)
+        {
+            if (CertificateType.OpenPGP == certificateType)
+                throw new TlsFatalAlert(alertDescription, "The OpenPGP certificate type MUST NOT be used with TLS 1.3");
+
+            return certificateType;
+        }
+
         // TODO[api] Not needed once GetHandshakeResendTimeMillis() has been added to TlsPeer
         internal static int GetHandshakeResendTimeMillis(TlsPeer tlsPeer)
         {