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

Add crypto extern to behavioral-model #834

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Conversation

hesingh
Copy link
Contributor

@hesingh hesingh commented Dec 12, 2019

Please see this Issue:

#697

A .cpp file is added where the new extern is implemented.
A .p4 filed is added where the extern is defined and used.

Thanks to the folks who developed this code and put it in at this repo:

https://github.com/uni-tue-kn/p4-macsec

@hesingh hesingh mentioned this pull request Dec 12, 2019
@jafingerhut
Copy link
Contributor

I am sure Antonin will have his own comments, but my high level comment is that I would be happy to try this out to see if it works, but only if there is an explicit README file somewhere, in the simple_switch/externs/crypto directory, that gave step-by-step instructions for:

How to compile the .cpp file, with actual working commands, and all options used. It is reasonable to do so only for the main supported platform, i.e. Ubuntu 16.04 Linux.

How to compile the P4 program, with all command line options specified fully.

How to run simple_switch or simple_switch_grpc, with all command line options specified fully.

Bonus points if the README explains which C++ functions are called by simple_switch directly, and an example of what the parameter values are in the P4 program, and what they become (if it isn't obvious) in the C++ function call, what the C++ function return value is (unless it is void), and if that value becomes visible in the P4 program somehow. Also how out and inout parameters are handled, if they are relevant for the example.

@jafingerhut
Copy link
Contributor

Also good to mention what the license is on the code that you copied or based this on, and preserve the names of the original authors, and link to the original in comments. In particular, it seems best not to have a boilerplate copyright with Barefoot Networks in there, unless someone at Barefoot Networks wrote it.

We should not relicense code in a legally incompatible way, unless the original authors are willing to release it under that license, too.

@hesingh
Copy link
Contributor Author

hesingh commented Dec 12, 2019

Sure, will add README.md. However, I am little lost for instructions because the original repo has no README.md file but the code uses a controller, mininet, and also simple_switch. These instructions may take a little while to put together.

Regarding license, please see one file they have changed at this link:

https://github.com/uni-tue-kn/p4-macsec/blob/master/p4/target/simple_switch.cpp

The original Apache 2.0 license is retained.

I have pinged the folks by filing this issue: uni-tue-kn/p4-macsec#1

@jafingerhut
Copy link
Contributor

I don't think it is necessary to give a working example that uses Mininet here.

Instructions for running a single simple_switch or simple_switch_grpc process, with all command line options needed to use the new extern, should be enough. Let those who wish to use the new extern in a Mininet network figure out the changes to their own software outside of simple_switch.

@hesingh
Copy link
Contributor Author

hesingh commented Dec 12, 2019

OK.

I am looking at the Travis failure right now. I have let the authors know what would help in the README.md file. thanks.

@hesingh
Copy link
Contributor Author

hesingh commented Dec 12, 2019

I could not reproduce the Travis build failure on my local machine using Ubuntu 18.04. I will see how to force another Travis build.

@jafingerhut
Copy link
Contributor

Most of the messages I saw looked like style warnings in the syntax/formatting of the C++ source code file, e.g. must be a space after "//" comment beginning, or line too long. Search for the string "should have a space" on this Travis log page: https://travis-ci.org/p4lang/behavioral-model/jobs/624343929?utm_medium=notification&utm_source=github_status

@hesingh
Copy link
Contributor Author

hesingh commented Dec 12, 2019

Right - I did see that and fixed all those already. I am fixing other style issues - thanks.

@codecov-io
Copy link

codecov-io commented Dec 13, 2019

Codecov Report

Merging #834 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #834   +/-   ##
=======================================
  Coverage   74.77%   74.77%           
=======================================
  Files         115      115           
  Lines       10193    10193           
=======================================
  Hits         7622     7622           
  Misses       2571     2571

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ef3248...f187058. Read the comment docs.

@jafingerhut
Copy link
Contributor

@hesingh Thanks for continuing to enhance this PR with more details. I would like to try them out when there are directions I can try following. I see a recent commit that claims it adds a README.md file, but there is no README.md file in the current PR. Did you forget to add that file using git on your end?

@jafingerhut
Copy link
Contributor

Ignore my previous comment. I see a README.md file. Let me see if it looks complete. Thanks.

@hesingh
Copy link
Contributor Author

hesingh commented Dec 16, 2019

Sorry, during a checkin, part of the README.md file was not incorporated. So, I checked in another version that now has the complete README.md with one TODO and maybe requiring more clarifications.

@hesingh
Copy link
Contributor Author

hesingh commented Dec 16, 2019

The original macsec code uses send.py and receive.py in https://github.com/uni-tue-kn/p4-macsec/p4/p4. There is no README.md to show which IP address/mac-address to send a packet to or how is simple_grpc used to send and receive packets. This is why such notes are pending for README.md or this PR.

@hesingh
Copy link
Contributor Author

hesingh commented Dec 16, 2019

Also good to mention what the license is on the code that you copied or based this on, and preserve the names of the original authors, and link to the original in comments. In particular, it seems best not to have a boilerplate copyright with Barefoot Networks in there, unless someone at Barefoot Networks wrote it.

We should not relicense code in a legally incompatible way, unless the original authors are willing to release it under that license, too.

Few days back, I asked the authors to add a CONTRIBUTING.md file to their repo so that their code's licensing is clear. They didn't get back yet. This is why my code changes do not have licensing changes.

@hesingh
Copy link
Contributor Author

hesingh commented Dec 16, 2019

@jafingerhut I am compiling their basic.p4 after fixing mark_to_drop(). I see an error with recirculate. I checked your p4-info code but didn't find a use for recirculate like what this code has. Please see basic.p4 in this PR which fails to compile.

hemant@ubuntu:~/p4c/build$ p4c-bm2-ss --std p4-16 --emit-externs --p4runtime-files foo.txt /tmp/int/behavioral-model/targets/simple_switch/externs/sym_crypto/basic.p4 
/tmp/int/behavioral-model/targets/simple_switch/externs/sym_crypto/basic.p4(288): [--Wwarn=unsupported] warning: recirculate: recirculate with non-empty argument not supported
                recirculate({meta.intrinsic_metadata, standard_metadata, meta.user_metadata});
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@jafingerhut
Copy link
Contributor

@hesingh I don't see any strong reason that an example P4 program for a crypto (or any) extern function needs to also have a resubmit/recirculate/clone operation. Does the code compile if you just delete or comment out that part of the P4 code?

@hesingh
Copy link
Contributor Author

hesingh commented Dec 18, 2019

@hesingh Thanks for continuing to enhance this PR with more details. I would like to try them out when there are directions I can try following. I see a recent commit that claims it adds a README.md file, but there is no README.md file in the current PR. Did you forget to add that file using git on your end?

If you'd like you can try the steps at macsec repo to test the crypto extern. The scripts use an older version of p4c, behavoiral-model, and PI code. Or you can wait till the repo updates code using latest p4c, etc. thanks.

@jafingerhut
Copy link
Contributor

I'd like to wait until their README is updated, to see if that version looks accurate, and provide comments on it.

Is there any reason to have both a .cpp file and the diffs file with nearly identical contents in this PR?

Is there any reason to have either one of those, or the .p4 program, if they also exist in the linked other Github repository, along with the instructions for how to use them?

It seems to me either:

(a) this PR should be a README with a link to the other repo, and pretty much nothing else

or

(b) it should be possible without installing mininet, or anything else not absolutely required to install behavioral-model simple_switch according to behavioral-model's current README instructions, to follow README instructions completely within behavioral-model's repository to run a single simple_switch executable with an extra extern added.

Personally, I am content either way. It seems the current PR is much more than (a), and much different than (b).

@hesingh
Copy link
Contributor Author

hesingh commented Dec 18, 2019

I'd like to wait until their README is updated, to see if that version looks accurate, and provide comments on it.

Is there any reason to have both a .cpp file and the diffs file with nearly identical contents in this PR?

Is there any reason to have either one of those, or the .p4 program, if they also exist in the linked other Github repository, along with the instructions for how to use them?

It seems to me either:

(a) this PR should be a README with a link to the other repo, and pretty much nothing else

or

(b) it should be possible without installing mininet, or anything else not absolutely required to install behavioral-model simple_switch according to behavioral-model's current README instructions, to follow README instructions completely within behavioral-model's repository to run a single simple_switch executable with an extra extern added.

Personally, I am content either way. It seems the current PR is much more than (a), and much different than (b).

I went with (a). Please see a checkin I made right now. I think the README.md instructions should get updated in a day or two for the macsec repo.

@hesingh
Copy link
Contributor Author

hesingh commented Dec 19, 2019

I'd like to wait until their README is updated, to see if that version looks accurate, and provide comments on it.

The README has been updated today. Please take a look - thanks.

@hesingh
Copy link
Contributor Author

hesingh commented Dec 19, 2019

Is there any reason to have either one of those, or the .p4 program, if they also exist in the linked other Github repository, along with the instructions for how to use them?

It seems to me either:

(a) this PR should be a README with a link to the other repo, and pretty much nothing else

There is a reason. In the macsec repo, they have an old version of simple_switch.cpp in p4/targets/ with the crypto extern. For a new user, why sift through an old version of simple_switch.cpp in the macsec repo to find the extern. This is why I have added just the extern implementation in crypt.cpp of this PR. Also, even if the macsec repo grabs the latest simple_switch.cpp, in one month, simple_switch.cpp is old again. With code in crypt.cpp, a user can take the code and add to any version of simple_switch.cpp with ease.

basic.p4 is self-contained implementation and this is why I have deleted it from this repo and referenced it in the README to macsec repo

@hesingh
Copy link
Contributor Author

hesingh commented Dec 20, 2019

File a new issue with macsec repo to fix cpplint errors in their code.

uni-tue-kn/p4-macsec#2

@hesingh
Copy link
Contributor Author

hesingh commented Dec 24, 2019

The macsec repo accepted my cpplint fixes in their code and updated their repo. Then, I copied their extern to crypto.cpp of this PR. We could consider committing this PR's code change.

@hesingh
Copy link
Contributor Author

hesingh commented Dec 27, 2019

Now, the macsec repo README.md has its final edit made one week back. Just run ./setup.sh in the root directory and the software is installed.

The code changes are this PR are also final until any other changes are suggested.

const Data &in_ipv4_hdr) {
std::vector<unsigned char> secure_association_key = get_char_vector(
in_sak.get_string(), SAK_SIZE);
// cout << "[p4sec] secure_association_key" << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

are all these commented-out lines useful? why not use the bmv2 logger with a high verbosity level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original authors have all this commented out code. Either we remove them or change to bmv2 logger. Let me think about this one.

std::vector<unsigned char> secure_association_key = get_char_vector(
in_sak.get_string(), SAK_SIZE);
// cout << "[p4sec] secure_association_key" << std::endl;
// hexdump((char*)&secure_association_key[0],
Copy link
Member

Choose a reason for hiding this comment

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

is there an hexdump definition available anywhere? what if I try to comment-out this line and compile this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

thanks, I totally missed it

@@ -0,0 +1,22 @@
# ADD CRYPTO to SIMPLE_SWITCH
Copy link
Member

Choose a reason for hiding this comment

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

strange typography

@@ -0,0 +1,22 @@
# ADD CRYPTO to SIMPLE_SWITCH

For reference, the extern is implemented in crypto.cpp in this directory.
Copy link
Member

Choose a reason for hiding this comment

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

There are no instructions here on how to compile the attached P4 program using p4c.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add text. In the macsec repo this Makefile is used to compile with p4c.

https://github.com/uni-tue-kn/p4-macsec/blob/master/p4/p4/Makefile


For reference, the extern is implemented in crypto.cpp in this directory.
This cpp code is to be incorporated in
behavioral-model/targets/simple_switch/simple_switch.cpp
Copy link
Member

Choose a reason for hiding this comment

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

We should provide a mechanism for using this code that does not require copying the code to simple_switch.cpp.
My preferred option would be to have a standalone Makefile that can build a .so. The .so can then be imported into simple_switch using --load-modules. An alternative, which is not as good IMO (but I have no strong objection) is to just make this part of the bmv2 build system.
I gave some extra information there: #697

Copy link
Member

Choose a reason for hiding this comment

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

Thinking of this more, adding a separate Makefile and not making it part of the simple_switch build is definitely the right way to go. There is no reason to unconditionally compile and link this example extern to bmv2, and I think the separate Makefile is more representative of the "correct" workflow for someone who wants to add an extern type without changing any of the bmv2 code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a Makefile to behavioral-model/targets/simple_switch/sample_extern/macsec with a recent commit.

@antoninbas
Copy link
Member

Where does the name "sym_crypto" comes from? Why not "macsec" to make it clear what this does?

@antoninbas
Copy link
Member

antoninbas commented Dec 28, 2019

We shouldn't place this directly under the "externs" directory because to me it implies that this is an official extern for simple_switch / v1model. Maybe a "sample_externs" directory would be better.

I think @jafingerhut may have pointed this out somewhere else previously: while I think there is value in having a more complex extern type such as this one, maybe we should start with a very basic one that just prints the time or something like this. We can add a more complex one when we figure out exactly how we want to organize the "sample_externs" directory.

Edit: if we go with a basic extern, we still want something that takes a parameter, in order to keep things interesting. Maybe some accumulator with one method to "accumulate" (i.e. increment an internal counter by a specified amount) and one method to log the current counter value.

std::vector<unsigned char>& out_secure_data,
std::vector<unsigned char>& out_integrity_check_value
) {
// hier evtl assertions fuer die Laenge der Parameter
Copy link
Member

Choose a reason for hiding this comment

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

this comment is in German?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the authors are at a university in Germany. I can change the comment to English.

return vec;
}

void protection_function(std::vector<unsigned char> secure_association_key,
Copy link
Member

Choose a reason for hiding this comment

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

the in parameters should be passed by const reference, unless making a copy is actually required. The rest of the code base uses a pointer, and not a reference, for out parameters, as per the Google code style guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API for the extern is defined below.

https://github.com/uni-tue-kn/p4-macsec/blob/master/p4/p4/basic.p4#L182

The C++ backend code is here and uses "const reference.

https://github.com/hesingh/behavioral-model/blob/master/targets/simple_switch/externs/sym_crypto/crypto.cpp#L75

Regarding the protection_function(), all its args can be changed to const reference except the last two. The last two can change to pointers.

if ((i % 16) == 0) {
// Just don't print ASCII for the zeroth line.
if (i != 0)
printf(" %s\n", buff);
Copy link
Member

Choose a reason for hiding this comment

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

I don't know why we switch to printf here, instead of streaming to std::cout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the authors wanted to use %02x etc. and went with printf.

return result;
}

void hexdump(char *addr, int len) {
Copy link
Member

Choose a reason for hiding this comment

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

it makes no sense to cast the argument to a char * at the callsite, only to cast it back to an unsigned char * at the beginning of this function.

@hesingh
Copy link
Contributor Author

hesingh commented Dec 28, 2019

Where does the name "sym_crypto" comes from? Why not "macsec" to make it clear what this does?

macsec uses a symmetric crypto. In future, I was thinking of asymmetric crypto (RSA) and IPSec. I can change the name to macsec.

@hesingh
Copy link
Contributor Author

hesingh commented Dec 28, 2019

We shouldn't place this directly under the "externs" directory because to me it implies that this is an official extern for simple_switch / v1model. Maybe a "sample_externs" directory would be better.

I think @jafingerhut may have pointed this out somewhere else previously: while I think there is value in having a more complex extern type such as this one, maybe we should start with a very basic one that just prints the time or something like this. We can add a more complex one when we figure out exactly how we want to organize the "sample_externs" directory.

Edit: if we go with a basic extern, we still want something that takes a parameter, in order to keep things interesting. Maybe some accumulator with one method to "accumulate" (i.e. increment an internal counter by a specified amount) and one method to log the current counter value.

Since a Cornell student added a simple PSA extern, we could ask that team to add the basic extern to simple_switch. With this PR, let's check in a sample_extern for macsec,

@hesingh
Copy link
Contributor Author

hesingh commented Jan 6, 2020

@antoninbas I did add a makefile you requested. Please review and let me know if there are any more changes to be made to this PR - thanks.

Copy link
Member

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

I suggest renaming the directory from sample_extern to sample_externs


https://github.com/uni-tue-kn/p4-macsec

The extern is defined in the following P4 file. Search for ExternCrypt
Copy link
Member

Choose a reason for hiding this comment

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

I recommend copy-pasting the P4 extern type definition here since it's only a few lines, in case the link becomes broken in the future.


basic.p4 is used to test the cryto extern code with simple_switch.

basic.p4 is compiled using p4c with args shown in
Copy link
Member

Choose a reason for hiding this comment

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

again, please provide the command-line as part of this README, I think it's valuable.

@@ -0,0 +1,25 @@
# ADD CRYPTO EXTERN to SIMPLE_SWITCH
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# ADD CRYPTO EXTERN to SIMPLE_SWITCH
# Add macsec extern to simple_switch


For reference, the extern is implemented in crypto.cpp in this directory.
This cpp code is to be incorporated in
behavioral-model/targets/simple_switch/simple_switch.cpp
Copy link
Member

Choose a reason for hiding this comment

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

That's not accurate. You should instead include the following instructions:

  • configure bmv2 with the --enable-modules flag so that extern definitions can be provided at runtime
  • build the macsec extern as a shared library by running make inside this directory
  • when starting simple_switch, load the shared library with --load-modules libexterncrypto.so

Please also make sure that these instructions work properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a --enable-modules as an arg to simple_switch. How do I configure it then?


clean:
rm -rf libexterncrypto.so

Copy link
Member

Choose a reason for hiding this comment

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

Since this extern is a bit complicated, I think we should have a simple C++ test and a make check target in this Makefile to make sure that the extern definition is not broken and that encryption / decryption work properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me think about this one and get back.

// must pass byte to external function
// use 0x54 T as true
// use 0x46 F as false
// cout << "[p4sec] prepend IPv4 Header ? "
Copy link
Member

Choose a reason for hiding this comment

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

did you mean to keep this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove the comments and keep the prepend code. Note, the extern has a protect method with one arg as prepend_ipv4_hdr. This is why the prepend code has to be retained.

if (in_prepend_ipv4_hdr.get_string().compare("T") == 0) {
prepend_ipv4 = true;
} else {
// cout << "[p4sec] do not prepend IPv4 Header" << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

same here?

std::vector<unsigned char> ipv4_hdr;
if (prepend_ipv4) {
ipv4_hdr = get_char_vector(in_ipv4_hdr.get_string(), IPV4_HDR_SIZE);
// hexdump((unsigned char*)&ipv4_hdr[0], ipv4_hdr.size());
Copy link
Member

Choose a reason for hiding this comment

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

same here?

Base automatically changed from master to main February 9, 2021 02:44
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.

4 participants