Skip to content

Address Copilot review follow-ups for runtime trace and SSL handling #87

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

Merged
matt2005 merged 1 commit into from
Mar 15, 2026
Merged

Address Copilot review follow-ups for runtime trace and SSL handling #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 11 additions & 5 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,12 +344,18 @@ install(FILES "${CMAKE_CURRENT_BINARY_DIR}/include/aasdk/Version.hpp"
)

# Install SSL certificate and key files
install(FILES
"${CMAKE_CURRENT_SOURCE_DIR}/cert/headunit.crt"
"${CMAKE_CURRENT_SOURCE_DIR}/cert/headunit.key"
install(FILES
"${CMAKE_CURRENT_SOURCE_DIR}/cert/headunit.crt"
DESTINATION /etc/aasdk
PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ
COMPONENT runtime
PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
COMPONENT runtime
)

install(FILES
"${CMAKE_CURRENT_SOURCE_DIR}/cert/headunit.key"
DESTINATION /etc/aasdk
PERMISSIONS OWNER_READ OWNER_WRITE
COMPONENT runtime
)

# Export the targets to a script
Expand Down
18 changes: 12 additions & 6 deletions debian/postinst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,26 @@ case "$1" in
cp -f "$legacy_dir/headunit.key" "$key_file"
fi

# Use pi group when available so non-root Crankshaft/OpenAuto runtimes can read certs.
# Prefer a dedicated service group when present.
cert_group="root"
if getent group pi >/dev/null 2>&1; then
cert_group="pi"
if getent group aasdk >/dev/null 2>&1; then
cert_group="aasdk"
fi

if [ -f "$cert_file" ]; then
chown root:"$cert_group" "$cert_file" || true
chmod 640 "$cert_file" || true
chmod 644 "$cert_file" || true
fi

if [ -f "$key_file" ]; then
chown root:"$cert_group" "$key_file" || true
chmod 640 "$key_file" || true
chown root:root "$key_file" || true
chmod 600 "$key_file" || true

# Optional compatibility mode for non-root runtimes in the aasdk group.
if [ "$cert_group" = "aasdk" ]; then
chown root:aasdk "$key_file" || true
chmod 640 "$key_file" || true
fi
fi

# Update the dynamic linker cache
Expand Down
23 changes: 13 additions & 10 deletions src/Messenger/MessageInStream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@
#include <aasdk/Error/Error.hpp>
#include <aasdk/Common/Log.hpp>
#include <aasdk/Common/ModernLogger.hpp>
#include <aap_protobuf/service/control/ControlMessageType.pb.h>
#include <algorithm>
#include <atomic>
#include <chrono>
#include <cstdlib>
#include <iostream>
Expand Down Expand Up @@ -92,7 +94,7 @@ namespace aasdk::messenger {
}

static auto shouldTraceMessage(ChannelId channelId) -> bool {
static size_t counter = 0;
static std::atomic<uint64_t> counter{0};
const MessageTraceConfig cfg = getMessageTraceConfig();
if (!cfg.enabled) {
return false;
Expand All @@ -102,8 +104,8 @@ namespace aasdk::messenger {
return false;
}

++counter;
return (counter % static_cast<size_t>(cfg.sampleEvery)) == 0;
const uint64_t current = ++counter;
return (current % static_cast<uint64_t>(cfg.sampleEvery)) == 0;
}

} // namespace
Expand Down Expand Up @@ -142,7 +144,7 @@ namespace aasdk::messenger {
void MessageInStream::receiveFrameHeaderHandler(const common::DataConstBuffer &buffer) {
FrameHeader frameHeader(buffer);

AASDK_LOG(info) << "[MessageInStream] Processing Frame Header: Ch "
AASDK_LOG(debug) << "[MessageInStream] Processing Frame Header: Ch "
<< channelIdToString(frameHeader.getChannelId()) << " Fr "
<< frameTypeToString(frameHeader.getType())
<< " Enc " << (frameHeader.getEncryptionType() == EncryptionType::ENCRYPTED ? "ENCRYPTED" : "PLAIN")
Expand Down Expand Up @@ -213,7 +215,7 @@ namespace aasdk::messenger {

FrameSize frameSize(buffer);
frameSize_ = (int) frameSize.getFrameSize();
AASDK_LOG(info) << "[MessageInStream] Frame size parsed: frameSize=" << frameSize.getFrameSize()
AASDK_LOG(debug) << "[MessageInStream] Frame size parsed: frameSize=" << frameSize.getFrameSize()
<< " totalSize=" << frameSize.getTotalSize();
transport_->receive(frameSize.getFrameSize(), std::move(transportPromise));
}
Expand All @@ -223,7 +225,7 @@ namespace aasdk::messenger {
const bool traceMessage = shouldTraceMessage(channelId);
const size_t payloadSizeBefore = message_->getPayload().size();

AASDK_LOG(info) << "[MessageInStream] Payload handler: ch=" << channelIdToString(message_->getChannelId())
AASDK_LOG(debug) << "[MessageInStream] Payload handler: ch=" << channelIdToString(message_->getChannelId())
<< " enc=" << (message_->getEncryptionType() == EncryptionType::ENCRYPTED ? "ENCRYPTED" : "PLAIN")
<< " msg=" << (message_->getType() == MessageType::CONTROL ? "CONTROL" : "SPECIFIC")
<< " frameType=" << frameTypeToString(thisFrameType_)
Expand All @@ -242,12 +244,13 @@ namespace aasdk::messenger {
(buffer.cdata[1] == 0x03);

if (message_->getChannelId() == ChannelId::CONTROL && looksLikeTlsRecord) {
message_->insertPayload(messenger::MessageId(3).getData());
message_->insertPayload(messenger::MessageId(
aap_protobuf::service::control::message::ControlMessageType::MESSAGE_ENCAPSULATED_SSL).getData());
}

message_->insertPayload(buffer);
if (traceMessage) {
AASDK_LOG(info) << "[MessageTrace] encrypted-pass-through"
AASDK_LOG(debug) << "[MessageTrace] encrypted-pass-through"
<< " ch=" << channelIdToString(channelId)
<< " payloadBytes=" << buffer.size
<< " payloadSizeAfter=" << message_->getPayload().size();
Expand All @@ -256,7 +259,7 @@ namespace aasdk::messenger {
try {
const size_t decryptedBytes = cryptor_->decrypt(message_->getPayload(), buffer, frameSize_);
if (traceMessage) {
AASDK_LOG(info) << "[MessageTrace] decrypt"
AASDK_LOG(debug) << "[MessageTrace] decrypt"
<< " ch=" << channelIdToString(channelId)
<< " frameSize=" << frameSize_
<< " encryptedBytes=" << buffer.size
Expand All @@ -283,7 +286,7 @@ namespace aasdk::messenger {
if ((thisFrameType_ == FrameType::BULK || thisFrameType_ == FrameType::LAST) && isValidFrame_) {
AASDK_LOG_MESSENGER(debug, "Resolving message.");
if (traceMessage) {
AASDK_LOG(info) << "[MessageTrace] resolve"
AASDK_LOG(debug) << "[MessageTrace] resolve"
<< " ch=" << channelIdToString(channelId)
<< " frameType=" << frameTypeToString(thisFrameType_)
<< " totalPayloadBytes=" << message_->getPayload().size();
Expand Down
37 changes: 27 additions & 10 deletions src/Transport/SSLWrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,17 +220,34 @@ namespace aasdk {
int SSLWrapper::getError(SSL *ssl, int returnCode) {
const int sslErrorCode = SSL_get_error(ssl, returnCode);
const int savedErrno = errno;
const bool fatalError =
sslErrorCode != SSL_ERROR_NONE &&
sslErrorCode != SSL_ERROR_WANT_READ &&
sslErrorCode != SSL_ERROR_WANT_WRITE &&
sslErrorCode != SSL_ERROR_WANT_X509_LOOKUP;

if (fatalError) {
AASDK_LOG(error) << "[SSLWrapper] getError returnCode=" << returnCode
<< " ssl_error=" << sslErrorCode
<< "(" << sslErrorToString(sslErrorCode) << ")"
<< " errno=" << savedErrno
<< "(" << std::strerror(savedErrno) << ")"
<< " state="
<< (ssl ? SSL_state_string_long(ssl) : "<null-ssl>");
} else {
AASDK_LOG(debug) << "[SSLWrapper] getError returnCode=" << returnCode
<< " ssl_error=" << sslErrorCode
<< "(" << sslErrorToString(sslErrorCode) << ")"
<< " errno=" << savedErrno
<< "(" << std::strerror(savedErrno) << ")"
<< " state="
<< (ssl ? SSL_state_string_long(ssl) : "<null-ssl>");
}

AASDK_LOG(error) << "[SSLWrapper] getError returnCode=" << returnCode
<< " ssl_error=" << sslErrorCode
<< "(" << sslErrorToString(sslErrorCode) << ")"
<< " errno=" << savedErrno
<< "(" << std::strerror(savedErrno) << ")"
<< " state="
<< (ssl ? SSL_state_string_long(ssl) : "<null-ssl>");

while (auto err = ERR_get_error()) {
AASDK_LOG(error) << "[SSLWrapper] SSL Error " << ERR_error_string(err, NULL);
if (fatalError) {
while (auto err = ERR_get_error()) {
AASDK_LOG(error) << "[SSLWrapper] SSL Error " << ERR_error_string(err, NULL);
}
}
return sslErrorCode;
}
Expand Down
8 changes: 4 additions & 4 deletions src/Transport/USBTransport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ namespace aasdk {

void USBTransport::enqueueReceive(common::DataBuffer buffer) {
const auto inEndpoint = aoapDevice_->getInEndpoint().getAddress();
AASDK_LOG(info) << "[USBTransport] enqueueReceive endpoint=0x" << std::hex
AASDK_LOG(debug) << "[USBTransport] enqueueReceive endpoint=0x" << std::hex
<< static_cast<int>(inEndpoint) << std::dec
<< " requestedBytes=" << buffer.size;

auto usbEndpointPromise = usb::IUSBEndpoint::Promise::defer(receiveStrand_);
usbEndpointPromise->then([this, self = this->shared_from_this(), inEndpoint](auto bytesTransferred) {
AASDK_LOG(info) << "[USBTransport] receiveComplete endpoint=0x"
AASDK_LOG(debug) << "[USBTransport] receiveComplete endpoint=0x"
<< std::hex << static_cast<int>(inEndpoint)
<< std::dec << " bytesTransferred=" << bytesTransferred;
this->receiveHandler(bytesTransferred);
Expand All @@ -58,7 +58,7 @@ namespace aasdk {
void USBTransport::doSend(SendQueue::iterator queueElement, common::Data::size_type offset) {
const auto outEndpoint = aoapDevice_->getOutEndpoint().getAddress();
const auto remainingBytes = queueElement->first.size() - offset;
AASDK_LOG(info) << "[USBTransport] doSend endpoint=0x" << std::hex
AASDK_LOG(debug) << "[USBTransport] doSend endpoint=0x" << std::hex
<< static_cast<int>(outEndpoint) << std::dec
<< " offset=" << offset
<< " remainingBytes=" << remainingBytes
Expand All @@ -67,7 +67,7 @@ namespace aasdk {
auto usbEndpointPromise = usb::IUSBEndpoint::Promise::defer(sendStrand_);
usbEndpointPromise->then(
[this, self = this->shared_from_this(), queueElement, offset, outEndpoint](size_t bytesTransferred) mutable {
AASDK_LOG(info) << "[USBTransport] sendComplete endpoint=0x" << std::hex
AASDK_LOG(debug) << "[USBTransport] sendComplete endpoint=0x" << std::hex
<< static_cast<int>(outEndpoint) << std::dec
<< " offset=" << offset
<< " bytesTransferred=" << bytesTransferred
Expand Down
Loading