CloudLabs-MOC / GitHub-Advanced-Security-with-Azure-DevOps

0 stars 1 forks source link

GHASDO Feedback for content #2

Open sumitmalik51 opened 1 month ago

sumitmalik51 commented 1 month ago

Please add you feedback below

jasonkeicher commented 1 month ago

General

Lab 01: Configure GHAzDO in Azure DevOps

Lab 02: Secret Scanning

Lab 03: Dependency Scanning

Lab 04: Code Scanning

Lab 05: Microsoft Defender for Cloud DevOps security

tjcorr commented 1 month ago

GHAzDO Feedback

Page 1

Page 2

Page 3 / Lab 1

Page 4 / Lab 2

Page 5 / Lab 3

Page 6 / Lab 4

sumitmalik51 commented 3 weeks ago

Hi Everyone, All the feedbacks are accomodated and added inline responses as well and linked with the mentioned PR #3 , Please do review and let me know if anything is pending and needs improvement.

sumitmalik51 commented 3 weeks ago

General

  • Used New Experience
  • Pull Request Integration: Use GitHubโ€™s pull request checks to ensure that code changes meet security standards before merging. Integrate these checks with Azure Pipelines to automate the process.

    • Let's not confuse that for dependbot pull request functionality. SPEKTRA: Updated with required content
  • Failed to connect first time SPEKTRA: Can be a temp issue, refresh should resolve the issue.
  • Image
  • Should passwords be visible in the guide? SPEKTRA: This is dynamic value and only will be visible if the lab environment is running.
    • Exploring Your Lab Resources
  • It feels like the copy-paste experience is inconsistent, especially when needing to copy an Azure resource group name and accidentally getting a space after it, which the Azure portal doesn't like. I think all the values needed should be copy boxes. SPEKTRA: We will have a look into it, have informed the platform engg. team for the same, in the meantime a hard refresh should resolve the issue.

    Lab 01: Configure GHAzDO in Azure DevOps

  • On Step 4, the screenshot goes to an org with an existing project, but in my lab, I went to an org with no project and the create project screen, so it was a little confusing what to do next. SPEKTRA: Updated with required content
  • Create Work Item, step 3: Why is the description blurred out? SPEKTRA: Updated with required content
  • Also, it would be nice to have these in copy boxes. SPEKTRA: As we are using integrated experience, its not yet available to use copy box.
  • PAT, step 7: I think the C# line should already be in the file, and the user can just paste the PAT in. SPEKTRA: As we are generating and creating the packages from public repo, updating the line is not possible.
  • Update the pipeline and create a pull request:

    • In this task, you will remove the Azure deployment task codes from the pipeline.
    • Why? It is unclear why we are doing this. SPEKTRA: We have updated the steps to avoid the confusion and added new wordings.
  • Update Pipeline, Step 4: This wording is confusing.

    • Make sure you remove the code in the pipeline which includes the test and production deployments tasks (from line 70 till the end). If you won't remove the deployment task, the pipeline might fail.

SPEKTRA: We have updated the steps to avoid the confusion and added new wordings.

Lab 02: Secret Scanning

  • It should mention changing the dropdown to "Recently Completed" to find the app sec work item. SPEKTRA: This has been removed as per TH's feedback
  • Fixing Exposes Secret: step 5, why click regenerate twice? SPEKTRA: We have updated the steps
  • Task 3 Dismiss: Step 4: "View Other Alerts" doesn't exist.

    • Go to the Azure DevOps Advanced Security dashboard, click on Secrets, and subsequently click on View other alerts. You will see a list of other exposed secret alerts that have been found. SPEKTRA: We have updated the wordings.

Lab 03: Dependency Scanning

  • Task 1: Dep scanning is on line 37 for me. SPEKTRA: Updated the screenshot
  • We have already run the pipeline and have dependency results, so run again is optional. SPEKTRA: Updated the instruction accordingly.
  • Point to examples of direct vs transitive in the repo. Spektra: Still looking into it to find the best example.
  • Task 3, step 3, needs a better explanation. SPEKTRA: Updated the instruction accordingly.
  • VS takes a while to load. Maybe ask them to start it at the beginning. SPEKTRA: Have moved the steps in the begning.
  • Validation stuck "In Progress". SPEKTRA: Team is checking and should be fix by Monday.

Lab 04: Code Scanning

  • Task 1: A little more explanation of what these tasks do would be nice. SPEKTRA: Updated the instruction accordingly.
  • Task 3.1: This is a cmd line command and not a SQL statement. It is a little confusing. SPEKTRA: Updated the instruction accordingly.
  • Task 3.7: We clicked the "create pull request" checkbox, then the instructions state to click the Create a Pull request button. SPEKTRA: Fixed the instruction accordingly.
  • Task 3.8: Why are the title and description blurred? SPEKTRA: Updated the Screenshot.

Lab 05: Microsoft Defender for Cloud DevOps security

  • Was trying to get Lab 3 to Validate and I think I hit the refresh all and lab 5 indicated a success even though I did not start it yet. SPEKTRA: This is a bug and have reported to team, should be fix by Monday.
  • Note: It might take up to 8hrs to reflect the real-time status.

    • Hard to see success with this delay. Spektra: As Defender for cloud takes this much time to gather the data and populate it on dashboard, we will not be able to do anything, hence added the task as READ ONLY.
sumitmalik51 commented 3 weeks ago

GHAzDO Feedback

Page 1

  • [x] Under Integrating GitHub Advanced Security with Azure DevOps:

    • [x] You don't use GitHub Actions in Azure Pipelines. This all runs native with ADO pipelines.
    • [x] Secret Scanning isn't part of the pipeline it is built-in
    • [x] Dependency management runs as part of the pipeline
  • [ ] Benefits of Integration

    • [ ] Comprehensive Reporting -> You can't see any GHAzDO findinds within GitHub
    • [ ] Improved Collaboration -> We aren't talking better together / hybrid here so I don't think this applies or is relevant.

SPEKTRA: Updated the instruction accordingly.

Page 2

  • [x] There is no "Maybe Later" button, you need to click cancel instead
  • [x] Why did we login to the Azure Portal? The next step just has us go to ADO

SPEKTRA: Updated the instruction accordingly and removed Azure login.

Page 3 / Lab 1

  • [x] Task 1

    • [x] Unable to setup billing to Azure sub (worked after retrying a minute or 2 later): Sorry, we are unable to set up billing. Give it a few minutes and then please try again SPEKTRA: Can be a temp issue.
    • [x] Can't we automate all this away? In GHAS lab I was giving an org with repos already setup ready to go. SPEKTRA: It cant be automated as there are not API for setting up the billing as of now
    • [x] What is the work item for? Looks like it is just make the branch policy happy. Maybe just remove that policy instead? Feels awkward working with the same item over and over again. SPEKTRA: Have updated the instruction and removed the not required step for policy and work items.
    • [x] Why are we creating a PAT and checking it in? This is what we are advising against. I would turn on GHAzDO first to showcase push protection. It looks like this is just so we can fix it later. If so just include this in the template? SPEKTRA: This was implemented based on the initial feedback, as we need to view the secret scanning alerts in Lab2 - which highlights the PAT secret visibility within the repo
  • [x] Task 2

    • [x] "To ensure Azure DevOps Advanced Security is enabled in your organization" -> We are only turning it on for 1 repo here. SPEKTRA: Updated the instruction and added links for project and org enablement.
    • [x] Consider adding a link that explains how the billing works SPEKTRA: Added more links
    • [x] Why don't we just use a template that has a proper pipeline so we aren't deleting a bunch of stuff that doesn't work. SPEKTRA: We have updated the template in labguide itself, where user will copy and paste in pipeline.
    • [x] No real explanation of what this pipeline is doing SPEKTRA: Have added more description.
    • [x] The publish results tasks is no longer needed SPEKTRA: Removed
    • [x] You don't need ms.advancedsecurity-tasks.codeql.init. / ms.advancedsecurity-tasks.dependency-scanning. / ms.advancedsecurity-tasks.codeql.analyze. prefixes on the GHAzDO tasks SPEKTRA: Updated
    • [x] Not clear to me why we did these 2 tasks on a branch only to just merge it in. SPEKTRA: Added more instruction and the result steps.
    • [x] The permissions example of making admins able to manage settings doesn't seem that realistic since admins already had permissions outside of manage settings (turning on/off advanced security). Normally I wouldn't set the permissions like this. We should really just explain what the settings are and scenarios when you might want to change them. SPEKTRA: Updated the instructions.

Page 4 / Lab 2

  • [x] Task 1

    • [x] Why are we re-opening this work item?
    • [x] My work item wasn't easy to find. I ultimately opened it through the link from my PR. SPEKTRA: Removed this task as per your suggestion.
  • [x] Task 2

    • [x] We mention we are fixing the alert so why are we renamming the PAT, that clearly isn't fixing the problem. There would be better ways to demonstrate push protection. SPEKTRA: Updated instruction.
    • [x] Adding the PAT as a secret in the pipeline doesn't actually make it to the code. I know we probably removed the complex replace logic since it wasn't core to the exercise. Maybe instead we could just mention you could do it via environment variables or something else that would actually work. SPEKTRA: updated with few links and steps to use environment variable.
    • [x] Again we aren't really explaining why we are doing these steps. SPEKTRA: Added more detailed isntructions
    • [x] Why did we change the PR merge options? SPEKTRA: Updated.

Page 5 / Lab 3

  • [ ] Task 1

    • [x] We don't really need to run the pipeline since it would have already been triggered after we merged in the code. SPEKTRA: Updated and added as a optional
    • [x] Task 3
    • [x] Visual Studio was very slow to spin up and not very responsive to make the code changes. Do we need a beefier VM or a way to make the changes in just ADO itself? SPEKTRA: Have moved the step in the begning, and will also update the VM SKU to give good performance.
    • [ ] Lab validation just says in progress. No option to validate again. SPEKTRA: This is bug and should be fixed by Monday.
  • [x] Lab Validation listed here but not on other excercises. The Lab Validation page has options to validate other sections. SPEKTRA: Updated

Page 6 / Lab 4

  • [x] Task 1

    • [x] No mention of the settings for CodeQL or a link for more information SPEKTRA: Added links and more information
  • [x] Task 2

    • [x] The ProTip on step 4 mention vulnerable component. That looks like copy/paste from the last task. Should be changed to talk about code not componenets. SPEKTRA: Updated
  • [x] Task 3

    • [x] By hardcoding here we are losing some functionality in the original application. SPEKTRA: Yes, but we are not using the application and just demonstrating the fix thats all.
    • [x] step 7/8 aren't needed since you had the commit create a PR automatically SPEKTRA: Updated
  • [x] Task 4

    • [x] You need to wait not just for the PR to merge but then the main scan as well. SPEKTRA: Updated and added the instruction.
      =
  • [x] Originally we mention Pull Request Integration but we don't really do that anywhere in the labs. SPEKTRA: Could you please elaborate more for this one.
ChrisRomp commented 2 weeks ago
trigger:
 - main

pool:
  vmImage: ubuntu latest

extends: 
  template: template.yaml
  parameters:
    stages:
      - stage: Build
        displayName: 'Build'
        jobs:
        - job: Build
          steps:
          - checkout: self

          - task: DotNetCoreCLI@2
            displayName: Restore 
            inputs:
              command: restore
              projects: '**/*.csproj'

          - task: AdvancedSecurity-Codeql-Init@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Initialize CodeQL'
            inputs:
              languages: csharp
              querysuite: default

          - task: DotNetCoreCLI@2
            displayName: Build
            inputs:
              projects: '**/*.csproj'
              arguments: '--configuration $(BuildConfiguration)'

          - task: AdvancedSecurity-Dependency-Scanning@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Dependency Scanning'

          - task: AdvancedSecurity-Codeql-Analyze@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Perform CodeQL analysis'

          - task: AdvancedSecurity-Publish@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Publish Results'

          - task: DotNetCoreCLI@2
            displayName: Test
            inputs:
              command: test
              projects: '[Tt]ests/**/*.csproj'
              arguments: '--configuration $(BuildConfiguration) --collect:"Code coverage"'

          - task: PublishBuildArtifacts@1
            displayName: 'Publish Artifact'
            inputs:
              PathtoPublish: '$(build.artifactstagingdirectory)'
            condition: succeededOrFailed()
Srikanthssp commented 2 weeks ago
  • Providing the sign-in creds inline in the lab guide is nice.
  • Cosmetic: Can we set the Edge new tab page to blank in the base image (both labs)?
    • Disable feed and sponsored links
    • It looks like a tabloid

SPEKTRA : We cannot configure this

  • Demo generator has a note to enable class build & release pipelines, but the org has the default settings to disable creation of classic pipelines, and no step to enable them.

SPEKTRA : Since we are not showcasing any DevOps pipeline functionalities, we have not enabled those steps as this lab is focussing on Advanced Security

  • No deployment errors reported by demo generator
  • Seems to deploy YAML pipelines ๐Ÿ‘
  • Lab 01, Page 3: Update azure-pipelines.yml using copy/paste from docs - error on validate and save:

    • /azure-pipelines.yml (Line: 4, Col: 2): While parsing a block collection, did not find expected '-' indicator.
    • Image
    • Image
    • The pool: and extends: blocks are indented by one space, should not be
    • Working code:
trigger:
 - main

pool:
  vmImage: ubuntu latest

extends: 
  template: template.yaml
  parameters:
    stages:
      - stage: Build
        displayName: 'Build'
        jobs:
        - job: Build
          steps:
          - checkout: self

          - task: DotNetCoreCLI@2
            displayName: Restore 
            inputs:
              command: restore
              projects: '**/*.csproj'

          - task: AdvancedSecurity-Codeql-Init@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Initialize CodeQL'
            inputs:
              languages: csharp
              querysuite: default

          - task: DotNetCoreCLI@2
            displayName: Build
            inputs:
              projects: '**/*.csproj'
              arguments: '--configuration $(BuildConfiguration)'

          - task: AdvancedSecurity-Dependency-Scanning@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Dependency Scanning'

          - task: AdvancedSecurity-Codeql-Analyze@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Perform CodeQL analysis'

          - task: AdvancedSecurity-Publish@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Publish Results'

          - task: DotNetCoreCLI@2
            displayName: Test
            inputs:
              command: test
              projects: '[Tt]ests/**/*.csproj'
              arguments: '--configuration $(BuildConfiguration) --collect:"Code coverage"'

          - task: PublishBuildArtifacts@1
            displayName: 'Publish Artifact'
            inputs:
              PathtoPublish: '$(build.artifactstagingdirectory)'
            condition: succeededOrFailed()

SPEKTRA โ€“ We did not face any error while validating the pipeline, and also with copy/paste from the labguide

  • Page 4, task 2: Step numbering is inconsistent and seems to reset to 1. more than once.

SPEKTRA โ€“ There are subtasks in the Task-2, and the numbering for these steps starts at 1 for each sub-task.

  • Bypass push protection step 1: The text says skip-secret-scanning: true with a space, which won't work. The other instructions (no space) are correct.

SPEKTRA โ€“ We have removed the extra space added in the instruction.

  • Image

    • eShopOnWeb build fails for "Fixed secret" PR on PATFIX branch:
  • src/Web/Constants.cs(5,36): Error CS0133: The expression being assigned to 'Constants.AZDO_PAT' must be constant

  • Image

  • Changed to: public static readonly string AZDO_PAT = Environment.GetEnvironmentVariable("AZ_PAT");

    • Cause: C# const keyword requires value to be constant at compile time; Environment.GetEnvironmentVariable() can only execute at runtime.

SPEKTRA โ€“ we have added a screenshot showing the code where the task adds public static readonly string AZDO_PAT = Environment.GetEnvironmentVariable("AZ_PAT"); to avoid any confusion

  • After build succeeded, PR showed optional check for coverage status failed. Continued with merge.
  • Image
  • Some subsequent builds (later steps) did not have the error, some did.

SPEKTRA โ€“ we have modified the pipeline in lab-01 by removing the code coverage configuration from the Test task and addressed the optional check error.

  • Page 7, Lab 05, step 4, it may be worth noting that the "Management" section is now collapsed by default, and the user should expand that to select "Environment settings"

SPEKTRA โ€“ Modified the instructions with necessary screenshot.

  • Step 9 the user should wait for the connectivity status to show Connected (shown in the screenshot)

    • During provisioning it will show "In progress..."
    • Image
    • This seems to take a very, very long time - instructors should be made aware
    • I eventually stopped waiting to avoid dying of old age first.

SPEKTRA โ€“ It could take up to 2 hours for the status to get updated, added the note in the guide.

ChrisRomp commented 2 weeks ago

Working YAML for pipeline:

trigger:
 - main

pool:
  vmImage: ubuntu latest

extends: 
  template: template.yaml
  parameters:
    stages:
      - stage: Build
        displayName: 'Build'
        jobs:
        - job: Build
          steps:
          - checkout: self

          - task: DotNetCoreCLI@2
            displayName: Restore 
            inputs:
              command: restore
              projects: '**/*.csproj'

          - task: AdvancedSecurity-Codeql-Init@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Initialize CodeQL'
            inputs:
              languages: csharp
              querysuite: default

          - task: DotNetCoreCLI@2
            displayName: Build
            inputs:
              projects: '**/*.csproj'
              arguments: '--configuration $(BuildConfiguration)'

          - task: AdvancedSecurity-Dependency-Scanning@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Dependency Scanning'

          - task: AdvancedSecurity-Codeql-Analyze@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Perform CodeQL analysis'

          - task: AdvancedSecurity-Publish@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Publish Results'

          - task: DotNetCoreCLI@2
            displayName: Test
            inputs:
              command: test
              projects: '[Tt]ests/**/*.csproj'
              arguments: '--configuration $(BuildConfiguration)'

          - task: PublishBuildArtifacts@1
            displayName: 'Publish Artifact'
            inputs:
              PathtoPublish: '$(build.artifactstagingdirectory)'
            condition: succeededOrFailed()
Srikanthssp commented 1 week ago
  • Page 3, Task 2, Update DevOps pipeline with copied code:

    • Still introducing erroneous space. Source code shows spaces present as well. This feedback was also sent in Teams earlier, so if it's already been addressed then disregard.
    • Image
    • Video of issue: https://1drv.ms/v/s!AjF3UCQDPZZUk_x2aVWam-jkKQzFaQ?e=yGXYtC
    • Diff comparison in VS Code: Image
    • Working YAML below.

SPEKTRA - We have fixed the YAML issue and regarding space in the lab guide, that is expected

  • Step 9, PR build failed, though I can't say why. Maybe there's a network error in the build agent. Re-queue seems to resolve the issue. I saw this happen one other time last time I ran the lab, so it seems to be an intermittent error outside of our control. Up to you if you want to make a note in the lab guide or instructor notes.

    • Image

SPEKTRA - Added a note to re-que the build when the PR build fails

  • Failed again during Dependency scanning exercise - it took 5 total attempts to clear the Restore step in the pipeline. I don't see anything wrong with the pipeline (it's just doing dotnet restore behind the scenes); it seems like the NuGet provider is timing out... perhaps it's just having a bad day.

SPEKTRA - Added a note to re-run the pipeline if fails due to this error

  • Somehow Advanced Security toggled itself off for the eShopOnWeb repository while I was working through the build issues.

    • No idea how this could have happened. I didn't change any configuration. ๐Ÿ‘ป
    • After re-enabling in repo settings, I started a new run of the pipeline on main to complete the security scan.

SPEKTRA - Added a note to re-enable if the pipeline fails stating this error

  • Task 3 validation check remains "In Progress" for some reason, even though the work has been completed and the alert (Task 4) has been closed.

    • Image

SPEKTRA - we have Fixed the validation issue

  • I skipped the Defender lab in this review.

Working YAML for pipeline:

trigger:
 - main

pool:
  vmImage: ubuntu latest

extends: 
  template: template.yaml
  parameters:
    stages:
      - stage: Build
        displayName: 'Build'
        jobs:
        - job: Build
          steps:
          - checkout: self

          - task: DotNetCoreCLI@2
            displayName: Restore 
            inputs:
              command: restore
              projects: '**/*.csproj'

          - task: AdvancedSecurity-Codeql-Init@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Initialize CodeQL'
            inputs:
              languages: csharp
              querysuite: default

          - task: DotNetCoreCLI@2
            displayName: Build
            inputs:
              projects: '**/*.csproj'
              arguments: '--configuration $(BuildConfiguration)'

          - task: AdvancedSecurity-Dependency-Scanning@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Dependency Scanning'

          - task: AdvancedSecurity-Codeql-Analyze@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Perform CodeQL analysis'

          - task: AdvancedSecurity-Publish@1
            condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest'))
            displayName: 'Publish Results'

          - task: DotNetCoreCLI@2
            displayName: Test
            inputs:
              command: test
              projects: '[Tt]ests/**/*.csproj'
              arguments: '--configuration $(BuildConfiguration)'

          - task: PublishBuildArtifacts@1
            displayName: 'Publish Artifact'
            inputs:
              PathtoPublish: '$(build.artifactstagingdirectory)'
            condition: succeededOrFailed()

SPEKTRA - we have included working YAML Code in the labguide