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

Security V1.3.1 #130

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

Security V1.3.1 #130

wants to merge 11 commits into from

Conversation

JordiMarias
Copy link

This pull request adds the functionality of the security V1.3.1; as specified in the following document:
https://www.etsi.org/deliver/etsi_ts/103000_103099/103097/01.03.01_60/ts_103097v010301p.pdf

I've opened this pull request to get the code already developed to be reviewed, so I'm sure we are on the same page. Although the V1.3.1 it's already developed and working, I'm planning to update the Certify tool and add testing in the future. To run this new security layer it can be done by compiling "socktap" and running it with the following command and the certificates below:
./socktap -p static --certificate 3_Root_CA_Cert.bin --certificate-key 3_Root_CA_private.pem --security certs --security-version 3

To ensure retro compatibility I've used boost::variant on both the Certificate class and the SecuredMessage class. Which are split into SecuredMessageV2 and SecuredMessageV3 and used equally with SecuredMessageVariant. Which can be fundamentally used the same way the old SecuredMessage object could. Exactly the same applies to the CertificateVariant object.

Finally both the "verify service" and the "sign service" are under an interface and can be used indistinctly and adapt themselves to the
the version of the security used.

I'll look forward to and answer any review.

P.D: I've developed this for a project and still have some months ahead to further develop features of the security layer. My intention is NOT to directly merge my code is to start a review and improvement process until an optimal point of development is reached.

Example Certificate:

Certificate: https://drive.google.com/file/d/1nA4nwp5ItymthmwnrTeVWYBhlSaR3Az2/view?usp=sharing
Private Key: https://drive.google.com/file/d/15qBeUI1zDyZKk76VDQ7iuAUkrbkV-Pdh/view?usp=sharing

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.

Thanks for your effort and sorry for my delayed review. Looks quite good overall, I have just some minor change requests. See the inline comments for details.

vanetza/common/archives.hpp Show resolved Hide resolved
vanetza/geonet/parser.cpp Outdated Show resolved Hide resolved
vanetza/security/basic_elements.hpp Outdated Show resolved Hide resolved
vanetza/security/certificate.hpp Outdated Show resolved Hide resolved
vanetza/security/certificate.hpp Outdated Show resolved Hide resolved
vanetza/security/sign_header_policy.cpp Show resolved Hide resolved
vanetza/security/tests/certificate.cpp Outdated Show resolved Hide resolved
vanetza/security/tests/certificate.cpp Outdated Show resolved Hide resolved
vanetza/security/tests/certificate.cpp Outdated Show resolved Hide resolved
vanetza/security/verify_service.hpp Show resolved Hide resolved
@JordiMarias
Copy link
Author

JordiMarias commented Mar 3, 2022

Hi, again Riebl! I know haven't been active in this PR for quite a long time, but I'd like to contribute to the project and merge it.
I've finally reviewed all the comments again and it seems now that everything is in order. Could you review it again, to check if there is something missing?

Best regards,
Jordi Marias

@JordiMarias
Copy link
Author

Hi, again Riebl! I know haven't been active in this PR for quite a long time, but I'd like to contribute to the project and merge it. I've finally reviewed all the comments again and it seems now that everything is in order. Could you review it again, to check if there is something missing?

Best regards, Jordi Marias

PD: I'm working quickly to make all UT run fine!

@riebl
Copy link
Owner

riebl commented Mar 5, 2022

Hi @JordiMarias, welcome back! :-)
Let me know if you face trouble fixing the unit tests. Otherwise, I will simply wait for your update.

@JordiMarias
Copy link
Author

I'm having a hard time fixing the remaining failing tests. Most of the problems that appear regard the boost::variant implementation from the fact that there is compatibility with both versions. More specifically, it boils down to the Certificate provider, when is called the "own_certificate()" method it returns the new "CertificateVariant" type of object which is a visitable variable that can either be the v1.2.1 security cert or the v1.3.1 security cert. Apparently, nothing gets returned when there is returned a boost::variant type of message.
I'm working on it but if you can take a glance at it, it would be great!

@riebl
Copy link
Owner

riebl commented Mar 13, 2022

Update: I am currently extracting the security v1.2.1 encoding to the vanetza::security::v2 namespace, so vanetza::security contains only generic code working for both versions. As part of that, I am also refactoring the interfaces slightly to make the version agnostic. That should help us to get your changes into place as soon as I am done with it.

@JordiMarias
Copy link
Author

Hi Riebl, what are your plans to be able to merge this pull request? Did you were able to extract the V2 security to the vanetza::security::v2? I can try to do it myself in this PR.

@riebl
Copy link
Owner

riebl commented Jun 26, 2022

Hi @JordiMarias, I have been able to extract most of the V2 security code to its namespace. Unfortunately, I have not yet been able to run all tests successfully again.

@kenog
Copy link
Contributor

kenog commented Oct 13, 2022

Hi together,
are there any news about this? We would be very interested in using this soon. Let me know if I can help somehow.

Best regards
Keno

@riebl
Copy link
Owner

riebl commented Oct 18, 2022

Hi @kenog,

no news, I had no time yet to complete this effort. However, I could tidy up my branch a little bit and share it with you. Are you interested?

@kenog
Copy link
Contributor

kenog commented Oct 19, 2022

Yes, that would be great! Thank you very much :)

@riebl
Copy link
Owner

riebl commented Oct 23, 2022

@kenog I have pushed branch sec_v2_namespace. Please note that the SecurityEntity unit test segfaults. It is still in a rough state…

Update 6 November: I have fixed the segfault issue. The branch builds and runs fine now. Next step is to add v3::SecuredMessage to the SecuredMessage variant.

@serserHR
Copy link

serserHR commented Oct 3, 2023

Hi @riebl,
I'd like to know the current state of this pull request and if it is possible to use this branch for providing simply CAM and RTCMEM communication in v3 way with it. If It isn't, when do you estimate that this pull request will be closed?
Thanks in advance.

@riebl
Copy link
Owner

riebl commented Oct 5, 2023

Hi @serserHR, this PR is no longer compatible with our upstream master branch. Secured v3 messages can be received meanwhile. Support for outgoing v3 secured messages is a top priority but my spare time is quite limited.

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

5 participants