Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC for bundler checksum verification #50
base: master
Are you sure you want to change the base?
RFC for bundler checksum verification #50
Changes from 5 commits
0029908
3941ef0
5958473
8291281
e713a07
76794b9
7c9b3e8
59224ad
27c23fc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... musing on this a bit, should we instead allow overriding a specific sha in the gemfile? that way, users have a way to still securely pin to a checksum, and only override the single gem, rather than blanket disabling verification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think
gem "foo", checksum_override: "abc123"
is probably the most reasonable way to do this... if we actually want to allow it? I'm not sure if we do.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would have to be a hash, because each platform would have its own checksum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error could output the command to run to ignore this specific gem's mismatch and then we would just record it in the gemfile.lock. I imagine just changing the relevant line in the CHECKSUMS section to
rake (1.0.0) IGNORED
. Then since it's transiently in the lockfile, the next update to a different version would either produce a new error or resolve correctly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we entirely remove the message about disabling the feature and just update the docs?
This would be in addition to adding a command for ignoring a single checksum mismatch, if we decide to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the use case for needing this? Could we instead allow users to vendor the specific gem with this checksum? Having random hardcoded checksums in the Gemfile feels weird. Everybody will need to have access to the source
.gem
for it to work, right? So how come would the mismatch happen?Maybe alternatively (if I understand correctly how this feature would play with vendoring) we could allow users to vendor specific
.gem
files? I think this would be similar to what was requested at rubygems/rubygems#4495?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be printed even if there's a checksum coming from the remote source? if so, that feels like it would get annoying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there's a conflict between storing the checksums from the .gem on install and not installing unverified gems in production when the platform is missing. I think as you commented below, we want to be transparent about this unless the bundle is frozen.
I think we need to add this feature then: If new checksums are recorded during install while the bundle is frozen, then fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, let me refine what I said there, because I don't think it captures the whole picture and I think there are conflicts.
Let's just talk about the frozen bundle situation. Without frozen, installs will never be blocked unless a checksum mismatches.
Here's the bundler docs about frozen:
Given the lock (and assuming these gems are all added to the bundle already, and would install normally before 2.5.0):
When does installing the gem fail during a frozen bundle?
This is a real challenge because this is going to happen to a lot of people:
This means we need to make exceptions for checksums being found from gem installs even when frozen. It could print a warning saying that we ignored missing checksums. (we also then need to distinguish between missing and ignored checksums)
Or we need to say "if you're running in frozen mode, all checksums must be recorded". This means running
bundle lock
before you push and might require that we inform people when we first write the CHECKSUMS section that they need to runlock.
After people are aware of this constraint, they would expect us not to install a gem without checksums during a frozen install. Imposing this constraint immediately is just going to cause a bunch of deployment failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be fixed in we started recording checksums only for fresh lockfiles? That means people with existing lockfiles (and thus everyone using frozen) would not be affected initially. We could also add a
bundle lock --add-checksums
flag for people to opt-in to the feature on an existing lockfile.