Miscellaneous small improvements for socktap, btp and geonet #106
Conversation
This is especially important when receiving messages signed by a ticket from the same authorization authority because then the AA certificate is requested in the next CAM although it is already known because it has been provided via the command line.
The class Router's setter for its security_entity member needs a pointer to a SecurityEntity which may as well be nullptr. With this in mind, RouterContext should also only deal with a pointer. This way, if no security is wanted/needed, one does not have to create a dummy SecurityEntity just to satisfy RouterContext's interface.
According to the GN standard, the GN indication's field security_report shall be set to the corresponding value returned by the security entity in SN-DECAP.confirm. For packets without a secure header, SN-DECAP is never invoked, so security_report should not be initialized. This way, since Vanetza uses a boost::optional for that field, the upper layers can directly tell whether a packet had a secure header without guessing from the other security-related indication fields.
| std::cout << "sent packet to " << request.destination << " (" << bytes_sent << " bytes)\n"; | ||
| std::ostringstream ostream; | ||
| ostream << "sent packet to " << request.destination << " (" << bytes_sent << " bytes)\n"; | ||
| std::cout << ostream.str(); |
There was a problem hiding this comment.
Why is this beneficial? From my point of view, this causes an additional memory allocation and thus this should be justifiable.
There was a problem hiding this comment.
When Vanetza shares stdout with other threads that manipulate the flags for stdout, it can happen (although rarely and unpredictable) that the std::hex flag set in the streaming operator overload of MacAddress is not properly cleared. One could probably move the special treatment to the streaming operator overload.
If you don't see that as a problem Socktap/Vanetza should care about, feel free to omit this commit.
There was a problem hiding this comment.
Okay, but socktap is not multi-threaded ;-) If this is going to change, we will need some more sophisticated logging anyway.
| @@ -398,7 +398,6 @@ void Router::indicate_basic(IndicationContextBasic& ctx) | |||
| indicate_secured(ctx, *basic); | |||
| } else if (basic->next_header == NextHeaderBasic::Common) { | |||
| if (!m_mib.itsGnSecurity || SecurityDecapHandling::Non_Strict == m_mib.itsGnSnDecapResultHandling) { | |||
| indication.security_report = security::DecapReport::Unsigned_Message, | |||
There was a problem hiding this comment.
No, security_report is not optional in geonet::DataIndication though EN 302 636-4-1 says so. Why do you think that Unsigned_Message is unsuitable?
There was a problem hiding this comment.
Oh, sorry, that commit/change needs to be moved to #105. There, security_report is made optional because of EN 302 636-4-1 and EN 302 636-5-1.
Apart from that, please have a look at the explanation in the commit message. Essentially, the security report is the report from the security entity which is never invoked for packets without secure header.
| @@ -144,6 +148,7 @@ int main(int argc, const char** argv) | |||
| for (auto& chain_path : vm["certificate-chain"].as<std::vector<std::string> >()) { | |||
| auto chain_certificate = security::load_certificate_from_file(chain_path); | |||
| chain.push_back(chain_certificate); | |||
| cert_cache.insert(chain_certificate); | |||
|
|
|||
| // Only add root certificates to trust store, so certificate requests are visible for demo purposes. | |||
There was a problem hiding this comment.
Regarding this comment and the change above: Demonstrating certificate requests by using tickets signed by different AAs is a cleaner way and closer to the use of this feature in the real world.
If you want to keep this need for a request for the own AA certificate as a demo feature, I would propose that it is off by default and can be activated via a command line option.
|
Merged except for "status message as single string" and the not-yet optional |
This PR is a collection of different small improvements for btp, geonet, socktap.
std::coutas one string.ItnGnSnDecapResultHandling.SecurityEntityparameter ofRouterContext's constructor a pointer to conform with the interface/requirements ofRouter.