Skip to content
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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

peterphonic
Copy link

IXWebSocket can now validate certificate with not only hostname, but with IP addresses as well. @bsergean

#else
#include <fnmatch.h>
#endif
#if OPENSSL_VERSION_NUMBER < 0x10100000L

Copy link
Collaborator

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.

Copy link
Author

@peterphonic peterphonic Feb 28, 2023

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;
}

Copy link
Collaborator

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 ?

Copy link
Author

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... ?

Copy link
Collaborator

@bsergean bsergean left a 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.

@peterphonic peterphonic marked this pull request as draft February 28, 2023 18:45
@bsergean
Copy link
Collaborator

bsergean commented Feb 28, 2023 via email

@@ -95,4 +95,19 @@ TEST_CASE("socket", "[socket]")
testSocket(host, port, request, socket, expectedStatus, timeoutSecs);
}
#endif

TEST_CASE("isValidIpAddress") {
Copy link
Author

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....

Copy link
Collaborator

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)

@bsergean
Copy link
Collaborator

All the PR checks have failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants