Hello,
I have created an RFC to add the function openssl_get_channel_binding: https://wiki.php.net/rfc/openssl_get_channel_binding
PR: https://github.com/php/php-src/pull/24035
The function exposes data related the an established TLS connection, making it possible for applications to build on top of the security provided by TLS. It is needed for SASL mechanisms such as SCRAM-SHA-256-PLUS.
Regards,
Sjoerd Langkemper
Hi
I have created an RFC to add the function openssl_get_channel_binding:
https://wiki.php.net/rfc/openssl_get_channel_bindingPR: https://github.com/php/php-src/pull/24035
The function exposes data related the an established TLS connection,
making it possible for applications to build on top of the security
provided by TLS. It is needed for SASL mechanisms such as
SCRAM-SHA-256-PLUS.
Thank you for the RFC. I have given it a read. First things first: Don't
forget to adjust the RFC Status to “Under Discussion” and to link the
discussion thread (https://news-web.php.net/php.internals/132941).
Content-wise:
- “The function returns null when the requested type is not applicable
to the connection.”
An operation that is not applicable sounds like an Exception to me. Is
there currently any other situation where null is returned?
Edit: I'm noting that this is already listed under “Rejected Features”.
I'm not sure I agree with the reasoning, though: Exceptions are the
standard way of indicating unsupported operations / error situations and
the Exception hierarchy is specifically designed to be caught.
- “$stream is a stream, but it has no active OpenSSL TLS transport”
That sounds like a programmer error, thus Openssl\OpensslError (or
potentially ValueError).
- “An OpenSSL call fails while computing the value”
Is this one reachable in practice or is this more of a generic “catch
all” error that cannot really happen?
- “Because the second parameter is an enum, the function itself no
longer has an unknown-type error. […]”
This paragraph should probably not be mentioned in the RFC since it
effectively describes a prior version of the PR / describes how enums
work.
- “The backing values preserve interoperability: $type->value is the
IANA name, so it can be forwarded to SASL or other protocol code
unchanged.”
This indeed seems to be a rare case of a backed enum being the right
choice.
- “Userland code that declares a function with the same name without a
function_exists()guard would fail with “Cannot redeclare function
openssl_get_channel_binding()”
As I had noted in https://news-web.php.net/php.internals/132938, a
function_exists() check is no guarantee that the code keeps working,
because the previous userland implementation might do something else.
That should be rephrased - but of course openssl_*() is reserved by
ext/openssl and defining new symbols is explicitly acceptable as per
policy.
- RFC Impact to “To Existing Extensions”
Are there perhaps any extensions that might want to make use of the new
functionality internally? For example pdo_pgsql, I'm noting that the RFC
mentions tests against PostgreSQL?
Best regards
Tim Düsterhus
Thank you for the thorough review.
Hi
I have created an RFC to add the function openssl_get_channel_binding:
https://wiki.php.net/rfc/openssl_get_channel_binding
- “The function returns null when the requested type is not applicable
to the connection.”An operation that is not applicable sounds like an Exception to me. Is
there currently any other situation wherenullis returned?
No. I can see the appeal of throwing a specific exception (e.g. UnsupportedChannelBinding extends OpensslExtension). Then if the function returns something the caller knows it is a valid channel binding value.
The RFC already identifies the case for tls-unique over TLS 1.3. I think using tls-server-end-point over TLS-PSK also may not work (because there's no certificate) and should also throw such an exception.
- “$stream is a stream, but it has no active OpenSSL TLS transport”
That sounds like a programmer error, thus
Openssl\OpensslError(or
potentially ValueError).
Yes, I agree.
- “An OpenSSL call fails while computing the value”
Is this one reachable in practice or is this more of a generic “catch
all” error that cannot really happen?
This was meant as a generic catch all for things that should not normally fail.
- “Because the second parameter is an enum, the function itself no
longer has an unknown-type error. […]”This paragraph should probably not be mentioned in the RFC
Yes, I agree. I removed this.
- RFC Impact to “To Existing Extensions”
Are there perhaps any extensions that might want to make use of the new
functionality internally? For example pdo_pgsql, I'm noting that the RFC
mentions tests against PostgreSQL?
I hadn't considered this. Not really. Most extensions don't use a connection managed by ext/openssl. E.g. for postgres, libpq does the TLS handling and also the channel binding. Mysqlnd does use a PHP stream, but MySQL/mariadb don't have any channel binding functionality.
Regards,
Sjoerd Langkemper
Thank you for the thorough review.
An operation that is not applicable sounds like an Exception to me.
I can see the appeal of throwing a specific exception (e.g. UnsupportedChannelBinding extends OpensslExtension). Then if the function returns something the caller knows it is a valid channel binding value.
I changed the RFC to reflect this. openssl_get_channel_binding can no longer return null, but it can throw a (newly introduced) UnsupportedChannelBindingException.
Regards,
Sjoerd
Hi
I changed the RFC to reflect this. openssl_get_channel_binding can no
longer return null, but it can throw a (newly introduced)
UnsupportedChannelBindingException.
Thank you. I've given the RFC another read and have some more comments:
-
The naming of the
$channel_binding_typeparameter. It seems there
is no current policy and using underscores is consistent with
ext/openssl overall, but from what I see, new parameters are typically
using camelCase. No strong opinion either way from my side, but
potentially worth a discussion. -
“Introducing a new Error hierarchy for ext/openssl is a larger API
decision that would affect more than this function, and none of the
existing ext/openssl APIs currently use an Error subclass”
Not really. The “look and feel” of Openssl\OpensslError is already
required by policy, so there's no API design to be resolved here. I
don't disagree with the choice of ValueError (and even suggested it as
one of two alternatives), but that argument in the RFC stood out and
should probably be rephrased or dropped.
- Overall and tying a little into (2), I feel that the RFC
“overjustifies” some of its choices. This is also visible in the “why an
enum” paragraph where the RFC goes great lengths to explain why a union
type is not chosen and how there is prior art - when an enum is just
obvious and consistent with what we recently did. To me this made the
RFC a bit tedious to read and harder to extract the parts where an
actual design decision happened. Similarly the “Mapping a configured
name to the enum” example just shows how enums work (which can be
assumed to be general knowledge at this point). Not a blocker of course,
but potentially something to keep in mind for future RFCs.
Best regards
Tim Düsterhus