Removed HandshakeParser::parseError and hid/renamed its parseDone.

The presence of two "persistent" parsing outcomes (done and error) was
confusing to the callers: Which one do I check and when? The adjusted
interface uses exceptions for errors and a false parseHandshake() return
value to signal "need more data". This simplifies the API and untangles
the calling code quite a bit.

diff --git a/src/client_side.cc b/src/client_side.cc
index 14d1795dac7003487b897f71fdd43d869ad2be84..917471ed02291cad4692f2767f470cb1fa4f82a1 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2187,19 +2187,12 @@ void
 ConnStateData::afterClientRead()
 {
 #if USE_OPENSSL
-    if (atTlsPeek) {
-        assert(!inBuf.isEmpty());
-        if (!tlsParser.parseHello(inBuf)) {
-            if (!tlsParser.parseError) {
-                readSomeData();
-                return;
-            }
-        }
-        atTlsPeek = false;
-        afterClientHelloPeeked();
+    if (parsingTlsHandshake) {
+        parseTlsHandshake();
         return;
     }
 #endif
+
     /* Process next request */
     if (pipeline.empty())
         fd_note(clientConnection->fd, "Reading next request");
@@ -2432,9 +2425,9 @@ ConnStateData::ConnStateData(const MasterXaction::Pointer &xact) :
     needProxyProtocolHeader_(false),
 #if USE_OPENSSL
     switchedToHttps_(false),
+    parsingTlsHandshake(false),
     sslServerBump(NULL),
     signAlgorithm(Ssl::algSignTrusted),
-    atTlsPeek(false),
 #endif
     stoppedSending_(NULL),
     stoppedReceiving_(NULL)
@@ -3150,23 +3143,37 @@ ConnStateData::switchToHttps(HttpRequest *request, Ssl::BumpMode bumpServerMode)
     // a bumbed "connect" request on non transparent port.
     receivedFirstByte_ = false;
     // Get more data to peek at Tls
-    atTlsPeek = true;
+    parsingTlsHandshake = true;
     readSomeData();
 }
 
 void
-ConnStateData::afterClientHelloPeeked()
+ConnStateData::parseTlsHandshake()
 {
+    Must(parsingTlsHandshake);
+
+    assert(!inBuf.isEmpty());
     receivedFirstByte();
+    fd_note(clientConnection->fd, "Parsing TLS handshake");
+
+    bool unsupportedProtocol = false;
+    try {
+        if (!tlsParser.parseHello(inBuf)) {
+            // need more data to finish parsing
+            readSomeData();
+            return;
+        }
+    }
+    catch (const std::exception &ex) {
+        debugs(83, 2, "error on FD " << clientConnection->fd << ": " << ex.what());
+        unsupportedProtocol = true;
+    }
 
-    // Avoid the check for tlsParser error, in the case of bug in our tls parser.
-    // The bumpServerFirst and bumpClientFirst should not depend on tlsParser
-    // result. For the peek-and-splice mode, the tlsParser.parseError checked and
-    // handled inside startPeekAndsSplice method.
+    parsingTlsHandshake = false;
 
-    // Record parsed info
-    if (!tlsParser.parseError)
-        clientConnection->tlsNegotiations()->retrieveParsedInfo(tlsParser.details);
+    // Even if the parser failed, each TLS detail should either be set
+    // correctly or still be "unknown"; copying unknown detail is a no-op.
+    clientConnection->tlsNegotiations()->retrieveParsedInfo(tlsParser.details);
 
     // We should disable read/write handlers
     Comm::SetSelect(clientConnection->fd, COMM_SELECT_READ, NULL, NULL, 0);
@@ -3180,7 +3187,7 @@ ConnStateData::afterClientHelloPeeked()
         FwdState::fwdStart(clientConnection, sslServerBump->entry, sslServerBump->request.getRaw());
     } else {
         Must(sslServerBump->act.step1 == Ssl::bumpPeek || sslServerBump->act.step1 == Ssl::bumpStare);
-        startPeekAndSplice();
+        startPeekAndSplice(unsupportedProtocol);
     }
 }
 
@@ -3202,9 +3209,9 @@ ConnStateData::spliceOnError(const err_type err)
 }
 
 
-void ConnStateData::startPeekAndSplice()
+void ConnStateData::startPeekAndSplice(const bool unsupportedProtocol)
 {
-    if (tlsParser.parseError) {
+    if (unsupportedProtocol) {
         if (!spliceOnError(ERR_PROTOCOL_UNKNOWN))
             clientConnection->close();
         return;

diff --git a/src/client_side.h b/src/client_side.h
index ea83290ac3e163d909b8c4aa5eee83c6161e699c..34d7e58b99d9f678b7e757bcb9a3e9b2d3822ace 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -203,7 +203,7 @@ public:
     void postHttpsAccept();
 
     /// Initializes and starts a peek-and-splice negotiation with the SSL client
-    void startPeekAndSplice();
+    void startPeekAndSplice(const bool unknownProtocol);
     /// Called when the initialization of peek-and-splice negotiation finidhed
     void startPeekAndSpliceDone();
     /// Called when a peek-and-splice step finished. For example after
@@ -235,7 +235,7 @@ public:
     void sslCrtdHandleReply(const Helper::Reply &reply);
 
     void switchToHttps(HttpRequest *request, Ssl::BumpMode bumpServerMode);
-    void afterClientHelloPeeked();
+    void parseTlsHandshake();
     bool switchedToHttps() const { return switchedToHttps_; }
     Ssl::ServerBump *serverBump() {return sslServerBump;}
     inline void setServerBump(Ssl::ServerBump *srvBump) {
@@ -352,6 +352,8 @@ private:
 
 #if USE_OPENSSL
     bool switchedToHttps_;
+    bool parsingTlsHandshake; ///< whether we are getting/parsing TLS Hello bytes
+
     /// The SSL server host name appears in CONNECT request or the server ip address for the intercepted requests
     String sslConnectHostOrIp; ///< The SSL server host name as passed in the CONNECT request
     SBuf sslCommonName_; ///< CN name for SSL certificate generation
@@ -360,7 +362,6 @@ private:
     /// HTTPS server cert. fetching state for bump-ssl-server-first
     Ssl::ServerBump *sslServerBump;
     Ssl::CertSignAlgorithm signAlgorithm; ///< The signing algorithm to use
-    bool atTlsPeek;
 #endif
 
     /// the reason why we no longer write the response or nil

diff --git a/src/security/Handshake.cc b/src/security/Handshake.cc
index fa3cda2067fd587c0800b9a8a8e1498e6ccbc823..21ce9ce9f196b31ca991d00ea373d31007d92d6c 100644 (file)
--- a/src/security/Handshake.cc
+++ b/src/security/Handshake.cc
@@ -147,9 +147,8 @@ operator <<(std::ostream &os, const DebugFrame &frame)
 Security::HandshakeParser::HandshakeParser():
     state(atHelloNone),
     ressumingSession(false),
-    parseDone(false),
-    parseError(false),
     currentContentType(0),
+    done(nullptr),
     expectingModernRecords(false)
 {
 }
@@ -162,7 +161,7 @@ Security::HandshakeParser::parseVersion2Record()
     details->tlsVersion = record.version;
     parseVersion2HandshakeMessage(record.fragment);
     state = atHelloReceived;
-    parseDone = true;
+    done = "SSL v2 Hello";
 }
 
 /// RFC 5246. Appendix E.2. Compatibility with SSL 2.0
@@ -240,12 +239,13 @@ void
 Security::HandshakeParser::parseChangeCipherCpecMessage()
 {
     Must(currentContentType == ContentType::ctChangeCipherSpec);
-    // we are currently ignoring Change Cipher Spec Protocol messages
-    // Everything after this message may be is encrypted
-    // The continuing parsing is pointless, abort here and set parseDone
+    // We are currently ignoring Change Cipher Spec Protocol messages.
     skipMessage("ChangeCipherCpec msg");
+    
+    // Everything after the ChangeCipherCpec message may be encrypted.
+    // Continuing parsing is pointless. Stop here.
     ressumingSession = true;
-    parseDone = true;
+    done = "ChangeCipherCpec";
 }
 
 void
@@ -269,7 +269,7 @@ Security::HandshakeParser::parseHandshakeMessage()
             Must(state < atHelloReceived);
             Security::HandshakeParser::parseClientHelloHandshakeMessage(message.body);
             state = atHelloReceived;
-            parseDone = true;
+            done = "ClientHello";
             return;
         case HandshakeType::hskServerHello:
             Must(state < atHelloReceived);
@@ -285,7 +285,7 @@ Security::HandshakeParser::parseHandshakeMessage()
             Must(state < atHelloDoneReceived);
             // zero-length
             state = atHelloDoneReceived;
-            parseDone = true;
+            done = "ServerHelloDone";
             return;
     }
     debugs(83, 5, "ignoring " <<
@@ -447,20 +447,17 @@ Security::HandshakeParser::parseHello(const SBuf &data)
         // data contains everything read so far, but we may read more later
         tkRecords.reinput(data, true);
         tkRecords.rollback();
-        while (!tkRecords.atEnd() && !parseDone)
+        while (!done)
             parseRecord();
-        debugs(83, 7, "success; done: " << parseDone);
-        return parseDone;
+        debugs(83, 7, "success; got: " << done);
+        // we are done; tkRecords may have leftovers we are not interested in
+        return true;
     }
     catch (const BinaryTokenizer::InsufficientInput &) {
         debugs(83, 5, "need more data");
-        Must(!parseError);
+        return false;
     }
-    catch (const std::exception &ex) {
-        debugs(83, 2, "parsing error: " << ex.what());
-        parseError = true;
-    }
-    return false;
+    return false; // unreached
 }
 
 #if USE_OPENSSL

diff --git a/src/security/Handshake.h b/src/security/Handshake.h
index ac2d71da4a38b8cf87b565f8cbb69c52cd6279f1..4364bf49a0409da3bd162764a8dcd6522f405f36 100644 (file)
--- a/src/security/Handshake.h
+++ b/src/security/Handshake.h
@@ -184,11 +184,13 @@ public:
     HandshakeParser();
 
     /// Parses the initial sequence of raw bytes sent by the SSL agent.
-    /// Returns true upon successful completion (HelloDone or Finished received).
-    /// Otherwise, returns false (and sets parseError to true on errors).
+    /// Returns true upon successful completion (e.g., got HelloDone).
+    /// Returns false if more data is needed.
+    /// Throws on errors.
     bool parseHello(const SBuf &data);
 
-    TlsDetails::Pointer details;
+    TlsDetails::Pointer details; ///< TLS handshake meta info or nil.
+
 #if USE_OPENSSL
     Ssl::X509_STACK_Pointer serverCertificates; ///< parsed certificates chain
 #endif
@@ -197,9 +199,6 @@ public:
 
     bool ressumingSession; ///< True if this is a resumming session
 
-    bool parseDone; ///< The parser finishes its job
-    bool parseError; ///< Set to tru by parse on parse error.
-
 private:
     bool isSslv2Record(const SBuf &raw) const;
     void parseRecord();
@@ -228,6 +227,9 @@ private:
 #endif
 
     unsigned int currentContentType; ///< The current SSL record content type
+
+    const char *done; ///< not nil iff we got what we were looking for
+
     /// concatenated TLSPlaintext.fragments of TLSPlaintext.type
     SBuf fragments;
 

diff --git a/src/ssl/bio.cc b/src/ssl/bio.cc
index 09bc5873dc0eba2d1d6d7ba3246dab9b37436c66..34faf322d84b039e0c859087f10fa16ccc41f180 100644 (file)
--- a/src/ssl/bio.cc
+++ b/src/ssl/bio.cc
@@ -198,6 +198,22 @@ Ssl::ClientBio::read(char *buf, int size, BIO *table)
     return -1;
 }
 
+/* ServerBio */
+
+Ssl::ServerBio::ServerBio(const int anFd):
+    Bio(anFd),
+    helloMsgSize(0),
+    helloBuild(false),
+    allowSplice(false),
+    allowBump(false),
+    holdWrite_(false),
+    record_(false),
+    parsedHandshake(false),
+    bumpMode_(bumpNone),
+    rbufConsumePos(0)
+{
+}
+
 void
 Ssl::ServerBio::stateChanged(const SSL *ssl, int where, int ret)
 {
@@ -214,10 +230,10 @@ Ssl::ServerBio::setClientFeatures(Security::TlsDetails::Pointer const &details,
 int
 Ssl::ServerBio::read(char *buf, int size, BIO *table)
 {
-    if (!parser_.parseDone && !parser_.parseError) // not done parsing yet
-        return readAndParse(buf, size, table);
-    else
+    if (parsedHandshake) // done parsing TLS Hello
         return readAndGive(buf, size, table);
+    else
+        return readAndParse(buf, size, table);
 }
 
 /// Read and give everything to OpenSSL.
@@ -240,7 +256,7 @@ Ssl::ServerBio::readAndGive(char *buf, const int size, BIO *table)
 }
 
 /// Read and give everything to our parser.
-/// When/if parsing is done, start giving to OpenSSL.
+/// When/if parsing is finished (successfully or not), start giving to OpenSSL.
 int
 Ssl::ServerBio::readAndParse(char *buf, const int size, BIO *table)
 {
@@ -248,15 +264,19 @@ Ssl::ServerBio::readAndParse(char *buf, const int size, BIO *table)
     if (result <= 0)
         return result;
 
-    if (!parser_.parseHello(rbuf)) {
-        if (!parser_.parseError) {
+    try {
+        if (!parser_.parseHello(rbuf)) {
+            // need more data to finish parsing
             BIO_set_retry_read(table);
             return -1;
         }
-        debugs(83, DBG_IMPORTANT, "ERROR: Failed to parse SSL Server Hello Message"); // XXX: move to catch{}
+        parsedHandshake = true; // done parsing (successfully)
+    }
+    catch (const std::exception &ex) {
+        debugs(83, 2, "parsing error on FD " << fd_ << ": " << ex.what());
+        parsedHandshake = true; // done parsing (due to an error)
     }
 
-    Must(parser_.parseDone || parser_.parseError);
     return giveBuffered(buf, size);
 }
 

diff --git a/src/ssl/bio.h b/src/ssl/bio.h
index 4b05235bb511efc4445c91e78cef3f485415c27f..5df82116e04ae9477921b39ed4fcca7ecbdd752c 100644 (file)
--- a/src/ssl/bio.h
+++ b/src/ssl/bio.h
@@ -111,7 +111,8 @@ private:
 class ServerBio: public Bio
 {
 public:
-    explicit ServerBio(const int anFd): Bio(anFd), helloMsgSize(0), helloBuild(false), allowSplice(false), allowBump(false), holdWrite_(false), record_(false), bumpMode_(bumpNone), rbufConsumePos(0) {}
+    explicit ServerBio(const int anFd);
+
     /// The ServerBio version of the Ssl::Bio::stateChanged method
     virtual void stateChanged(const SSL *ssl, int where, int ret);
     /// The ServerBio version of the Ssl::Bio::write method
@@ -166,6 +167,7 @@ private:
     bool allowBump;  ///< True if the SSL stream can be bumped
     bool holdWrite_;  ///< The write hold state of the bio.
     bool record_; ///< If true the input data recorded to rbuf for internal use
+    bool parsedHandshake; ///< whether we are done parsing TLS Hello
     Ssl::BumpMode bumpMode_;
 
     ///< The size of data stored in rbuf which passed to the openSSL