An OSS tale, or 2

Hi there y'all!

Today, the title of my post is: "An OSS tale, or 2".

If you've been around where I am, or somewhat following the things I've been doing, you may remember a talk I gave where I ran brakeman on some OSS repositories, GitLab included, and compared the results.

I even put together a talk called "Let's Brakeman" and submitted to RubyConf Thailand, but I also submitted a "The World of Passkeys 🤝🏽 Ruby", which ended up being accepted over the brakeman one...

I also managed to inject a console.log into an SVG that got printed out in the browser's console...

But let me get back to the OSS tale of this post!

So, a month or so ago (or maybe more), I started poking around GitLab repo.

If you are not familiar with GitLab, don't worry... I created an account 2 years ago, but only a few months ago I actually started using... To put it in perspective, I created my GitHub account 12 years ago.

I had run 'brakeman' on it and the report was massive... given the repo itself is massive and for many other reasons...

But the GitLab brakeman report had too much stuff to review that I've decided not to try to look at it first.

So, I ran bundler-audit, and the first result: exception! Psych::Disallowed! :-D

Tried to load unspecified class: Date (Psych::DisallowedClass)

First GitLab contribution opportunity: update bundler-audit.

The main issue that was causing the exception was due to an old version of bundler-audit but a newer version of Psych.

If you've been around for a while, it is likely you had to deal with this issue.

You can see in the link https://github.com/rubysec/bundler-audit/issues/302#issuecomment-909362584 that this has been fixed on bundler-audit 0.9.0, and GitLab was still using v0.7.0.1.

Issue found, code changed, problem solved, and here is the link to the MR that got merged: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132470.

It only updates the version in Gemfile, and as a consequence updates Gemfile.lock and Gemfile.checksum.

But it took me a bit to get this merged as I ran into the GitLab learning curve.

So allow me a side note here to talk about what I came to call: The GitLab way!

GitLab's way of contribution isn't the typical fork it, make changes, open a PR...

GitLab has and maintains a Community Fork that you should push your changes to! And if you are paying close attention you may have noticed a reference to MR, instead of PR.

You can still fork it, and make changes, but that isn't the official way and you will run into issues if you intend to make changes and contribute to it (you can find some of those here: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132435 - this was my first MR and first attempt on updating bundler-audit on my own forked repo! :-D).

So, once I changed to using the GitLab Community fork, I was able to push my changes, let CI run, and send the MR for review by the GitLab team.

In total, from my first MR/commit until approval it took about 2 days.

It is worth mentioning that I've spent a few days (an hour here, and hour there...) to be able to have the issue figured out and be in a position to open the MR!

But now I can run bundler-audit successfully! And... And...

And the result? 3 gems with CVE alert: uri, sidekiq, and RedCloth!

Current status (as of 'now' - the time of writing this post): I've managed to open MRs to update both uri and sidekiq! RedCloth is still on the version flagged by bundler-audit as having a CVE (perhaps more details about in the next post)!

The easiest one: uri!

This seemed to me to be the easiest one because it was a minor version update: from v0.12.1 to v0.12.2. And it indeed was easy. No issues during the review process, and it went from MR to approval in about 2 days.

One important detail: these are CVE and security related MRs. So the GitLab AppSec team gets flagged for review, and it usually takes precedence over other things... if it runs CI successfully, if the security scans runs successfully and looks good, the MR is cleared to be merged...

But still small MRs (it changes 1 gem, 1 minor version) gets a lot of eyeballs! That is because the AppSec Team gets tagged, lots of Security Engineers end up checking it out.

Here is the link to the MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132743 .

Noticed that 23 participants were involved (some bots, mostly humans). There is a Sec Bot that triggers internal actions, vulnerability scan results must be checked etc.

Ok, now bundler-audit is good, uri is good, so what is next? RedCloth and Sidekiq!

This was the first time I've heard of RedCloth (which is ok, normal, expected...), but I've heard a dozen times about Sidekiq.

I don't recall now which gem I started looking into first, but as for this post, I will talk first about Sidekiq as this one has been updated already.

GitLab was running sidekiq version 6.5.7, a not too old version in the 6.x series.

At the time I first started looking into this, the bundler-audit suggested solution was to move to the 7.x series.

When I upgraded and ran bundle install, guess what happened? It failed!

It complained about dependency conflict! GitLab uses a gem called gitlab-sidekiq-fetcher that supports only Sidekiq 6.x series.

I will not talk much about this gem, but you can check it out here: https://gitlab.com/gitlab-org/ruby/gems/sidekiq-reliable-fetch.

So, given I needed Sidekiq 7.x (to include the CVE fix) and sidekiq-reliable-fetch only supports 6.x, the only alternative would be to remove the sidekiq-reliable-fetch gem.

I did some research on this gem and started writing up this MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/133042 with the strategy of updating sidekiq to 7.x series and removing sidekiq-reliable-fetch.

With some twists and turns, this MR took 4 weeks to be merged, since the day I created it.

If you are still here with me and plan to continue on reading, I will explain some of the why's it took 4 weeks for this MR to be merged! (remember: the first 2 MRs took 2 days to be merged)

I first created the MR as a draft and I knew the only way out would be to include the GitLab Team and ask tons of questions. Removing a gem that patches sidekiq on a code base as big as GitLab is a lot of work and not very suitable for community/OSS contributions. This usually needs a lot of internal considerations, talking to several people, higher risk of causing unwanted effects, and so on and so forth. I was not very optimistic this would have chances to be merged.

But I dug into the gitlab-reliable-fetcher gem to understand it and found out it was created due to scalability reasons. Under the hood, it uses Redis "rpoplpush".

You can check more about that here: https://github.com/TEA-ebook/sidekiq-reliable-fetch.

I even created an issue on sidekiq-reliable-fetch to support Sidekiq 7.1 (ref: https://gitlab.com/gitlab-org/ruby/gems/sidekiq-reliable-fetch/-/issues/35) but later I found a comment from Mike Perham (creator of Sidekiq) (ref: https://github.com/sidekiq/sidekiq/issues/5788#issuecomment-1736298208) indicating Sidekiq is already using the Redis command that deprecated the "rpoplpush".

To me, that was another reason to deprecate the usage of this gem all together and migrate to Sidekiq 7.1. It was also even more clear this would be a long battle but one worth fighting. Perhaps not me pushing for it, but I could at least raise the questions and let the GitLab Team to prioritize it. I would still be an extra pair of eyes following along with the work and helping where I can, as an OSS contributor.

Migrating sidekiq one major version (from 6 to 7), deprecating an usage of another gem, that uses a deprecated Redis command (this part would actually help accomplish the task), in a repository as big as GitLab, while competing with all other internal priorities sealed the deal for me: this isn't a good work for a community/OSS MR!

I still plan on getting that ball rolling on GitLab: migrate to Sidekiq 7.x series and remove sidekiq-reliable-fetch. But this MR is a "I want to get rid of the CVE", and not make a revolution. I ok if this marks the beginning of one...

And so I've got busy with other things and left the MR in Draft, for a few days.

Until one day that I ran bundler-audit again, for some random reason, and noticed that there was a Sidekiq version on the 6.x series that fixed the CVE! That was just the perfect thing!

First I don't need to open the sidekiq-reliable-fetch Pandora's box, nor discuss whether Sidekiq 7.x suffice for the scalability reasons that made the GitLab team to invest in the sidekiq-reliable-fetch, and this is now a minor version upgrade! I could just upgrade to the latest 6.x and the CVE alert would be cleared.

So I renamed the MR title and the description to indicate that the goal now was to update to the latest 6.x and not 7.x anymore. Created a fresh branch based on the latest root branch, upgraded Sidekiq to latest on 6.x series (including Gemfile.checksum), and (force) pushed the MR upstream: let CI run!

And CI did run... and once it ran... the result?!?! Failed!

I was still confident it would be easier than upgrading to the 7.x series but perhaps not as easy as the uri gem!

While digging through the failures, I found 2 that traces back to a validation like this:

if Gem::Version.new(Sidekiq::VERSION) != Gem::Version.new('6.5.7')
  raise 'New version of sidekiq detected, please remove or update this patch'
end

Basically what this does is to fail CI if you update the Sidekiq version and require the developer to validate whether the sidekiq patches are being affected! It could have fixed the patch or it could have changed the underlying code requiring the patch to evolve.

And there are 2 of those inside the GitLab repository.

After digging into each one of them and I concluded (after perhaps a few hours of work, across a few days, thinking, reading, and reviewing code in Sidekiq and GitLab, and thinking a little more) that it was safe to just update the version of Sidekiq in these patches as well, from 6.5.7 to 6.5.12.

With those changes, the CI ran successfully and all tests were green (or should I say purple?).

Finally, I changed the MR from Draft to Open, tagged it for review, and added comments to these lines indicating I had done some review, but I would like the reviewer to validate my analysis.

You can check all that history here: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/133042.

This MR was merged a few days ago, on October 25th, which means it took 26 days from creation to being approved and merged.

So, this is now scheduled to be release with Gitlab v16.6 on November 10th 2023 (ref: https://gitlab.com/groups/gitlab-org/-/milestones/93#tab-issues).

The intriguing thought in my head now is that if I, and the others reviewed this MR, missed something, in a few days we may have some production issues... which may be traced back to these changes, and to this tale!

If you are still here with me, first are foremost: keep your fingers crossed for me! And don't forget to say Hi on .

Once I was done with sidekiq upgrade, the next on deck is the RedCloth gem! Or should I say RedRedCloth gem?

And that, esteemed gentleperson, is my OSS tale, or 2!

If you liked it, say Hi on or by (the good and old fashioned) ! :-D