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

[Serverless] Fix panic when running the extension without appsec enabled. #16054

Merged
merged 3 commits into from
Mar 22, 2023

Conversation

purple4reina
Copy link
Contributor

@purple4reina purple4reina commented Mar 10, 2023

What does this PR do?

This pull request fixes a bug when running a java function in aws lambda without appsec enabled.

The problem stems from the fact that when a go interface can be implemented by a nil pointer, the interface itself is considered not nil. In the example below, the interface io.Reader is implemented in three ways, but only the first of which is considered nil. This is despite the second implementation be a nil pointer, as is the case which triggered the panic this PR addresses.

func main() {
	var r io.Reader
	fmt.Println(r == nil)    // true

	r = (*bytes.Buffer)(nil)
	fmt.Println(r == nil)    // false
	
	r = &bytes.Buffer{}
	fmt.Println(r == nil)    // false
}

Motivation

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Use the major_change label if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.
  • A release note has been added or the changelog/no-changelog label has been applied.
  • Changed code has automated tests for its functionality.
  • Adequate QA/testing plan information is provided if the qa/skip-qa label is not applied.
  • At least one team/.. label has been applied, indicating the team(s) that should QA this change.
  • If applicable, docs team has been notified or an issue has been opened on the documentation repo.
  • If applicable, the need-change/operator and need-change/helm labels have been applied.
  • If applicable, the k8s/<min-version> label, indicating the lowest Kubernetes version compatible with this feature.
  • If applicable, the config template has been updated.

@purple4reina purple4reina added changelog/no-changelog [deprecated] qa/skip-qa - use other qa/ labels [DEPRECATED] Please use qa/done or qa/no-code-change to skip creating a QA card team/serverless labels Mar 10, 2023
@purple4reina purple4reina requested a review from a team as a code owner March 10, 2023 18:52
@purple4reina purple4reina added this to the Triage milestone Mar 10, 2023
@purple4reina purple4reina changed the title [Serverless] [Serverless] Fix panic when running the extension without appsec enabled. Mar 10, 2023
@purple4reina purple4reina force-pushed the rey.abolofia/nil-pointer-interface branch from 6aeaf95 to 5c8f81d Compare March 10, 2023 19:59
Comment on lines +110 to +124
func TestInvocationSubProcessorNilInterface(t *testing.T) {
lp := &invocationlifecycle.LifecycleProcessor{
DetectLambdaLibrary: func() bool { return true },
SubProcessor: (*httpsec.InvocationSubProcessor)(nil),
}

assert.True(t, lp.SubProcessor != nil)

lp.OnInvokeStart(&invocationlifecycle.InvocationStartDetails{
InvokeEventRawPayload: []byte(
`{"requestcontext":{"stage":"purple"},"httpmethod":"purple","resource":"purple"}`),
})

lp.OnInvokeEnd(&invocationlifecycle.InvocationEndDetails{})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest you move this test into the lifecycle processor test suite as this is testing its nil-check is working as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I worked to move this test to the lifecycle processor suite, I hit the import cycle issue. Working around it created a pretty large diff. While I agree this isn't the perfect place for this test, I do think it is the best place for this test.

@purple4reina
Copy link
Contributor Author

purple4reina commented Mar 17, 2023

I've determined that the Serverless Integration Tests failures are also occurring on main and therefore are not caused by this pull request.

We are seeing the following log line for java and dotnet functions:

{
    "level": "ERROR",
    "message": "datadog: Malformed _X_AMZN_TRACE_ID value: Root=1-6414d1a3-4c0e6f6d4dce99ae7c78561a;Parent=69e50da0d91875c2;Sampled=1;Lineage=f1f89451:0"
}

if err != nil {
log.Error("appsec: could not start: ", err)
}
if subProcessor != nil {
appsecSubProcessor = subProcessor
} else if proxySubProcessor != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are both appsecSubProcessor and appsecProxyProcessor supposed to be set? This else if will only get called if subProcessor is not equal to nil and it wasn't clear if that was intended. I'm guessing we use either a subprocessor or proxy, but not both?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you got this right: appsec is either a "proxy subprocessor", or a "regular subprocessor".

@purple4reina purple4reina merged commit 6d1e451 into main Mar 22, 2023
@purple4reina purple4reina deleted the rey.abolofia/nil-pointer-interface branch March 22, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog [deprecated] qa/skip-qa - use other qa/ labels [DEPRECATED] Please use qa/done or qa/no-code-change to skip creating a QA card team/serverless
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants