-
Notifications
You must be signed in to change notification settings - Fork 178
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
Check ip adress #445
base: master
Are you sure you want to change the base?
Check ip adress #445
Conversation
#else | ||
#include <fnmatch.h> | ||
#endif | ||
#if OPENSSL_VERSION_NUMBER < 0x10100000L | ||
|
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.
I'm pretty positive that old openssl installation will not have this x509v3.h header, so we should keep this OPENSSL_VERSION_NUMBER check or we will lose compatibility with old openssl.
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.
@bsergean , I am just a little bit confused here.
Before my change, the code was like this :
#if OPENSSL_VERSION_NUMBER < 0x10100000L
#include "openssl/x509v3.h"
#endif
The way I read this is we do include x509v3.h
for version below 0x10100000L . Version higher doesn't include x509v3.h
With my change, I need x509v3.h
So, would that make sense to do this instead
#if OPENSSL_VERSION_NUMBER < 0x10100000L || OPENSSL_VERSION_NUMBER >= 0x30000000L
In this case, we keep the actual functionality and x509v3.h
is added only on version higher or equal to 3.0.0
@@ -77,6 +83,43 @@ namespace | |||
|
|||
return true; | |||
} | |||
|
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.
Can you add unittest for this new function ?
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.
I guess so, where should I add this? Is is the reason I got message that test failed under linux, linux_asan, and mac_tsan... ?
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.
Thanks for contributing. See inline comments.
There is a tests subfolder at the top, you can create a new file in there or add to an existing file.
… On Feb 28, 2023, at 10:45 AM, Pierre-Luc Boily ***@***.***> wrote:
@peterphonic commented on this pull request.
In ixwebsocket/IXSocketOpenSSL.cpp <#445 (comment)>:
> @@ -77,6 +83,43 @@ namespace
return true;
}
+
I guess so, where should I add this? Is is the reason I got message that test failed under linux, linux_asan, and mac_tsan... ?
—
Reply to this email directly, view it on GitHub <#445 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC2O6UIQP5G23UEBBE2KUC3WZZBT7ANCNFSM6AAAAAAVLBWC6Q>.
You are receiving this because you were mentioned.
|
@@ -95,4 +95,19 @@ TEST_CASE("socket", "[socket]") | |||
testSocket(host, port, request, socket, expectedStatus, timeoutSecs); | |||
} | |||
#endif | |||
|
|||
TEST_CASE("isValidIpAddress") { |
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.
I don't know if this works, I am not able to compile the test folder under windows. I also tried under Centos7 with build_linux.sh, but some files are missing. So, I don't know if I am supposed to build....
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.
Try to read the top level makefile.dev / enabling the test is done through a cmake option.
(this is for running things locally/manually on your end)
All the PR checks have failed. |
IXWebSocket can now validate certificate with not only hostname, but with IP addresses as well. @bsergean