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

[IAST] Add manual.keep tag when a vulnerability is reported #3850

Merged
merged 1 commit into from
Sep 23, 2022

Conversation

smola
Copy link
Member

@smola smola commented Sep 16, 2022

What Does This Do

Add manual.keep tag when a vulnerability is reported.

Motivation

Given that this should be relatively infrequent, and is an important event, we want to keep traces when a vulnerability is reported.

Additional Notes

This is analogous to what we do with AppSec threats.

@smola smola added the comp: asm iast Application Security Management (IAST) label Sep 16, 2022
@smola smola requested review from a team September 16, 2022 12:11
reqCtx.getTraceSegment().setDataTop("iast", batch);
final TraceSegment segment = reqCtx.getTraceSegment();
segment.setDataTop("iast", batch);
segment.setTagTop(DDTags.MANUAL_KEEP, true);

Choose a reason for hiding this comment

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

At some point we have to be careful regarding when to set the manual.keep due to increased costs for customers (for instance, we had the idea to only set the flag when a new vulnerability not seen previously pops up)

Copy link
Contributor

@dougqh dougqh Sep 16, 2022

Choose a reason for hiding this comment

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

I think we need to define the semantics here a bit more carefully.

I have two concerns...
1 - There needs to be an indicator of why this is being kept, so that it can show up properly in the ingestion page
2 - We need to decide whether to set this when the trace is already being sampled or not

Since this is a common situation, I think we might want to encapsulate this in a helper method. Otherwise, we'll get the same bug repeated across the different products.

Something like segment.overrideSampling(Product.IAST)

Copy link
Member Author

Choose a reason for hiding this comment

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

@dougqh Noted this for the next guild meeting. Is this a concern before merging this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it's fine to merge this as is for now, maybe with a comment in the code. The AppSec code has the same issue.

@smola smola force-pushed the smola/iast-manual-keep branch from 1bd1766 to 15469d6 Compare September 23, 2022 12:56
@smola smola merged commit 8a7400f into master Sep 23, 2022
@smola smola deleted the smola/iast-manual-keep branch September 23, 2022 14:17
@github-actions github-actions bot added this to the 0.109.0 milestone Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: asm iast Application Security Management (IAST)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants