Skip to content

internal/appsec: request blocking based on user ID #1706

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

Merged
merged 21 commits into from
Feb 16, 2023

Conversation

Hellzy
Copy link
Contributor

@Hellzy Hellzy commented Jan 31, 2023

What does this PR do?

This change adds a new SDK to the appsec top level package, SetUser(). SetUser() wraps the existing tracer.SetUser()
and performs a WAF call to check if the user ID passed to SetUser is on a block list, in which case an error is returned,
instructing the SDK user to block the request execution as soon as possible.

Additionally:

  • A new remote configuration capability is added - ASMUserBlocking, and registered during remote config initialization by appsec
  • TrackUserLoginSuccessEvent() now uses appsec.SetUser() and returns an error if the user should be blocked

Motivation

This is a planned feature.

Describe how to test/QA your changes

The change includes integration tests to test both the high level API usage (in the appsec package) and
the internal mechanics (internal/appsec), as well as the gRPC integration (contrib/google.golang.org/grpc).

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

Sorry, something went wrong.

@Hellzy Hellzy added this to the v1.48.0 milestone Jan 31, 2023
@pr-commenter
Copy link

pr-commenter bot commented Jan 31, 2023

Benchmarks

Comparing candidate commit 8595bb5 in PR branch francois.mazeau/user-blocking with baseline commit af63a73 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 6 cases.

@Hellzy Hellzy force-pushed the francois.mazeau/user-blocking branch 9 times, most recently from 5cde1f1 to bb27190 Compare February 3, 2023 13:11
Copy link
Contributor Author

@Hellzy Hellzy left a comment

Choose a reason for hiding this comment

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

Peer review comments

@Hellzy Hellzy force-pushed the francois.mazeau/user-blocking branch 2 times, most recently from d9fb6dd to c4dbc67 Compare February 13, 2023 10:06
@Hellzy Hellzy marked this pull request as ready for review February 13, 2023 10:35
@Hellzy Hellzy requested review from a team as code owners February 13, 2023 10:35
@Hellzy Hellzy changed the title [WIP] internal/appsec: implement request blocking based on user ID internal/appsec: implement request blocking based on user ID Feb 13, 2023
@Hellzy Hellzy changed the title internal/appsec: implement request blocking based on user ID internal/appsec: request blocking based on user ID Feb 13, 2023
@Hellzy Hellzy force-pushed the francois.mazeau/user-blocking branch from c4dbc67 to 87f3fb6 Compare February 13, 2023 14:34
appsec/appsec.go Outdated
Comment on lines 25 to 26
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a problem: we need to keep our dependency tree as lean as possible.
Here is an example of the resulting issue if not: an HTTP-only service will suddenly depend on gRPC (and all its sub-dependencies).

The reason why this is currently imported isn't valid as we discussed previously: the exact reason of the blocking will have to be split into separate sub-packages of this one (ie. public httpsec and grpcsec packages). But from now on, I suggest you to keep it simple and focused on our main goal here: returning an error without details means blocking.

Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: net/http is considered acceptable because it lives in the standard library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

956681d cleans this up. Everything is abstracted under the error interface and set within grpc/http dependant packages. I also changed the gRPC start operation to directly hold an error rather that a status code since status codes are communicated through errors anyway. The result is much cleaner and simpler - while we wait to refactor the actions system.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
SetUser() can be used to associate a user ID to a tracer.
With the appsec version, an error is returned to indicate
whether requests attributed to this user ID should be blocked.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@Hellzy Hellzy force-pushed the francois.mazeau/user-blocking branch from 03d8b04 to da5abdd Compare February 14, 2023 08:44
@Hellzy Hellzy force-pushed the francois.mazeau/user-blocking branch from bc3da20 to 6fe5fc5 Compare February 14, 2023 09:49
@Hellzy Hellzy force-pushed the francois.mazeau/user-blocking branch from e857a5e to b5b9601 Compare February 14, 2023 15:08
@Hellzy Hellzy requested a review from Julio-Guerra February 15, 2023 08:15
@Hellzy Hellzy force-pushed the francois.mazeau/user-blocking branch from b5b9601 to 88d7881 Compare February 15, 2023 08:19
Co-authored-by: Julio Guerra <julio@datadog.com>
@Hellzy Hellzy requested a review from Julio-Guerra February 16, 2023 09:33
@Hellzy Hellzy merged commit dc290bf into main Feb 16, 2023
@Hellzy Hellzy deleted the francois.mazeau/user-blocking branch February 16, 2023 10:31
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.

None yet

2 participants