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.

Sign up for GitHub

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

Jump to bottom

Add a force disable of appsec when using Ruby >= 3.3 with old ffi #3969

Merged
merged 3 commits into from
Oct 7, 2024
Merged

Add a force disable of appsec when using Ruby >= 3.3 with old ffi #3969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51a803f
Add a force disable of appsec when using Ruby >= 3.3 with old ffi
y9v Oct 2, 2024
b4f6641
Add guard clause to AppSec::Component#incompatible_ffi_version?
y9v Oct 2, 2024
03f3bc9
Use Gem::Version for comparing Ruby and ffi versions
y9v Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion lib/datadog/appsec/component.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ module AppSec
class Component
class << self
def build_appsec_component(settings, telemetry:)
return unless settings.respond_to?(:appsec) && settings.appsec.enabled
return if !settings.respond_to?(:appsec) || !settings.appsec.enabled
return if incompatible_ffi_version?

processor = create_processor(settings, telemetry)

Expand All @@ -28,6 +29,20 @@ def build_appsec_component(settings, telemetry:)

private

def incompatible_ffi_version?
ffi_version = Gem.loaded_specs['ffi'] && Gem.loaded_specs['ffi'].version
return false unless Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('3.3') &&
ffi_version < Gem::Version.new('1.16.0')
Comment on lines +33 to +35
Copy link
Contributor

@Strech Strech Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how it could be relevant and don't want to over complicate it, but if we check that Gem.loaded_specs['ffi'] not nil, what if it's nil?

Then entire ffi_version is nil, right? And it could fail next check

❯❯❯ irb
irb(main):001> ffi_version = Gem.loaded_specs['ffi'] && Gem.loaded_specs['ffi'].version
=> nil
irb(main):002*  Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('3.3') &&
irb(main):003>             ffi_version < Gem::Version.new('1.16.0')
(irb):3:in `<main>': undefined method `<' for nil (NoMethodError)

            ffi_version < Gem::Version.new('1.16.0')
                        ^
	from <internal:kernel>:187:in `loop'```

But I made it up, maybe it never happen

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will still add a guard clause for this, just in case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although I'm not even sure what to do in this case - we don't want to create a processor, and we probably don't want to use the same warning message. Do we want to silently return true in such case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be fine to just not warn in this situation and carry on to subsequent code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can consider it to be "not there" ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I merged to early. Here is a follow-up PR: #3978


Datadog.logger.warn(
'AppSec is not supported in Ruby versions above 3.3.0 when using `ffi` versions older than 1.16.0, ' \
'and will be forcibly disabled due to a memory leak in `ffi`. ' \
'Please upgrade your `ffi` version to 1.16.0 or higher.'
)

true
end

def create_processor(settings, telemetry)
rules = AppSec::Processor::RuleLoader.load_rules(
telemetry: telemetry,
Expand Down
14 changes: 14 additions & 0 deletions spec/datadog/appsec/component_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,20 @@
expect(component).to be_a(described_class)
end

context 'when using old ffi version with Ruby 3.3.x' do
before do
stub_const('RUBY_VERSION', '3.3.0')
allow(Gem).to receive(:loaded_specs).and_return('ffi' => double(version: Gem::Version.new('1.15.4')))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use 1.9.0 for the FFI version the test will fail right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it is still green:

Gem::Version.new('1.9.0') < '1.16.0'
=> true

I think this is because we are comparing Gem::Version with a String (not sure this will work correctly in older ruby versions though)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can dismantle version on major, minor and test piece-by-piece (just in case)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, https://github.com/rubygems/rubygems/blob/master/lib/rubygems/version.rb#L358, added in rubygems/rubygems@7e0dbb7 2 years ago, which sounds like this functionality is probably not going to exist in Ruby 2.5?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rails/rails#47480 conveniently says that the threshold for this feature is Ruby 3.1.

end

it 'returns a Datadog::AppSec::Component instance with a nil processor' do
expect(Datadog.logger).to receive(:warn)

component = described_class.build_appsec_component(settings, telemetry: telemetry)
expect(component).to be_nil
end
end

context 'when processor is ready' do
it 'returns a Datadog::AppSec::Component with a processor instance' do
expect_any_instance_of(Datadog::AppSec::Processor).to receive(:ready?).and_return(true)
Expand Down
Loading