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

Switch to fiat-crypto Rust crate #201

Closed
vlmutolo opened this issue May 17, 2021 · 9 comments · Fixed by #252
Closed

Switch to fiat-crypto Rust crate #201

vlmutolo opened this issue May 17, 2021 · 9 comments · Fixed by #252
Assignees
Labels
new feature New feature or request
Milestone

Comments

@vlmutolo
Copy link
Contributor

Summary

We use fiat-crypto code to implement various low-level cryptographic primitives. If the generated code from fiat-rust changes, it could mean there's a security vulnerability in it. An easy way to ensure we're notified of these changes would be to fail CI tests if the hash of the file(s) we use changes.

Alternatives

  • We could set up some kind of Matrix bot that detects if the fiat-rust hash changes and posts to the Orion room. This would require maintaining the bot, though.
@vlmutolo vlmutolo added the new feature New feature or request label May 17, 2021
@vlmutolo vlmutolo self-assigned this May 17, 2021
@brycx
Copy link
Member

brycx commented May 27, 2021

I think the best approach is still to make it a part of the daily tests in form a script or similar. Failing the daily tests seems fine IMO.

A bot for the room would be nice as an additional reminder, but as you said it would require to maintain it. Also, then we'd have to start scrolling through chat-logs every once in a while to check if there's a update missing. Of course, you're welcome to add the bot in addition to CI/CD just for fun, if you'd like.

@vlmutolo
Copy link
Contributor Author

Do we also want to take the dependabot approach here? It could issue a PR instead of failing the tests.

@brycx
Copy link
Member

brycx commented May 27, 2021

Not really sure if we can. Do you know of some custom way to script Dependabot?

Or was it another approach you had in mind?

@JasonGross
Copy link

If the generated code from fiat-rust changes, it could mean there's a security vulnerability in it

It's more likely that a change will mean that we tweaked the documentation, added a new operation, or tweaked some optimization. I guess it's possible that we got some parentheses or cast transcriptions wrong (the only unverified part is where we go from our internal AST to Rust code).

Regardless, is there any reason to inline the Rust code rather than depending on our published Rust crate?

@brycx
Copy link
Member

brycx commented Nov 25, 2021

Regardless, is there any reason to inline the Rust code rather than depending on our published Rust crate?

When I initially looked at it, IIRC the generated Rust code in the repository was newer than that of the Rust crate. So I simply thought to be able to get the newest version, there'd have to be a manual check. Maybe I misunderstood something there?

Just looked again and there seems to be automatic publishing of the Rust crate now (maybe there was before too?). If this is the case, then I don't see a reason as to why we wouldn't prefer the crate over inlined Rust code. How regularly does the automatic publishing happen, in terms of changes to the Rust code?

Anyway, thanks for chiming in here!

@JasonGross
Copy link

Yeah, we update the crate automatically any time we release a version, which we do anytime there are significant changes to the code (and I plan to bump the major version if ever we make a backwards-incompatible change to the interface).

@JasonGross
Copy link

JasonGross commented Nov 25, 2021

Also, if you ever discover that the crate is out of date in a meaningful way, feel free to open an issue asking us to make a new release. (But there's no reason to be pegged to the latest version. Certainly if we find any correctness issues in the generated code we'll immediately make a new release when they're fixed.)

Also, if there's something we have to do on our end to get dependabot to notice when we do a release, I'm happy to set that up on our side.

@brycx
Copy link
Member

brycx commented Nov 25, 2021

Yeah, we update the crate automatically any time we release a version, which we do anytime there are significant changes to the code (and I plan to bump the major version if ever we make a backwards-incompatible change to the interface).

Then I definitely don't see a reason not to switch to the crate. Thanks for expanding upon it.

Also, if there's something we have to do on our end to get dependabot to notice when we do a release, I'm happy to set that up on our side.

There shouldn't be. As long as the version number is bumped, which it must either way, Dependabot should detect it.

@brycx brycx changed the title Fail CI tests if generated fiat code changes upstream Switch to fiat-crypto Rust crate Nov 27, 2021
@brycx
Copy link
Member

brycx commented Nov 27, 2021

Changed scope of issue. Please raise any possible concerns @vlmutolo, especially if you don't want to remain assigned to this issue, considering the change.

My initial thinking is to include this in the next minor release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants