From: Christos Tsantilas Date: Wed, 18 May 2016 16:26:16 +0000 (+0300) Subject: Polishing fixes X-Git-Tag: SQUID_4_0_11~29^2~1 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=d9219c2bd9f66f53eb28d55b20b46ad1ae6ff5f5;p=thirdparty%2Fsquid.git Polishing fixes Code formatting, variables fixing, comments and debug messages Most of them proposed by Amos on squid-dev review procedure. --- diff --git a/src/anyp/ProtocolType.h b/src/anyp/ProtocolType.h index c08f84c3b1..425575203e 100644 --- a/src/anyp/ProtocolType.h +++ b/src/anyp/ProtocolType.h @@ -36,8 +36,8 @@ typedef enum { PROTO_URN, PROTO_WHOIS, PROTO_ICY, - PROTO_SSL, PROTO_TLS, + PROTO_SSL, PROTO_UNKNOWN, PROTO_MAX } ProtocolType; diff --git a/src/client_side.cc b/src/client_side.cc index 4c355dfc13..8e826f4712 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -3208,7 +3208,8 @@ ConnStateData::spliceOnError(const err_type err) return false; } -void ConnStateData::startPeekAndSplice(const bool unsupportedProtocol) +void +ConnStateData::startPeekAndSplice(const bool unsupportedProtocol) { if (unsupportedProtocol) { if (!spliceOnError(ERR_PROTOCOL_UNKNOWN)) @@ -3260,15 +3261,7 @@ ConnStateData::splice() // normally we can splice here, because we just got client hello message if (fd_table[clientConnection->fd].ssl.get()) { - - // The following block does not needed, inBuf and rbuf have the same content. - // BIO *b = SSL_get_rbio(ssl); - // Ssl::ClientBio *bio = static_cast(b->ptr); - // SBuf const &rbuf = bio->rBufData(); - // inBuf.assign(rbuf); - // debugs(83,5, "Bio for " << clientConnection << " read " << rbuf.length() << " helo bytes"); - - // Do splice: + // Restore default read methods fd_table[clientConnection->fd].read_method = &default_read_method; fd_table[clientConnection->fd].write_method = &default_write_method; } diff --git a/src/client_side.h b/src/client_side.h index e5944774ae..c3d0761d68 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -256,6 +256,9 @@ public: bool serveDelayedError(Http::Stream *); Ssl::BumpMode sslBumpMode; ///< ssl_bump decision (Ssl::bumpEnd if n/a). + + /// Tls parser to use for client HELLO messages parsing on bumped + /// connections. Security::HandshakeParser tlsParser; #else bool switchedToHttps() const { return false; } diff --git a/src/parser/BinaryTokenizer.cc b/src/parser/BinaryTokenizer.cc index 0bf042ab65..d9370678d5 100644 --- a/src/parser/BinaryTokenizer.cc +++ b/src/parser/BinaryTokenizer.cc @@ -9,7 +9,7 @@ /* DEBUG: section 24 SBuf */ #include "squid.h" -#include "BinaryTokenizer.h" +#include "parser/BinaryTokenizer.h" Parser::BinaryTokenizer::BinaryTokenizer(): BinaryTokenizer(SBuf()) { diff --git a/src/parser/BinaryTokenizer.h b/src/parser/BinaryTokenizer.h index 6309990f7c..61807b7683 100644 --- a/src/parser/BinaryTokenizer.h +++ b/src/parser/BinaryTokenizer.h @@ -6,8 +6,8 @@ * Please see the COPYING and CONTRIBUTORS files for details. */ -#ifndef SQUID_PARSER_BINARY_TOKENIZER_H -#define SQUID_PARSER_BINARY_TOKENIZER_H +#ifndef SQUID_SRC_PARSER_BINARYTOKENIZER_H +#define SQUID_SRC_PARSER_BINARYTOKENIZER_H #include "sbuf/SBuf.h" @@ -21,7 +21,7 @@ class BinaryTokenizerContext { public: /// starts parsing named object - inline explicit BinaryTokenizerContext(BinaryTokenizer &tk, const char *aName); + explicit BinaryTokenizerContext(BinaryTokenizer &tk, const char *aName); ~BinaryTokenizerContext() { close(); } /// ends parsing named object; repeated calls OK @@ -31,12 +31,13 @@ public: inline void success(); BinaryTokenizer &tokenizer; ///< tokenizer being used for parsing - const BinaryTokenizerContext *parent; ///< enclosing context or nullptr - const char *name; ///< this context description or nullptr + const BinaryTokenizerContext * const parent; ///< enclosing context or nullptr + const char *const name; ///< this context description or nullptr uint64_t start; ///< context parsing begins at this tokenizer position }; /// Safely extracts byte-oriented (i.e., non-textual) fields from raw input. +/// Assume that the integers are stored in network byte order. /// Supports commit points for atomic incremental parsing of multi-part fields. /// Throws InsufficientInput when more input is needed to parse the next field. /// Throws on errors. @@ -69,13 +70,13 @@ public: /// parse a single-byte unsigned integer uint8_t uint8(const char *description); - // parse a two-byte unsigned integer + /// parse a two-byte unsigned integer uint16_t uint16(const char *description); - // parse a three-byte unsigned integer (returned as uint32_t) + /// parse a three-byte unsigned integer (returned as uint32_t) uint32_t uint24(const char *description); - // parse a four-byte unsigned integer + /// parse a four-byte unsigned integer uint32_t uint32(const char *description); /// parse size consecutive bytes as an opaque blob @@ -144,5 +145,5 @@ BinaryTokenizerContext::success() { } /* namespace Parser */ -#endif // SQUID_PARSER_BINARY_TOKENIZER_H +#endif // SQUID_SRC_PARSER_BINARYTOKENIZER_H diff --git a/src/security/Handshake.cc b/src/security/Handshake.cc index 09e80183ef..cd8f418d7f 100644 --- a/src/security/Handshake.cc +++ b/src/security/Handshake.cc @@ -190,7 +190,7 @@ Security::TlsDetails::TlsDetails(): Security::HandshakeParser::HandshakeParser(): details(new TlsDetails), state(atHelloNone), - ressumingSession(false), + resumingSession(false), currentContentType(0), done(nullptr), expectingModernRecords(false) @@ -275,7 +275,7 @@ Security::HandshakeParser::parseMessages() parseApplicationDataMessage(); continue; } - skipMessage("unknown ContentType msg"); + skipMessage("unknown ContentType msg [fragment]"); } } @@ -284,11 +284,11 @@ Security::HandshakeParser::parseChangeCipherCpecMessage() { Must(currentContentType == ContentType::ctChangeCipherSpec); // We are currently ignoring Change Cipher Spec Protocol messages. - skipMessage("ChangeCipherCpec msg"); + skipMessage("ChangeCipherCpec msg [fragment]"); // Everything after the ChangeCipherCpec message may be encrypted. // Continuing parsing is pointless. Stop here. - ressumingSession = true; + resumingSession = true; done = "ChangeCipherCpec"; } @@ -344,7 +344,7 @@ void Security::HandshakeParser::parseApplicationDataMessage() { Must(currentContentType == ContentType::ctApplicationData); - skipMessage("app data"); + skipMessage("app data [fragment]"); } void diff --git a/src/security/Handshake.h b/src/security/Handshake.h index 4bb4c5b231..d3ff94a146 100644 --- a/src/security/Handshake.h +++ b/src/security/Handshake.h @@ -10,10 +10,8 @@ #define SQUID_SECURITY_HANDSHAKE_H #include "anyp/ProtocolVersion.h" -#include "base/RefCount.h" #include "base/YesNoNone.h" #include "parser/BinaryTokenizer.h" -#include "sbuf/SBuf.h" #if USE_OPENSSL #include "ssl/gadgets.h" #endif @@ -56,15 +54,16 @@ std::ostream &operator <<(std::ostream &os, Security::TlsDetails const &details) return details.print(os); } -/// Incremental SSL Handshake parser. -class HandshakeParser { +/// Incremental TLS/SSL Handshake parser. +class HandshakeParser +{ public: /// The parsing states typedef enum {atHelloNone = 0, atHelloStarted, atHelloReceived, atCertificatesReceived, atHelloDoneReceived, atNstReceived, atCcsReceived, atFinishReceived} ParserState; HandshakeParser(); - /// Parses the initial sequence of raw bytes sent by the SSL agent. + /// Parses the initial sequence of raw bytes sent by the TLS/SSL agent. /// Returns true upon successful completion (e.g., got HelloDone). /// Returns false if more data is needed. /// Throws on errors. @@ -78,7 +77,7 @@ public: ParserState state; ///< current parsing state. - bool ressumingSession; ///< True if this is a resumming session + bool resumingSession; ///< True if this is a resuming session private: bool isSslv2Record(const SBuf &raw) const; @@ -110,17 +109,21 @@ private: static X509 *ParseCertificate(const SBuf &raw); #endif - unsigned int currentContentType; ///< The current SSL record content type + unsigned int currentContentType; ///< The current TLS/SSL record content type - const char *done; ///< not nil iff we got what we were looking for + const char *done; ///< not nil if we got what we were looking for /// concatenated TLSPlaintext.fragments of TLSPlaintext.type SBuf fragments; - Parser::BinaryTokenizer tkRecords; // TLS record layer (parsing uninterpreted data) - Parser::BinaryTokenizer tkMessages; // TLS message layer (parsing fragments) + /// TLS record layer (parsing uninterpreted data) + Parser::BinaryTokenizer tkRecords; + + /// TLS message layer (parsing fragments) + Parser::BinaryTokenizer tkMessages; - YesNoNone expectingModernRecords; // Whether to use TLS parser or a V2 compatible parser + /// Whether to use TLS parser or a V2 compatible parser + YesNoNone expectingModernRecords; }; } diff --git a/src/security/NegotiationHistory.cc b/src/security/NegotiationHistory.cc index 2004f18d91..57fcdecf6c 100644 --- a/src/security/NegotiationHistory.cc +++ b/src/security/NegotiationHistory.cc @@ -17,7 +17,7 @@ Security::NegotiationHistory::NegotiationHistory() #if USE_OPENSSL - : cipher(NULL) + : cipher(nullptr) #endif { } @@ -38,18 +38,26 @@ static AnyP::ProtocolVersion toProtocolVersion(const int v) { switch(v) { -#if OPENSSL_VERSION_NUMBER >= 0x10001000L +#if defined(TLS1_2_VERSION) case TLS1_2_VERSION: return AnyP::ProtocolVersion(AnyP::PROTO_TLS, 1, 2); +#endif +#if defined(TLS1_1_VERSION) case TLS1_1_VERSION: return AnyP::ProtocolVersion(AnyP::PROTO_TLS, 1, 1); #endif +#if defined(TLS1_VERSION) case TLS1_VERSION: return AnyP::ProtocolVersion(AnyP::PROTO_TLS, 1, 0); +#endif +#if defined(SSL3_VERSION) case SSL3_VERSION: return AnyP::ProtocolVersion(AnyP::PROTO_SSL, 3, 0); +#endif +#if defined(SSL2_VERSION) case SSL2_VERSION: return AnyP::ProtocolVersion(AnyP::PROTO_SSL, 2, 0); +#endif default: return AnyP::ProtocolVersion(); } diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc index 3679bb2fe9..f1b7b97b0a 100644 --- a/src/ssl/PeekingPeerConnector.cc +++ b/src/ssl/PeekingPeerConnector.cc @@ -269,6 +269,7 @@ Ssl::PeekingPeerConnector::noteWantWrite() Ssl::ServerBio *srvBio = static_cast(b->ptr); if ((srvBio->bumpMode() == Ssl::bumpPeek || srvBio->bumpMode() == Ssl::bumpStare) && srvBio->holdWrite()) { + debugs(81, DBG_IMPORTANT, "hold write on SSL connection on FD " << fd); checkForPeekAndSplice(); return; } diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc index 44a0e292df..0e22b8ce70 100644 --- a/src/ssl/PeerConnector.cc +++ b/src/ssl/PeerConnector.cc @@ -352,7 +352,7 @@ Ssl::PeerConnector::handleNegotiateError(const int ret) break; } - // Log connections details if there is any + // Log connection details, if any recordNegotiationDetails(); noteSslNegotiationError(ret, ssl_error, ssl_lib_error); } @@ -360,8 +360,8 @@ Ssl::PeerConnector::handleNegotiateError(const int ret) void Ssl::PeerConnector::noteWantRead() { - const int fd = serverConnection()->fd; setReadTimeout(); + const int fd = serverConnection()->fd; Comm::SetSelect(fd, COMM_SELECT_READ, &NegotiateSsl, this, 0); } diff --git a/src/ssl/bio.cc b/src/ssl/bio.cc index a81ea13265..dca2b430ca 100644 --- a/src/ssl/bio.cc +++ b/src/ssl/bio.cc @@ -57,8 +57,6 @@ static BIO_METHOD SquidMethods = { NULL // squid_callback_ctrl not supported }; -/* Ssl:Bio */ - BIO * Ssl::Bio::Create(const int fd, Ssl::Bio::Type type) { @@ -198,8 +196,6 @@ Ssl::ClientBio::read(char *buf, int size, BIO *table) return -1; } -/* ServerBio */ - Ssl::ServerBio::ServerBio(const int anFd): Bio(anFd), helloMsgSize(0), @@ -329,7 +325,7 @@ adjustSSL(SSL *ssl, Security::TlsDetails::Pointer const &details, SBuf &helloMes // If the client supports compression but our context does not support // we can not adjust. #if !defined(OPENSSL_NO_COMP) - const bool requireCompression = (details->compressionSupported && ssl->ctx->comp_methods == NULL); + const bool requireCompression = (details->compressionSupported && ssl->ctx->comp_methods == nullptr); #else const bool requireCompression = details->compressionSupported; #endif @@ -497,7 +493,7 @@ Ssl::ServerBio::flush(BIO *table) bool Ssl::ServerBio::resumingSession() { - return parser_.ressumingSession; + return parser_.resumingSession; } /// initializes BIO table after allocation @@ -625,8 +621,9 @@ applyTlsDetailsToSSL(SSL *ssl, Security::TlsDetails::Pointer const &details, Ssl // To increase the possibility for bumping after peek mode selection or // splicing after stare mode selection it is good to set the // SSL protocol version. - // The SSL_set_ssl_method is not the correct method because it will strict - // SSL version which can be used to the SSL version used for client hello message. + // The SSL_set_ssl_method is wrong here because it will restrict the + // permitted transport version to be identical to the version used in the + // ClientHello message. // For example will prevent comunnicating with a tls1.0 server if the // client sent and tlsv1.2 Hello message. #if defined(TLSEXT_NAMETYPE_host_name) diff --git a/src/ssl/bio.h b/src/ssl/bio.h index 5df82116e0..a06592660d 100644 --- a/src/ssl/bio.h +++ b/src/ssl/bio.h @@ -10,7 +10,6 @@ #define SQUID_SSL_BIO_H #include "fd.h" -#include "sbuf/SBuf.h" #include "security/Handshake.h" #include @@ -58,7 +57,7 @@ public: /// Tells ssl connection to use BIO and monitor state via stateChanged() static void Link(SSL *ssl, BIO *bio); - const SBuf &rBufData() {return rbuf;} + const SBuf &rBufData() {return rbuf;} ///< The buffered input data protected: const int fd_; ///< the SSL socket we are reading and writing SBuf rbuf; ///< Used to buffer input data. @@ -70,7 +69,7 @@ protected: class ClientBio: public Bio { public: - explicit ClientBio(const int anFd): Bio(anFd), holdRead_(false), holdWrite_(false), helloSize(0)/*, wrongProtocol(false)*/ {} + explicit ClientBio(const int anFd): Bio(anFd), holdRead_(false), holdWrite_(false), helloSize(0) {} /// The ClientBio version of the Ssl::Bio::stateChanged method /// When the client hello message retrieved, fill the @@ -84,6 +83,10 @@ public: virtual int read(char *buf, int size, BIO *table); /// Prevents or allow writting on socket. void hold(bool h) {holdRead_ = holdWrite_ = h;} + + /// Sets the buffered input data (Bio::rbuf). + /// Used to pass payload data (normally client HELLO data) retrieved + /// by the caller. void setReadBufData(SBuf &data) {rbuf = data;} private: /// True if the SSL state corresponds to a hello message @@ -145,7 +148,7 @@ public: void mode(Ssl::BumpMode m) {bumpMode_ = m;} Ssl::BumpMode bumpMode() {return bumpMode_;} ///< return the bumping mode - /// Return the TLS Details advertised by TLS server. + /// \return the TLS Details advertised by TLS server. const Security::TlsDetails::Pointer &receivedHelloDetails() const {return parser_.details;} const Ssl::X509_STACK_Pointer &serverCertificatesIfAny() { return parser_.serverCertificates; } /* XXX: may be nil */ @@ -170,9 +173,9 @@ private: bool parsedHandshake; ///< whether we are done parsing TLS Hello Ssl::BumpMode bumpMode_; - ///< The size of data stored in rbuf which passed to the openSSL + /// The size of data stored in rbuf which passed to the openSSL size_t rbufConsumePos; - Security::HandshakeParser parser_; ///< The SSL messages parser. + Security::HandshakeParser parser_; ///< The TLS/SSL messages parser. }; } // namespace Ssl