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 [email protected]

[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.

[tsaarni]

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

[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]

@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 :wink:.

[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 :heart: 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 https://github.com/benoitc/gunicorn/pull/2649#issuecomment-1265176331, 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.