Skip to content

Add readable encryption type reporting#80

Merged
PrzemyslawKlys merged 3 commits into
masterfrom
codex/readable-encryption-types
May 6, 2026
Merged

Add readable encryption type reporting#80
PrzemyslawKlys merged 3 commits into
masterfrom
codex/readable-encryption-types

Conversation

@PrzemyslawKlys

Copy link
Copy Markdown
Member

Summary

  • add a local msDS-SupportedEncryptionTypes decoder with current Kerberos bit mappings
  • surface raw, readable, state, defaults, and encryption-family fields in Users, Computers, and ServiceAccounts reports
  • highlight DES, RC4, domain-default fallback, and explicit no-AES cases separately
  • update trust object decoding to use the same local decoder

Why

Invoke-ADEssentials only decoded supported encryption types for ServiceAccounts, while Users and Computers did not request or display the attribute. The existing dependency helper also did not include the AES session-key flag, and 0/not-set values needed to be shown as KDC/domain-default fallback rather than as explicit no-encryption evidence.

Validation

  • Test-ModuleManifest .\ADEssentials.psd1
  • Import-Module .\ADEssentials.psd1 -Force
  • Decoder samples for 0, $null, and 28
  • git diff --cached --check

@claude

claude Bot commented May 6, 2026

Copy link
Copy Markdown

Claude finished @PrzemyslawKlys's task —— View job


Code Review Complete ✅

I've reviewed PR #80 which adds readable encryption type reporting functionality. Here's my comprehensive feedback:

Todo List:

  • Read and analyze the new ConvertTo-WinADSupportedEncryptionTypes.ps1 decoder
  • Review configuration updates for Users, Computers, and ServiceAccounts
  • Examine public function changes
  • Check build script modifications
  • Assess code quality and PowerShell best practices
  • Evaluate potential bugs and security concerns
  • Consider performance implications
  • Review test coverage
  • Provide comprehensive feedback summary

Code Quality Assessment

Strengths

  1. Well-structured encoder function: The ConvertTo-WinADSupportedEncryptionTypes.ps1 function is well-designed with:

    • Comprehensive error handling for invalid values
    • Clear separation of different state scenarios (null/empty, configured as 0, configured with flags)
    • Proper type casting and validation
    • Consistent output object structure with meaningful boolean flags
  2. Security-focused design: The function correctly identifies security concerns:

    • DES encryption detection (UsesDESEncryption)
    • RC4 encryption detection (UsesRC4Encryption)
    • Domain default fallback scenarios (UsesDomainDefaults)
    • AES session key enforcement (EnforcesAESSessionKeys)
  3. Consistent integration: The encryption type decoder is properly integrated across Users, Computers, and ServiceAccounts with consistent field naming

  4. Good error handling: Functions include proper try-catch blocks and graceful handling of missing/invalid data

🚨 Issues Found

  1. Missing property in Get-WinADComputers.ps1:58-61:

    'DistinguishedName', 'LastLogonDate', 'PasswordLastSet', 'Enabled', 'DnsHostName', 'PasswordNeverExpires', 'PasswordNotRequired',
    'PasswordExpired', 'ManagedBy', 'OperatingSystemVersion', 'OperatingSystem' , 'TrustedForDelegation', 'WhenCreated', 'WhenChanged', 'PrimaryGroupID'
    'nTSecurityDescriptor', 'msDS-SupportedEncryptionTypes'

    Problem: Missing comma after 'PrimaryGroupID' line - this creates a syntax error.

  2. Missing property in Get-WinADUsers.ps1:54-58:

    'msExchRecipientDisplayType', 'pwdLastSet', "msDS-UserPasswordExpiryTimeComputed",
    'WhenCreated', 'WhenChanged'
    'nTSecurityDescriptor',
    'Country', 'Title', 'Department'
    'msds-resultantpso', 'msDS-SupportedEncryptionTypes'

    Problem: Missing commas after 'WhenChanged' and 'Department' lines.

⚠️ Potential Issues

  1. Type mapping completeness: The encryption type map in ConvertTo-WinADSupportedEncryptionTypes.ps1:55-66 covers current Kerberos encryption types, but consider adding comments about RFC standards and version compatibility.

  2. Performance considerations: Adding 'msDS-SupportedEncryptionTypes' to property lists will increase AD query payload. This is acceptable for the security benefits but worth noting.

  3. Trust object changes: The changes to Get-WinADTrustObject.ps1:80 are minimal and appropriate, but ensure trust objects actually populate this attribute.

📋 Recommendations

  1. Fix syntax errors: The missing commas in property arrays will cause PowerShell parsing errors and need immediate correction.

  2. Add parameter validation: Consider adding [ValidateNotNull()] to the $Value parameter in the encryption decoder.

  3. Enhance documentation: The encryption type constants could benefit from RFC references or Microsoft documentation links.

  4. Test coverage: Ensure test cases cover:

    • Null/empty encryption type values
    • Various bit combinations
    • Invalid input handling
    • Domain default scenarios

Security Assessment

Good security practices:

  • Properly identifies weak encryption (DES/RC4)
  • Highlights domain default fallback scenarios
  • Flags missing AES configuration appropriately
  • HTML reporting includes visual indicators for security issues

Performance Impact

⚠️ Moderate impact: Adding encryption type queries will increase AD query overhead slightly, but this is justified by the security visibility gained.


Overall Assessment: This is a valuable security-focused enhancement with good implementation quality. The syntax errors need immediate fixing, but the core functionality is well-designed and follows PowerShell best practices.

* Updated `ModuleVersion` to `1.0.3` in `ADEssentials.psd1`.
* Refactored `Build-Module.ps1` for improved clarity and organization.
* Ensured compatibility with required modules and updated documentation paths.
…ation

* Fixed the typo from `[PSCUstomObject]` to `[PSCustomObject]` to ensure proper object creation.
@PrzemyslawKlys
PrzemyslawKlys merged commit d82734d into master May 6, 2026
1 check passed
@PrzemyslawKlys
PrzemyslawKlys deleted the codex/readable-encryption-types branch May 6, 2026 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant