Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(MS.AAD.6.1): password expiration must be configured for all domains #603

Merged
merged 6 commits into from
Mar 6, 2025

Conversation

jed-exotic
Copy link
Contributor

@jed-exotic jed-exotic commented Dec 26, 2024

addresses #594

This may be an opinionated change, and I am happy to discuss.

To comply with MS.AAD.6.1, all verified domains should be evaluated for this configuration value.

If configuring the primary domain is sufficient for compliance (i.e. the configured value for non-primary domains becomes irrelevant), then the isDefault attribute should be used to filter the results down to a single domain.

This PR is with the following requirement in mind:

ALL verified managed domains shall be configured to not require password expiry.

@jed-exotic jed-exotic requested a review from a team as a code owner December 26, 2024 17:27
@weyCC81
Copy link
Contributor

weyCC81 commented Dec 27, 2024

In my opinion, it's a good practice to check all domains, as the 'primary domain' does not always technically appear as 'isDefault.'

@merill
Copy link
Contributor

merill commented Jan 7, 2025

Added comment #594 (comment)

@merill
Copy link
Contributor

merill commented Feb 10, 2025

Holding PR to update test to align with suggestion from @soulemike (see link above).

I agree more details here would be good. Likely returning each domain in a table with the configuration and pass state, similar to other tests. Something like the below. With the test result validating all verified managed and federated domains are valid.

Domain (Default) Verified Type Validation
contoso.com (✔️) Verified Managed ✅ Pass
sub.contoso.com () Verified Managed ❌ Fail
contoso.onmicrosoft.com () Verified Managed ✅ Pass
oldbrand.com () Verified Federated 🗄️ Skipped
google.com () Unverified Managed 🗄️ Skipped

Reviewing the CISA REGO it looks like they do something similar and measure the aggregate, so just looks like something I missed when wiring this up. https://github.com/cisagov/ScubaGear/blob/main/PowerShell/ScubaGear/Rego/AADConfig.rego#L681

@weyCC81 let us know if you would like to pick this up, if not I can take a stab. Thanks!

@merill merill added the enhancement New feature or request label Feb 10, 2025
@soulemike
Copy link
Contributor

soulemike commented Mar 6, 2025

Updated PR to include details. Realized I missed a skip condition. Adding it quick.

Copy link
Contributor

@merill merill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@merill merill merged commit 3cf0744 into maester365:main Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants