Repository navigation
Conversation
Roman3349
changed the title
Roman3349
changed the title
Roman3349
force-pushed
the
fix/gh-25088-sni-ctx-inherit-policy
branch
from
October 31, 2026 21:08
20525c5 to
ddd1f9e
Compare
bukka
requested changes
Nov 11, 2026
bukka
left a comment
bukka
left a comment
Member
There was a problem hiding this comment.
I think it makes sense. It could break potentially someone but I think everyone would expect to have those option inherited so it seems more like a bug fix to me.
I got just NIT's
| --SKIPIF-- | ||
| <?php | ||
| if (!function_exists("proc_open")) die("skip no proc_open"); | ||
| if (!defined("OPENSSL_VERSION_NUMBER") || OPENSSL_VERSION_NUMBER < 0x10101000) die("skip OpenSSL 1.1.1+ required"); |
Member
There was a problem hiding this comment.
1.1.1 is minimum so no need to check this.
Comment on lines
+1460
to
+1463
| /* SSL_set_SSL_CTX() in the SNI callback switches the handshake to the per-host | ||
| * context, so the cipher list, ALPN selection callback, DH parameters and | ||
| * security level are taken from that context instead of the base one. Apply | ||
| * the base context policy to the per-host context. */ |
Member
There was a problem hiding this comment.
We don't use that long comment - reduce to one line or so.
Member
There was a problem hiding this comment.
well I prefer to usually reduce them but they sometime slip in - in this case it's too much explaining though.
|
|
||
| return SUCCESS; | ||
| } | ||
| /* }}} */ |
Member
There was a problem hiding this comment.
The {{{ and }}} are not used for new functions so please remove it.
The SNI callback switches the handshake to the per-host SSL_CTX, from which OpenSSL takes the cipher list, ALPN callback, DH params and security level. Apply the ciphers, alpn_protocols, dh_param and security_level options to the per-host contexts too. Fixes phpGH-25088 Signed-off-by: Roman Ondráček <mail@romanondracek.cz>
Signed-off-by: Roman Ondráček <mail@romanondracek.cz>
Roman3349
force-pushed
the
fix/gh-25088-sni-ctx-inherit-policy
branch
from
November 11, 2026 19:19
ddd1f9e to
e7c84f8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The SNI callback switches the handshake to the per-host SSL_CTX, from which OpenSSL takes the cipher list, ALPN callback, DH params and security level. Apply the ciphers, alpn_protocols, dh_param and security_level options to the per-host contexts too.
Fixes GH-25088