Skip to content
This repository has been archived by the owner on Feb 13, 2020. It is now read-only.

Add optional SNI support via TxSNI library from pypi #538

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

troglodyne
Copy link

Three configuration options are added to caldavd.plist to make this work:

  • SNIMode (BOOL): Whether to use SNI. If not, falls back to old SSL code.
  • SSLDir (STRING): Directory where all the key/cert/CAcert bundle .pem
    files live, indexed by hostname
  • DefaultSSLBundle (STRING): What file to load as certificate when no
    hostname has been indicated by the client when connecting or no cert
    exists for the given host.

Certificate structure will be as such:

|_DirectoryNameIConfiguredInCaldavd.plist
  |_my.supercool.host.pem
  |_some.other.host.pem
  |_...

Symlinks also work if you have multiple domains covered on a single
cert.

CAVEATS: This mode of operation assumes that no certificates were
generated with keys that are protected by passphrases. The old
code which exists to account for this won't help you in SNI mode, as we
no longer go down those code paths (TxSNI doesn't appear to support
that, though with SNI it does sound somewhat hairy to have to deal with
for potentially a lot of certs).

No updates to the testsuite have been made for this change.
Considering that I didn't break the old pathways for SSL/TLS (and that
this doesn't appear to break the current testsuite), I thought that to be
an acceptable way to implement this, as this is my first real foray into
python (and I'm not really all that familiar with the test harness that
is in user here, much less proper mocking techniques for python tests).

Three configuration options are added to caldavd.plist to make this work:
* SNIMode (BOOL): Whether to use SNI. If not, falls back to old SSL code.
* SSLDir (STRING): Directory where all the key/cert/CAcert bundle .pem
  files live, indexed by hostname
* DefaultSSLBundle (STRING): What file to load as certificate when no
  hostname has been indicated by the client when connecting or no cert
  exists for the given host.

Certificate structure will be as such:
|_DirectoryNameIConfiguredInCaldavd.plist
  |_my.supercool.host.pem
  |_some.other.host.pem
  |_...

Symlinks also work if you have multiple domains covered on a single
cert.

CAVEATS: This mode of operation assumes that no certificates were
generated with keys that are protected by passphrases. The old
code which exists to account for this won't help you in SNI mode, as we
no longer go down those code paths (TxSNI doesn't appear to support
that, though with SNI it does sound somewhat hairy to have to deal with
for potentially a *lot* of certs).

No updates to the testsuite have been made for this change, but
considering that I didn't break the old pathways for SSL/TLS, it at
least doesn't break the current testsuite.
@troglodyne
Copy link
Author

Secondary note:
Any pointers on a good test in the current testsuite to update would be appreciated, and I am willing to update any relevant tests as needed. When I get some extra time I'll start spelunking in there and try to find something to update if I get no reply.

Context on the PR:
I am working on integrating CCS with cPanel (as they are currently my employer), and not having SNI support was a bit of a dealbreaker. Adding this seemed like less work than fooling with other calendar servers to get the feature set that was desired.

Anyways, thanks for all the work that went into this project thus far. I know I kinda shot this one in as a PR out of the blue, so I won't feel too bad if it gets declined :)

@dreness
Copy link
Member

dreness commented May 21, 2019

Any pointers on a good test in the current testsuite to update would be appreciated, and I am willing to update any relevant tests as needed.

Hi! I'd start in a couple places:

  • unit tests: have a look in calendarserver/tap/test for some of the existing tests, which you can either use directly (if they're sufficient to verify correct operation with your change) or as a template for whatever changes might be needed.
  • integration tests: use CalDAVTester for this, which spawns a server in a test config and runs client requests over the network. In this case you shouldn't have to touch the client requests much, if at all - I think testing this change would be mostly about customizing the CalDAVTester server configs.

Anyways, thanks for all the work that went into this project thus far. I know I kinda shot this one in as a PR out of the blue, so I won't feel too bad if it gets declined :)

Thanks for the contribution! I need to figure out why the buildbot stopped building... Oh, it didn't stop building, but... the little icon isn't showing up on GitHub for some reason. Looks like this PR passed the existing tests (as you said), however there is one failure that cropped up around the start of 2019. I have played with it a bit, but so far I can't quite put my finger on how it's broken.

@troglodyne
Copy link
Author

Thanks for the feedback. I'll try to get the testing bits in later this week.

@troglodyne
Copy link
Author

Update: This has taken a lot longer to get to than I had hoped previously. Currently I see two options I have to resolving the difficulties I've been having here:

  • Resolve the bugs in integration test fixture code which leads to constant "reactor was unclean" errors (as was also seen in PR Allow checkdatabaseschema.py to work with non-standard db name and password #473) that cause almost all tests in calendarserver/tap/test/test_caldav.py to fail reliably.
  • Go with a 100% mocked out solution. I am less comfortable with doing this, as integration level testing appears to be the approach taken with the test suite in general.

Hopefully the first option is possible to resolve in a reasonable amount of time. That's what I'm going with for now, as some guides do exist online regarding how to properly ensure you are disconnected before attempting to connect via twisted's trial framework.

@troglodyne
Copy link
Author

Cause of that error in PR #473 is because the test was being ran as root, and as such pg_ctl explodes. The error message is simply eaten, unfortunately (so you are left scratching your head for the most part).

Ran the tests as the calendarserver's daemon user I had setup for it and everything works fine. Only figured this out though due to merging in some code I had to optimize the startup of pg by not blithely trying to start it when you could just first check whether it's up and connect to that if it exists (helps startup time of the bin/ helper scripts like calendarserver_export). Incidentally, I had added extra logging there for the return of pg_ctl's status line, which tipped me off to the actual problem.

Hopefully can wrap this up today.

Will expand on this soon, will want to add assertions upon the actual
state of the SSL cert returned when passing the HOST header, etc.

Probably will add a cert to the same dir as that other test cert for
some.domain just to see we got something other than the default cert
after that
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants