64bit ssl op #15230
64bit ssl op #15230
Conversation
Less tersely: converted SSL_get_options, SSL_set_options, SSL_CTX_get_options and SSL_CTX_get_options to take and return uint64_t since we were running out of 32 bits. Fixes: 15145
FdaSilvaYY's comments.
|
I'm in favor of 64-bit options field; I had tried it a while ago when one of my features would've pushed the limit. |
|
fixup commit pushed. |
t8m
added
the
approval: review pending
include/openssl/ssl.h.in
Outdated
| # define SSL_OP_PRIORITIZE_CHACHA 0x00200000U | ||
| # define SSL_OP_NO_SSL_MASK ( \ | ||
| SSL_OP_NO_SSLv3 | SSL_OP_NO_TLSv1 | SSL_OP_NO_TLSv1_1 \ | ||
| | SSL_OP_NO_TLSv1_2 | SSL_OP_NO_TLSv1_3 ) |
There was a problem hiding this comment.
Nit: I think this is more readable:
# define SSL_OP_NO_SSL_MASK \
( SSL_OP_NO_SSLv3 | SSL_OP_NO_TLSv1 | SSL_OP_NO_TLSv1_1 \
| SSL_OP_NO_TLSv1_2 | SSL_OP_NO_TLSv1_3 )There was a problem hiding this comment.
(to be applied generally, not just in this spot)
There was a problem hiding this comment.
I think this is more readable
Okay, fixed in all places.
|
Surely someone on the git project sees things like "fixup! fixup! fixup! fixup! fixup! Slightly reformat ssl.h.in " and wants to make "spam" and alias for "fixup" :) |
People need to learn how to use |
|
The |
I know. I was making a joke, a reference to the famous (https://www.dailymotion.com/video/x9fly1 etc) |
|
You have removed all the older OP which were defined as zero and were being left for compatibility reasons. |
|
|
||
| /* Removed from OpenSSL 1.1.0. Was 0x00000001L */ | ||
| /* Related to removed SSLv2. */ | ||
| # define SSL_OP_MICROSOFT_SESS_ID_BUG 0x0 |
There was a problem hiding this comment.
@t-j-h The old SSL_OPs are still here, but the comments surrounding them have been removed.
There was a problem hiding this comment.
I wouldn't remove the comments indicating they are obsolete ...
There was a problem hiding this comment.
There is a block comment before the obsolete ones that says "these are obsolete" Saying what they uses to do seems pointless since they no longer do it. :) Showing the old value is even more useless.
There was a problem hiding this comment.
The old sources won't disappear from Internet for the historians to dig it.
mattcaswell
added
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
|
Pushed! Thanks. |
Less tersely: converted SSL_get_options, SSL_set_options, SSL_CTX_get_options and SSL_CTX_get_options to take and return uint64_t since we were running out of 32 bits. Fixes: 15145 Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #15230)
Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #15230)
Less tersely: converted SSL_get_options, SSL_set_options, SSL_CTX_get_options and SSL_CTX_get_options to take and return uint64_t since we were running out of 32 bits. Fixes: 15145 Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#15230)
Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#15230)
The last commit is a code readability issue. It duplicates the cleanup in #15184 so either this or that (heh) could be merged first without causing major headaches. Expressing things in terms of bits is much more clear than comparing large hex numbers, especially 64-bit hex numbers :)
The OTC wanted this for beta as it's an API change.