We have several security features, and those features create reports for vulnerabilities.
Sometimes there are false positives that create noise in the report. It would be useful to implement a way to dismiss them and save this feedback for later use in signal-to-noise.
If the problem is real, an issue can be created from there to implement a solution.
Proposal
We want the users to have "Dismiss" buttons and record the result in the DB. This will allow us, in future iterations, to suggest a "score" based on such collected statistics. The score could influence the issue rank, and would nuance the data reported by our tools. As @gonzoyumo said, we should gather more data from the user, and understand why the issue has been dismissed. We just want to know it has been dismissed, whatever the reason.
We could also have a button to directly open an issue with the corresponding security alerts. If we are able to link an issue with its resolution (a commit), then it would be valuable for the users as well.
Design
Scope
In scope:
Create new issue
Dismiss/Undismiss vulnerability
Dismissed vulnerabilities are strikethrough and ordered at the bottom of the list
Open modal
Information => See mockups
Project scoped (dismissed vulnerabilities are dismissed for the whole project)
Vulnerabilities can be dismissed from all branches
Masters and developers are able to dismiss vulnerabilities
Out of scope:
Reason for dismissal
We cannot create double issues from vulnerabilities (changes create issue button to go to issue button after issue has already been created)
When user creates an issue he should not be able to dismiss and vice-versa
Better mobile support (as we are using modals)
Hide full path of "file" on item in merge request widget and pipeline detail view. That information will be visible in the modal image
Flows
Dismissal:
User sees vulnerability
User clicks vulnerability title/anchor
Modal opens
User dismisses vulnerability
Modal auto closes
The vulnerability is dismissed
Gets strikethrough styling
Gets reordered to the bottom of the vulnerabilities list
The user who dismissed the vulnerability gets recorded and is displayed in the modal, including the pipeline from which it happened (only shown when opening the vulnerability again
No confirmation message is shown of any kind (keeping it simple and quick)
New issue:
User sees vulnerability
User clicks vulnerability title/anchor
Modal opens
The user creates a new issue
Page changes to new issue creation screen with title and description pre-populated.
User clicks create
Issue is created
The user is on the new issue page
Reverting dismissal of vulnerability:
User sees dismissed vulnerability
User clicks vulnerability title/anchor
Modal opens
User reverts dismissal of vulnerability
Modal auto closes
The vulnerability is undismissed
Undoes strikethrough styling
Gets reordered to the normal position in the vulnerabilities list
The user who dismissed the vulnerability gets deleted. The vulnerability is back as is
No confirmation message is shown of any kind (keeping it simple and quick)
Dismissal/undissal failure:
User dismisses/undismisses vulnerability
Modal does not auto close
The vulnerability is not dismissed/undismissed
Modal gets an additional inline error message stating that the user should try again:
Mockups
Undismissed vulnerability modal:
Dismissed vulnerability modal
Instances box within the modal has a max height and becomes scrollable if it has many items (160 or 240 px max height)
Hide items that are not available to some tests
Learn more links towards documentation (cc: @axil can you provide the link?)
Create issue button will pre-fill the issue description and title with the contents of the vulnerability
Title:
Investigate vulnerability: VULNERABILITY TITLE
Description:
The following vulnerability from PIPELINE should be addressed:- Description: Long text, could be a simple text for now and maybe later improved to be markdown with code formatting. We should be careful on the rendering of this value that can hold new lines.- File: link to repo's file with start + end line (or just start line)- Identifier(s): comma separated list of identifiers and potentially linking to external DB (CVE, CWE, etc.)- Severity: Short text (usually one of: Critical, High, Medium, Low, Unknown)- Solution: Long text, could be a simple text for now and maybe later improved to be markdown with code formatting. We should be careful on the rendering of this value that can hold new lines.- Confidence Level: Short text (usually one of High, Medium, Low, Unknown)- Source(s): a List of URLs pointing to external sources of the vulnerability
DAST modal specifics
before
after
Error specifics:
Merge request widget and pipeline security tab:
Title becomes clickable
Mockup for each report since they are all different?
The whitelisting should be generic, and applied directly on the "final" result, so it is not specific for tools that are getting results.
tools are run as usual
the results from various tools are collected and normalized
entries in the whitelist are removed from the repo, if present
the final report is saved as an artifact
We can define if we want to use a single file for all the whitelists, or one for each feature. The format could be JSON, so it is easy to manage in any language.
For each feature, we should identify the identifier that we want to include in the whitelist, so filtering can be done by that name.
@bikebilly This feels a bit heavy-weight. Editing a file and committing it to source control might be enough of a hurdle that people don't bother, and possibly turn off, or ignore security warnings altogether rather than bother curate it. If the signal-to-noise ratio is low, is there something we can do to improve that SNR itself?
@sytses has suggested making it easy to dismiss a warning directly through the UI, or create an issue to resolve the issue. From that information, we can help tune the perceived value of the warnings, and not only apply that factor to the project, but to other projects.
I don't know how effective that would be. And it could lead to systemic problems where a class of warning is very often useless, but occasionally critical, and with no additional insight, we might mute the warning - creating a false negative.
But still, the potential of something like this, to automatically make the SNR better for all consumers of our security testings seems to have potential.
The challenge is that your proposal seems at odds with the SNR-reduction direction and I fear that if we go down this path, we'll never go down the other, potentially better, path.
One possible way to combine the two, and reduce to a first MVC, is to add the UI to dismiss or triage the warnings directly from the UI, and store that information outside version control. This way it's both easier for the user, and easier for us to experiment with actions on the aggregated information (for .com at least, unless we also report this information back from self-hosted installs).
One possible way to combine the two, and reduce to a first MVC, is to add the UI to dismiss or triage the warnings directly from the UI, and store that information outside version control.
@markpundsack But sometimes a security alert can be closed because a workaround has been implemented in the source code. That's why the "whitelist" needs to be under version control.
So I would proceed in two iterations:
introduce the "whitelist" file above-mentioned
make it possible to close/acknowledge security alerts from the UI with the side-effect of updating the "whitelist" file
GitLab would automatically create a MR when the user close security alerts from the UI. That may look like a heavy process but in many cases the changes need to be discussed and reviewed so the MR is a perfect fit.
I find the idea of keeping the whitelist under version control a good thing too. And later the convenience of an UI to update it. +1 to @fcatteau 's proposal.
@markpundsack it would handle the case where there are different branches of a project, possibly long term like release branches or two stable major versions. We certainly will have to store the whitelist per branch otherwise in whatever storage we'll use, that looks like equally viable but more work imho. Plus we lose the MR on whitelisting, which looks nice to have for big teams.
@markpundsack Let's consider the scenario where a project is vulnerable via one of its dependencies, and that's it's not possible to upgrade the affected dependency to a fixed version. In that case, the developers would create a branch to implement some workaround (described in the security advisory), create a MR, test the workaround and make sure it doesn't introduce any regressions. The security alert would be added to the whitelist in the MR (where the workaround is implemented) but not in the target branch (which is still vulnerable).
I understand the reasons in favor of having the whitelist available as a file in the repo that simply will "filter away" security warnings from the final report. I also see it may impact on the "signal to noise" proposal.
So my question now is: if we implement this (still to be defined), how can we guarantee that we will still be able to get "signal to noise" information from the feedback users will give?
I don't think we should be against the whitelist approach in general. We should avoid it if it creates more problems than benefits. Problems include also that users could abuse the feature and impact on related ones like signal to noise.
I think whitelisting and SNR improvement are aiming two different goals:
whitelisting is in favour of a specific user, for a short term gain. It won't benefit from our potential crowding power and let the user doing both the flagging of contextually fixed vulns and false positives on his own. This is a borring but effective solution.
SNR improvement via feedback is in favour of the whole community of our users, for a long term gain but won't fill the whitelisting needs. As already stated, pushing too far SNR improvement could bring false negatives, reducing the value of that feature.
I would consider specific user needs (contextual whitelisting) as a priority as it directly impacts the usability of Security features. Then we can iterate to leverage our community potential to increase SNR via feeback.
To avoid merging both concepts, we could type the vulnerability "flagging" made by the user.
graph LR A[Dismiss vulnerability] --> C{Why?} C --> D[Fixed with a workaround] C --> E[Doesn't apply to me] C --> F[...] D -.-> D2[`contextual-fix`, ignored by SNR improvement] E -.-> E2[`false-positive`, helps improving SNR]
Under the hood, the result will be the same for the current user who made that action: the vulnerability no longer appears for that project (that could be discussed later to have a more fine grained action e.g. apply to this occurence only, apply to all occurrences, apply to all projects in group/instance, etc.). Then the feedback could be sent to our central server and depending of it's type it could go into our SNR improvement process or not.
That way we ensure we are satisfying the user as he will have immediate benefit from his action and he will also help the whole community in the long term by allowing us to gather that feedback.
The SNR improvement could then take the form of a metric applied to vulnerabilities to display them at a lower position in the list and indicate that this issue is likely to be a false positive because x% of affected users say so. It would still be the responsability of the user to effectively flag this issue as non relevant for this particular project. For users that really trust community and want to go further we could also provide a threshold setting to automatically hide vulnerabilities reaching a defined limit (Like CONFIDENCE_LEVEL does for Static Analysis).
That being said I think we would greatly benefit from having that feature's data stored in DB instead of files, as it would give us way more flexibility like:
ability to migrate data as the model evolves (using files would require to involve ours users to updates them and generate a lot of "git noise")
cascading (to helps big companies handling group or instance wide vulnerability flagging)
Regarding the loss of ability to discuss it using MR and long lived branches support that brings the file storage approach... well we probably could work on that later and does it weight more than the aforementioned advantages?
e.g. we could integrate the discussion in the MR by allowing user to start a discussion that allows threaded comments and prevent merge until it is resolved.
I agree with @gonzoyumo, whitelisting and SNR reduction are 2 different things. Let's focus on the first in this issue, and create another for the later.
Having a file in the repo makes sense, and will save us a lot of time with the UI. This kind of change (whitelisting a security issue) must be reviewed, validated, tracked, and made auditable. The repo and MRs are a perfect place for that.
I also agree with @bikebilly that we should have an agnostic file, which will be compatible by default with every tool because we just filter the output. I suggest using .gitlab-sec.ignore or something similar.
Having the ability to ignore issues directly in the UI is nice-to-have for now I think, and we can easily iterate on this: These links would stack issues to whitelist, and once the review is done, a new MR is created. The target branch is the current branch (MR, or the one from CI view). This will allow users to review issues in a clean process.
Now that we have the base to start, we need to resolve how we're going to identify the issues?
SAST: we can use the fingerprint provided by the tools. If they don't provide a fingerprint, we will need to forge one (as the impacted line in the code may move in the future).
Dependency Scanning: the CVE will be enough. If the issue doesn't have an identifier, we need to create one.
Container Scanning: we can use CVEs, as all issues are already clearly identified.
DAST: We will probably have to create a fingerprint based on the message + url
If we're ok with this implementation, and we want an mvp, we will need a way to explain to the user what to add to the file (ie: CVEs or Fingerprints). I'm afraid that if we leave this part "as it", with just a doc to explain how to add issues, people will run away from the security features like @markpundsack mentioned.
@dimitrieh, @filipa, do you think we can find an implementation that would fit into 10.7?
Suggestions:
link "Dismiss" next to the issue, on hover of line. It expands a code area, with the snippet to copy paste to the file
checkboxes "Dismiss" next to the issue, on hover of line. It would add the snippet to a code area, with all other items to whitelist. The user has to copy paste to .gitlab-sec.ignore.
Ok, thanks for the heads-up. Let’s start with the backend ready, and we’ll see from there.
I’m wondering about the file naming/purpose. Maybe we should make it even more generic, to handle future configurations for security products.
“Ignore” would be the first key supported, and we leave space for future ones.
I think we should think a little further. At one point, we will need to let the user specify some configuration.
Ignoring paths and issues is part of this configuration. The file would be more generic then, with sections for each security product:
sast:ignore:paths:-tmp-node_modules-vendorissues:-fingerprint:634721d9da222050d41dce164d9de35fe475aa7anote:Not applying to us-fingerprint:aa3687fbc2e4bc07e07d06240b67d199e21e0d45note:False positivedeps:[...]
As we can see here, it's really hard to follow what has been whitelisted. Using something else then fingerprint will lead to other errors.
Note: (brakeman scanner)
The warning fingerprint is a SHA256 hash of five values:
Another advantage of this implementation: the configuration variables like SAST_CONFIDENCE_LEVEL away from the job definition (remember we want to make it includable). We could avoid the SAST_ prefix, because they would directly inside the sast namespace of the configuration file.
We also have the need for this kind of file for dependency scanning.
And we're going to use this file for license checks (gitlab-org/gitlab-ee#1125) as well, so the file name should be even more generic (.gitlab-checks.yml? or maybe directly in .gitlab-ci.yml...).
As discussed with @plafoucriere, let's spend some additional time to discuss on this topic. Since we also unlikely have resources available to deliver an MVP, I'm postponing it to %10.8.
@gonzoyumo Referring to your chart, "doesn't apply to me" does not necessarily imply this is a false positive. It's possible that the vulnerability does not apply because the project doesn't match the conditions listed in "impacted" (if any). For instance, let's consider the case where the advisory applies to Windows environments only (as mentioned in the "impacted") and the project runs on a Linux environment. It doesn't apply but the advisory is totally valid and shouldn't be silenced. Ideally we'd be able to detect that it doesn't apply but I'm convinced this is quite expensive and not always possible.
By the way, we can raise a special warning when the "impacted" field is not empty, saying that "it may not apply to your project".
Then the feedback could be sent to our central server and depending of it's type it could go into our SNR improvement process or not.
@gonzoyumo Users should be able to disable this feature. To me this is similar to sending anonymous data to some central server when a software crashes (for instance, to Apple when something crashes on OS X). I suggest the is enabled by default.
The SNR improvement could then take the form of a metric applied to vulnerabilities to display them at a lower position in the list and indicate that this issue is likely to be a false positive because x% of affected users say so.
@gonzoyumo This percentage is irrelevant if we haven't collected enough data. We shouldn't use the information if the sample is too small, so we need to investigate on the required sample size. By the way, it's possible that in some cases the sample never gets big enough even if this is a false positive; if the sample is small then the noise doesn't impact many users (probably).
Wording is so important regarding these alerts "that are likely to be false positives". I recommend something like "This alert has been silenced by a large number of projects" because it's factual and not misleading.
Ideally we would write that "this alert has been silenced in a large number similar projects" but then we need a way to compare projects or, more precisely, to measure the distance b/w two projects. This is something we've thought about on when working on Gemnasium. For instance, in the context of Dependency Scanning, we would measure the distance between two sets of dependencies. But I'm getting ahead of myself here. :D cc @plafoucriere
@fcatteau sure, the chart was not meant as a spec, just a demo of how both features could be achieved at once using the same interface. There are of course multiple cases that need to be well defined.
Just to be clear: We don't have "false positives" in our DB. On the other hand, we can have a security advisory that doesn't affect a lot of projects. That doesn't mean we should hide or remove it.
Philippe Lafoucrièrechanged title from Support whitelisting for all the security features to Dismiss issues in security reports
changed title from Support whitelisting for all the security features to Dismiss issues in security reports
Following discussions with @sytses, and later @bikebilly, I'm updating the title of this issue to reflect our vision.
We should not use "whitelisting", where users would just copy/paste or include large lists from somewhere else.
Instead, we want the users to have "Dismiss" buttons and record the result in the DB. This will allow us, in future iterations, to suggest a "score" based on such collected statistics. The score could influence the issue rank, and would nuance the data reported by our tools. As @gonzoyumo said, we should gather more data from the user, and understand why the issue has been dismissed.
We could also have a button to directly open an issue with the corresponding security alerts. If we are able to link an issue with its resolution (a commit), then it would be valuable for the users as well. But let's create a first mvp with the Dismiss button.
Yet, the configuration file for security products seems to be necessary for the future, I will open another issue for that, since it's no more related to this one.
We could also have a button to directly open an issue with the corresponding security alerts. If we are able to link an issue with its resolution (a commit), then it would be valuable for the users as well. But let's create a first mvp with the Dismiss button.
@plafoucriere Do you mean that users could report a security issue when spotting some vulnerability in the source code of a GitLab project, and that we would then create or pre-fill a package advisory if we know that the repo hosts the source code of one particular package? Or is it something else? Do we have an open issue for this?
Dismiss issues in security reports
This new title is indeed better and more user oriented! Great!
One thing that may be harder to achieve with the DB storage is where to apply the dismissal.
Using a file it's obvious that we dismiss it in every branches the file is present and contains that dismissal, and it's automatically handled when merging branches or cherry-picking. Example workflow:
add a commit to the branch with a workaround or a patch to fix a vuln
dismiss the vuln in the branch using UI or committing the adding in the dismiss file (other branches are still affected by this vuln at that point)
merge the branch into master => master is no longer affected by this vuln
every branches rebased on latest master will have the vuln dismissed too
How can we achieve such a natural and obvious workflow with DB storage? I'm not aware of how much we can keep track of where a dismissal is propagated as merges and cherry-picking are done using DB instead of Git.
BTW the simple way would be to apply the dismissal to the whole project but that may generate false negatives for other long lived branches where the fix is not merged.
@bikebilly + @filipa: While scanning quickly over this issue, are we actually there yet that we have for the frontend full actionable items at the beginning of the 10.8 cycle or will this issue need more planning/decision time and then make just a UX deliverable and/or backend out of it for now? But I could be also mistaken if yes, please @filipa do an weight estimation.
@timzallmann I believe this still misses a lot of UX work and product defintion, from what I can see in the description only a general idea is proposed. There seems to be some backend work already done.
Regarding the weight, if I understood it correctly we only need a button to whitelist each issue. If it's only that, it seems small, a 4
Dependending on UX and the states we want to show it might be bigger.
I can't tell before having UX.
Discussed with @bikebilly, as the direction changed and UX is working now on the new proposal and as then there is a lot of backend work that needs to be done with currently unclear requirements we are putting this on hold for the frontend.
@dimitrieh do you think you can have a design for the MVP of this issue ready by the end of this week? This is quite important to allow enough time for backend and frontend implementation.
Let me know if you need more information by me. Thanks!
As for the solution (first concept, so would love some feedback):
Replace any existing whitelisting functionality, as to not confuse (can that be done with container vulnerabilities?)
Dismissal of security features should span across branches, as to not repeatedly have to dismiss a security vulnerability (it should either be a vulnerability to be fixed or not, let me know if this is inline with your thoughts)
Undo of dismissal should be possible, therefore requiring a project-wide dashboard (in line with our 2018 vision)
Should this feature be accessible from mobile? If so we need to think of having a vulnerability detail view, where further actions can be taken. (let's not dillute the experience to much with always visible buttons)
Within merge request widget and pipeline detail views:
Icon Button appears on hover (not accessible for mobile)
Dashboard:
Can live under CI/CD or Overview
Includes vulnerability report on protected branches (we can perhaps separate this out to another issue, but would love some thoughts on this)
Slightly off topic, but in the first image above, how is the user supposed to tell the difference between the two identically named vulnerabilities? Would dismissing one, dismiss the other (by accident)? What record information do we store with the dismissal? Presumably it can't just be the vulnerability text, or even category, as each occurrence is actually unique, right?
@markpundsack This will depend on how the vulnerability is uniquely identified across different executions of the tool. This may vary with the category of the issue (SAST, Dependency Scanning, Container Scanning, etc...) So we have to work on this identification in this feature too. What's problematic is that there is currently no way to visually distinguish multiple occurrences for some issues. This was supposed to be covered with https://gitlab.com/gitlab-org/gitlab-ee/issues/5043 but it's been delayed... I'll try to put the most necessary things here to reduce that issue.
What I am concerned about is that with this proposal there is no way to distinguish the reason why the user want to dismiss an issue. Even for a first iteration I think we could at least suggest some choices (fixed, not affected, etc...) Otherwise this won't serve the broader goal of SNR improvement.
The eye icon is not clear to me. Security is something that we needs to be very clear to the users. Should we use text instead of icons?
I agree. If that helps, here is what we provided at Gemnasium for a similar feature:
The mute action was keeping the alert open but stopped notifications. The close button is more equivalent to the dismiss feature we're implementing here.
Gemnasium was sending a scheduled reminder for all open alerts (on top of instant notification). The goal of Mute was to avoid spamming when you acknowledged you're affected but consider you still have time to fix it and don't want to be repeatedly reminded about it. Still, you don't want to close it as you're still affected.
Slightly off topic, but in the first image above, how is the user supposed to tell the difference between the two identically named vulnerabilities? Would dismissing one, dismiss the other (by accident)? What record information do we store with the dismissal? Presumably it can't just be the vulnerability text, or even category, as each occurrence is actually unique, right?
@markpundsack I was asking this question in chat, but apparently there is no uniquely identifiable information unless we add something like file/line specific information which is based on the branch.
What I am concerned about is that with this proposal there is no way to distinguish the reason why the user want to dismiss an issue. Even for a first iteration I think we could at least suggest some choices (fixed, not affected, etc...) Otherwise this won't serve the broader goal of SNR improvement.
@markpundsack@gonzoyumo I could look into this. My thinking here would be a modal that pops up where the user can input/select the reason. In turn this can be displayed in the table. wdyt?
@filipa what about the whitelisting on container testing?
just an heads up: Should we keep consistency between CI view and MR widget?
Yes this is my intention
The eye icon is not clear to me. Security is something that we needs to be very clear to the users. Should we use text instead of icons?
My intention here is to be clear, but also mindful of space while not diluting the visuals too much. In that way I think an icon would be best...
The tooltip should make it a little clearer though.
Perhaps @hazelyang could assist me here, what she thinks would be a better solution or icon. (We need some way of dismissing individual vulnerabilities inline in the merge request widget or pipeline security detail view. The current idea is to have on hover an icon appear where the user can click on.)
The eye icon is not clear to me. Security is something that we needs to be very clear to the users. Should we use text instead of icons?
@filipa@gonzoyumo this is why I suggested only showing the one from which we have dismissed it the first time.
Another solution could be to list all the branches on which that security vulnerability has been dismissed, but that has other problems including can we pair them? what about listing the reason for vulnerabilities... etc
Perhaps @hazelyang could assist me here, what she thinks would be a better solution or icon. (We need some way of dismissing individual vulnerabilities inline in the merge request widget or pipeline security detail view. The current idea is to have on hover an icon appear where the user can click on.)
Dismissal of security features should span across branches, as to not repeatedly have to dismiss a security vulnerability (it should either be a vulnerability to be fixed or not, let me know if this is inline with your thoughts)
I disagree with the reasoning behind this choice. We can go this way as a first iteration but we should keep in mind that this is not the best behavior and that it won't suit for all cases. Please read https://gitlab.com/gitlab-org/gitlab-ee/issues/5151#note_66192102 for a more detailed explanation.
Also @plafoucriere or @bikebilly please reflect the final choice in the issue description to ensure we're all on the same page on this point.
showing the branch where the issue was dismissed is something I never thought about. Could be interesting but how will that behave when the branch is deleted after the MR is merged? There won't be any link so we'll just keep the branch name as plain text?
No dashboard, just dismissal of the security problems on the current branch
Description of vulnerability becomes a clickable link, which summons a modal
System note, telling who dismissed what and when
The vulnerability which is whitelisted stays visible, moves to the bottom of this list, and gets strikethrough styling, plus has the reason for dismissal notated after it.
Whitelisted vulnerabilities are no longer counted against the total, but are still shown in the list (At the bottom)
Whitelisted vulnerabilities are still to be inspected and can be un-dismissed through the modal as well
I would rather use a dropdown list of choices instead of a free text field for the Reason of dismissal as it will be too hard to work on such unstructured data. We could extend the list at will if other reasons show up.
I also like the ability to easily enrich the data we provide to the user for a vulnerability. This will allow us to maintain a common basic display and have extended and specific data for each Category of vulnerabilities.
To clean up a bit the UI we could also hide the dismissed issues by default and allow to expand the list like we do today with the Show full list of vulnerabilities link. Though this may complexify things a bit.
I prefer this version too.
Anyway, I see two issues here:
1- "dismissal of the security problems on the current branch" -> I really don't see how we can manage branches at this point. Even if we store information about the commit or the branch, the backend doesn't have any idea of the git structure. That means if the user dismisses an issue in a branch, it will still be reported in the target branch and all other branches checked out from that point. (that's why we wanted a file in the repo in the first place). So the option I see is to dismiss issues based on their identifier for the whole project, and all branches at once.
On the other hand, not having support for branches will prevent us from adding it in the future (if we don't store the information now, we won't be able to migrate it later, in another iteration). Also, this prevents the user from doing what @gonzoyumo explained here: https://gitlab.com/gitlab-org/gitlab-ee/issues/5151#note_66192102@dzaporozhets do you see a solution here?
@bikebilly what is your point of view regarding branches?
2- As Olivier said, we should have a dropdown list for the Reason of dismissal otherwise we won't be able to extract information from that data.
So the option I see is to dismiss issues based on their identifier for the whole project, and all branches at once.
Yes, I agree here. It makes no sense to dismiss something on one branch only. Ex. I dismissed vulnerability in branch feature but once it merged into master it appears again so efforts are wasted.
As Olivier said, we should have a dropdown list for the Reason of dismissal otherwise we won't be able to extract information from that data.
@plafoucriere what do you want to do with this information that you need it to be limited to few options?
I'd like to go back to the most complex use cases but only to suggest something quite simple (my response to Philippe's comment).
It makes no sense to dismiss something on one branch only. Ex. I dismissed vulnerability in branch feature but once it merged into master it appears again so efforts are wasted.
@dzaporozhets Sometimes one particular security advisory can be safely ignored because a workaround has been implemented in one particular commit of one particular branch. It's then safe to ignore the security issue in the next commits. GitLab should propagate the dismissed state through the tree, especially when a feature branch gets merged into master. This is why storing the "dismissed" state in a file would work, even though they are other means to achieve that. Files are not the best way to deal with conflicts anyways.
In some cases, users investigate and eventually come to the conclusion that the security advisory does not apply to their project at all. But it's not always so simple and sometimes the project is so complex or evolves so quickly that we can't be sure about the entire project. In that case, users would probably review one particular commit of a maintained branch and possibly dismiss the security advisory in that particular again. Again, we should propagate the dismissed through the git tree.
Also, users introduce a regression. For instance, it's possible that a workaround to a vulnerability gets removed inadvertently. In that case the "dismissed" state should be removed where the regression is found, and users will dismiss again after fixing the regression in a different commit.
We won't be able to support these use cases in the first iteration but there's something we can do to prepare the future: track the context in which the security advisory has been dismissed.
So the option I see is to dismiss issues based on their identifier for the whole project, and all branches at once.
@plafoucriere I suggest we dismiss security issues globally but track the context (branch & commit) in which they've been dismissed. In a next iteration we would propagate the "dismissed" state following the git tree using that particular context. We would have to explain this new behavior to the users though.
Also, I suggest we keep the list open otherwise it may have a negative impact on the quality of the data we collect. Also, an open list could be a good way to detect new categories we haven't thought of.
@fcatteau yes but for the first iteration, there will be little value in the feature if we don't apply dismiss action across all branches. But I agree with you it's nice to store some context like commit sha.
Collect data in order to better detect false positives?
@fcatteau like if the same vulnerability was dismissed in few projects with identical reason?
@dzaporozhets Agreed! First iteration: collect context (commit SHA) but dismiss security advisory everywhere (all commits of all branches). In the future: propagate "dismissed" state in the git tree. Even better: make it possible to "confirm" a security advisory and resolve conflict between "dismissed" and "confirmed" states when merging a branch! But maybe I'm pushing the idea a bit too far.
like if the same vulnerability was dismissed in few projects with identical reason?
@dzaporozhets If the dismissal reason is "does not apply" we can leverage the data and estimate the probability that the vulnerability applies to some other project. But we shouldn't do that if the dismissal reason is "workaround applied" (even though there's probably something else we can do in that case). I won't get into the details here but that's the idea. Also, we'd better collect data right now even if we're not able to use it at the moment.
I would rather use a dropdown list of choices instead of a free text field for the Reason of dismissal as it will be too hard to work on such unstructured data. We could extend the list at will if other reasons show up.
@gonzoyumo This is certainly possible. I was wondering what the preferred method would be. In this case, I would need to know all the possibilities though! :) cc: @plafoucriere
We need a clear table of possible dismiss actions.
I also like the ability to easily enrich the data we provide to the user for a vulnerability. This will allow us to maintain a common basic display and have extended and specific data for each Category of vulnerabilities.
To clean up a bit the UI we could also hide the dismissed issues by default and allow to expand the list like we do today with the Show full list of vulnerabilities link. Though this may complexify things a bit.
@gonzoyumo Certainly within possibility! In the next revision, I'll take a look at this (this has no urgency to it :) ).
So the option I see is to dismiss issues based on their identifier for the whole project, and all branches at once.
Yes, I agree here. It makes no sense to dismiss something on one branch only. Ex. I dismissed vulnerability in branch feature but once it merged into master it appears again so efforts are wasted.
@plafoucriere@dzaporozhets so this would mean that if you dismiss a vulnerability once, it will be dismissed everywhere right?
If that is the case, I wonder what data we want to gather as meta data. For example: Dismiss author, dismiss reason, dismiss identifier in the form of branch/pipeline/commit (is this not possible at all?) and/or point in time, and affected active/protected branches?
We could show this information along with the vulnerability data.
We need a clear table of which data to collect.
I suggest we dismiss security issues globally but track the context (branch & commit) in which they've been dismissed. In a next iteration we would propagate the "dismissed" state following the git tree using that particular context. We would have to explain this new behavior to the users though.
@fcatteau@plafoucriere just a thought here, what if we both had the ability to dismiss for current branch and for the whole project?
@plafoucriere@dzaporozhets@gonzoyumo I am thinking we can also add a create a new issue from vulnerability button inside of the modal . We could in that case automatically create a description for that issue.
After I receive reactions to this comment, I'll begin gathering data and update the description of this issue with a somewhat solid proposal
@fcatteau@dzaporozhets@dimitrieh please keep in mind that storing the commit is not reliable as it may be squashed before or during the merge action. Storing the branch name would be only for informative purpose as once merged there is no way to track it in the git history. We could update all the dismissed vulns when merging and store the latest commit of the branch before the merge. Not sure how difficult/complex this is though.
And BTW we could also allow to create a new discussion directly in the MR, like any diff discussion that would prevent the merge until it is resolved.
that would be really nice
please keep in mind that storing the commit is not reliable as it may be squashed before or during the merge action.
agree but its best we can have for now.
We could update all the dismissed vulns when merging and store the latest commit of the branch before the merge. Not sure how difficult/complex this is though.
It sounds complex enough so I would recommend it for next iteration.
just a thought here, what if we both had the ability to dismiss for current branch and for the whole project?
At least I would do that. Since we can’t manipulate git from the api, we will rely on the branch name.
So just to be clear: An advisory can be dismissed:
Globally: it won't appear anymore in any branch (Since this is impacting also protected branches, I wonder if it could be reserved to masters
Locally: The issue is hidden only for the current branch. Once merged, it will appear again.
The worse scenario we want to avoid: Someone has dismissed an issue somewhere (so, globally). Security Report is all green. Vulnerability just got deployed to production...
Option 2: We can allow dismiss only in master, and it's applied widely to the project. This way, only masters have the rights to manipulate this sensitive list.
I have put this information in the create a new issue from vulnerability issue together with the create sub-discussion from vulnerability
please keep in mind that storing the commit is not reliable as it may be squashed before or during the merge action. Storing the branch name would be only for informative purpose as once merged there is no way to track it in the git history. We could update all the dismissed vulns when merging and store the latest commit of the branch before the merge. Not sure how difficult/complex this is though.
agree but its best we can have for now.
@gonzoyumo@dzaporozhets I think it is most important to have some date/time information.. so we can relate it back to when "it" happened. We could also store the information that a ref commit has been squashed or merged etc
I wonder if it could be reserved to masters
@plafoucriere Perhaps we could introduce the functionality that if a vulnerability gets dismissed on a protected branch, it will be dismissed everywhere automatically. While normal/other branches will only dismiss it for that particular branch.
This would also play into your option 2.
The worse scenario we want to avoid: Someone has dismissed an issue somewhere (so, globally). Security Report is all green. Vulnerability just got deployed to production...
Agreed here! Therefore for this iteration the vulnerability will stay visible as in the mockups (strikethrough). So for now this information will remain visible upon further inspection.
Summary of this discussion
I will conclude the information in a comment in normal discussion thread below: TODO
Another case to consider and which depend on how we handle the branches:
What do we do if the branch is closed without being merged?
Should we consider the dismissal to be permanently attached to the project so that if it pops again in another MR it will still be considered dismissed?
I would also recommend to stop using the word issue as it may be confusing with the existing Issues feature of GitLab. vulnerabiliy occurrence would be more accurate but probably too long, I think vulnerability should be fine.
I have made the following decisions to move this issue forwards. Let me know if something is not possible or you have some serious doubts. I need some additional information presented in the actionable items in order to finalize this proposal
Decisions
Vulnerability will be the leading keyword going forwards
Vulnerabilities can only be dismissed on protected branches
It is possible to dismiss vulnerabilities on merge requests which have a protected branch as the target branch.
As discussed with @sytses yesterday, we won't implement the dropbox [update: "dropdown"] for now.
Either you dismiss a vulnerability or you create an issue (or link to an existing one). We want to keep it very simple (remember the workflow will maybe have to be repeated many times for each vulnerability). @bikebilly FYI
As discussed with @sytses yesterday, we won't implement the dropbox for now.
Either you dismiss a vulnerability or you create an issue (or link to an existing one). We want to keep it very simple (remember the workflow will maybe have to be repeated many times for each vulnerability).
Can you give a little more detail here @plafoucriere? It would be helpful from a UX standpoint to understand/have insight on what was discussed and the reason for this decision. And by dropbox...do you mean Database?
Name of the branch where the dismissal was done (source branch for MR view, pipeline's ref for CI view)
category
enum
Identify SP feature. Can be one of [sast, dependency_scanning, container_scanning, dast, codequality]
fingerprint
text
Generated by SP feature tools (sast, dependency scanning, etc.). Ensure unique identification of a vulnerability occurrence across executions. Must be used to match with the content of json report artifacts
A Dismissal must:
have these properties filled in
have a unique fingerprint for a given project and category
I also thought about these properties but they probably aren't necessary anymore regarding the latest updates of the spec:
Property
Type
Description
reason
enum
List of possible reasons why the user want to dismiss the vulnerability
vulnerability_data
text
Serialized data of the vulnerability (could be usefull for display purpose or later migration)
vulnerability_data_format_version
string
Format version of the vuln data (allow for evolution as SP tools provide different JSON report)
I already have implemented the create, destroy and index actions for the above described Dismissal ressource:
Action
Url
Method
Params
Create
:project_id/dismissals
POST
QueryString: project_id. Body: [category, branch_name, fingerprint]. (Every other properties are inferred)
Destroy
:project_id/dismissals/:id
DELETE
QueryString: project_id, id (of dismissal to be destroyed)
Index
:project_id/dismissals
GET
QueryString: project_id. We could add an optional category params to ease the fetching for filtering in MR widget.
I would like to discuss with whoever is gonna implement the FrontEnd before going further to ensure I'm providing the correct items at the right place.
@timzallmann will it be @filipa? As she already has a clear understanding of this area it probably would help having this done in time, if she as enough availability of course.
Also, as this is the first feature SP team is implementing in Gitlab Rails Backend it may be a good idea to have someone with good knowlegde of backend and CE/EE split to do a quick pair programming session.
Can you give a little more detail here @plafoucriere? It would be helpful from a UX standpoint to understand/have insight on what was discussed and the reason for this decision.