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

Additions to cross-layer interfaces according to the ETSI standards #105

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

glmax
Copy link
Contributor

@glmax glmax commented Feb 24, 2020

This PR adds several fields/"parameters" to the primitives that are passed between

It also changes the datatype of the source position in BTP/GN-DATA.indication.

Together with the introduction of the permissions field for the request primitives comes the possibility to check whether the permissions requested by the facilities/application (derived from the payload) match the available permissions in the authorization ticket used for signing the message.

This is in accordance with ETSI EN 302 636-4-1 and ETSI EN 302 636-5-1.
Transport type is part of the BTP-Data.indication primitive according
to the BTP standard (EN 302 636-5-1). Remaining hop limit just seems to
be missing in the standard because it is present in GN-Data.indication
and currently, it just perishes in the BTP layer without any use.
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.
Copy link
Owner

@riebl riebl left a comment

Choose a reason for hiding this comment

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

I am not yet convinced by the introduction of SecurityProfile as it only complicates the interface without any benefit, at least with the current feature set.
Furthermore, I am still thinking about how to report SN-ENCAP/SN-SIGN failures: Maybe we shall add actual error codes instead of only omitting the optional secured message result?

if (encap_result) {
payload = std::move(encap_result.get());
} else {
// TODO: How to throw an error here?
Copy link
Owner

Choose a reason for hiding this comment

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

As far as I know, no error reporting is foreseen at this point by specifications.
We may invoke a hook here so custom error logging can be realised without hassle.

@@ -1209,26 +1236,32 @@ std::unique_ptr<GbcPdu> Router::create_gbc_pdu(const GbcDataRequest& request)
return pdu;
}

Router::DownPacketPtr Router::encap_packet(ItsAid its_aid, Pdu& pdu, DownPacketPtr packet)
boost::optional<Router::DownPacketPtr> Router::encap_packet(boost::optional<security::SecurityProfile> security_profile, ItsAid its_aid, ByteBuffer permissions, Pdu& pdu, DownPacketPtr packet)
Copy link
Owner

Choose a reason for hiding this comment

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

return an empty pointer instead of a boost::optional?

/**
* Input data for encapsulating a packet in a secured message
* \see TS 102 723-8 for SN-ENCAP.request
*/
struct EncapRequest {
DownPacket plaintext_payload; // mandatory
boost::optional<SecurityProfile> sec_services; // optional
Copy link
Owner

Choose a reason for hiding this comment

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

I know that sec_services is defined in TS 102 723-8 but it does not make any sense if we are not going to implement encryption. Do you have plans to implement encryption? Is anybody using this feature at all?
Since SecurityProfile::Default exists, I see no point in wrapping it into a boost::optional.

*
* For easy bytewise SSP checking, one can use bitmasks to mark where which type of check is required.
*
* This map contains such a bitmask for all supported applications per supported SSP version of the respective application.
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add some references please?

// EXPECT_EQ(DecapReport::Invalid_Certificate, decap_confirm.report);
// ASSERT_FALSE(decap_confirm.certificate_validity);
// EXPECT_EQ(CertificateInvalidReason::Insufficient_ITS_AID, decap_confirm.certificate_validity.reason());
//}
Copy link
Owner

Choose a reason for hiding this comment

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

Please fix this test case.

@@ -34,6 +35,7 @@ struct DataIndication
geonet::LongPositionVector source_position;
geonet::TrafficClass traffic_class;
boost::optional<geonet::Lifetime> remaining_packet_lifetime;
boost::optional<unsigned> remaining_hop_limit;
Copy link
Owner

Choose a reason for hiding this comment

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

I suppose remaining_packet_lifetime and remaining_hop_limit are optional to implement but always available from a GN protocol perspective. What do you think about making these two fields "mandatory" in Vanetza then?

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.

None yet

2 participants