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

Roadrunner appsec support #2443

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Roadrunner appsec support #2443

merged 1 commit into from
Jan 23, 2024

Conversation

cataphract
Copy link
Contributor

@cataphract cataphract commented Dec 29, 2023

Description

  • Adds AppSec support for RoadRunner.
  • Adds integration tests to AppSec
  • Adds (limited) ability to suppress function calls, to throw exceptions from advice, and to do recursive calls in advice.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

Tickets

APPSEC-14734 APPSEC-12702 APPSEC-12703 APPSEC-12705

@cataphract cataphract requested review from a team as code owners December 29, 2023 16:56
Base automatically changed from glopes/exec-integration to master December 29, 2023 17:45
ext/hook/uhook.c Outdated Show resolved Hide resolved
ext/hook/uhook.c Outdated Show resolved Hide resolved
ext/hook/uhook.stub.php Outdated Show resolved Hide resolved
ext/hook/uhook.c Outdated Show resolved Hide resolved
ext/ddtrace.c Outdated Show resolved Hide resolved
ext/hook/uhook.c Outdated Show resolved Hide resolved
ext/hook/uhook.c Outdated Show resolved Hide resolved
@cataphract cataphract force-pushed the glopes/roadrunner branch 2 times, most recently from 0e8be64 to 367910f Compare January 10, 2024 09:29
Copy link
Collaborator

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Ack for the uhook changes :-)

Maybe emit a LOG() line if suppressCall is attempted on an internal function, but that's it.

@cataphract cataphract force-pushed the glopes/roadrunner branch 9 times, most recently from fff87b3 to 18bd6b8 Compare January 11, 2024 10:41
ext/user_request.stub.php Outdated Show resolved Hide resolved
ext/user_request.c Outdated Show resolved Hide resolved
ext/user_request.c Outdated Show resolved Hide resolved
Copy link
Contributor

@Anilm3 Anilm3 left a comment

Choose a reason for hiding this comment

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

Still a lot to look through...

appsec/CMakeLists.txt Show resolved Hide resolved
appsec/src/extension/ddappsec.h Show resolved Hide resolved
appsec/src/extension/ddappsec.h Show resolved Hide resolved
appsec/src/extension/ddtrace.c Show resolved Hide resolved
appsec/src/extension/ddappsec.c Show resolved Hide resolved
appsec/src/extension/msgpack_helpers.c Show resolved Hide resolved
DDAPPSEC_G(enabled) = get_DD_APPSEC_ENABLED() ? APPSEC_FULLY_ENABLED
: APPSEC_FULLY_DISABLED;
DDAPPSEC_G(active) = get_DD_APPSEC_ENABLED() ? true : false;
DDAPPSEC_G(to_be_configured) = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure If I missed something but, when appsec is enabled dynamically by remote config, it status can change many times. So saying it does not have to be configure sounds confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"To be configured" means "yet to be configured" (=not configured yet") or "it is to be configured" (=it's arranged/programmed for it to be configured in the future"). The purpose of this setting is just to indicate in MINIT if we got a configuration setting for the state of remote config yet (if, for instance, we can't contact the daemon, then to_be_configured will stay with the value true). In this branch, appsec is explicitly enabled or disabled, so we're never waiting for a setting from remote config. Does this make sense? I also added some comments in bbdb003

@cataphract cataphract force-pushed the glopes/roadrunner branch 3 times, most recently from 37441cf to 1e358a6 Compare January 16, 2024 11:53
@pr-commenter
Copy link

pr-commenter bot commented Jan 16, 2024

Benchmarks

Benchmark execution time: 2024-01-16 18:34:11

Comparing candidate commit 5fa4475 in PR branch glopes/roadrunner with baseline commit 32251e3 in branch master.

Found 2 performance improvements and 1 performance regressions! Performance is the same for 38 metrics, 49 unstable metrics.

scenario:HookBench/benchHookOverheadInstallHookOnFunction

  • 🟥 execution_time [+24.981µs; +57.154µs] or [+3.289%; +7.525%]

scenario:PHPRedisBench/benchRedisBaseline

  • 🟩 execution_time [-351.544µs; -184.114µs] or [-11.649%; -6.101%]

scenario:PHPRedisBench/benchRedisOverhead

  • 🟩 execution_time [-404.658µs; -247.311µs] or [-12.636%; -7.722%]

@cataphract cataphract merged commit c510ee1 into master Jan 23, 2024
622 of 623 checks passed
@cataphract cataphract deleted the glopes/roadrunner branch January 23, 2024 11:00
@github-actions github-actions bot added this to the 0.98.0 milestone Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants