maester icon indicating copy to clipboard operation
maester copied to clipboard

Align warnings with available licenses. Report unlicensed features as "unlicensed" (in grey).

Open GeldHades27355 opened this issue 9 months ago • 1 comments

For example: M365 BP doesn't include EID P2, so there is no advanced user risk management. But all tests for risk management still fail, bringing the score down.

(Secure Score and Security Admin Center is also not great at this)

Is there a way to correlate the warnings with available licenses, so reports more accurately reflect open actions?

So include the tests, just make sure folks know there's nothing they can do about some results without upgrading.

GeldHades27355 avatar Apr 30 '24 16:04 GeldHades27355

We already have tests that change depending on the available license or are completely skipped if the license is not present in the tenant. Could you provide me with a few examples so I can check if those can be easily skipped as well?

f-bader avatar May 18 '24 20:05 f-bader

Hi @GeldHades27355 if you could provide some examples I think we can help

f-bader avatar Jun 05 '24 20:06 f-bader

I'd love to, but I'm seriously busy on other things right now, sorry!

I mentioned one example in the initial post: Business Premium does not include Entra ID P2, only P1. So User Risk management is not included, but I still get a "fail". I'd expect a "warning" at best, or a "Not licensed" ideally. And I'd expect a score related to the maximum possible with the current license.

tbh I'm not sure how to deal with mixed environments, with both P1 and P2 active... maybe one score using max P1 and a second using max P1 as benchmarks?

In general, this shouldn't be hard. Entra P1 va. free vs. P2 comparison tables exist galore, for example here: https://www.microsoft.com/en-us/security/business/microsoft-entra-pricing

GeldHades27355 avatar Jun 06 '24 14:06 GeldHades27355

@GeldHades27355 M365 BP does include Entra ID P1. You can see in the service plan reference for M365 BP that it includes the license GUID 41781fb2-bc02-4b7c-bd55-b576c07bb09d, which would be satisfy the appropriate skips.

Though @f-bader this did make me think, would it make sense to have a MD section added to the report for licenses skipped because of unlicensed features, and storing that in the module variable.

I am not super familiar with the report building cmdlets but some rough pseudocode

#Test-*.Tests.ps1
if(-not $licensed){
  $__MtSession += #test ID or test invocation
}

#Get-MtMarkdownReport.ps1
function GetUnlicensed {}
$templateMarkdown = $templateMarkdown -replace '%UnlicensedSummary%', (GetUnlicnesed)

Snozzberries avatar Jun 06 '24 14:06 Snozzberries

Yes, as I mentioned, BP includes Entra P1. So I get a "fail" for user risk management, which is, indeed, not enabled. That doesn't seem right to me. It should return "unlicensed" or "warning" at worst.

Unless this was already fixed in the last few weeks? (sorry, no time to test)

Is there a chance that check is failing?

GeldHades27355 avatar Jun 06 '24 16:06 GeldHades27355

Ahh, understood. It would be helpful if you could confirm what tests you specifically see failing though you feel they should be skipped.

Here is an initial list I would consider:

  • [ ] https://github.com/maester365/maester/blob/main/tests/Maester/Entra/Test-ConditionalAccessBaseline.Tests.ps1#L36
  • [ ] https://github.com/maester365/maester/blob/main/tests/Maester/Entra/Test-ConditionalAccessBaseline.Tests.ps1#L39
  • [ ] https://github.com/maester365/maester/blob/main/tests/EIDSCA/Test-EIDSCA.Generated.Tests.ps1#L65
  • [ ] https://github.com/maester365/maester/blob/main/tests/EIDSCA/Test-EIDSCA.Generated.Tests.ps1#L130
  • [ ] https://github.com/maester365/maester/blob/main/tests/EIDSCA/Test-EIDSCA.Generated.Tests.ps1#L180-L363

Snozzberries avatar Jun 06 '24 16:06 Snozzberries

Sorry for the exasperation - does anyone READ what I post?

User Risk fails, as an example. P1 doesnt't support that - so BP doesn't either.

I dont' have time for more tests right now, sorry!

GeldHades27355 avatar Jun 06 '24 19:06 GeldHades27355

Thanks for sharing your concern @GeldHades27355, there are multiple user risk tests that appear may not have logic to skip when the tenant is unlicensed (see above), I am sure we will continue improving the solution and hopefully those improvements will address your concern.

Snozzberries avatar Jun 06 '24 20:06 Snozzberries

I've verified that all risky user related tests check for P2 requirement and are skipped if the tenant does not have P2.

This is most probably a case where there are some users licensed for P2 but not all. Even if that were the case the tests will pass if the policies exist and are scoped to the number of users that have the P2 license.

Closing this for now.

merill avatar Jun 23 '24 03:06 merill

Cool. Thanks @merill. 👍 JFYI: none of the tenant(s) I tested on had EP2 in any way. Neither explicitly as a subcription, nor as part of a bundle, such as M365 E5. All tenants I tested on have M365 Business Premium (including P1) only.

GeldHades27355 avatar Jun 23 '24 14:06 GeldHades27355

Got it. I think I know the issue. We use the /organizations endpoint for the licence check for P1 and P2 which might not be the right way to check.

The /subscribedSkus would be the most accurate.

merill avatar Jun 23 '24 21:06 merill

@merill having a look at tests\Maester\Entra\Test-ConditionalAccessWhatIf.Tests.ps1 I can see there is a BeforeDiscovery section that defines $EntraIDPlan which then defines Skip logic in the various tests

I don't see the same logic in tests\Maester\Entra\Test-ConditionalAccessBaseline.Tests.ps1 and this results in tests like

  • MT.1012: At least one Conditional Access policy is configured to require MFA for risky sign-ins
  • MT.1013: At least one Conditional Access policy is configured to require new password when user risk is high from being skipped

I tried adding the following to the ConditionalAccessBaseLine.Tests.ps1 file but doesn't seem to make a difference or output anything useful to the run in GitHub Actions

BeforeDiscovery {
    $EntraIDPlan = Get-MtLicenseInformation -Product "EntraID"
    Write-Verbose "EntraIDPlan: $EntraIDPlan"
}

Am I missing something in the logic of how the code should work

MattWhite-personal avatar Jul 22 '24 18:07 MattWhite-personal

Hi @MattWhite-personal this is very curious because the test you mentioned has the same test logic inside of the function itself. So the result should be that they show up as skipped.

https://github.com/maester365/maester/blob/bec4dcdbdf139f556458a6f1c39f28c05e20dc2e/powershell/public/Test-MtCaMfaForRiskySignIn.ps1#L22

Basically we don't skip them completly in pester but run them, that way we can add a custom skip reason

f-bader avatar Jul 22 '24 20:07 f-bader

@f-bader this isn't what I see when I run the tests on a tenant with Business Premium (Entra ID P1) licenses applied

image

is there a way I can get maester to be more verbose when running the GitHub Action

          # Configure test results
          $PesterConfiguration = New-PesterConfiguration
          $PesterConfiguration.Output.Verbosity = 'None'

not sure if this is a section that can be modified to support debug or whether theres an easier way to test and repro the issue on a client device?

MattWhite-personal avatar Jul 22 '24 20:07 MattWhite-personal

Would you be able to run this and send me the output. I don't have a business premium tenant to test this and it would help me a lot

Invoke-MtGraphRequest -ApiVersion beta -RelativeUri 'organization' | Select-Object -ExpandProperty assignedPlans | Where-Object service -EQ "AADPremiumService" | Select-Object -ExpandProperty servicePlanId

For the pester action in GitHub you can configure the verbosity using something like this.

jobs:
  run-maester-tests:
    name: Run Maester Tests
    runs-on: ubuntu-latest
    steps:
    - name: Run Maester action
      uses: maester365/maester@main
      with:
        client-id: ${{ secrets.AZURE_CLIENT_ID }}
        tenant-id: ${{ secrets.AZURE_TENANT_ID }}
        include_public_tests: true
        pester_verbosity: 'Detailed'

f-bader avatar Jul 22 '24 21:07 f-bader

Let me look at the GitHub action - i still have the home brew action logic rather than the published Maester Action (on my todo list to update /fix)

I'll run the MtGraphRequest cmdlet and post the results back

MattWhite-personal avatar Jul 22 '24 21:07 MattWhite-personal

@f-bader results as follows

PS C:\Users\MatthewWhite> Invoke-MtGraphRequest -ApiVersion beta -RelativeUri 'organization' | Select-Object -ExpandProperty assignedPlans | Where-Object service -EQ "AADPremiumService" | Select-Object -ExpandProperty servicePlanId
eec0eb4f-6444-4f95-aba0-50c24d67f998
de377cbc-0019-4ec2-b77c-3f223947e102
41781fb2-bc02-4b7c-bd55-b576c07bb09d

MattWhite-personal avatar Jul 22 '24 21:07 MattWhite-personal

It's quite possible that I have had an AAD P2 (as it was) trial on this tenant from a while back but that has long expired and no longer active

MattWhite-personal avatar Jul 22 '24 21:07 MattWhite-personal

Thanks @MattWhite-personal that's very intersting because the "eec0eb4f-6444-4f95-aba0-50c24d67f998" is basically telling maester that you got P2.

Can you test if the subscribedSkus is also reporting this back and if yes if the UI is doing the same (Entra portal

Invoke-MtGraphRequest -RelativeUri "subscribedSkus" | Select-Object -ExpandProperty servicePlans | Select-Object -ExpandProperty servicePlanId | Where-Object { $_ -eq "eec0eb4f-6444-4f95-aba0-50c24d67f998" }

f-bader avatar Jul 22 '24 21:07 f-bader

and changing the query to not just expand the SKU values

PS C:\Users\MatthewWhite> Invoke-MtGraphRequest -ApiVersion beta -RelativeUri 'organization' | Select-Object -ExpandProperty assignedPlans | Where-Object service -EQ "AADPremiumService"

assignedDateTime    capabilityStatus service           servicePlanId
----------------    ---------------- -------           -------------
31/03/2017 17:32:50 Deleted          AADPremiumService eec0eb4f-6444-4f95-aba0-50c24d67f998
13/01/2019 18:06:25 Enabled          AADPremiumService de377cbc-0019-4ec2-b77c-3f223947e102
31/01/2019 19:58:31 Enabled          AADPremiumService 41781fb2-bc02-4b7c-bd55-b576c07bb09d

The bottom two are Entra ID P1 and base Entra ID

you can see that the eec0eb4f-6444-4f95-aba0-50c24d67f998 which is EntraID P2 is Deleted

MattWhite-personal avatar Jul 22 '24 21:07 MattWhite-personal

@f-bader just checked and on a separate tenant that only ever had Business Premium the test is skipped

image

If the test logic for licensing can check the capabilityStatus is Enabled then I think this will correct my error. Not sure if @GeldHades27355 has also previously had an Entra P2 trial activated on their tenant

MattWhite-personal avatar Jul 22 '24 21:07 MattWhite-personal

I really hope the subscribedSkus endpoint will just report back the valid licenses only, This way I could switch it to this endpoint and don't have to double verify in the code. If you would be able to run the above cmd that would be awesome

f-bader avatar Jul 22 '24 21:07 f-bader

@MattWhite-personal It should be fixed in the latest preview version. I could verify in an older tenant that is now down to free and had P2 in the past and I could repro with the org endpoint. With the subscribedSkus this does no longer pose a problem

Install-Module -Name Maester -AllowPrerelease

Thank you very much

PS: Release process takes a few minutes

f-bader avatar Jul 22 '24 21:07 f-bader

PS C:\Users\MatthewWhite> Invoke-MtGraphRequest -RelativeUri "subscribedSkus" | Select-Object -ExpandProperty servicePlans | Select-Object -ExpandProperty servicePlanId | Where-Object { $_ -eq "eec0eb4f-6444-4f95-aba0-50c24d67f998" }
PS C:\Users\MatthewWhite>

LGTM

My alternative was to suggest the following change

Invoke-MtGraphRequest -ApiVersion beta -RelativeUri 'organization' | Select-Object -ExpandProperty assignedPlans | Where-Object {$_.service -EQ "AADPremiumService" -and $_.capabilityStatus -EQ "Enabled"}

either way think this should fix it

MattWhite-personal avatar Jul 22 '24 21:07 MattWhite-personal

https://www.powershellgallery.com/packages/Maester/0.2.36-preview

There it is @MattWhite-personal

f-bader avatar Jul 22 '24 21:07 f-bader

image

summary looks to have skipped MT.1012 and MT.1013 however the output looks different on the preview version

image

I don't know if this is other preview features that are causing change in the test output but looks better than before

MattWhite-personal avatar Jul 22 '24 21:07 MattWhite-personal

@f-bader One other observation is that the Entra recommendations also dont filter out based on P1 vs P2 status. Happy to raise a new issue to look at this but

image

MattWhite-personal avatar Jul 22 '24 21:07 MattWhite-personal

Hi @MattWhite-personal Would be great if you could create a separate issue. I will look into it over the course of the day

f-bader avatar Jul 23 '24 04:07 f-bader