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.

Sign up for GitHub

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

Jump to bottom

Mutual authentication Implementation #820

Merged
merged 12 commits into from
Mar 3, 2021
Merged

Mutual authentication Implementation #820

merged 12 commits into from
Mar 3, 2021

Conversation

ihsandemir
Copy link
Collaborator

@ihsandemir ihsandemir commented Feb 26, 2021 •
edited
Loading

Added mutual authentication capability to the C++ client.

Also a few fixes on the peer verification mode of the ssl socket.

For windows, we had to define an environment variable SSL_CERT_FILE=C:\cacert.pem and downloaded the file from https://curl.se/docs/caextract.html which is Mozilla CA store. This way OpenSSL library can find the intermediate CA authorities correctly and the test ssl_enabled_trust_default_certificates works as expected.

@ihsandemir
Added mutual authentication capability to the C++ client.
11c5dcc 
Also a few fixes on the peer verification mode of the ssl socket.
@ihsandemir
more tests are added.
120fc88
@ihsandemir ihsandemir added this to the 4.1 milestone Feb 26, 2021
@ihsandemir ihsandemir self-assigned this Feb 26, 2021
@devOpsHazelcast
Copy link
Contributor

Linux test PASSed.

@devOpsHazelcast
Copy link
Contributor

Windows test FAILed.

@ihsandemir ihsandemir marked this pull request as ready for review March 1, 2021 09:27
sancar
sancar previously approved these changes Mar 1, 2021
View reviewed changes
@devOpsHazelcast
Copy link
Contributor

Linux test PASSed.

1 similar comment
@devOpsHazelcast
Copy link
Contributor

Linux test PASSed.

@devOpsHazelcast
Copy link
Contributor

Windows test FAILed.

1 similar comment
@devOpsHazelcast
Copy link
Contributor

Windows test FAILed.

yemreinci
yemreinci reviewed Mar 2, 2021
View reviewed changes
scripts/start-rc.sh Outdated Show resolved Hide resolved
yemreinci
yemreinci suggested changes Mar 2, 2021
View reviewed changes
scripts/start-rc.sh Outdated Show resolved Hide resolved
scripts/start-rc.bat Outdated Show resolved Hide resolved
scripts/start-rc.bat Outdated Show resolved Hide resolved
hazelcast/include/hazelcast/client/internal/socket/SocketFactory.h Outdated Show resolved Hide resolved
@yemreinci
Copy link
Contributor

yemreinci commented Mar 2, 2021 •
edited
Loading

Doesn't compile without SSL:

$ cmake .. -DWITH_OPENSSL=0
$ make
...
/usr/local/include/boost/asio/ssl/detail/openssl_types.hpp:23:10: fatal error: 'openssl/conf.h' file not found
...

@ihsandemir
Copy link
Collaborator Author

verify-windows

@devOpsHazelcast
Copy link
Contributor

Windows test FAILed.

@ihsandemir
Copy link
Collaborator Author

verify-windows

@devOpsHazelcast
Copy link
Contributor

Windows test PASSed.

@ihsandemir
Copy link
Collaborator Author

Doesn't compile without SSL:

$ cmake .. -DWITH_OPENSSL=0
$ make
...
/usr/local/include/boost/asio/ssl/detail/openssl_types.hpp:23:10: fatal error: 'openssl/conf.h' file not found
...

Good catch. Changed and tested it.

@ihsandemir
Review comment fixes.
2355965 
- Make project compile withoput SSL.
- Obeyed the user provided context for peer verification, do not override it.
@ihsandemir @yemreinci
Apply suggestions from code review
23b2582 
Co-authored-by: yemreinci <18687880+yemreinci@users.noreply.github.com>
yemreinci
yemreinci reviewed Mar 2, 2021
View reviewed changes
examples/tls/BasicTLSClient.cpp Outdated
Comment on lines 26 to 28
boost::asio::ssl::context ctx(boost::asio::ssl::context::method::tlsv12_client);
ctx.set_default_verify_paths();
ctx.load_verify_file("/path/to/my/server/public/certificate");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also do ctx.set_verify_mode(ssl::verify_peer) to make sure the certificate is actually verified? What is the default behavior if you don't set the verify mode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

sancar
sancar previously approved these changes Mar 3, 2021
View reviewed changes
@ihsandemir
Review comment fix.
105b9b7 
Test failure fix.
@devOpsHazelcast
Copy link
Contributor

Linux test PASSed.

@devOpsHazelcast
Copy link
Contributor

Windows test PASSed.

@ihsandemir
Review comment fix. No need for `boost::asio::ssl::context::verify_fa…
d560e1c 
…il_if_no_peer_cert` mode option since it is ignored for client side.
sancar
sancar previously approved these changes Mar 3, 2021
View reviewed changes
@devOpsHazelcast
Copy link
Contributor

Linux test PASSed.

@devOpsHazelcast
Copy link
Contributor

Windows test PASSed.

yemreinci
yemreinci reviewed Mar 3, 2021
View reviewed changes
examples/tls/BasicTLSClient.cpp Outdated Show resolved Hide resolved
examples/tls/BasicTLSClient.cpp Outdated Show resolved Hide resolved
hazelcast/src/hazelcast/client/network.cpp Outdated Show resolved Hide resolved
hazelcast/test/src/HazelcastTests1.cpp Show resolved Hide resolved
hazelcast/test/src/HazelcastTests1.cpp Show resolved Hide resolved
hazelcast/test/src/HazelcastTests1.cpp Outdated Show resolved Hide resolved
@ihsandemir @yemreinci
Apply suggestions from code review
038deda 
Co-authored-by: yemreinci <18687880+yemreinci@users.noreply.github.com>
@sancar sancar removed their request for review March 3, 2021 13:22
@devOpsHazelcast
Copy link
Contributor

Linux test PASSed.

yemreinci
yemreinci previously approved these changes Mar 3, 2021
View reviewed changes
@devOpsHazelcast
Copy link
Contributor

Linux test PASSed.

@devOpsHazelcast
Copy link
Contributor

Windows test PASSed.

1 similar comment
@devOpsHazelcast
Copy link
Contributor

Windows test PASSed.

@ihsandemir ihsandemir requested a review from yemreinci March 3, 2021 15:54
@devOpsHazelcast
Copy link
Contributor

Linux test PASSed.

@devOpsHazelcast
Copy link
Contributor

Windows test PASSed.

@ihsandemir ihsandemir merged commit 283c21c into hazelcast:master Mar 3, 2021
@ihsandemir ihsandemir deleted the mutual_authentication branch March 3, 2021 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sancar sancar sancar left review comments

@yuce yuce Awaiting requested review from yuce

@mdumandag mdumandag Awaiting requested review from mdumandag

@yemreinci yemreinci Awaiting requested review from yemreinci

Assignees

@ihsandemir ihsandemir

Labels
Type: Enhancement
Projects
None yet
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ihsandemir @devOpsHazelcast @yemreinci @sancar @mdumandag