Skip to content
Snippets Groups Projects

Split `Security` into separate contexts for each domain

Closed Brian Williams requested to merge bwill/split-security-into-distinct-contexts into master

What does this MR do and why?

Split Security into separate contexts for each domain

Each feature category inside the Security namespace is a distinct product domain and requires specific domain knowledge. Security is a very broad term which could cover security testing, application security, vulnerability management, scanning, authentication / authorization, and a plethora of other domains. To encourage separation of concerns and avoid unsafe cross-domain usage, we should avoid lumping so many different domains together inside the same bounded context.

MR acceptance checklist

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

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Michał Zając approved this merge request

    approved this merge request

    • :wave: @Quintasan, thanks for approving this merge request.

      This is the first time the merge request has been approved. To ensure we don't only run predictive pipelines, and we don't break master, a new pipeline will be started shortly.

      Please wait for the pipeline to start before resolving this discussion and set auto-merge for the new pipeline. See merging a merge request for more details.

  • added pipelinetier-2 label and removed pipelinetier-1 label

  • This change looks sensible to me as far as I can tell. :thumbsup:

  • Gregory Havenga approved this merge request

    approved this merge request

  • E2E Test Result Summary

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :white_check_mark: test report for 43b19824

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Verify      | 30     | 0      | 2       | 29    | 32    | ✅     |
    | Create      | 121    | 0      | 10      | 93    | 131   | ✅     |
    | Package     | 19     | 0      | 12      | 19    | 31    | ✅     |
    | Plan        | 54     | 0      | 2       | 47    | 56    | ✅     |
    | Govern      | 64     | 0      | 1       | 41    | 65    | ✅     |
    | Analytics   | 1      | 0      | 1       | 0     | 2     | ✅     |
    | Data Stores | 31     | 0      | 0       | 22    | 31    | ✅     |
    | Manage      | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Release     | 5      | 0      | 0       | 5     | 5     | ✅     |
    | Monitor     | 8      | 0      | 0       | 7     | 8     | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 333    | 0      | 29      | 263   | 362   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+

    e2e-package-and-test: :white_check_mark: test report for 43b19824

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Govern      | 129    | 0      | 10      | 1     | 139   | ✅     |
    | Create      | 318    | 0      | 36      | 6     | 354   | ✅     |
    | Monitor     | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Verify      | 8      | 0      | 2       | 0     | 10    | ✅     |
    | Data Stores | 22     | 0      | 0       | 0     | 22    | ✅     |
    | Plan        | 44     | 0      | 4       | 0     | 48    | ✅     |
    | Package     | 6      | 0      | 8       | 0     | 14    | ✅     |
    | Release     | 2      | 0      | 0       | 0     | 2     | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 537    | 0      | 60      | 7     | 597   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    • @bwill From your slack comment:

      I'm curious why there is a dedicated namespace for DependencyManagement while VulnerabilityManagement gets lumped under Security which feels like a kitchen sink due to the number of feature categories under it.

      That's a good question and I can't recall the rationale for how we split those :thinking:. I do think it makes sense to split them out but I'd like to know how we were thinking about this to understand how we ended up here.

      Policies and Vulnerability Management seem fairly distinct although there's some feature-specific overlap like:

      I can also see some upcoming need for unified configuration, such as exploring scan profiles for SAST, similar to those used by DAST. That to me encompasses a larger category like scan execution vs result management (the latter might split b/w vulns and dependencies as you noted)

      The minimal change previously was making Security encapsulate the global groupings but I do agree it's quite broad. We don't need it to be ambiguous but also see some overlap I feel we would lose by over-specifying contexts... I would be curious to get another member of devopssecure to chime in as well.

      @fcatteau you were a member of the WG too, wdyt?

      Edited by Lucas Charles
    • I'm curious why there is a dedicated namespace for DependencyManagement while VulnerabilityManagement gets lumped under Security which feels like a kitchen sink due to the number of feature categories under it.

      I don't know how we ended there. :slight_smile:

      The minimal change previously was making Security encapsulate the global groupings but I do agree it's quite broad.

      @theoretick I totally agree. AFAIR the working group brought that up, but it was decided that having a broad Security context was a good first step. At the very least it was a way to move forward with the proposal without having to define boundaries b/w vulnerability management, dependency management, types of scans, etc. :slight_smile:

    • @fcatteau @theoretick @bwill Just trying to understand the motivation behind it. Within Security we can still have dedicate subdomains like Security::Dast, Security::Vurnerabilities, Security::DependencyManagement, etc. If we don't know how to properly split those then we maybe don't need to do that yet and leverage the sub-domains instead.

      If instead we prefer to have completely separate domains, I'm ok with that too but as I said in !153581 (comment 1921000154) we could have strange cases like shared functionalities that we don't know where they should live.

      A good time to split bounded contexts is when we understand well how the new bounded contexts would depend on each other. We have a similar problem in Ci::, as you can see it's almost double the size of Security:: :smile: . We will eventually extract smaller dedicated bounded contexts (like Runners or perhaps Artifacts) but we are not clear yet where we would end up.


      Kind of related: Are you by any chance interested in enforcing sub-domains? E.g. ensure that Security::Vulnerabilities is used instead of Security::VulnerabilityManagement or a bunch of other similar terms? If so, this may be very easy to add to.

    • Within Security we can still have dedicate subdomains like Security::Dast, Security::Vurnerabilities, Security::DependencyManagement, etc. If we don't know how to properly split those then we maybe don't need to do that yet and leverage the sub-domains instead.

      @fabiopitino Thanks for bringing this up. I totally missed that the new domains were not subdomains of Security. I think they should be subdomains at this point. :thumbsup:

      A good time to split bounded contexts is when we understand well how the new bounded contexts would depend on each other.

      Yes. We might extract DependencyManagement at some point it's connected to policies and compliance, and not only related to Security. Same thing for SCA, which covers both vulnerability scanning and license scanning.

      Edited by Fabien Catteau
    • @bwill @fcatteau do you still want to split the domains now or leverage the sub-domains inside Security:: module (existing strategy)?

    • @fabiopitino At this point I'd rather have sub-domains under the existing Security:: module.

    • Author Maintainer

      If instead we prefer to have completely separate domains, I'm ok with that too but as I said in !153581 (comment 1921000154) we could have strange cases like shared functionalities that we don't know where they should live.

      @fabiopitino Isn't that true of all code? For example, where do we put Projects, Groups, Users, etc? How should this problem be addressed at a global level?

      Kind of related: Are you by any chance interested in enforcing sub-domains? E.g. ensure that Security::Vulnerabilities is used instead of Security::VulnerabilityManagement or a bunch of other similar terms? If so, this may be very easy to add to.

      Yes, I am interested in this because when we have separate top-level domains we are actually able to enforce the contexts that are used. When we rely on sub-domains inside Security::, we are right back at square one in that we do not have any enforced conventions.

      do you still want to split the domains now or leverage the sub-domains inside Security:: module (existing strategy)?

      Here are the problems I see with using the Security:: domain. Some of these could be fixed with sub-domain enforcement:

      1. There is a lot code which currently is not categorized (Ex: Security Policies vs Vulnerabilities), and there is currently no mechanism of correctly categorizing them.
      2. Because of the wide domain, all miscellaneous security code will end up going here even if it's not related to any of the product categories in this domain.
      3. If we use subdomains, we're forcing everybody to write code 3+ modules deep which will become quite verbose.

      /cc @fcatteau

    • For example, where do we put Projects, Groups, Users, etc? How should this problem be addressed at a global level?

      For such concepts we should use the patterns described in https://docs.gitlab.com/ee/development/software_design.html#taming-omniscient-classes and avoid adding behavior to these classes. A typical case is a feature that has group/project level settings. We tend to define those in Projects or ProjectSetting but they should be defined inside the related bounded context. E.g. Security::Vunerabilities::ProjectSetting.

      Yes, I am interested in this because when we have separate top-level domains we are actually able to enforce the contexts that are used. When we rely on sub-domains inside Security::, we are right back at square one in that we do not have any enforced conventions.

      :thumbsup: we can explore this avenue, but having strict sub-domains you can have less flexibility in where to put new or shared concepts. Thinking of it more, I guess it doesn't add much value and it would be better split the domains entirely.

      Because of the wide domain, all miscellaneous security code will end up going here even if it's not related to any of the product categories in this domain.

      I feel this is a crucial aspect to discuss with others in the team in order to reach a consensus. Ultimately it's up you as domain experts.

      If you feel there are clear boundaries between all these security features then we should probably go for separate top-level concerns.

      I used Security:: as initial bounded context because it was already widely used and because a lot of the security code is already deeply nested. We have a similar pattern with Ci:: because we have multiple interrelated concepts and can be hard to consider in isolation.

      /cc @fcatteau @mcavoj

    • Author Maintainer

      If you feel there are clear boundaries between all these security features then we should probably go for separate top-level concerns.

      @fabiopitino Vulnerabilities is already a distinct top-level namespace with lots of code in it. I believe that moving this under Security:: would actually be taking a step backwards.

      SecurityPolicies is also a very distinct domain which would benefit from being separated from Security::.

      For other code that we're not sure where to place, I think it would benefit us to pause for a moment and consider where it should go. Is it supporting Vulnerability management? Or is it a new product domain? My opinion is that separating these would cause people to ask these questions and categorize things more intentionally.

      Edited by Brian Williams
    • @bwill That's ok by me. Compliance is already a separate bounded context but that's probably already a distinct concept. You (teams in Secure and Govern) may find helpful having a rough representation of how dependencies among these bounded contexts would be when making a decision.

      A rough (probably wrong) example:

      flowchart TD
        SecurityPolicies --> SAST --> Vulnerabilities
        SecurityPolicies --> DAST --> Vulnerabilities
        SecurityPolicies --> SoftwareCompositionAnalysis --> Vulnerabilities
        Compliance --> SecurityPolicies

      Are there other concepts that are missing from the list? E.g. SAST is not in this MR AFAIK.

    • Author Maintainer

      @fcatteau Can you elaborate on why you think these domains should reside under the Security:: namespace?

      Are there other concepts that are missing from the list? E.g. SAST is not in this MR AFAIK.

      Concepts like SAST exist but currently do not have any code inside the Rails monolith.

      @fabiopitino As a short-term compromise, could we split out Vulnerabilities but keep the Security namespace until we can resolve this discussion? Some groupthreat insights team members currently have their MRs blocked because we would like to continue to add code to the existing Vulnerabilities namespace. If this sounds reasonable, could we please merge !154618 (merged)? I think your suggestion regarding dependencies is a reasonable one but I don't have time to do the analysis for this at the moment.

      Edited by Brian Williams
    • @bwill I'm merging Split `Vulnerabilities` out from `Security` (!154618 - merged) :thumbsup: Thanks - I didn't notice this decision was blocking other MRs.

      Let me know when/if this MR is ready to be merged. I have no problems either way but it sounds like we don't have a verdict yet.

      Please assign it back to me when ready :thumbsup:

    • Please register or sign in to reply
  • Fabien Catteau
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading