Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSL_set_SSL_CTX is a bad idea #6109

Open
mattcaswell opened this issue Apr 27, 2018 · 7 comments
Open

SSL_set_SSL_CTX is a bad idea #6109

mattcaswell opened this issue Apr 27, 2018 · 7 comments
Labels
triaged: documentation The issue/pr deals with documentation (errors) triaged: feature The issue/pr requests/adds a feature
Milestone

Comments

@mattcaswell
Copy link
Member

The whole concept behind SSL_set_SSL_CTX() is just a bad idea. I think we should deprecate it and replace it with an alternative API.

In #1652 @vdukhovni said:

As soon as possible it would be good to have a sensible replacement for this in support for alternative chains for SNI. It should not be necessary to have a whole new context just to vend a different certificate chain. A much lighter-weight mechanism should I think be available.

If users want to do more, and also customize groups,..., perhaps use a separate session cache based on SNI, then a per-SNI context comes into play, but if so, it should likely be more fully-featured. This might mean delaying initialization of settings in the SSL handle until they are needed, and then picking them up from the extant context (if not explicitly set in the SSL handle itself).

And @t-j-h said:

Changing the SSL_CTX after a certain point shouldn't be possible - in fact the whole concept of altering your "factory" really is problematic - and this API was added explicitly to "support extensions" in commit ed3883d - there should be a different way to do this - and I would be in favour of removing this API at the earliest opportunity.

This can't be done in the 1.1.1 timeframe, so setting this with a post-1.1.1 milestone.

@mattcaswell mattcaswell added this to the Post 1.1.1 milestone Apr 27, 2018
@richsalz richsalz added after 1.1.1 and removed after 1.1.1 branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch labels May 6, 2018
@triska
Copy link
Contributor

triska commented Jun 2, 2018

From the perspective of a library user, I have two comments about SSL_set_SSL_CTX:

First, I am using this function in the SWI-Prolog OpenSSL bindings, to implement server-side SNI. In Prolog, SNI works via a user-extensible predicate that yields an SSL context depending on the hostname. For example, such a user-defined predicate may look as follows:

sni(C0, Hostname, C) :- ...

where C0 is the default SSL context, and the predicate uses the given Hostname to compute a new context C which is then dynamically plugged in via SSL_set_SSL_CTX to further negotiate the connection.

For the particular use case of Prolog-based OpenSSL bindings, I consider it essential that no destructive modifications be required to carry out all desired operations. Currently, this is the case! For example, to implement server-side SNI, I can currently create a new SSL context and plug it in via SSL_set_SSL_CTX, without having to modify any existing context. This property ensures thread-safety of the whole library, and I would greatly appreciate if this consideration could be taken into account when devising a new API. This issue is hence related to #2165.

Second, I agree that the way SSL_set_SSL_CTX currently works can yield counter-intuitive results and is thus not ideal. For example, to mention just one particular case I noticed: This API cannot be used to negotiate different minimal or maximal protocol versions depending on the hostname. This may be due to limitations of the protocol itself, or also due to incomplete exchange of context properties as mentioned in #1652 (comment).

@kaduk
Copy link
Contributor

kaduk commented Jun 2, 2018

Second, I agree that the way SSL_set_SSL_CTX currently works can yield counter-intuitive results and is thus not ideal. For example, to mention just one particular case I noticed: This API cannot be used to negotiate different minimal or maximal protocol versions depending on the hostname. This may be due to limitations of the protocol itself, or also due to incomplete exchange of context properties as mentioned in #1652 (comment).

I think this is more because up until recently, there was not an opportunity for user code (e.g., servername callback) to run before version negotiation had occurred. Starting with 1.1.1 there is a "client hello callback" that runs extremely early in the handshake processing, and is able to alter the supported versions based on the hostname.

@james-callahan
Copy link
Contributor

I found myself using SSL_set_SSL_CTX today from the ALPN callback (to pick a different cert and various parameters based on selected ALPN).
The fact that it doesn't copy all parameters caused a bit of confusion.

@vdukhovni
Copy link

Some time has passed, and we're still in the same boat. SSL_set_SSL_CTX remains undocumented (except in passing).
It is I think important (since it has not been replaced in 3.0.0 as yet, and likely insufficient time remains to do so) to at least partly document it.

  • Which context is used for session management, ticket callbacks, session modes, ...
  • Which context is used for min/max protocol version bounds and the option bits?
  • Which context is used for the candidate cipher lists, the supported curves, ...?
  • Of course the SNI context is used for selecting the server certificate...
    ...
    In other words which settings does the new context override, and which continue to be used from the initial context?

@vdukhovni vdukhovni added the issue: documentation The issue reports errors in (or missing) documentation label Jul 27, 2020
@kroeckx kroeckx modified the milestones: Post 1.1.1, Post 3.0.0 Oct 4, 2020
@kroeckx kroeckx added the triaged: feature The issue/pr requests/adds a feature label Oct 4, 2020
@t8m t8m added triaged: documentation The issue/pr deals with documentation (errors) and removed issue: documentation The issue reports errors in (or missing) documentation labels Oct 21, 2021
@bach001
Copy link

bach001 commented Dec 29, 2021

I came across a situation that only this function came to my rescue: based on the customized messages communicated with the client side, the server side dynamically switching to using a different context using a different certificate.

@nhorman
Copy link
Contributor

nhorman commented Jun 17, 2024

@mattcaswell consider for 4.0?

@mattcaswell
Copy link
Member Author

I don't think this necessarily needs to wait for a major release. Basically we need to design and implement a new solution and deprecate the old function. All of that could be done in a minor release. The only thing we couldn't do is actually remove the old SSL_set_SSL_CTX function.

So some future minor release would be ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triaged: documentation The issue/pr deals with documentation (errors) triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

No branches or pull requests