rossfuhrman (bb1ee550) at 19 Mar 23:48

rossfuhrman (2f17315a) at 19 Mar 23:01

@m-omokoh As promised, here are some thoughts. Would love to hear thoughts/feedback on this, and I'm happy to move to an issue if that will make discussion easier.

cc @mfangman @or-gal @nrotaru @gkatz1

Security inventory analyzers only change status based on default branch pipelines Vulnerabilities only show up in the dashboard for default branch pipelines

I wonder if Security inventory and Vulnerability Dashboards tracking default branch pipelines vs. Scan profiles tracking all pipelines will lead to a confusing user experience? I imagine some typical workflows might involve bouncing back and forth between those three areas of the product.

On the Source field - I think showing the scan trigger (Push Protection, MR Pipelines, Branch Pipelines) is meaningful for users.

• ❌ Push Protection: this is not a typical analyzer and won't ever be represented with this screen, as far as I understand things, or at least will be represented quite differently (e.g. no CI job to link to)
• ❓ MR Pipelines: Scan profile jobs will be injected for this, but we would not track these with current backend proposal, meaning a change in status for an MR initiated scan would never change the status of the analyzer
• ✅ Branch Pipelines - from default branch: this will be tracked, and the current backend proposal would only track this
• ❌ Branch Pipelines - from non-default branch: this won't ever be a source until we support branch pipeline triggers. we currently only inject Scan profile jobs into MR pipelines and default branch pipelines

These are separate surfaces and we shouldn't assume they need to track the same things.

Agreed! And this is why I brought it up. I don't think anything in the current backend proposal accounts for tracking the status of kinds of pipelines that we weren't already tracking for Security inventory. If we need to track MR pipelines in a way that will be represented in the Scan profile view for projects, we will need to re-evaluate the backend approach.

• The source that we are tracking for a given job needs to be "Scan profile" in order to tie the job to Scan profiles.
  • An example YAML-driven SD job https://gitlab.com/gitlab-org/gitlab/-/jobs/13527623890 has a Source like this: "Source: Merge Request"
  • This job view for a Scan profile injected job would have a Source like this: "Source: Scan profile"
  • The job has a pipeline, and that pipeline maintains the original source for the pipeline (Merge request, Push, etc.)
• Doing so would require a significant increase in the amount of pipelines we would be processing.
• Changing the status of an analyzer based off of every pipeline that runs could definitely lead to a lot of thrashing for the status.
• Who will be using this info for troubleshooting? What value does changing status based on an MR pipeline provide? One consideration is that if pipelines are failing widely across an Instance (GitLab.com, Self-hosted, etc.), they will be failing for MR pipelines as well as default branch pipelines.

Using gitlab-org/gitlab as an example:

• roughly ~50–100 default branch pipelines per business day
• roughly ~500 non-default branch pipelines (mostly MR pipelines) per business day Sources: !49014 (merged) and !32171 (merged) Note: these rough numbers are at least 5 years old, and are most likely even higher today.

My proposal would be

• Do not track non-default branch pipelines at this time
• Do not show the source at this time, as it would only ever be "Scan profile" under this proposal
• Consider evaluating performance impact of processing 5-10x more pipelines
• Consider evaluating ways to maintain tracking pipeline source (merge request, branch pipeline, etc.) separate from pipeline injection source (Scan profile, Security policy, YAML)

@nrotaru I think this looks very good, but I did have a few questions/comments for you. 🏓

Apologies if I missed the discussion, but am I understanding this table will have a 1:1 relationship with security_scan_profile_projects records?

And if so, did we discuss/consider adding the necessary columns to security_scan_profile_projects, as opposed to creating a new table?

The related security scan_profile has a scan_type. Do we need to duplicate that here?

(question/nitpick, non-blocking) I would expect stale? would only need to consider last_scan_at and not be concerned with not_configured?.

I think a scan can be stale regardless of whether the profile is configured or not.

Either way, if we need to discuss further, this can be tweaked in follow-up MRs.

There are a lot of changes here that should not be included in this MR.

Thanks @vpedak1, LGTM!

@gkatz1 Can you take the Maintainer review? Thanks!

What does this MR do and why?

Fix feature category in the metrics related to Security Scan Profiles

Earlier we identified that scan profiles related components were incorrectly assigned wrong feature category ( #592069 ). This commit assigns the correct feature category to the corresponding metrics.

Fixes: #592069

Discussion: !227536 (comment 3164478674)

We have confirmed with product manager and product analyst.

References

Screenshots or screen recordings

N/A non functional maintenance change.

How to set up and validate locally

N/A non functional maintenance change.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

@eugielimpin @ccharnolevsky things look good from the BE perspective. 👍

What does this MR do and why?

Context: Security Managers are allowed to view Security Configuration page but are not allowed to configure Security testing scanners.

This MR updates the Security Configuration (Security testing tab) page to disable buttons that lead to authorization errors when the current user doesn't have the required permissions (e.g. current user is a Security Manager).

References

#589681.

Screenshots or screen recordings

Before	After

How to set up and validate locally

1. Start GDK with support for Security Manager role

   $ export GITLAB_SECURITY_MANAGER_ROLE=true
   $ gdk start

2. Ensure that your instance has an active EE license

3. Login with root then navigate to a project

4. Add another user as a Security Manager

5. Logout then login with the Security Manager user

6. Navigate to the project's Secure -> Security configuration page and verify that buttons to enable scanners are disabled

7. Navigate to the project's parent group's Secure -> Security inventory page, filter for the project, click on the kebab button > Manage security configuration button, and verify that buttons to enable scanners are disabled

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

rossfuhrman (3799fcad) at 18 Mar 18:57

rossfuhrman (527a3bf9) at 18 Mar 18:57

... and 1 more commit

What does this MR do and why?

The scan profile related components were incorrectly categorised under the security_asset_inventories feature category, wheres they should be under the security_testing_configuration.

Fix the assigned feature category for scan profile backend components

• Updated the feature category assignment for all scan profile related backend components from security_asset_inventories to security_testing_configuration to properly reflect their functionality and purpose
• Updated the auto-generated docs by running bundle exec rake gitlab:custom_roles:compile_docs
• Updated workers definitions but running a rake task as linter suggested

Fixes: #592069

References

Screenshots or screen recordings

N/A. It is a maintenance backend change.

How to set up and validate locally

N/A. It is a maintenance backend change.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

@vpedak1 LGTM 🚀