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

Update HACL-C snapshot #2199

Merged
merged 4 commits into from
Dec 4, 2017
Merged

Update HACL-C snapshot #2199

merged 4 commits into from
Dec 4, 2017

Conversation

podhrmic
Copy link
Member

Removing hacl-c from modules and adding into sw/ext directory. Hacl code itself shouldn't be reviewed, as it is a part of generated snapshot (with the exception of kremlib_embedded.h, comment directly at HACL* .
Appropriate changes in haclc.xml
Added hacl module in test config (Minion Lia)

@kirkscheper
Copy link
Member

Why include the code and not add https://github.com/mitls/hacl-c as a submodule? That would really ensure that 'Hacl code itself shouldn't be reviewed'.

You can always apply a patch to the submodule when it is updated to implement the custom kremlib_embedded.h (similar to what is done with opencv_bebop), or just copy the kremlib_embedded.h into the submodule as part of the make process.

@podhrmic
Copy link
Member Author

podhrmic commented Nov 29, 2017

I was thinking about it, but HACL* itself is quite large, and we really need only a tiny fraction of it - hence the direct include. So for sake of keeping the paparazzi repo small enough, I would prefer this solution:-)

In the future, HACL maintainers will probably have a separate repo just with the C snapshot - in which case I ll use it as a submodule, and replace the current code.

Edit: the snapshot already exists here https://github.com/mitls/hacl-c but at this point is outdated - I am hoping it will become useful in next few weeks at which point I will switch.

@kirkscheper
Copy link
Member

'In the future, HACL maintainers will probably have a separate repo just with the C snapshot' isn't this just the https://github.com/mitls/hacl-c? in the readme they state 'HACL* verified C code'.

Correct me if I'm wrong but the files you added seem to be almost exactly from https://github.com/mitls/hacl-c.

Additionally, by making it a submodule you are actually reducing the size of the paparazzi repo so the code is not stored locally but only downloaded when the user needs it and initiates the module with submodule init (again, quite similar to the opencv_bebop).

@podhrmic
Copy link
Member Author

The C snapshot repo is not up to date, let me see if I can ping them to update it.
We will still need either a patch stored somewhere (ideas?) or a paparazzi fork of the repo. What is better?

@gautierhattenberger
Copy link
Member

why not making a pull request with your modifications to hacl-c, it might be interesting for other people doing embedded software, right ?

@podhrmic
Copy link
Member Author

I already did submit a few modifications:-) This patch (kremlin_embedded.h) just makes it more convenient for our use case, but I can as well have the file stored outside the submodule (just don't know where would be the best).

@podhrmic
Copy link
Member Author

So after looking at it some more - the snapshot is not good to go yet without additional modifications. I am working on this with HACL developers.

Will merge as is, and exchange hacl-c for a submodule once it is ready (should be next few days). Functionality will remain the same.

@kirkscheper
Copy link
Member

What's the rush? I don't think anyone else working on master desperately needs this. I will leave this for @gautierhattenberger but IMHO its better to do it right than rush it through. Once its accepted there is no real motivation to update it.

@gautierhattenberger
Copy link
Member

I agree that temporary solutions for this kind of things are usually longer than expect. I'm supposed to convert the tlsf lib to submodule know that it is available in a git repository since ... too much time to recall. So if you think that HACL developers are fast to integrate changes or if patches can be easily applied, I suggest to do that from the start.

@podhrmic
Copy link
Member Author

podhrmic commented Dec 2, 2017

Converted into proper submodule. Using a paparazzi fork of the repo so I don't have to wait for the upstream changes (but will happily switch that later)

The reason I am kind of rushing this is because I want to avoid having a large PR and want to make small PRs for each part of the secure link. And without having hacl merged into master I can'r really move on (I am still hoping for mid-december release of secure link)

@podhrmic
Copy link
Member Author

podhrmic commented Dec 4, 2017

Good to merge?

@gautierhattenberger gautierhattenberger merged commit 2c35ebe into master Dec 4, 2017
biancabndris pushed a commit to biancabndris/paparazzi that referenced this pull request Aug 29, 2018
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

3 participants