Opened 3 years ago

Closed 3 years ago

#5449 regression closed fixed (fixed)

new SSL implementation gives AttributeError when using an invalid key file

Reported by: wulczer Owned by: exarkun
Priority: normal Milestone: Twisted-12.0
Component: core Keywords:
Cc: Branch: branches/early-context-error-5449
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

The attached snippet tries to open an SSL connection using /etc/passwd as the key file.

In Twisted 11.0.0 this gave a traceback with an OpenSSL.SSL.Error, in Twisted 11.1.0 it also tracebacks with an AttributeError.

Attachments (1)

ssl.py (778 bytes) - added by wulczer 3 years ago.
snippet demonstrating the change in behaviour

Download all attachments as: .zip

Change History (10)

Changed 3 years ago by wulczer

snippet demonstrating the change in behaviour

comment:1 Changed 3 years ago by wulczer

  • Summary changed from new SSL implementation gives AttributeError when using an invalid key file results to new SSL implementation gives AttributeError when using an invalid key file

comment:2 Changed 3 years ago by itamar

  • Owner set to itamar

comment:3 Changed 3 years ago by itamar

  • Milestone set to Twisted-12.0

comment:4 Changed 3 years ago by therve

Here's the output:

Unhandled Error
Traceback (most recent call last):
  File "/home/therve/Projects/Twisted/trunk/twisted/python/log.py", line 84, in callWithLogger
    return callWithContext({"system": lp}, func, *args, **kw)
  File "/home/therve/Projects/Twisted/trunk/twisted/python/log.py", line 69, in callWithContext
    return context.call({ILogContext: newCtx}, func, *args, **kw)
  File "/home/therve/Projects/Twisted/trunk/twisted/python/context.py", line 118, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "/home/therve/Projects/Twisted/trunk/twisted/python/context.py", line 81, in callWithContext
    return func(*args,**kw)
--- <exception caught here> ---
  File "/home/therve/Projects/Twisted/trunk/twisted/internet/posixbase.py", line 591, in _doReadOrWrite
    why = selectable.doWrite()
  File "/home/therve/Projects/Twisted/trunk/twisted/internet/tcp.py", line 419, in doConnect
    self._connectDone()
  File "/home/therve/Projects/Twisted/trunk/twisted/internet/tcp.py", line 427, in _connectDone
    self.protocol.makeConnection(self)
  File "/home/therve/Projects/Twisted/trunk/twisted/protocols/tls.py", line 293, in makeConnection
    tlsContext = self.factory._contextFactory.getContext()
  File "ssl.py", line 11, in getContext
    ctx.use_certificate_file('/etc/passwd')
OpenSSL.SSL.Error: [('PEM routines', 'PEM_read_bio', 'no start line'), ('SSL routines', 'SSL_CTX_use_certificate_file', 'PEM lib')]
Unhandled Error
Traceback (most recent call last):
  File "ssl.py", line 38, in <module>
    reactor.run()
  File "/home/therve/Projects/Twisted/trunk/twisted/internet/base.py", line 1169, in run
    self.mainLoop()
  File "/home/therve/Projects/Twisted/trunk/twisted/internet/base.py", line 1181, in mainLoop
    self.doIteration(t)
  File "/home/therve/Projects/Twisted/trunk/twisted/internet/pollreactor.py", line 167, in doPoll
    log.callWithLogger(selectable, _drdw, selectable, fd, event)
--- <exception caught here> ---
  File "/home/therve/Projects/Twisted/trunk/twisted/python/log.py", line 84, in callWithLogger
    return callWithContext({"system": lp}, func, *args, **kw)
  File "/home/therve/Projects/Twisted/trunk/twisted/python/log.py", line 69, in callWithContext
    return context.call({ILogContext: newCtx}, func, *args, **kw)
  File "/home/therve/Projects/Twisted/trunk/twisted/python/context.py", line 118, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "/home/therve/Projects/Twisted/trunk/twisted/python/context.py", line 81, in callWithContext
    return func(*args,**kw)
  File "/home/therve/Projects/Twisted/trunk/twisted/internet/posixbase.py", line 599, in _doReadOrWrite
    self._disconnectSelectable(selectable, why, inRead)
  File "/home/therve/Projects/Twisted/trunk/twisted/internet/posixbase.py", line 266, in _disconnectSelectable
    selectable.connectionLost(failure.Failure(why))
  File "/home/therve/Projects/Twisted/trunk/twisted/internet/tcp.py", line 433, in connectionLost
    Connection.connectionLost(self, reason)
  File "/home/therve/Projects/Twisted/trunk/twisted/internet/tcp.py", line 277, in connectionLost
    protocol.connectionLost(reason)
  File "/home/therve/Projects/Twisted/trunk/twisted/protocols/tls.py", line 455, in connectionLost
    self._tlsConnection.bio_shutdown()
  File "/home/therve/Projects/Twisted/trunk/twisted/protocols/policies.py", line 112, in __getattr__
    return getattr(self.transport, name)
exceptions.AttributeError: 'NoneType' object has no attribute '_tlsConnection'
TIMEOUT

comment:5 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/early-context-error-5449

(In [33454]) Branching to 'early-context-error-5449'

comment:6 Changed 3 years ago by exarkun

(In [33455]) Make IReactorSSL.connectSSL raise a synchronous exception if the context factory passed to it is broken

refs #5449

comment:7 Changed 3 years ago by exarkun

  • Keywords review added
  • Owner itamar deleted

The behavior in 11.0.0 is:

Unhandled Error
Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/branches/twisted-11.0.0/twisted/python/log.py", line 84, in callWithLogger
    return callWithContext({"system": lp}, func, *args, **kw)
  File "/home/exarkun/Projects/Twisted/branches/twisted-11.0.0/twisted/python/log.py", line 69, in callWithContext
    return context.call({ILogContext: newCtx}, func, *args, **kw)
  File "/home/exarkun/Projects/Twisted/branches/twisted-11.0.0/twisted/python/context.py", line 118, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "/home/exarkun/Projects/Twisted/branches/twisted-11.0.0/twisted/python/context.py", line 81, in callWithContext
    return func(*args,**kw)
--- <exception caught here> ---
  File "/home/exarkun/Projects/Twisted/branches/twisted-11.0.0/twisted/internet/selectreactor.py", line 146, in _doReadOrWrite
    why = getattr(selectable, method)()
  File "/home/exarkun/Projects/Twisted/branches/twisted-11.0.0/twisted/internet/tcp.py", line 674, in doConnect
    self._connectDone()
  File "/home/exarkun/Projects/Twisted/branches/twisted-11.0.0/twisted/internet/ssl.py", line 149, in _connectDone
    self.startTLS(self.ctxFactory)
  File "/home/exarkun/Projects/Twisted/branches/twisted-11.0.0/twisted/internet/tcp.py", line 571, in startTLS
    if Connection.startTLS(self, ctx, client):
  File "/home/exarkun/Projects/Twisted/branches/twisted-11.0.0/twisted/internet/tcp.py", line 402, in startTLS
    self.socket = SSL.Connection(ctx.getContext(), self.socket)
  File "ssl.py", line 11, in getContext
    ctx.use_certificate_file('/etc/passwd')
OpenSSL.SSL.Error: [('PEM routines', 'PEM_read_bio', 'no start line'), ('SSL routines', 'SSL_CTX_use_certificate_file', 'PEM lib')]
TIMEOUT

However, that sucks. It's quite unclear in what state the TCP connection is left, and it's a very difficult error for an application to handle, since it's merely logged as unhandled, not passed to any application code.

I don't think this behavior was ever considered, it merely occurred by accident. Better behaviors would be:

  1. Raise the exception from connectSSL
  2. Pass the exception to the factory's clientConnectionFailed.

The latter of these two is rather tempting, however the former is much simpler to implement and more closely mirrors the server-side behavior.

Hence, the behavior in the branch is now:

$ python ssl.py 
Traceback (most recent call last):
  File "ssl.py", line 36, in <module>
    reactor.connectSSL('twistedmatrix.com', 443, Factory(), CtxFactory())
  File "/home/exarkun/Projects/Twisted/branches/early-context-error-5449/twisted/internet/posixbase.py", line 452, in connectSSL
    tlsFactory = tls.TLSMemoryBIOFactory(contextFactory, True, factory)
  File "/home/exarkun/Projects/Twisted/branches/early-context-error-5449/twisted/protocols/tls.py", line 589, in __init__
    contextFactory.getContext()
  File "ssl.py", line 11, in getContext
    ctx.use_certificate_file('/etc/passwd')
OpenSSL.SSL.Error: [('PEM routines', 'PEM_read_bio', 'no start line'), ('SSL routines', 'SSL_CTX_use_certificate_file', 'PEM lib')]

I expect it would be possible to return exactly to the 11.0.0 (and earlier - apparently back as far as 8.0.0, where I stopped checking) behavior. I'm not sure that's desirable, though.

Build results

comment:8 Changed 3 years ago by itamar

  • Keywords review removed
  • Owner set to exarkun

Throwing immediately seems reasonable, given all the relevant APIs are synchronous. And it's a more clever solution than I would've come up with.

Remaining issues:

  1. Equivalent test for startTLS. Looking at the code, I assume it will just work. Could be done in separate ticket.
  2. listenSSL presumably has the same issue. Doing this in separate ticket would also be fine.
  3. News file.

Given fixes/new tickets for above, feel free to merge.

comment:9 Changed 3 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [33468]) Merge early-context-error-5449

Author: exarkun
Reviewer: itamar
Fixes: #5449

Change the users of application-supplied SSL context factories to invoke
the getContext method early and allow errors to propagate. This improves
the behavior in the case of broken context factories which cannot produce
a context object by making the error happen closer to where the broken
object is given to Twisted.

Note: See TracTickets for help on using tickets.