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

Relax back serde dependency #614

Closed
pinkforest opened this issue Aug 21, 2023 · 17 comments · Fixed by #615
Closed

Relax back serde dependency #614

pinkforest opened this issue Aug 21, 2023 · 17 comments · Fixed by #615

Comments

@pinkforest
Copy link
Contributor

pinkforest commented Aug 21, 2023

Hello I think the dependency can be now relaxed

https://github.com/serde-rs/serde/releases
serde-rs/serde#2590

The precompiled binaries in the existing serde_derive versions have after long manual effort + process been ~reasonably audited / reproducible reverse-engineered (kind of) to some degrees so it may be perhaps fine to relax back to full range of possible versions - looking for input on this.

EDIT: Sadly cargo does not support relaxing two sets of ranges so would have to relax either 1) all or 2) provide cfg-gating

@jhpratt
Copy link
Member

jhpratt commented Aug 21, 2023

Thank you for this information. I will review it later today (I'm about to sleep). Assuming the information is true, which is trivially checked, I will allow those versions greater than or equal to that one. However, I will still prevent use of the versions with precompiled binaries until such time as they can be fully reproduced.

@pinkforest
Copy link
Contributor Author

pinkforest commented Aug 21, 2023

We've reproduced reverse-engineered them - but I can add a SemVer 🪄 here if you that would be the preference ?

EDIT: Disclaimer - There is no formal manual proof as such and yes I would not accept it either by just believing someone without results :)

@kayabaNerve
Copy link

The binaries have not been reproduced @pinkforest.

@pinkforest
Copy link
Contributor Author

We spent a lot of time reverse-engineering them - but I understand the preference so I will scope out accordingly.

@kayabaNerve
Copy link

kayabaNerve commented Aug 21, 2023

I wasn't saying they haven't been understood. I was saying your claim, as written, is factually wrong. Not that this is my issue, but I wouldn't personally object to highlighting how understood they are as justification to move on.

@pinkforest
Copy link
Contributor Author

pinkforest commented Aug 21, 2023

Manual reproducibility is different than automatic that is true and I understand people want automatic and I fully agree with that. But regardless I will adjust the range. Thanks

EDIT: Disclaimer - There is no formal manual proof as such and yes I would not accept it either by just believing someone without results :)

@pinkforest
Copy link
Contributor Author

Problem also is there is no way to define multiple ranges in the SemVer statement, I will have to cfg-gate so proposal coming up shortly.

@pinkforest
Copy link
Contributor Author

Ok latest commit shows alternative approach - see how do you like this approach -

We can gate optionally at --cfg to allow extended version range but by default it would only allow >= x.y.z after the building from source was allowed again.

Is there a way to gate like here: rustsec/advisory-db#1417 - I don't see a way in Cargo ?

@pinkforest
Copy link
Contributor Author

PR's up:

Couple of approaches there I've outlined - lmk and I can adjust there ❤️

@kayabaNerve
Copy link

From my understanding, even attempts at manual reproduction have not produced an exact match.

@pythoneer
Copy link

pythoneer commented Aug 21, 2023

@pinkforest where is the "result" or the steps that lead to the "manual reproduction effort". Or do i need to take your word for it? Sorry that i am unable to find it myself ... the situation is very unclear and messy right now. Is there a central point that explains the situation, makes clear what steps are the result of it and where we stand right now? Currently its all scattered among various PR comments and issues across multiple projects.

@pinkforest
Copy link
Contributor Author

pinkforest commented Aug 21, 2023

@kayabaNerve @pythoneer There is no formal proof - that is entirely correct - and it is also correct that it is not that would pass the "stink test" as what we can call entirely reproducible and this is my error.

It is correct that I should have used different wording so thank you on testing me that and I will correct myself up.

My apologies there is no question of my wording being mistaken here -

This also shows that we need to do this more work in the ecosystem via building safeguards until we have tooling for more formalised proofs - and - this is why we have peer-reviews to check our biases / errors in our processes - especially when we have had to revert to manual errorneous process when we've had to adjust our workflows to new challenges that can lead to poor verification when trying to rush with limited resources scrambled together on our free time.

If there is a silver lining - that is that we have more awareness to bring more tooling in like cackle.

Also please see this article by boats - it would be great to solve this as well - but we have to do what we have:
https://without.boats/blog/rust-2019/

EDIT: I've corrected what I wrote re: reproducibility - thanks for checking - if it's any salt to teh wounds I was in a rush 🤦‍♀️

@pinkforest
Copy link
Contributor Author

pinkforest commented Aug 21, 2023

@kpcyrd Since you've done more documented attempts at this and otherwise great work on making the reproducibility experience much much better :)

By any chance did you happen to get somewhere formally documenting that the binaries on >= 1.0.172, <= 1.0.185 other than the original comments in that origin issue in serde repo ?

p.s. loved the work here:
https://github.com/kpcyrd/i-probably-didnt-backdoor-this

Any chance to bring some formality / more documentation into these binaries ?

Cc/ @Shnatsel @alex - ideas ? It would be great to be able to relax the time dependency more if poss here ?

@roblabla
Copy link

However, I will still prevent use of the versions with precompiled binaries until such time as they can be fully reproduced.

Can we please not? non-semver version constraints tend to break cargo's resolver in very weird ways, inducing more pain than necessary. Now that serde is fixed, the use of precompiled binaries should slowly go away.

@BlackDex
Copy link

This constraint also breaks some tooling like cargo outdated ;)

@jhpratt
Copy link
Member

jhpratt commented Aug 22, 2023

@roblabla See #611. The poll had a very clear result.

@roblabla
Copy link

@roblabla See #611. The poll had a very clear result.

The MR that was merged uses a normal semver requirement, so I'm happy with the outcome.

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 a pull request may close this issue.

6 participants