Update SSLContext handling

[tsaarni]

• Change deprecated ssl.wrap_socket() to SSLContext.wrap_context().
• Add new server hook to allow user to create custom SSLContext.
• Updated the documentation.

Fixes #1140

Signed-off-by: Tero Saarni tero.saarni@est.tech

[tsaarni]

Notes for reviewers

1. The proposed PR does not change the --ssl_version behavior. With the current python and openssl versions the ssl.PROTOCOL_ flags have very little effect anymore and exposing SSLContext.options and other parameters would be required for more control. However, I did not add --ssl-options for setting options. Instead I added a server hook for user to override the default SSL context.

2. I think typically SSLContext and wrapped socket would be created for the server socket, before accept() call. However sync, gthread and eventlet workers do this for the socket associated with the client, after accept() call. Other workers seem to differ due to their respective event frameworks. I did not change the existing behavior though, since it might be backwards incompatible and their current implementation has (possibly inadvertent?) beneficial side-effect of reading certificates from disk at every client connect, therefore they support certificate rotation / hot reload. With the new custom SSL context hook, it could be possible for the user to cache SSL context if performance requires that.

[tilgovi]

I'm going to try to give this a review, but before I forget I just want to link it to #2118 so that we don't forget to consider implication for reload.

[tilgovi]

Very clean. I like it a lot.

[tsaarni]

Kind reminder, I'd still be interested in getting this merged :)

[javabrett]

@tsaarni good stuff here, thanks.

Having considered implementing this myself in the past ... one of the things I was trying to decide is whether SSLContext should come from a factory, and if so, how often instances should be created, and where and how long they should be retained. This would presumably have performance impacts and potentially runtime considerations for how TLS configuration such as certificates etc. could be allowed to change.

IIUC your implementation is a factory via a config callable ... which is called for each TLS wrap of a socket when handling connections. My interest is - is that necessary, or should we be reusing SSLContext instances. I don't have details of what, if anything could be expensive in their construction, but provided safe, reusing a SSLContext should have some performance benefit, assuming loading certs and keys costs at least something.

Could you confirm my understanding and comment on whether it would be beneficial to change the lifecycle of SSLContext instances to be something other than single use, e.g. storing them in an attribute in BaseSocket?

[tsaarni]

I think that most servers do not use SSLContext like Gunicorn does and I believe it is meant to be reused instead of recreated. I don't have any guestimates on how much performance might be lost: I doubt real disk I/O happens since files are likely kept in page cache anyways. There is some other unnecessary work when the PEM files are decoded, but it is just base64 decode for few hundred bytes, so very minor. If people feel that the current performance has been good enough, maybe there is no need to change this?

If changing the current logic and reusing the SSLContext, then at least in my use case there would need to be some other mechanism to "hot" reload certificates to support rotation without server restart. It is bit complicated topic though and I would prefer not to go there in this PR, if acceptable. Some alternative approaches for certificate hot reload are:

1. Start a thread/task watching for file changes using inotify and other OS specific means
2. Cache SSLContext for time-to-live period. Check at every accept() if we are over the TTL period and create new context if we are. To ensure that the new certificate is reloaded in timely manner, the TTL period should be set relative to the overlapping validity period of old and new server certificate.

[javabrett]

@tsaarni agreed. Now that I look at the deprecated wrap_socket: https://github.com/python/cpython/blob/ee0ac328d38a86f7907598c94cb88a97635b32f8/Lib/ssl.py#L1421-L1449

... it is simply chaining to construct a disposable SSLContext anyway, so cannot be worse.

In future though, without a reused SSLContext it will be impossible to take advantage of e.g. cached client SSLSessions.

[javabrett]

LGTM.

[javabrett]

@tsaarni I wanted to check whether you considered using ssl.create_default_context(...) here instead of constructing directly.

From docs:

• https://github.com/python/cpython/blob/5765eaa13654e5f812a286700da7d6b8e144da0e/Lib/ssl.py#L740-L746

The protocol and settings may change anytime without prior
deprecation. The values represent a fair balance between maximum
compatibility and security.

• https://docs.python.org/3/library/ssl.html?highlight=sslkeylogfile#ssl.create_default_context

The settings are chosen by the ssl module, and usually represent a higher security level than when calling the SSLContext constructor directly.

Looks like Python is encouraging use here if you want to adopt over-time the baseline secure default-options and features for ssl, with a leaning towards most-secure-defaults. When changes are made in future versions of Python to disable or enable features, they might be made here.

So I think a trade-off between no-unplanned-changes seen by Gunicorn, vs auto-adopt best defaults, which are then overridable.

Interesting also that when the key-logging option was added in python/cpython@c7f7069 , it was added to create_default_context() and not the ctor.

WDYT?

Edit: I have a commit for this I can push.

[tsaarni]

Hey @javabrett, thanks for your comments!

I wanted to check whether you considered using ssl.create_default_context(...) here instead of constructing directly.
...
Looks like Python is encouraging use here if you want to adopt over-time the baseline secure default-options and features for ssl, with a leaning towards most-secure-defaults. When changes are made in future versions of Python to disable or enable features, they might be made here.

Using that would mean removing the existing --ssl_version config option since ssl.create_default_context() does not accept that option, unlike SSLContext does. People who care about TLS details are likely aware that --ssl_version has not fully worked anymore with recent versions of Python, but I was bit unsure about touching that. I assumed the PR might be easier to accept if it is fully backwards compatible, especially since there has not been that much development activity in the project recently. But I do understand if some kind of deprecation process might be wanted to reduce old baggage!

Looking at the current implementation of ssl.create_default_context() there is not much there at this point besides of the SSLKEYLOGFILE support. But of course in future this may change and in that case it wuold be nice to pick up secure defaults from there.

(edit 2022-02-18: moved part of the comment down below to better preserve chronological order of discussion)

[benoitc]

i kepe that discussio open. I would like also go to defaults. A release is planned this week, i will try to add that change myself. If it's a quick change for you feel free to dit by now :)

[tsaarni]

@benoitc How should the deprecation of the existing config option be done? Should it raise some error that points out the new way? Are we ok with non-backwards compatible change that might potentially break applications?

[benoitc]

good question. That will be a major version, so breaking changes will need at least to be documented. If we want to make it cleaner maybe we can raise a Deprecation warning when the option is used and just ignore it?

[javabrett]

@tsaarni I did some ergonomics testing on the new config setting and it highlighted another idea I had earlier ... to test, I took a subset of the functionality in the example config you provided: say just want to set TLSv1.3 only (minimum).

I found I wanted to write something like config.py:

def ssl_context(conf):
    import ssl

    context = conf._create_default_ssl_context()
    context.minimum_version = ssl.TLSVersion.TLSv1_3
    return context

Changes being:

• I don't have to know/copy/maintain the standard Gunicorn-specific standard configuration logic of the default SSLContext, as I (optionally) obtain the base object from conf._create_default_ssl_context() (name could probably use some work). Currently this knowledge has to be copied from example_config.py or sock.py, which might later change.
• Configuration of the default context is pulled-up to a Config class method.
• Factory has access to config self, so no longer need to conf....(conf)

The difficulty in doing that is that it makes default config harder, since the default value of ssl_context would have to rely on other settings as this stand, and this isn't possible. I proposed a factory method which checks whether ssl_config (default None is set; if-so, use it, otherwise call the default factory method.

    def create_default_ssl_context(self):
        factory = self.settings['ssl_context'].get()
        if callable(factory):
            return factory(self)
        else:
            return self._create_default_ssl_context()

I have a commit for this - keen for your thoughts and can push it to your branch for review.

[tsaarni]

(edit 2022-02-18: moved below part from my previous comment here to better preserve chronological order of discussion)

@tsaarni I did some ergonomics testing on the new config setting and it highlighted another idea I had earlier ... to test, I took a subset of the functionality in the example config you provided: say just want to set TLSv1.3 only (minimum).

Good idea! I added a commit in this PR that adds the functionality but I'd like to propose following: It looked to me like Config class has not been used for method like this, so I just added a factory function default_ssl_context_factory as the second parameter to the hook function. I've also updated the example code accordingly. I hope this has the same usability improvement as adding capability to Config to construct SSLContexts.

[dumblob]

I think only the .rst documentation needs updating (ssl_context() signature has a second argument) and then this should be ready to merge 😉.

[tsaarni]

Good catch @dumblob, thanks!

I've rebased and regenerated the .rst doc. Apparently the .py vs .rst docs had ended up bit out of sync even previously which I tried to fix now. Furthermore, some defaults depend on which system the .rst generation is executed (default syslog address for example) so they seem to be toggling back and forth :)

[dumblob]

Furthermore, some defaults depend on which system the .rst generation is executed (default syslog address for example) so they seem to be toggling back and forth :)

Whoa! Wouldn't expect this sneaky evil behavior. Thanks for pointing it out!

Anyone with permissions to merge this?

[tilgovi]

@javabrett I've been kinda expecting you to push the button. Any reason we're not ready to merge this?

[uedvt359]

What exactly is holding this up? This PR blocks a lot of TLS improvements we'd like to roll out, so if there is anything I could help with to get this merged and released, please let me know!

[tsaarni]

What exactly is holding this up? This PR blocks a lot of TLS improvements we'd like to roll out, so if there is anything I could help with to get this merged and released, please let me know!

@uedvt359 It seems there has not been much activity overall in the project, so I guess the likely reason is that maintainers are just busy and not able to look at this PR.

But in case there is any concerns with this PR, I'm of course also interested to work and address them..

[tsaarni]

Kind reminder ❤️ I'm still interested in workign for this getting merged :)

[uedvt359]

It looks like this PR is stuck waiting for approval/rereview by @javabrett. Brett, could you please take a look at this again? In your last comment you mentioned you wanted to publish a patch regarding simplifying the default config.

Alternatively, can @tilgovi and/or @shevisj and/or @benoitc go forward despite Brett's outstanding comments?

[dumblob]

Could some new maintainers get permission to push to this repo? I think gunicorn is still very useful nowadays despite all the async hype and it is a pity the community is being really slowed down, nearly stopped, by just the lack of permissions.

[benoitc]

i am testing this change. there is a new pending release coming.

On Mon 3 Oct 2022 at 11:31, dumblob ***@***.***> wrote: Could some new maintainers get permission to push to this repo? I think gunicorn is still very useful nowadays despite all the async hype and it is a pity the community is being really slowed down, nearly stopped, by just the lack of permissions. — Reply to this email directly, view it on GitHub <#2649 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADRISM5LGHTKFXJI3EUT3WBKRVPANCNFSM5D2UAOGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Sent from my Mobile

[uedvt359]

@benoitc: I noticed you made a "prepare for release " commit a week ago. Will this PR be included in this upcoming release?

[benoitc]

@benoitc: I noticed you made a "prepare for release " commit a week ago. Will this PR be included in this upcoming release?

yes among other stuffs. I am side tracked by some PR reviews at the moment :)

[uedvt359]

Apologies for pestering, but is there an update to the situation?

[benoitc]

merged. I think we shoudl remove some options and just use the default context , will introduce it in a separate change. Thanks for the patch anyway and sorry for that delay to commit it.