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

feat(appsec): add fingerprints #2955

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

Leiyks
Copy link
Contributor

@Leiyks Leiyks commented Nov 15, 2024

Description

Add support for endpoint, header, network and session fingerprints.

In order to do this, the following changes have been made:

  • Bump ruleset config from 1.10.0 to 1.13.2
  • Add unitary and integration tests to make sure fingerprint are generated and propagated for each request type
  • Clean variable names in the helper

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

Related Jiras: APPSEC-55148

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.97%. Comparing base (3abddef) to head (fcd4b25).
Report is 21 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2955      +/-   ##
============================================
+ Coverage     72.46%   73.97%   +1.51%     
  Complexity     2527     2527              
============================================
  Files           135      108      -27     
  Lines         14402    10360    -4042     
  Branches        991        0     -991     
============================================
- Hits          10436     7664    -2772     
+ Misses         3422     2696     -726     
+ Partials        544        0     -544     
Flag Coverage Δ
appsec-extension ?
tracer-php 73.97% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 27 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3abddef...fcd4b25. Read the comment docs.

---- 🚨 Try these New Features:

@Leiyks Leiyks changed the title feat(appsec): add fingerprint feat(appsec): add endpoint fingerprint Nov 18, 2024
@Leiyks Leiyks force-pushed the leiyks/add-fingerprinting branch 2 times, most recently from b3e4518 to 61c0df6 Compare November 18, 2024 15:33
Signed-off-by: Alexandre Rulleau <[email protected]>
@Leiyks Leiyks marked this pull request as ready for review November 19, 2024 09:54
@Leiyks Leiyks requested a review from a team as a code owner November 19, 2024 09:54
@estringana
Copy link
Contributor

Nice PR @Leiyks 👏 . I didn't find anything major but I left some comments

Signed-off-by: Alexandre Rulleau <[email protected]>
@Leiyks Leiyks changed the title feat(appsec): add endpoint fingerprint feat(appsec): add fingerprints Nov 20, 2024
return tmpl;
}

std::string create_sample_rules_ok_with_fingerprint()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do this need to be outside of the create_sample_rules_ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I separated both configurations because the processors seems to make multiple other tests crash

dynamic_cast<network::request_shutdown::response *>(res.get());
EXPECT_STREQ(msg_res->actions[0].verdict.c_str(), "block");

EXPECT_TRUE(std::regex_match(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can extract all these assertions to a function since they are the same in all the tests

Signed-off-by: Alexandre Rulleau <[email protected]>
appsec/src/helper/subscriber/waf.cpp Outdated Show resolved Hide resolved
EXPECT_TRUE(
std::regex_match(msg_res->meta["_dd.appsec.fp.session"].c_str(),
std::regex("\"ssn(-[a-zA-Z0-9]*){4}\"")));
}
Copy link
Contributor

@cataphract cataphract Nov 20, 2024

Choose a reason for hiding this comment

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

It's not clear the intent of the test here... If it's to test client <-> Server communication, the request lifecycle, the fingerprinting generation in the WAF, the inclusion of the fingerprint gotten from the waf in the final message, and so on.

Instead, it's a quasi integration test with the associated disadvantages (complexity, difficulty to debug) and fewer of the advantages because of all the mock interactions (less realistic). To be fair, it's not really your fault, this whole file is like this, but I would advise that you instead:

  • Write simple unit tests, with few assertions. If you need mocks, especially several of them, reconsider what you're doing.
  • Avoid writing tests that will make difficult future refactoring. Prefer testing through public/stable interfaces.
  • Don't retest WAF functionality in the unit tests.
  • Leave the tests where you're testing a full http request to the integration tests (appsec/tests/integration)

Copy link
Contributor

Choose a reason for hiding this comment

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

The test is validating that fingerprints are forwarded to the extension in request shutdown so from that perspective I don't see an issue with it. What I do miss is a test of the WAF subscriber to validate that the fingerprints are correctly stored and retrievable through the correct API.


EXPECT_TRUE(std::regex_match(
msg_res->meta["_dd.appsec.fp.http.network"].c_str(),
std::regex("\"net-[0-9]*-[a-zA-Z0-9]*\"")));
Copy link
Contributor

Choose a reason for hiding this comment

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

You can already do this in Gtest directly, with something along the lines of:

EXPECT_THAT(value, MatchesRegex("http-get(-[A-Za-z0-9]*){3}"));

More info: https://google.github.io/googletest/reference/matchers.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants