Follow-up from "Show error when the DAST feature not available"

The following discussion from !47484 (merged) should be addressed:

Since !47484 (merged) when a user configures DAST but does not have the correct license, the DAST job does not run and is replaced by the job dast_unlicensed. This job simply outputs an error message to the console stating Error: Your GitLab project is not licensed for DAST.

A number of concerns were raised in the original MR:

• rules duplication
• dast_unlicensed job rather than DAST job could cause confusion

Since the dast_unlicensed job should only run when dast job runs, differing only on the if dast is included in $GITLAB_FEATURES, results in the rules being largely duplicated.

One solution to avoid this duplication could be to move the $GITLAB_FEATURES dast check into the script section of the dast and remove the dast_unlicensed jobs. One possible implementation could be:

dast:
  stage: dast
  image:
    name: "$SECURE_ANALYZERS_PREFIX/dast:$DAST_VERSION"
  variables:
    GIT_STRATEGY: none
  allow_failure: true
  script:
    - export DAST_WEBSITE=${DAST_WEBSITE:-$(cat environment_url.txt)}
    - if [ -z "$DAST_WEBSITE$DAST_API_SPECIFICATION" ]; then echo "Either DAST_WEBSITE or DAST_API_SPECIFICATION must be set. See https://docs.gitlab.com/ee/user/application_security/dast/#configuration for more details." && exit 1; fi
    - if [ $GITLAB_FEATURES !~ /\bdast\b/ ]; then echo "Error: Your GitLab project is not licensed for DAST." && exit 1; fi
    - /analyze
  artifacts:
    reports:
      dast: gl-dast-report.json
  rules:
    - if: $DAST_DISABLED
      when: never
    - if: $DAST_DISABLED_FOR_DEFAULT_BRANCH &&
          $CI_DEFAULT_BRANCH == $CI_COMMIT_REF_NAME
      when: never
    - if: $CI_DEFAULT_BRANCH != $CI_COMMIT_REF_NAME &&
          $REVIEW_DISABLED && $DAST_WEBSITE == null &&
          $DAST_API_SPECIFICATION == null
      when: never
    - if: $CI_COMMIT_BRANCH &&
          $CI_KUBERNETES_ACTIVE &&
    - if: $CI_COMMIT_BRANCH &&
          $DAST_WEBSITE
    - if: $CI_COMMIT_BRANCH &&
          $DAST_API_SPECIFICATION

This implementation would also mean that the dast_unlicensed job would not show and would remove any confusion caused by it.

My main concern with adding logic like this to script is that it's hard to test (I'm unaware of a way to currently unit test this). I'm also under the impression that the DAST team is keen to remove all logic from the script, in favour of only running ./analyze.

Another option could be to move the $GITLAB_FEATURES check inside the analyze script. This would have the effect of coupling DAST to the gitlab-ci.yml file, which might not be desirable.

• @stanhu started a discussion:

  This seems fine for now, but it is a little confusing to the user that dast_unlicensed shows up as a job, and we have to duplicate the logic above to do that.

  Could the dast job just fail outright with an error in the script? What prevents someone from just copying this template and commenting out the regexps for \bdast\b?

Implementation Plan

• Move license check to DAST analyze script
• Remove GITLAB_FEATURES =~ /\bdast\b/ from DAST.latest.gitlab-ci.yml
• Remove GITLAB_FEATURES =~ /\bdast\b/ from DAST.gitlab-ci.yml