[squid-dev] [PATCH] GnuTLS session redo
Amos Jeffries
squid3 at treenet.co.nz
Mon Aug 8 12:48:08 UTC 2016
On 6/08/2016 9:41 a.m., Alex Rousskov wrote:
> On 08/05/2016 02:13 PM, Amos Jeffries wrote:
>> On 6/08/2016 6:37 a.m., Alex Rousskov wrote:
>>> On 08/03/2016 11:57 PM, Amos Jeffries wrote:
>>>> +Security::SetSessionResumeData(const Security::SessionPointer &s, const Security::SessionStatePointer &data)
>>>> +{
>>>> + if (data) {
>>>> +#if USE_OPENSSL
>>>> + (void)SSL_set_session(s.get(), data.get());
>>>> +#elif USE_GNUTLS
>>>> + (void)gnutls_session_set_data(s.get(), data->data, data->size);
>>>> +#endif
>>>> + }
>>>> +}
>
>
>>> Why cast to (void) here? The current code does not do that AFAICT.
>>> Casting the result to (void) usually means
>>>
>>> * I know this may fail, but I do not care.
>
>>> It should be accompanied by a comment that explains why we do not care,
>>> unless the code makes it obvious (the patched code does not).
>
>
>> Failures there just mean a clean new session is used.
>
> I do not think SSL_set_session() fails when "a clean new session is
> used" but perhaps I am misinterpreting what you mean by that.
>
> If you meant that an SSL_set_session() failure is not important because
> a new session will be eventually created instead of the old one being
> reused, then I agree regarding what will happen but disagree regarding
> that failure unimportance. Session reuse is very important in some
> environments, so the proposed "I do not care why it did not happen" is
> the wrong approach. We do care, at least for triage/debugging purposes.
>
> That kind of logic would be very similar to saying "I do not care that
> nothing gets cached due to a failed disk -- the users will get their
> responses from the origin server instead". While they will indeed, we
> still do care as far as stats/logging/debugging are concerned.
>
Nod. Thats what I meant. With no stats/logging/debugging in the earlier
patches there was no reason to care. Following up from your other mail
we do now need it for the debug if nothing else. So thats sorted now I
think in this patch.
>
>> I dont see any reason to throw. Resume is just a speed optimization for TLS.
>
> We should throw if the callers need to know about the failure. We should
> not throw if the callers do not care about the failure. Resume being an
> optional optimization is irrelevant in this particular decision --
> either the callers have some cleanup/setup/debugging code to accompany
> this call or they do not.
>
Well, the callers don't depend on the resume and don't do anything
explicitly different whether it works or not. So I'm leaving throw until
an actual need shows up.
>
>>>> +Security::SessionIsResumed(const Security::SessionPointer &s)
>>>> +{
>>>> + return
>>>> +#if USE_OPENSSL
>>>> + SSL_session_reused(s.get()) == 1;
>>>> +#elif USE_GNUTLS
>>>> + gnutls_session_is_resumed(s.get()) != 0;
>>>> +#else
>>>> + false;
>>>> +#endif
>>>> +}
>>>> +
>>>> +void
>>>> +Security::GetSessionResumeData(const Security::SessionPointer &s, Security::SessionStatePointer &data)
>>>> +{
>>>> + if (!SessionIsResumed(s)) {
>>>> +#if USE_OPENSSL
>>>> + data.reset(SSL_get1_session(s.get()));
>>>> +#elif USE_GNUTLS
>>>> + gnutls_datum_t *tmp = nullptr;
>>>> + (void)gnutls_session_get_data2(s.get(), tmp);
>>>> + data.reset(tmp);
>>>> +#endif
>>>> + }
>>>> +}
>>>
>>> Can we establish some rules for when to use "#else" in
>>> OpenSSL/GnuTLS/Security contexts?
>
>> Sure. So far I've been trying to only add #else when there was some
>> meaningful code that needed to be done in those builds.
>
> Not sure I understand the rule you are proposing (if you are proposing a
> rule). Can you detail/rephrase it? Please keep the following two
> questions in mind when doing so:
After considering your questions below I have come to some conclusion
thus [listed for backreferencing]:
A) I consider #if-conditionls as [almost] equivalent to
if()-conditionals for design and implementation purposes.
-> Just another set of code pathways to make correct, both for author
and auditor.
-> There is very likely somebody who will build Squid in such a way
that its code path happens.
B) We don't have special Squid rule(s) about particular meaning and
interpretation for missing 'else' on normal if-else statements.
-> having a rule(s) means having to enforce it.
==> more thing(s) for auditors to remember to check specially.
-> assuming (A) is also how most others code. Adding a rule would be
adding a Squid-specific case for these statements which does not exist
for normal if-conditions.
... unless we formally agree to make a rule of "treat them as normal C++
if-else for the purposes of auditing". I'm fine with that of course, its
just seems weird to need to formalize such a thing.
C) If the author has not considered the code path that would be created
when the #else condition is built they have probably added a bug (or many).
-> This bug is such a thing audit needs to be checking for as much as
any other code bug.
==> Implying (A) should be true in the general case not just me.
Thus the conclusion:
We can have a vague rule, more akin to a guideline. But not the type
you seem to be wanting - with definitive criteria that can have a
checklist of conditions to run against a patch submission.
If you want to insist on a rule the best one I can think of is:
"treat them as normal C++ if-else"
possibly with "when auditing" appended.
So any best practice or rule of thumb you may be aware of for auditing
C++ if-statements can be used here as well.
Better than what I had before, but still not very helpful for a checkist
either.
IMHO, avoiding messy is a noble goal, but "messy" is relative and we
should not strain ourselves trying to codify rules for it.
Even after cleanup what particular code needs wrapping is going to be
messy, just because of the library API differences. Which is why I added
the "Usually ..." bit. *so far* its usually been error related code,
though some things coming up will likely need #else to mean "anything
thats not for library X" for various X.
>
> 1. What are these rules trying to guarantee or accomplish?
> In other words, what will happen if we [do not] follow them?
IMO; Minimal amount of code wrapped inside pre-processor conditions
(within reason). To make more consistent behaviour between any two
builds of Squid.
I think thats what we both have been aiming for in audits so far. So can
agree on. Necessarily loose. Yes it does imply possible "mess".
IMO, in C++ code that 'Minimal amount of code wrapped' applied to #else,
equates to the best practice of reducing scope when it is unnecessary to
sub-scope things.
eg. this gem :
if ...
return
else
return
return // why bother? just 'cause reasons.
reduxes to:
if...
return
... else stuff
return
>
> 2. If I see no "#else" (or no "#elif"), what does that mean?
> * We have not checked yet. It may not run or even compile.
> * We know it compiles (and runs?) without #else (or #elif).
> * Something should go there but we have not figured out what yet.
> * We know that nothing should go there; all callers will be fine.
> Etc.
For this patch the build layers: default, minimal, maximus test the
respective three permutation cases of GnuTLS, neither, OpenSSL.
-> we know it builds.
Unit and Systemic 'black box' tests are incomplete so far.
-> uknown whether it runs. The old code did so I'm ~95% confident about
it. But then I was same about the initial patch too, which didn't in
your build and config.
For other uses of #if conditionals we also should have build layers
providing the same guarantee. Thats the point of build layers in the
test suite. If not, a build layer needs adding to cover it. Another test
related thing we've all been lax about :-(.
>
>> Usually it is for a default return statement or setting up error results
>> indicating the related TLS/SSL feature not working.
>
> Should the "default return statement" always indicate an error of some
> sort?
> If not, how to know when to implement the default success route
> (your first reason) and when to set up an error (your second reason)?
>
I think the number and nature of your questions is a good demonstration
of why we should *not* to have a detailed/strict rule about this.
Thinking about the possible answers shows that its a complex situation.
Also keep in mind this patch is an _uncommon_ case - code where the two
libraries have a 1:1 maping of API calls meaning. Differing mostly in
specific ABI and call style. Majority of their API dont have such a
clean alignment, and that misalignment is where most of the mess in our
TLS/SSL code comes from in the first place.
I used the vague words "usually" and "default return" because thats as
detailed as I can go and still accurately cover the general cases I know
about. We are still at early days (years?) and attempts to design it
down to a more fine grained rule before implementing is I think a big
waste of our time. Come what may, we will have to audit case-by-case anyway.
>
>> Code that is only
>> run when the feature being used usually does not need #else.
>
> Without a good set of rules, this gets messy very quickly IMO. For
> example, your own GetSessionResumeData() does not reset the data
> parameter in the [absent] #else case but does unconditionally reset it
> (possibly to a nil value) in other cases. Is this a bug waiting to
> happen, and intended side effect, or both? Without rules, it is
> difficult to say for sure.
Good example there. Since (for this patch and this #else) it is the
build-case of no TLS/SSL library. In the #else the data object should
never have been set by anything. If it was, it is not an SSL/TLS library
thing (so touch or not touch? - that is the Q).
In the other paths the library is providing authoritative new value
(which might be nil value). So the intention is to use that new value.
For lack of better knowledge I made it a shuffle patch, bug-compatible
with existing OpenSSL code. Then added the GnuTLS bits to make it work
the same.
If you think it should data.reset() in all code paths. Okay I'll do that.
>
>> I've built this with the full set of build layers.
>
> This is not simply a build matter IMO.
>
Nod. Just saying that was done.
>
>> /// Retrieve the data needed to resume this session on a later connection
>> /// if it was not itself resumed from earlier details
>> void MaybeGetSessionResumeData(...)
>
> We should say what happens if "it was itself resumed". The current
> "leave data pointer intact" logic looks very strange to me, but I cannot
> tell whether this is a bug in the existing code (that you do not have to
> fix, but can expose by adding XXX to your documentation) or intended
> [but difficult to understand] behavior.
>
I assumed it has something to do with ensuring that a 'child'/resumed
session does not extend the lifetime of associated credentials or
protocol settings for indefinitely long periods simply by using them for
itself or passing the resume on elsewhere. So left it bug-compatible
with existing code.
Looking at the OpenSSL documentation
<https://www.openssl.org/docs/manmaster/ssl/SSL_set_session.html> there
is a NOTE about session details only being able to be used on one CTX.
It might be related to that somehow.
Anyhow, I've altered the text in te attached patch. But to me it looks
like it might be even more confusing than before. Any better suggestions
of what to write?
>
>> Since this is all cosmetic, is/was any testing able to happen on your part?
>
> The rules decisions we should make here are as far from cosmetic
> polishing as they get IMO. The understanding of GetSessionResumeData()
> side effects may not be cosmetic either.
If we spend ages and I end up having to add "#else // do nothing" to
parts of the patch just to get past audit I think the rule making will
have failed.
Whether to data.reset() in the #else path is unrelated to the rule
making. So good its come up under audit to get resolved properly without
needing the rule to pre-exist.
>
> As for testing, I cannot volunteer for that at this time. And given the
> sad state of Squid automated QA, my testing with OpenSSL (or GnuTLS)
> would be about as good as what you can do anyway.
:-(
Thanks for your time on this anyhow.
New patch attached, including these commants and the debug requested in
the other mail.
Amos
-------------- next part --------------
=== modified file 'src/CachePeer.cc'
--- src/CachePeer.cc 2016-07-18 12:36:38 +0000
+++ src/CachePeer.cc 2016-08-07 12:39:49 +0000
@@ -25,41 +25,40 @@
typelist(NULL),
access(NULL),
weight(1),
basetime(0),
#if USE_CACHE_DIGESTS
digest(NULL),
digest_url(NULL),
#endif
tcp_up(0),
n_addresses(0),
rr_count(0),
next(NULL),
testing_now(false),
login(NULL),
connect_timeout(0),
connect_fail_limit(0),
max_conn(0),
domain(NULL),
#if USE_OPENSSL
sslContext(NULL),
- sslSession(NULL),
#endif
front_end_https(0),
connection_auth(2 /* auto */)
{
memset(&stats, 0, sizeof(stats));
stats.logged_state = PEER_ALIVE;
memset(&icp, 0, sizeof(icp));
icp.port = CACHE_ICP_PORT;
icp.version = ICP_VERSION_CURRENT;
#if USE_HTCP
memset(&htcp, 0, sizeof(htcp));
#endif
memset(&options, 0, sizeof(options));
memset(&mcast, 0, sizeof(mcast));
memset(&carp, 0, sizeof(carp));
#if USE_AUTH
memset(&userhash, 0, sizeof(userhash));
#endif
@@ -85,26 +84,23 @@
#if USE_CACHE_DIGESTS
cbdataReferenceDone(digest);
xfree(digest_url);
#endif
delete next;
xfree(login);
delete standby.pool;
// the mgr job will notice that its owner is gone and stop
PeerPoolMgr::Checkpoint(standby.mgr, "peer gone");
xfree(domain);
#if USE_OPENSSL
if (sslContext)
SSL_CTX_free(sslContext);
-
- if (sslSession)
- SSL_SESSION_free(sslSession);
#endif
}
=== modified file 'src/CachePeer.h'
--- src/CachePeer.h 2016-07-18 12:36:38 +0000
+++ src/CachePeer.h 2016-08-07 12:39:49 +0000
@@ -167,30 +167,28 @@
unsigned int hash;
double load_multiplier;
double load_factor; /* normalized weight value */
} sourcehash;
char *login; /* Proxy authorization */
time_t connect_timeout;
int connect_fail_limit;
int max_conn;
struct {
PconnPool *pool; ///< idle connection pool for this peer
CbcPointer<PeerPoolMgr> mgr; ///< pool manager
int limit; ///< the limit itself
bool waitingForClose; ///< a conn must close before we open a standby conn
} standby; ///< optional "cache_peer standby=limit" feature
char *domain; /* Forced domain */
/// security settings for peer connection
Security::PeerOptions secure;
Security::ContextPtr sslContext;
-#if USE_OPENSSL
- SSL_SESSION *sslSession;
-#endif
+ Security::SessionStatePointer sslSession;
int front_end_https;
int connection_auth;
};
#endif /* SQUID_CACHEPEER_H_ */
=== modified file 'src/Makefile.am'
--- src/Makefile.am 2016-08-01 11:11:47 +0000
+++ src/Makefile.am 2016-08-07 12:39:50 +0000
@@ -529,46 +529,46 @@
$(BUILT_SOURCES)
squid_LDADD = \
$(AUTH_ACL_LIBS) \
ident/libident.la \
acl/libacls.la \
acl/libstate.la \
$(AUTH_LIBS) \
acl/libapi.la \
clients/libclients.la \
servers/libservers.la \
ftp/libftp.la \
helper/libhelper.la \
http/libhttp.la \
dns/libdns.la \
base/libbase.la \
libsquid.la \
ip/libip.la \
fs/libfs.la \
DiskIO/libdiskio.la \
+ comm/libcomm.la \
+ security/libsecurity.la \
$(SSL_LIBS) \
ipc/libipc.la \
mgr/libmgr.la \
anyp/libanyp.la \
- comm/libcomm.la \
- security/libsecurity.la \
parser/libparser.la \
eui/libeui.la \
icmp/libicmp.la \
log/liblog.la \
format/libformat.la \
sbuf/libsbuf.la \
$(XTRA_OBJS) \
$(REPL_OBJS) \
$(NETTLELIB) \
$(CRYPTLIB) \
$(REGEXLIB) \
$(ADAPTATION_LIBS) \
$(ESI_LIBS) \
$(SNMP_LIBS) \
mem/libmem.la \
store/libstore.la \
$(top_builddir)/lib/libmisccontainers.la \
$(top_builddir)/lib/libmiscencoding.la \
$(top_builddir)/lib/libmiscutil.la \
$(ATOMICLIB) \
=== modified file 'src/adaptation/icap/ServiceRep.cc'
--- src/adaptation/icap/ServiceRep.cc 2016-07-18 12:36:38 +0000
+++ src/adaptation/icap/ServiceRep.cc 2016-08-07 12:39:50 +0000
@@ -17,43 +17,40 @@
#include "adaptation/icap/ServiceRep.h"
#include "base/TextException.h"
#include "comm/Connection.h"
#include "ConfigParser.h"
#include "Debug.h"
#include "fde.h"
#include "globals.h"
#include "HttpReply.h"
#include "ip/tools.h"
#include "SquidConfig.h"
#include "SquidTime.h"
#define DEFAULT_ICAP_PORT 1344
#define DEFAULT_ICAPS_PORT 11344
CBDATA_NAMESPACED_CLASS_INIT(Adaptation::Icap, ServiceRep);
Adaptation::Icap::ServiceRep::ServiceRep(const ServiceConfigPointer &svcCfg):
AsyncJob("Adaptation::Icap::ServiceRep"), Adaptation::Service(svcCfg),
sslContext(NULL),
-#if USE_OPENSSL
- sslSession(NULL),
-#endif
theOptions(NULL), theOptionsFetcher(0), theLastUpdate(0),
theBusyConns(0),
theAllWaiters(0),
connOverloadReported(false),
theIdleConns(NULL),
isSuspended(0), notifying(false),
updateScheduled(false),
wasAnnouncedUp(true), // do not announce an "up" service at startup
isDetached(false)
{
setMaxConnections();
theIdleConns = new IdleConnList("ICAP Service", NULL);
}
Adaptation::Icap::ServiceRep::~ServiceRep()
{
delete theIdleConns;
Must(!theOptionsFetcher);
delete theOptions;
}
=== modified file 'src/adaptation/icap/ServiceRep.h'
--- src/adaptation/icap/ServiceRep.h 2016-07-18 12:36:38 +0000
+++ src/adaptation/icap/ServiceRep.h 2016-08-07 12:39:50 +0000
@@ -94,43 +94,41 @@
void noteNewWaiter() {theAllWaiters++;} ///< New xaction waiting for service to be up or available
void noteGoneWaiter(); ///< An xaction is not waiting any more for service to be available
bool existWaiters() const {return (theAllWaiters > 0);} ///< if there are xactions waiting for the service to be available
//AsyncJob virtual methods
virtual bool doneAll() const { return Adaptation::Initiator::doneAll() && false;}
virtual void callException(const std::exception &e);
virtual void detach();
virtual bool detached() const;
public: // treat these as private, they are for callbacks only
void noteTimeToUpdate();
void noteTimeToNotify();
// receive either an ICAP OPTIONS response header or an abort message
virtual void noteAdaptationAnswer(const Answer &answer);
Security::ContextPtr sslContext;
-#if USE_OPENSSL
- SSL_SESSION *sslSession;
-#endif
+ Security::SessionStatePointer sslSession;
private:
// stores Prepare() callback info
struct Client {
Pointer service; // one for each client to preserve service
AsyncCall::Pointer callback;
};
typedef std::vector<Client> Clients;
// TODO: rename to theUpWaiters
Clients theClients; // all clients waiting for a call back
Options *theOptions;
CbcPointer<Adaptation::Initiate> theOptionsFetcher; // pending ICAP OPTIONS transaction
time_t theLastUpdate; // time the options were last updated
/// FIFO queue of xactions waiting for a connection slot and not yet notified
/// about it; xaction is removed when notification is scheduled
std::deque<Client> theNotificationWaiters;
=== modified file 'src/adaptation/icap/Xaction.cc'
--- src/adaptation/icap/Xaction.cc 2016-08-06 04:49:55 +0000
+++ src/adaptation/icap/Xaction.cc 2016-08-07 12:39:50 +0000
@@ -693,74 +693,62 @@
void Adaptation::Icap::Xaction::fillDoneStatus(MemBuf &buf) const
{
if (haveConnection() && commEof)
buf.appendf("Comm(%d)", connection->fd);
if (stopReason != NULL)
buf.append("Stopped", 7);
}
bool Adaptation::Icap::Xaction::fillVirginHttpHeader(MemBuf &) const
{
return false;
}
bool
Ssl::IcapPeerConnector::initializeTls(Security::SessionPointer &serverSession)
{
if (!Security::PeerConnector::initializeTls(serverSession))
return false;
-#if USE_OPENSSL
assert(!icapService->cfg().secure.sslDomain.isEmpty());
+#if USE_OPENSSL
SBuf *host = new SBuf(icapService->cfg().secure.sslDomain);
SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, host);
ACLFilledChecklist *check = static_cast<ACLFilledChecklist *>(SSL_get_ex_data(serverSession.get(), ssl_ex_index_cert_error_check));
if (check)
check->dst_peer_name = *host;
+#endif
- if (icapService->sslSession)
- SSL_set_session(serverSession.get(), icapService->sslSession);
-
+ Security::SetSessionResumeData(serverSession, icapService->sslSession);
return true;
-#else
- return false;
-#endif
}
void
Ssl::IcapPeerConnector::noteNegotiationDone(ErrorState *error)
{
if (error)
return;
-#if USE_OPENSSL
const int fd = serverConnection()->fd;
- auto ssl = fd_table[fd].ssl.get();
- assert(ssl);
- if (!SSL_session_reused(ssl)) {
- if (icapService->sslSession)
- SSL_SESSION_free(icapService->sslSession);
- icapService->sslSession = SSL_get1_session(ssl);
- }
-#endif
+ Security::MaybeGetSessionResumeData(fd_table[fd].ssl, icapService->sslSession);
}
void
Adaptation::Icap::Xaction::handleSecuredPeer(Security::EncryptorAnswer &answer)
{
Must(securer != NULL);
securer = NULL;
if (closer != NULL) {
if (answer.conn != NULL)
comm_remove_close_handler(answer.conn->fd, closer);
else
closer->cancel("securing completed");
closer = NULL;
}
if (answer.error.get()) {
if (answer.conn != NULL)
answer.conn->close();
debugs(93, 2, typeName <<
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2016-08-01 11:11:47 +0000
+++ src/client_side.cc 2016-08-07 12:39:50 +0000
@@ -2638,41 +2638,41 @@
/* NOTREACHED */
}
return 1;
}
/** negotiate an SSL connection */
static void
clientNegotiateSSL(int fd, void *data)
{
ConnStateData *conn = (ConnStateData *)data;
X509 *client_cert;
auto ssl = fd_table[fd].ssl.get();
int ret;
if ((ret = Squid_SSL_accept(conn, clientNegotiateSSL)) <= 0) {
if (ret < 0) // An error
conn->clientConnection->close();
return;
}
- if (SSL_session_reused(ssl)) {
+ if (Security::SessionIsResumed(fd_table[fd].ssl)) {
debugs(83, 2, "clientNegotiateSSL: Session " << SSL_get_session(ssl) <<
" reused on FD " << fd << " (" << fd_table[fd].ipaddr << ":" << (int)fd_table[fd].remote_port << ")");
} else {
if (Debug::Enabled(83, 4)) {
/* Write out the SSL session details.. actually the call below, but
* OpenSSL headers do strange typecasts confusing GCC.. */
/* PEM_write_SSL_SESSION(debug_log, SSL_get_session(ssl)); */
#if defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x00908000L
PEM_ASN1_write((i2d_of_void *)i2d_SSL_SESSION, PEM_STRING_SSL_SESSION, debug_log, (char *)SSL_get_session(ssl), NULL,NULL,0,NULL,NULL);
#elif (ALLOW_ALWAYS_SSL_SESSION_DETAIL == 1)
/* When using gcc 3.3.x and OpenSSL 0.9.7x sometimes a compile error can occur here.
* This is caused by an unpredicatble gcc behaviour on a cast of the first argument
* of PEM_ASN1_write(). For this reason this code section is disabled. To enable it,
* define ALLOW_ALWAYS_SSL_SESSION_DETAIL=1.
* Because there are two possible usable cast, if you get an error here, try the other
* commented line. */
PEM_ASN1_write((int(*)())i2d_SSL_SESSION, PEM_STRING_SSL_SESSION, debug_log, (char *)SSL_get_session(ssl), NULL,NULL,0,NULL,NULL);
=== modified file 'src/security/BlindPeerConnector.cc'
--- src/security/BlindPeerConnector.cc 2016-07-27 11:06:57 +0000
+++ src/security/BlindPeerConnector.cc 2016-08-07 12:39:50 +0000
@@ -29,56 +29,49 @@
return ::Config.ssl_client.sslContext;
}
bool
Security::BlindPeerConnector::initializeTls(Security::SessionPointer &serverSession)
{
if (!Security::PeerConnector::initializeTls(serverSession))
return false;
if (const CachePeer *peer = serverConnection()->getPeer()) {
assert(peer);
// NP: domain may be a raw-IP but it is now always set
assert(!peer->secure.sslDomain.isEmpty());
#if USE_OPENSSL
// const loss is okay here, ssl_ex_index_server is only read and not assigned a destructor
SBuf *host = new SBuf(peer->secure.sslDomain);
SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, host);
- if (peer->sslSession)
- SSL_set_session(serverSession.get(), peer->sslSession);
+ Security::SetSessionResumeData(serverSession, peer->sslSession);
} else {
SBuf *hostName = new SBuf(request->url.host());
SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, (void*)hostName);
#endif
}
return true;
}
void
Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error)
{
if (error) {
// XXX: forward.cc calls peerConnectSucceeded() after an OK TCP connect but
// we call peerConnectFailed() if SSL failed afterwards. Is that OK?
// It is not clear whether we should call peerConnectSucceeded/Failed()
// based on TCP results, SSL results, or both. And the code is probably not
// consistent in this aspect across tunnelling and forwarding modules.
if (CachePeer *p = serverConnection()->getPeer())
peerConnectFailed(p);
return;
}
-#if USE_OPENSSL
- const int fd = serverConnection()->fd;
- Security::SessionPtr ssl = fd_table[fd].ssl.get();
- if (serverConnection()->getPeer() && !SSL_session_reused(ssl)) {
- if (serverConnection()->getPeer()->sslSession)
- SSL_SESSION_free(serverConnection()->getPeer()->sslSession);
-
- serverConnection()->getPeer()->sslSession = SSL_get1_session(ssl);
+ if (auto *peer = serverConnection()->getPeer()) {
+ const int fd = serverConnection()->fd;
+ Security::MaybeGetSessionResumeData(fd_table[fd].ssl, peer->sslSession);
}
-#endif
}
=== modified file 'src/security/Session.cc'
--- src/security/Session.cc 2016-07-18 12:36:38 +0000
+++ src/security/Session.cc 2016-08-07 16:18:05 +0000
@@ -1,38 +1,102 @@
/*
* Copyright (C) 1996-2016 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/
+/* DEBUG: section 83 TLS session management */
+
#include "squid.h"
#include "anyp/PortCfg.h"
#include "base/RunnersRegistry.h"
+#include "Debug.h"
#include "ipc/MemMap.h"
#include "security/Session.h"
#include "SquidConfig.h"
#define SSL_SESSION_ID_SIZE 32
#define SSL_SESSION_MAX_SIZE 10*1024
+bool
+Security::SessionIsResumed(const Security::SessionPointer &s)
+{
+ bool result = false;
+#if USE_OPENSSL
+ result = SSL_session_reused(s.get()) == 1;
+#elif USE_GNUTLS
+ result = gnutls_session_is_resumed(s.get()) != 0;
+#endif
+ debugs(83, 7, "session=" << (void*)s.get() << ", query? answer: " << (result ? 'T' : 'F') );
+ return result;
+}
+
+void
+Security::MaybeGetSessionResumeData(const Security::SessionPointer &s, Security::SessionStatePointer &data)
+{
+ if (!SessionIsResumed(s)) {
+#if USE_OPENSSL
+ // nil is valid for SSL_get1_session(), it cannot fail.
+ data.reset(SSL_get1_session(s.get()));
+#elif USE_GNUTLS
+ gnutls_datum_t *tmp = nullptr;
+ const auto x = gnutls_session_get_data2(s.get(), tmp);
+ if (x != GNUTLS_E_SUCCESS) {
+ debugs(83, 3, "session=" << (void*)s.get() <<
+ " error: " << gnutls_strerror(x));
+ }
+ data.reset(tmp);
+#endif
+ debugs(83, 5, "session=" << (void*)s.get() << " data=" << (void*)data.get());
+ } else {
+ debugs(83, 5, "session=" << (void*)s.get() << " data=" << (void*)data.get() << ", do nothing.");
+ }
+}
+
+void
+Security::SetSessionResumeData(const Security::SessionPointer &s, const Security::SessionStatePointer &data)
+{
+ if (data) {
+#if USE_OPENSSL
+ if (!SSL_set_session(s.get(), data.get())) {
+ const auto ssl_error = ERR_get_error();
+ debugs(83, 3, "session=" << (void*)s.get() << " data=" << (void*)data.get() <<
+ " resume error: " << ERR_error_string(ssl_error, nullptr));
+ }
+#elif USE_GNUTLS
+ const auto x = gnutls_session_set_data(s.get(), data->data, data->size);
+ if (x != GNUTLS_E_SUCCESS) {
+ debugs(83, 3, "session=" << (void*)s.get() << " data=" << (void*)data.get() <<
+ " resume error: " << gnutls_strerror(x));
+ }
+#else
+ // critical because, how did it get here?
+ debugs(83, DBG_CRITICAL, "no TLS library. session=" << (void*)s.get() << " data=" << (void*)data.get());
+#endif
+ debugs(83, 5, "session=" << (void*)s.get() << " data=" << (void*)data.get());
+ } else {
+ debugs(83, 5, "session=" << (void*)s.get() << " no resume data");
+ }
+}
+
static bool
isTlsServer()
{
for (AnyP::PortCfgPointer s = HttpPortList; s != nullptr; s = s->next) {
if (s->secure.encryptTransport)
return true;
if (s->flags.tunnelSslBumping)
return true;
}
return false;
}
void
initializeSessionCache()
{
#if USE_OPENSSL
// Check if the MemMap keys and data are enough big to hold
// session ids and session data
assert(SSL_SESSION_ID_SIZE >= MEMMAP_SLOT_KEY_SIZE);
=== modified file 'src/security/Session.h'
--- src/security/Session.h 2016-07-18 12:36:38 +0000
+++ src/security/Session.h 2016-08-07 12:48:38 +0000
@@ -1,54 +1,81 @@
/*
* Copyright (C) 1996-2016 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/
#ifndef SQUID_SRC_SECURITY_SESSION_H
#define SQUID_SRC_SECURITY_SESSION_H
+#include "base/HardFun.h"
#include "security/LockingPointer.h"
#include <memory>
#if USE_OPENSSL
#if HAVE_OPENSSL_SSL_H
#include <openssl/ssl.h>
#endif
#endif
#if USE_GNUTLS
#if HAVE_GNUTLS_GNUTLS_H
#include <gnutls/gnutls.h>
#endif
#endif
namespace Security {
#if USE_OPENSSL
typedef SSL* SessionPtr;
CtoCpp1(SSL_free, SSL *);
typedef LockingPointer<SSL, Security::SSL_free_cpp, CRYPTO_LOCK_SSL> SessionPointer;
+typedef std::unique_ptr<SSL_SESSION, HardFun<void, SSL_SESSION*, &SSL_SESSION_free>> SessionStatePointer;
+
#elif USE_GNUTLS
typedef gnutls_session_t SessionPtr;
// Locks can be implemented attaching locks counter to gnutls_session_t
// objects using the gnutls_session_set_ptr()/gnutls_session_get_ptr ()
// library functions
CtoCpp1(gnutls_deinit, gnutls_session_t);
typedef LockingPointer<struct gnutls_session_int, gnutls_deinit_cpp, -1> SessionPointer;
+// wrapper function to get around gnutls_free being a typedef
+inline void squid_gnutls_free(void *d) {gnutls_free(d);}
+typedef std::unique_ptr<gnutls_datum_t, HardFun<void, void*, &Security::squid_gnutls_free>> SessionStatePointer;
+
#else
// use void* so we can check against NULL
typedef void* SessionPtr;
CtoCpp1(xfree, SessionPtr);
typedef LockingPointer<void, xfree_cpp, -1> SessionPointer;
+typedef std::unique_ptr<int> SessionStatePointer;
+
#endif
+/// whether the session is a resumed one
+bool SessionIsResumed(const Security::SessionPointer &);
+
+/**
+ * When the session is not a resumed session, retrieve the details needed to
+ * resume a later connection and store them in 'data'. This may result in 'data'
+ * becoming a nil Pointer if no details exist or an error occurs.
+ *
+ * When the session is already a resumed session, do nothing and leave 'data'
+ * unhanged.
+ * XXX: is this latter behaviour always correct?
+ */
+void MaybeGetSessionResumeData(const Security::SessionPointer &, Security::SessionStatePointer &data);
+
+/// Set the data for resuming a previous session.
+/// Needs to be done before using the SessionPointer for a handshake.
+void SetSessionResumeData(const Security::SessionPointer &, const Security::SessionStatePointer &);
+
} // namespace Security
#endif /* SQUID_SRC_SECURITY_SESSION_H */
=== modified file 'src/tests/stub_libsecurity.cc'
--- src/tests/stub_libsecurity.cc 2016-07-27 21:16:35 +0000
+++ src/tests/stub_libsecurity.cc 2016-08-07 12:39:50 +0000
@@ -69,20 +69,27 @@
Security::PeerOptions Security::ProxyOutgoingConfig;
void Security::PeerOptions::parse(char const*) STUB
Security::ContextPtr Security::PeerOptions::createClientContext(bool) STUB_RETVAL(NULL)
void Security::PeerOptions::updateTlsVersionLimits() STUB
Security::ContextPtr Security::PeerOptions::createBlankContext() const STUB
void Security::PeerOptions::updateContextCa(Security::ContextPtr &) STUB
void Security::PeerOptions::updateContextCrl(Security::ContextPtr &) STUB
void Security::PeerOptions::dumpCfg(Packable*, char const*) const STUB
long Security::PeerOptions::parseOptions() STUB_RETVAL(0)
long Security::PeerOptions::parseFlags() STUB_RETVAL(0)
void parse_securePeerOptions(Security::PeerOptions *) STUB
#include "security/ServerOptions.h"
//Security::ServerOptions::ServerOptions(const Security::ServerOptions &) STUB
void Security::ServerOptions::parse(const char *) STUB
void Security::ServerOptions::dumpCfg(Packable *, const char *) const STUB
Security::ContextPtr Security::ServerOptions::createBlankContext() const STUB
bool Security::ServerOptions::createStaticServerContext(AnyP::PortCfg &) STUB_RETVAL(false)
void Security::ServerOptions::updateContextEecdh(Security::ContextPtr &) STUB
+#include "security/Session.h"
+namespace Security {
+bool SessionIsResumed(const Security::SessionPointer &) STUB_RETVAL(false)
+void MaybeGetSessionResumeData(const Security::SessionPointer &, Security::SessionStatePointer &) STUB
+void SetSessionResumeData(const Security::SessionPointer &, const Security::SessionStatePointer &) STUB
+} // namespace Security
+
More information about the squid-dev
mailing list