-
Notifications
You must be signed in to change notification settings - Fork 847
Add configs for groups list #4040
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
Conversation
iocore/net/SSLUtils.cc
Outdated
| } | ||
| } | ||
|
|
||
| #if (OPENSSL_VERSION_NUMBER >= 0x10101000L) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any other ifdef or detection we can use? Or do we know that this will be for OpenSSL only, and never BroimgSSL etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another choice is adding macros to detect this API in /build/crypto.m4. Do you prefer that?
BoringSSL has this macro and it indicates API compatibility.
BoringSSL's OPENSSL_VERSION_NUMBER matches the OpenSSL version it targets. Version checks for OpenSSL should ideally work as-is in BoringSSL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ifdef SSL_CTX_set1_groups_list ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're willing to do a bit of meta programming, I have a header file with which you could do this
namespace
{
template <typename T>
auto
TS_SSL_CTX_set1_groups_list(T *, char *, ts::meta::CaseArg_0) -> bool
{
return true;
}
template <typename T>
auto
TS_SSL_CTX_set1_groups_list(T *ctx, char *groups, ts::meta::CaseArg_1)
-> decltype(SSL_CTX_set1_groups_list(static_cast<T *>(nullptr), static_cast<char*>(nullptr)), bool())
{
if (!SSL_CTX_set1_groups_list(ctx, groups)) {
SSLError("invalid groups list for client in records.config");
return false;
}
return true;
}
} // namespace
bool
TS_SSL_CTX_set1_groups_list(SSL_CTX *ctx, char *params)
{
return TS_SSL_CTX_set1_groups_list(ctx, params, ts::meta::CaseArg);
}
At the original call site, it's
if (params->client_groups_list != nullptr) {
if (!TS_SSL_CTX_set1_groups_list(client_ctx, params->client_groups_list)) {
SSLError("invalid groups list for client in records.config");
goto fail;
}
}
This doesn't depend on any #define or checking in configure.ac, it's completely self contained. It doesn't depend on knowing the openSSL version, and should work on BoringSSL. In fact, if BoringSSL has a functionally equivalent but differently named function, that could be easily incorporated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! Could you open a new PR to add the magic header file? It looks we should discuss ifdef v.s. meta programing for checking existence of libraries APIs. Because we have bunch of checks like this.
|
Pushed new commit using |
e6346b0 to
3d17822
Compare
mgmt/RecordsConfig.cc
Outdated
| , | ||
| {RECT_CONFIG, "proxy.config.ssl.server.groups_list", RECD_STRING, nullptr, RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL} | ||
| , | ||
| {RECT_CONFIG, "proxy.config.ssl.client.groups_lsit", RECD_STRING, nullptr, RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a typo on this line "lsit"
| of TLSv1.3 connections. The value is a colon separated list of | ||
| group NIDs or names, for example "P-521:P-384:P-256". | ||
|
|
||
| This configuration works with OpenSSL v1.1.1 and above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a link to the openssl documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean man page of SSL_CTX_set1_groups_list ?
https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set1_groups_list.html
or more general description?
https://wiki.openssl.org/index.php/TLS1.3#Groups
|
Fixed typo and added links of OpenSSL. |
Fix #3604. This needs #4039 to work correctly.
Add new configs for supported groups