TLSv1.3: Rename the elliptic_curves extension to supported_groups#1825
TLSv1.3: Rename the elliptic_curves extension to supported_groups#1825mattcaswell wants to merge 1 commit intoopenssl:masterfrom
Conversation
mattcaswell
changed the title
mattcaswell
force-pushed
the
tls1_3-supported_groups
branch
from
249722b to
e6df9a0
Compare
|
I rebased this onto latest master. This one should be straight forward to review. |
richsalz
left a comment
richsalz
left a comment
There was a problem hiding this comment.
You also need backward-compat for the old API. And a HISTORY section in the pod page talking about the rename.
test/ssl-tests/14-groups.conf
Outdated
There was a problem hiding this comment.
I also think this is wrong.
There was a problem hiding this comment.
Make Curves be a synonym for Groups in the test harness.
test/ssl-tests/14-groups.conf
Outdated
There was a problem hiding this comment.
I am not sure if renaming the tests and the test file is a good idea. These are testing curves. (Which are now, I guess, a subset of what a group is.) I would prefer it as-is.
test/ssl-tests/14-groups.conf.in
Outdated
There was a problem hiding this comment.
I also think this renaming is wrong. It's a CURVE, even though the API handles curves and groups.
This is a skin deep change, which simply renames most places where we talk about curves in a TLS context to groups. This is because TLS1.3 has renamed the extension, and it can now include DH groups too. We still only support curves, but this rename should pave the way for a future extension for DH groups.
mattcaswell
force-pushed
the
tls1_3-supported_groups
branch
from
e6df9a0 to
7336f93
Compare
There was already backward-compat macros?
And the HISTORY section was already updated? I backed out all test related changes from this PR. Updated commit pushed. |
richsalz
left a comment
richsalz
left a comment
There was a problem hiding this comment.
don't know how i missed the history/compat stuff. should not have been doing code-reviews last night while watching the results i guess.
| { | ||
| return cmd_Groups(cctx, value); | ||
| } | ||
|
|
There was a problem hiding this comment.
I want "curves" to be usable in test scripts. And I see your suggestion in ssl/ssl_conf.c is a good way to do that.
| @@ -543,6 +549,7 @@ static const ssl_conf_cmd_tbl ssl_conf_cmds[] = { | |||
| SSL_CONF_CMD_STRING(SignatureAlgorithms, "sigalgs", 0), | |||
| SSL_CONF_CMD_STRING(ClientSignatureAlgorithms, "client_sigalgs", 0), | |||
| SSL_CONF_CMD_STRING(Curves, "curves", 0), | |||
There was a problem hiding this comment.
Change it to SSL_CONF_CMD_STRING(Groups, "curves", 0), rather than defining another function that just passes arguments...
There was a problem hiding this comment.
Nice idea, but it doesn't work because of this:
typedef struct {
int (*cmd) (SSL_CONF_CTX *cctx, const char *value);
const char *str_file;
const char *str_cmdline;
unsigned short flags;
unsigned short value_type;
} ssl_conf_cmd_tbl;
/* Table of supported parameters */
#define SSL_CONF_CMD(name, cmdopt, flags, type) \
{cmd_##name, #name, cmdopt, flags, type}
You will note that the first argument to SSL_CONF_CMD_STRING is used to build both the name of the function, and the command string to use in configuration files. The second argument is just for command line args.
There was a problem hiding this comment.
Oh... argh
Ok then, never mind that
|
Yes |
|
Merged. Thanks. |
RFC 4492 introduced the Supported Elliptic Curves Extension, but this extension was renamed Supported Groups in RFC 7919. Following RFC 7919 (and TLSv1.3), `parse_tls_serverhello()` refers to this extension as "supported groups/#10". Since, at the moment, OpenSSL's s_client refers to this extension as "elliptic curves/#10", the extension sometimes appears twice in the "TLS extensions" line, if it is detected by both OpenSSL (in `get_server_certificate()`) and `tls_sockets()` (in `determine_tls_extensions()`): ``` TLS extensions (standard) "renegotiation info/#65281" "elliptic curves/#10" "EC point formats/#11" "supported groups/#10" ``` This PR fixes the problem of the extension appearing twice in the "TLS extensions" line by replacing any instances of "elliptic curves/#10" with "supported_groups/#10" in the `$tls_extensions` line extracted from `$OPENSSL s_client`. This PR also changes "supported groups/#10" to "supported_groups/#10" in `parse_tls_serverhello()`, since the current development branch of OpenSSL uses "supported_groups" to refer to this extension (see openssl/openssl#1825).
3 participants
Checklist
Description of change
This is a skin deep change, which simply renames most places where we talk about curves in a TLS context to groups. This is because TLS1.3 has renamed the extension, and it can now include DH groups too. We still only support curves, but this rename should pave the way for a future inclusion of DH groups.