Opened 8 years ago

Closed 8 years ago

#1976 defect closed fixed (fixed)

twisted.internet.ssl.Certificate.peerFromTransport can return invalid Certificates

Reported by: exarkun Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: Branch:
Author: Launchpad Bug:

Description

If there is no peer certificate, it creates a Certificate wrapped around None.

It should fail some other, better way.

Change History (8)

comment:1 Changed 8 years ago by glyph

It seems like there are 3 choices: return None, return a Deferred which fires when the cert is available, or raise an exception.

Right now I think I favor raising an exception.

comment:2 Changed 8 years ago by exarkun

I like exceptions.

comment:3 Changed 8 years ago by glyph

Ready for review in certerr-1976.

comment:4 Changed 8 years ago by glyph

  • Keywords review added

comment:5 Changed 8 years ago by glyph

  • Priority changed from high to highest

comment:6 Changed 8 years ago by jerub

  • Keywords review removed

Sweet change. Nice and simple, correct docstrings that exist, etc.

I'm confused about the lack of an actual error message to go in twisted.internet.error.CertificateError. It seems all the python code I've ever written has had both an exception, and some kind of information about the exception that occurred. Here, CertificateError() is created without any message. This causes str(CertificateError()) to equal . I don't think I like that.

Please alter the implementation or usage of CertificateError so that str() isn't dangerously low on information.

comment:7 Changed 8 years ago by glyph

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

(In [17837]) ssl.Certificate.peerFromTransport no longer returns invalid certificates.

This change fixes a defect where sometimes a transport without a certificate
would raise exceptions, sometimes return an invalid Certificate object wrapped
around None. This fixes that bug, as well as adding some test coverage for
those constructors and the error reporting surrounding them.

Author: glyph

Reviewer: jerub

Fixes #1976

comment:8 Changed 3 years ago by <automation>

  • Owner glyph deleted
Note: See TracTickets for help on using tickets.