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

CodecToStr and Codecs in v0.3.1 could cause data corruption #144

Closed
lidel opened this issue Sep 4, 2022 · 3 comments · Fixed by #145
Closed

CodecToStr and Codecs in v0.3.1 could cause data corruption #144

lidel opened this issue Sep 4, 2022 · 3 comments · Fixed by #145
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization P0 Critical: Tackled by core team ASAP

Comments

@lidel
Copy link
Member

lidel commented Sep 4, 2022

v0.2.0 we made a conscious decision to make a breaking change to FORCE people to refactor their code, and not be surprised by code changes. Rationale in #137.

In golang major bump is essentially a different package – bumping major does not help people who are already using INVALID mappings. That is why we did it in v0.2.

Recently merged PR #142 removed that safety and could cause unexpected code change if someone updates from v1.0 to v3.0. IIUC we now have this:

  • What if someone used cbor string and expected it to point at 0x71 (dag-cbor)?
    • v0.3.1 SILENTLY changes the mapping to 0x51 (cbor)
  • What if someone used protobuf string and expected it to point at0x70 (dag-pb)
    • v0.3.1 SILENTLY changes the mapping to 0x50 (protobuf)

Upgrading v0.1.x to v0.3.1 can now produce DAGs with different codec and get NO warning about this BREAKING CHANGE.

@Jorropo @rvagg If you reallly want to keep CodecToStr and Codecs, you should make sure they return hard error when someone tries to use them for impacted mappings above. Third-party apps use this library. People don't read release notes. Silent data corruption is not acceptable.

@lidel lidel added kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP need/triage Needs initial labeling and prioritization labels Sep 4, 2022
@ipfs ipfs deleted a comment from welcome bot Sep 4, 2022
@lidel lidel changed the title CodecToStr and Codecs in v0.3 could cause data corruption CodecToStr and Codecs in v0.3.1 could cause data corruption Sep 4, 2022
@Jorropo
Copy link
Contributor

Jorropo commented Sep 4, 2022

Ah hum ... mb Ill revert, open pr for 0.3.2 and fix Kubo's dep

@Jorropo Jorropo mentioned this issue Sep 4, 2022
@rvagg
Copy link
Member

rvagg commented Sep 5, 2022

Well, as I said in #142, the main break is experienced via CodecToStr, I don't really care that much about Codecs as I haven't seen any difficulty with that. The main source of difficulty is in the way go-libp2p performed some major upgrades at the same time as adapting to this change. So to pull an updated go-cid, a package that's also using go-libp2p has to go through the full upgrade there as well just to get a latest version. Because go-cid is so core to multiformats and ipld, you can't get an updated go-ipld-prime or related packages without going through all of these hoops.

So, in hindsight, I still think we made the wrong call in #137 and should have gone through a longer deprecation cycle, using the deprecation tools afforded by Go—which aren't awesome, but they exist.

More discussion in #142 would have been nice to work through this a bit further.

@lidel
Copy link
Member Author

lidel commented Sep 5, 2022

Just to be clear (and reiterate #137), both Codecs AND CodecToStr had invalid entries.

  • Codec table had invalid mapping cbor0x71 and protobuf0x70
  • CodecToStr table had invalid mapping 0x71cbor and 0x70protobuf

In my mind, this is not something we can "silently fix" like you did in #142.
Silently changing mappings in #142 introduced silent breaking changes in disguise of "deprecation".

My position is:

  • Invalid mapping in go-cid means downstream software could be producing invalid CIDs and DAGs and we would not even know its happening.
  • Given we talk about content-addressing software stack, this is not something one deprecates, this is a critical bug that had to be fixed ASAP.
    • Silently fixing and changing mapping is as bad or worse than having invalid one.
      Removal and forcing refactor is the only safe way.

Digression about work in Q1 and potential shortcuts:

I am deeply aware how core go-cid is, and how painful bubbling this up is 😿 Kubo team went over this in IPFS in Q1: had to open multiple surgical PRs to fix this mess in Kubo 0.13.0 and go-libp2p, and change many commands in a way that does not silently break existing users (see summary of BREAKING CHANGES in release notes).

Note the strategy around #137 was to replace use of Codecs and CodecToStr in places that are not impacted by invalid mappings WITHOUT bumping go-cid (e.g., libp2p/go-libp2p-core#240). The actual go.mod bump happened in go-libp2p 0.21, two months later.

Initially, we only bumped go-cid to v0.2.0 in Kubo to force refactor of impacted code paths and redesign of end user commands. Sure, it is a lot of tedious work, but the end result in Kubo 0.13.0 avoided exactly the type of silent data corruption described in my comment at the top (and briefly re-introduced in v0.3.1).

This was done months ago. Since then, unrelated refactors of go-libp2p landed (such as repo consolidation), and now you have even more technical debt to pay off.

You could re-introduce Codecs AND CodecToStr and then manually censor tables and remove entries with invalid mappings. This will ensure go-cid won't produce invalid data, and save you from bubbling up refactors, but will make also make some apps error randomly on cbor/protobuf/dag-pb.

I did not go that route because it felt like dealing with the fallout will be more work than doing things properly and refactoring away from go-cid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization P0 Critical: Tackled by core team ASAP
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants