Opened 3 years ago

Closed 7 months ago

#5190 enhancement closed fixed (fixed)

RFC 6125 ("Service Identity") implementation

Reported by: glyph Owned by: glyph
Priority: normal Milestone:
Component: core Keywords:
Cc: hs@… Branch: branches/service-identity-5190
(diff, github, buildbot, log)
Author: glyph Launchpad Bug:

Description (last modified by glyph)

You know how when an x509 certificate used in TLS has a subject, and the subject has some fields, and a user or admin somewhere typed something in order to access that domain name, and they're supposed to match? Since March of this year (2011), there's actually a specification that covers the expected behavior for that check, even in the face of weirdnesses like SRV record indirection, SNI, CNAMEs, and URIs which might not match hostnames exactly for some reason.

We should implement that spec. This would probably have to go into a smarter TLS endpoint, or endpoint wrapper, but at this point I think exactly where it would go is open to discussion, as I'm not an expert on the spec yet.

Change History (15)

comment:1 Changed 3 years ago by glyph

  • Description modified (diff)

comment:2 follow-up: Changed 9 months ago by hynek

  • Cc hs@… added

Can we expand this ticket to integrate it with CertificateOptions? Just adding a solitary function doesn’t seem very useful to me.

comment:3 in reply to: ↑ 2 Changed 8 months ago by glyph

Replying to hynek:

Can we expand this ticket to integrate it with CertificateOptions? Just adding a solitary function doesn’t seem very useful to me.

Let's start with the function :-). That by itself is going to be tricky enough, because we're going to need to get some ASN.1 parsing in there, extract subjectAltName extensions, somehow do something with the distinction between URI-ID, DNS-ID and SRV-ID... once we have something correct that a security person can audit, the integration portion should at least be relatively straightforward.

For example, it would be nice to be able to fully enumerate the inputs and outputs of the function, so we can see what needs to propagate through. I don't think that we can do this with CertificateOptions alone, for example, since it never gets passed a hostname.

comment:4 Changed 8 months ago by glyph

I'm not saying that this ticket can't include CertificateOptions integration, by the way, if it's short enough and makes sense; my estimation of the work involved may be off. What I'm saying is that I'd be just as happy to land something without it, since at least that improves the story in the docs; other than "you're on your own" I can say "call this function".

comment:5 follow-up: Changed 8 months ago by hynek

After my latest discoveries I kind of agree, since I’m not positive where to actually put the verification. I’m really starting to tend to it belongs into TLSMemoryBIOProtocol. So let’s concentrate on just the verification for the moment.

Currently it begs three questions for me:

  • do you want this function to be public? Might be even a good idea, although it will have to be OpenSSL-specific (X509, hostname) no matter what we do. Unless we define/have an interface for certificates…
  • do you want to avoid depending on pyasn1-modules? Because I thought itamar’s code already solved the core problem here in his branch? Or are there any problems/omissions? (haven’t dug into it yet)
  • do you want the API to return bool or to raise an exception? Personally, I tend to the later so it’s harder to ignore the result by accident.

comment:6 in reply to: ↑ 5 Changed 8 months ago by glyph

Replying to hynek:

After my latest discoveries I kind of agree, since I’m not positive where to actually put the verification. I’m really starting to tend to it belongs into TLSMemoryBIOProtocol. So let’s concentrate on just the verification for the moment.

If we can get that right it seems like the other discussion about how to invoke it will be a lot easier.

Currently it begs three questions for me:

  • do you want this function to be public?

Yes. This is way, way, way too *!@#ing hard to implement, and I would like to have this as a reference implementation that others can import and use, possibly even spin it out into a separate package, or at least into Cryptography.io. It is a God Damned Travesty that OpenSSL doesn't just implement this itself.

Might be even a good idea, although it will have to be OpenSSL-specific (X509, hostname) no matter what we do. Unless we define/have an interface for certificates…

"Be the public API you want to see in the world". As the prophets Augie and Nathaniel put it, practice anti-Rumsfeldian programming.

First, write something that just implements the algorithm in the RFC; ignore even stuff like encodings and asn.1; verify some in-memory structures. Then write some objects that implement those structures by parsing asn.1 junk. We already have twisted.internet.ssl.Certificate and twisted.internet.ssl.DistinguishedName. Perhaps this needs to add twisted.internet.ssl.TrustChain as well, or just some signature-chain public stuff on Certificate.

  • do you want to avoid depending on pyasn1-modules? Because I thought itamar’s code already solved the core problem here in his branch? Or are there any problems/omissions? (haven’t dug into it yet)

If we can avoid depending on pyasn1, great, but as far as I can tell you still need it to parse the extensions (and we need to do more than SANs if we want to do full trust chain verification).

  • do you want the API to return bool or to raise an exception? Personally, I tend to the later so it’s harder to ignore the result by accident.

I would like to have (at least) two levels of API, I think: one (low level) function which does the verification and returns a constant, since there are multiple ways to fail and at least one way to succeed, one (high level) function which calls the low level one and then translates it into an exception in any failure case.

Just a suggestion though. If you're only going to go with one or the other, go with the exception one, you're right that it should be hard to ignore the result of this.

comment:7 Changed 8 months ago by hynek

To transplant what has been discussed on IRC:

I’ve developed this externally to a) move faster and b) offer that code to other projects that are using pyOpenSSL: https://github.com/hynek/service_identity

I did my best to keep the APIs as agnostic as possible; they can be trivially moved to any another SSL implementation.

Politically, there are three possible ways forward:

  • it gets merged into pyOpenSSL (jp already mentioned that he’s not entirely enthusiastic to add code that isn‘t just a wrapper around something – which I fully understand),
  • it gets merged into Twisted (I’m not enthusiastic because it’s pyOpenSSL-specific code, not Twisted),
  • it stays separate. That’s what it currently looks like and I think once the API is finalized and docs are written (there are none because of the volatile API and the uncertainty of the project’s future form) I will move the project into the pyca GitHub organization.

Now, I really need feedback before I move further.

There’s also a pre-alpha release on pypi: https://pypi.python.org/pypi/service_identity/0.1 so you can play with it easier.

So please play with it, tell me about bugs/bad design (gladly here/per email) and make suggestions about the future.

Once more: no feedback/review = no progress.

Thank you, I’m dropping the mic here and go to work on my PyCon talk now (already getting e-mails about flight changes…).

comment:8 Changed 7 months ago by glyph

  • Author set to glyph
  • Branch set to branches/service-identity-5190

(In [42065]) Branching to service-identity-5190.

comment:9 Changed 7 months ago by glyph

I'd say it's close to done now; I just have to integrate hynek's changes for service_identity 0.2.

Well, I would say that, except for the fact that there's some bizarre hang on connection failure - like, clientConnectionFailed, that should have nothing to do with TLS - in my documentation example that doesn't replicate in any of the unit tests.

comment:10 Changed 7 months ago by glyph

Okay, as I said in a commit message, the race condition was actually totally a red herring, nothing to do with the implementation changes in this branch. It was a bug in the way the example was put together.

comment:11 Changed 7 months ago by glyph

  • Keywords review added

Ready for review, I think.

comment:12 Changed 7 months ago by hawkowl

  • Owner set to hawkowl

comment:13 Changed 7 months ago by hawkowl

https://github.com/twisted/twisted/compare/trunk...service-identity-5190#diff-08abb4fe549724dea7c09c774d9272efR1184

There seems to be two pump.flush()es here...

(not done yet, but before I forget)

comment:14 Changed 7 months ago by hawkowl

  • Keywords review removed
  • Owner changed from hawkowl to glyph

I can't find any problems. Fix the pump.flush() duplication and then please merge.

comment:15 Changed 7 months ago by glyph

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

(In [42118]) Merge service-identity-5190

Author: glyph

Reviewer: hawkowl

Fixes: #5190

twisted.internet.ssl.CertificateOptions now provides a 'hostname' field, allowing clients to specify and verify the identity of the peer they're communicating with. When used with the service_identity library from PyPI, this provides support for service identity verification from RFC 6125, as well as server name indication from RFC 6066.

Note: See TracTickets for help on using tickets.