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

Remove duplicate connection that had been used for payloads #896

Closed
jrcheli opened this issue Apr 20, 2022 · 1 comment · Fixed by #912
Closed

Remove duplicate connection that had been used for payloads #896

jrcheli opened this issue Apr 20, 2022 · 1 comment · Fixed by #912
Assignees
Milestone

Comments

@jrcheli
Copy link
Contributor

jrcheli commented Apr 20, 2022

Without a proxy to aggregate connections, AppScope can unintentionally act like a DDOS attack when configured to output data with network sockets, and when the process being scoped is forking/exec'ing child processes by the thousands. In cribl mode, every process that exists makes two outbound connections, one for standard metrics and events, and another for raw payload data.

Right now we don't expect to be sending payload data, so we've made this DDOS issue twice as bad as it needs to be when SCOPE_CRIBL_ENABLE is true.

By "don't expect to be sending payload data", I'm just observing:
o) SCOPE_PAYLOAD_ENABLE is false by default
o) There are three "internally defined" protocol definitions that are set up in src/state.c:initPayloadDetect().
None of these three protocol definitions set this payload flag.
o) Externally, in src/scope.yml, there are no protocol definitions by default. In addition, the payload flag is not set for any protocol definition example supplied in the comments. The payload flag of the protocol definitions are off by default.

Possibile solutions include:

  1. only creating the connection if payloads are configured to be on
  2. never create the payload connection, period
@jrcheli jrcheli added this to the Next Maintenance (1.0.4) milestone Apr 20, 2022
@jrcheli
Copy link
Contributor Author

jrcheli commented Apr 27, 2022

So the least impactful solution seems to be 1. above.
In src/cfgutils.c:initCtl(), it's as simple as changing from
if (cfgLogStreamEnable(cfg)) {
to
if (cfgLogStreamEnable(cfg) && (cfgPayEnable(cfg) || protocolDefinitionsUsePayloads())) {

This handles the possibility that a protocol definition will try to forward on payload data even if cfgPayEnable() is false. This is needed because src/state.c:doProtocol() currently looks like this:

        // Send payloads if enabled globally or by the detected protocol
        if (cfgPayEnable(g_cfg.staticfg)
            || (net && net->protoProtoDef && net->protoProtoDef->payload)) {
            extractPayload(sockfd, net, buf, len, src, dtype);
        }

I chose to add to the existing transport integration test to cover the testing of this.

jrcheli added a commit that referenced this issue Apr 29, 2022
* (#896) Don't create a second SCOPE_CRIBL connection unless payloads could
be sent.

* (#896) Clean up resources at end of new transport integration test case.
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 a pull request may close this issue.

1 participant