[2.0] Remove RubyGems Aggregate & support transitive source pinning by segiddins · Pull Request #5792 · rubygems/bundler

segiddins · 2017-06-20T02:25:13Z

What was the end-user problem that led to this PR?

The problem was that the resolver could resolve specs from any of the sources specified in the Gemfile, even if that source had nothing to do with the spec in question. This was such a large security vulnerability that, when discovered, it warranted a CVE and its own minor release of Bundler.

Closes #3671.
Closes #3696.
Closes #4059.

Was was your diagnosis of the problem?

My diagnosis was that we needed to get rid of the notion of a rubygems aggregate and enforce that specs could only come either from the source they were declared to come from (the top-level source if declared at the top-level of the Gemfile, else a scoped source), or a source that it transitively "inherited" from the gems that required it.

What is your fix for the problem, implemented in this PR?

My fix is to disable multiple top-level sources in the Gemfile, remove the RubyGems aggregate, and filter the sources gems could come from as described above.

Why did you choose this fix out of the possible options?

I chose this fix because it allows doing the filtering in a reasonably performant manner, and refactors the way we handle sources to abstract some of the grossness in such a way that the machinations to make sure that all of the necessary gem info is downloaded is encapsulated into a single method, driven from the definition, rather than being specific to rubygems sources.

See #4714 and #4930 for the prior implementation.

attritionorg · 2017-06-21T01:52:52Z

@segiddins "it warranted a CVE and its own minor release of Bundler" I know the CVE, but don't think I ever saw the versions impacted and the first fixing version. Can you provide that info? Thanks!

segiddins · 2017-06-21T17:08:19Z

but don't think I ever saw the versions impacted and the first fixing version

https://bundler.io/v1.7/whats_new.html

indirect · 2017-06-23T00:55:04Z

@bundlerbot r+

bundlerbot · 2017-06-23T00:55:05Z

📌 Commit 9181c06 has been approved by indirect

segiddins · 2017-06-23T01:15:30Z

@bundlerbot r-
I'd like to get the specs running on 2.0 mode first, if that's ok with you? Since I have a hunch this change in particular might break tests

indirect · 2017-06-23T01:28:12Z

👍

Also update them to modern bundler test syntax

# Conflicts: # spec/install/gemfile/sources_spec.rb

…le_multisource

segiddins · 2017-06-24T00:47:38Z

There's an off-chance this will pass now, so please review :)

indirect

This is looking good, and I'm super happy with the metadata source 🙌🏻

indirect · 2017-06-24T01:57:48Z

lib/bundler/resolver.rb

+              o << "Could not find gem '#{printable_dep(conflict.requirement)}' "
            end
+
+            o << if relevant_sources.empty?


please append inside the branches rather than indenting the whole conditional 👍🏻

segiddins · 2017-06-24T05:24:22Z

Passing for Bundler 1, down to 5 failures for Bundler 2 (I don't know how I keep on missing them locally, I'm sorry)

colby-swandale · 2017-06-24T11:08:30Z

nothing to be sorry about 😄

TimMoore

Phew! Nice work!

segiddins · 2017-06-27T16:09:41Z

Have two 👍, so going to merge this.

@bundlerbot r+

bundlerbot · 2017-06-27T16:09:42Z

📌 Commit 4dcd333 has been approved by segiddins

bundlerbot · 2017-06-27T21:48:44Z

⌛ Testing commit 4dcd333 with merge 87352ed...

[2.0] Remove RubyGems Aggregate & support transitive source pinning ### What was the end-user problem that led to this PR? The problem was that the resolver could resolve specs from _any_ of the sources specified in the Gemfile, even if that source had nothing to do with the spec in question. This was such a large security vulnerability that, when discovered, it warranted a CVE and its own minor release of Bundler. Closes #3671. Closes #3696. Closes #4059. ### Was was your diagnosis of the problem? My diagnosis was that we needed to get rid of the notion of a `rubygems aggregate` and enforce that specs could only come either from the source they were declared to come from (the top-level source if declared at the top-level of the Gemfile, else a scoped source), or a source that it transitively "inherited" from the gems that required it. ### What is your fix for the problem, implemented in this PR? My fix is to disable multiple top-level sources in the Gemfile, remove the RubyGems aggregate, and filter the sources gems could come from as described above. ### Why did you choose this fix out of the possible options? I chose this fix because it allows doing the filtering in a reasonably performant manner, and refactors the way we handle sources to abstract some of the grossness in such a way that the machinations to make sure that all of the necessary gem info is downloaded is encapsulated into a single method, driven from the definition, rather than being specific to rubygems sources. See #4714 and #4930 for the prior implementation.

bundlerbot · 2017-06-27T23:41:29Z

☀️ Test successful - status-travis
Approved by: segiddins
Pushing 87352ed to master...

Benjamin-Dobell · 2018-01-29T07:05:26Z

EDIT: My apologies, I'm running into issues with bundler 1.16.1, so I've removed the specifics of my issue (although related to transitive dependency source resolution). Knowing that I'll be moving to 2.0 once released, then I'd be happy to know the correct syntax to utilise for transitive dependency sources once 2.0 is released.

How would specify the preferred source for transitive dependencies?

e.g.

We have a dependency on some-gem, but some-gem has a dependency on private-gem. How do I ensure that Bundler correctly gets private-gem from my private Gem repository and not Rubygems?

Would something like the following work...?

source 'https://rubygems.org'

source 'https://myprivaterepository' do
  gem 'some-gem', git: 'https://myforkongithub'
end

segiddins requested review from TimMoore, colby-swandale and indirect June 20, 2017 02:25

segiddins force-pushed the seg-remove-rubygems-aggregate branch 2 times, most recently from 96057e7 to 4e27808 Compare June 23, 2017 22:32

segiddins and others added 10 commits June 23, 2017 19:40

Add a feature flag for the lockfile using separate RubyGems sources

93fd982

Failing spec for #3671.

049e700

Put new source pinning specs behind a feature flag

fe7ed25

Also update them to modern bundler test syntax

Add failing test for invalid warning

9c8dab3

# Conflicts: # spec/install/gemfile/sources_spec.rb

Disable mutisource gemfiles by default on 2.0

1e23246

Implement source pinning for 2.0

358736a

Avoid fetching the full index to get all dependency names

1867eaf

Consolidate the double-checking logic

ebd40b6

Error if lockfile_uses_separate_rubygems_sources is set without disab…

14b8eb6

…le_multisource

Fix plugin sources

f1ffdf6

segiddins force-pushed the seg-remove-rubygems-aggregate branch from 4e27808 to f243da6 Compare June 24, 2017 00:40

indirect approved these changes Jun 24, 2017

View reviewed changes

segiddins added 4 commits June 23, 2017 23:29

Get the Bundler 2 specs passing with transitive source pinning

6b40a8a

Always serve the Bundler gemspec from the metadata source

0f336c7

Fix plugin installation when the plugin depends upon Bundler

a415012

Only limit to 25 failures on CI

3497223

segiddins force-pushed the seg-remove-rubygems-aggregate branch from f243da6 to 4b0db68 Compare June 24, 2017 04:29

TimMoore approved these changes Jun 24, 2017

View reviewed changes

More bundler 2 spec fixes

4dcd333

segiddins force-pushed the seg-remove-rubygems-aggregate branch from 4b0db68 to 4dcd333 Compare June 27, 2017 15:20

bundlerbot merged commit 4dcd333 into master Jun 27, 2017

bundlerbot mentioned this pull request Jun 27, 2017

[Definition] Remove no-op code in expanded_deps #5387

Closed

segiddins deleted the seg-remove-rubygems-aggregate branch June 28, 2017 09:45

segiddins restored the seg-remove-rubygems-aggregate branch July 1, 2017 15:23

segiddins deleted the seg-remove-rubygems-aggregate branch July 1, 2017 15:23

colby-swandale modified the milestone: 1.16.2 Feb 4, 2018

deivid-rodriguez mentioned this pull request Apr 1, 2019

Remove lockfile incompatibility created by the lockfile_uses_separate_rubygems_sources setting #7007

Merged

Uh oh!

Conversation

segiddins commented Jun 20, 2017

What was the end-user problem that led to this PR?

Was was your diagnosis of the problem?

What is your fix for the problem, implemented in this PR?

Why did you choose this fix out of the possible options?

Uh oh!

attritionorg commented Jun 21, 2017

Uh oh!

segiddins commented Jun 21, 2017

Uh oh!

indirect commented Jun 23, 2017

Uh oh!

bundlerbot commented Jun 23, 2017

Uh oh!

segiddins commented Jun 23, 2017

Uh oh!

indirect commented Jun 23, 2017

Uh oh!

segiddins commented Jun 24, 2017

Uh oh!

indirect left a comment

Choose a reason for hiding this comment

Uh oh!

indirect Jun 24, 2017

Choose a reason for hiding this comment

Uh oh!

segiddins commented Jun 24, 2017

Uh oh!

colby-swandale commented Jun 24, 2017

Uh oh!

TimMoore left a comment

Choose a reason for hiding this comment

Uh oh!

segiddins commented Jun 27, 2017

Uh oh!

bundlerbot commented Jun 27, 2017

Uh oh!

bundlerbot commented Jun 27, 2017

Uh oh!

bundlerbot commented Jun 27, 2017

Uh oh!

Benjamin-Dobell commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Benjamin-Dobell commented Jan 29, 2018 •

edited

Loading