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

Development/websocket #1794

Closed
wants to merge 14 commits into from
Closed

Development/websocket #1794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6f4705c
[WebSocket/WebSocketLink] : Enable unsollicited 'Pong' and rename enu…
msieben Oct 25, 2024
aa8c25d
[WebSocket/WebSocketLink] : METROL-1087
msieben Nov 6, 2024
e923fcf
[WebSocket/WebSocketLink] : rename forgotten 'WEBSERVER' to 'WEBSERVICE'
msieben Nov 7, 2024
43c3174
Merge branch 'master' into development/websocket
pwielders Nov 7, 2024
4769766
[Tests/unit/core] : add 'test_websocket'
msieben Nov 7, 2024
45bfec5
[Tests/unit/core] : Add missing VERBOSE preprocessor directive to 'te…
msieben Nov 21, 2024
a206e19
[Tests/unit/core] : Add 'SecureSocketPort' tests to 'test_websocket'
msieben Nov 21, 2024
4261616
[Source/crypto/SecureSocketPort] : Initialize when context and SSL st…
msieben Nov 21, 2024
4ddd69f
[Source/crypto/SecureSocketPort] : (Re)initialize if socket has been …
msieben Nov 21, 2024
7916e61
[Source/crypto/SecureSocketPort] : Context settings do not (necessari…
msieben Nov 21, 2024
48b267b
[Source/crypto/SecureSocketPort] : check return value for setting hos…
msieben Nov 21, 2024
62f635d
[Source/crypto/SecureSocket] : improve return value checking in 'Hand…
msieben Nov 21, 2024
feef857
[Source/crypto/SecureSocket] : correct check for invalid context
msieben Nov 21, 2024
fe2b0a5
Merge branch 'master' into development/websocket
msieben Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 37 additions & 30 deletions Source/cryptalgo/SecureSocketPort.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,36 +131,46 @@ SecureSocketPort::Handler::~Handler() {
}

uint32_t SecureSocketPort::Handler::Initialize() {
uint32_t success = Core::ERROR_NONE;

_context = SSL_CTX_new(TLS_method());

_ssl = SSL_new(static_cast<SSL_CTX*>(_context));
SSL_set_fd(static_cast<SSL*>(_ssl), static_cast<Core::IResource&>(*this).Descriptor());
SSL_CTX_set_options(static_cast<SSL_CTX*>(_context), SSL_OP_ALL | SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);

// Trust the same certificates as any other application
if (SSL_CTX_set_default_verify_paths(static_cast<SSL_CTX*>(_context)) == 1) {
success = Core::SocketPort::Initialize();
} else {
TRACE_L1("OpenSSL failed to load certificate store");
success = Core::ERROR_GENERAL;
uint32_t success = Core::ERROR_GENERAL;

if ((_context = SSL_CTX_new(TLS_method())) != nullptr) {
if ( // Returns bit-mask after adding options
((SSL_CTX_set_options(static_cast<SSL_CTX*>(_context), SSL_OP_ALL | SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3) & (SSL_OP_ALL | SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3)) == (SSL_OP_ALL | SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3))
Copy link
Contributor

Choose a reason for hiding this comment

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

The SSL_CTX_set_options contruction feels a bit like you are checking the openSSL implementation if it does what you are asking it to do. The manual does not give any restrictions on mutal exclusive options. So since we do not actively want to check third party libraries in Production or Release I suggest putting this check in an ASSERT if you want to check OpenSSL

&& // Trust the same certificates as any other application
(SSL_CTX_set_default_verify_paths(static_cast<SSL_CTX*>(_context)) == 1)
&& ((_ssl = SSL_new(static_cast<SSL_CTX*>(_context))) != nullptr)
&& (SSL_set_fd(static_cast<SSL*>(_ssl), static_cast<Core::IResource&>(*this).Descriptor()) == 1)
) {
success = Core::SocketPort::Initialize();
} else {
TRACE_L1("OpenSSL failed to set protocol level or load certificate store");
success = Core::ERROR_GENERAL;
}
}

return success;
}

int32_t SecureSocketPort::Handler::Read(uint8_t buffer[], const uint16_t length) const {
int32_t result = SSL_read(static_cast<SSL*>(_ssl), buffer, length);
int32_t result = 0;

if (_handShaking != CONNECTED) {
const_cast<Handler&>(*this).Update();
if (_handShaking == CONNECTED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be interesting to test if an ASSERT(handshaking == CONNECTED); fires. If so I guess the Update is usefull for the system to update its information if it was not CONNECTED (something that now is turned off).
If the ASSERT does not fire, lets clean up and do not call Update() and make the code more lean and mean!

int value = SSL_read(static_cast<SSL*>(_ssl), buffer, length);
result = (value > 0 ? value : 0);
}

return (result);
}

int32_t SecureSocketPort::Handler::Write(const uint8_t buffer[], const uint16_t length) {
return (SSL_write(static_cast<SSL*>(_ssl), buffer, length));
int32_t result = 0;

if (_handShaking == CONNECTED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This was/is a design flaw, if someone would write already while there is no "OPEN"/"CONNECTED state, it might end up in a busy-loop. Suggest an ASSERT( _handShaking == CONNECTED); Programitically the user of the socket has to wait till the StateChange -> Open has ocured before the Write can be used!!

int value = (SSL_write(static_cast<SSL*>(_ssl), buffer, length));
result = (value > 0 ? value : 0);
}

return (result);
}


Expand Down Expand Up @@ -212,29 +222,26 @@ void SecureSocketPort::Handler::ValidateHandShake() {
}

void SecureSocketPort::Handler::Update() {
if (IsOpen() == true) {
int result;

if (IsOpen() == true && _ssl != nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

in the SecureSocketPort::Handler::Initialize() an error is returned if _ssl was not constructed. Please code defensively, just ASSERT here on an (_ssl != nullptr);

if (_handShaking == IDLE) {
SSL_set_tlsext_host_name(static_cast<SSL*>(_ssl), RemoteNode().HostName().c_str());
result = SSL_connect(static_cast<SSL*>(_ssl));
if (result == 1) {
int result{0};
Copy link
Contributor

Choose a reason for hiding this comment

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

Stick to coding as done every where int result = 0;


if( (result = SSL_set_tlsext_host_name(static_cast<SSL*>(_ssl), RemoteNode().HostName().c_str()) == 1)
&& (result = SSL_connect(static_cast<SSL*>(_ssl)) == 1)
) {
ValidateHandShake();
}
else {
} else { // Support non-blocking SocketPorts, result taken from SSL_connect
result = SSL_get_error(static_cast<SSL*>(_ssl), result);
if ((result == SSL_ERROR_WANT_READ) || (result == SSL_ERROR_WANT_WRITE)) {
_handShaking = EXCHANGE;
}
}
}
else if (_handShaking == EXCHANGE) {
} else if (_handShaking == EXCHANGE) {
if (SSL_do_handshake(static_cast<SSL*>(_ssl)) == 1) {
ValidateHandShake();
}
}
}
else if (_ssl != nullptr) {
} else if (_ssl != nullptr) {
_handShaking = IDLE;
_parent.StateChange();
}
Expand Down
10 changes: 7 additions & 3 deletions Source/cryptalgo/SecureSocketPort.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,14 @@ namespace Crypto {

// Signal a state change, Opened, Closed or Accepted
void StateChange() override {
if (IsOpen() && _context == nullptr) { // Initialize may not yet have happened
Copy link
Contributor

Choose a reason for hiding this comment

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

By design the ASSERT(_context != nullptr) and lets add ASSERT(_ssl != nullptr) could be added here, cause if either of these are a nullptr, the Initialize should have signaled that by sending out an error != CORE_ERROR_NONE and than the statechange will never be called!
So I consider this unusefull additional code and think that it is a design flaw if a socket could be "open" but it was not Initialized(), see:

Thunder/Source/core/SocketPort.cpp

Line 516 in fe2b0a5

if ((m_Socket != INVALID_SOCKET) && (Initialize() == Core::ERROR_NONE)) {

Initialize();
}

ASSERT(_context != nullptr);
Update();
};
if (_ssl != nullptr) {
Update();
}
};
inline uint32_t Callback(IValidator* callback) {
uint32_t result = Core::ERROR_ILLEGAL_STATE;

Expand Down
Loading
Loading