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

xADUser: Adds 'Negotiate' option when testing passwords for ADCS integration #97

Merged
merged 7 commits into from
Jul 8, 2016

Conversation

iainbrighton
Copy link
Contributor

@iainbrighton iainbrighton commented May 23, 2016


This change is Reviewable

… Directory Certificate Services integration

Adds descriptions to parameters
Adds descriptions to MSFT_xADUser.schema.mof
Fixes dsccommunity#61
@TravisEz13
Copy link
Contributor

Reviewed 1 of 2 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 976 [r2] (raw file):

        $UserName,
        $Password.GetNetworkCredential().Password,
        [System.DirectoryServices.AccountManagement.ContextOptions]::Negotiate

technically this is a downgrade of security, perhaps this should be a property?


Comments from Reviewable

@iainbrighton
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 976 [r2] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

technically this is a downgrade of security, perhaps this should be a property?

It appears the default value is `ContextOptions.Negotiate | ContextOptions.Signing | ContextOptions.Sealing`. Looking at the [documentation](https://msdn.microsoft.com/en-us/library/system.directoryservices.accountmanagement.contextoptions%28v=vs.110%29.aspx), it appears that `ContextOptions.SecureSocketLayer` needs to be added to support ADCS. However, I think this will break authentication when ADCS is not deployed.

I'll perform some checks locally and implement an UseSSL parameter if required. I'm not sure why ContextOptions.Negotiate would have worked on its own (with ADCS deployed) and ContextOptions.SecureSocketLayer not specified?


Comments from Reviewable

@iainbrighton
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 976 [r2] (raw file):

Previously, iainbrighton (Iain Brighton) wrote…

It appears the default value is ContextOptions.Negotiate | ContextOptions.Signing | ContextOptions.Sealing. Looking at the documentation, it appears that ContextOptions.SecureSocketLayer needs to be added to support ADCS. However, I think this will break authentication when ADCS is not deployed.

I'll perform some checks locally and implement an UseSSL parameter if required. I'm not sure why ContextOptions.Negotiate would have worked on its own (with ADCS deployed) and ContextOptions.SecureSocketLayer not specified?

Right - riddle me this, Batman. I've installed ADCS in a single DC domain. When attempting to set user passwords with the xADUser resource and ADCS installed,

Here's the original call to ValidateCredentials where I do get the errors this PR is supposed to fix:

return $principalContext.ValidateCredentials(
    $UserName,
    $Password.GetNetworkCredential().Password
);

Here's the output:

VERBOSE: [CONTROLLER]: LCM:  [ Start  Test     ]  [[xADUser]Iainbrighton]
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Importing the module MSFT_xADUser in force mode.
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Retrieving Active Directory user 'IainBrighton' (IainBrighton@lab.local) ...
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Active Directory user 'IainBrighton' (IainBrighton@lab.local) is present.
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Creating connection to Active Directory domain 'lab.local' ...
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Checking Active Directory user 'IainBrighton' password ...
Exception calling "ValidateCredentials" with "2" argument(s): "The server cannot handle directory requests."
    + CategoryInfo          : NotSpecified: (:) [], CimException
    + FullyQualifiedErrorId : DirectoryOperationException
    + PSComputerName        : localhost
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] User 'Password' property is NOT in the desired state. Expected '<Password>', actual '<Password>'.
VERBOSE: [CONTROLLER]: LCM:  [ End    Test     ]  [[xADUser]Iainbrighton]  in 3.3740 seconds.

If I add just the Negotiate option:

return $principalContext.ValidateCredentials(
    $UserName,
    $Password.GetNetworkCredential().Password,
    [System.DirectoryServices.AccountManagement.ContextOptions]::Negotiate 
);

And run this, it does work, but is potentially less secure:

VERBOSE: [CONTROLLER]: LCM:  [ Start  Test     ]  [[xADUser]Iainbrighton]
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Importing the module MSFT_xADUser in force mode.
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Retrieving Active Directory user 'IainBrighton' (IainBrighton@lab.local) ...
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Active Directory user 'IainBrighton' (IainBrighton@lab.local) is present.
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Creating connection to Active Directory domain 'lab.local' ...
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Checking Active Directory user 'IainBrighton' password ...
VERBOSE: [CONTROLLER]: LCM:  [ End    Test     ]  [[xADUser]Iainbrighton]  in 2.4220 seconds.

Now - if I add the "default" context options (as per the documentation):

return $principalContext.ValidateCredentials(
    $UserName,
    $Password.GetNetworkCredential().Password,
    [System.DirectoryServices.AccountManagement.ContextOptions]::Negotiate -bor
        [System.DirectoryServices.AccountManagement.ContextOptions]::Signing -bor
            [System.DirectoryServices.AccountManagement.ContextOptions]::Sealing
);

And run the configuration:

VERBOSE: [CONTROLLER]: LCM:  [ Start  Test     ]  [[xADUser]Iainbrighton]
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Importing the module MSFT_xADUser in force mode.
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Retrieving Active Directory user 'IainBrighton' (IainBrighton@lab.local) ...
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Active Directory user 'IainBrighton' (IainBrighton@lab.local) is present.
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Creating connection to Active Directory domain 'lab.local' ...
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Checking Active Directory user 'IainBrighton' password ...
VERBOSE: [CONTROLLER]: LCM:  [ End    Test     ]  [[xADUser]Iainbrighton]  in 2.2500 seconds.
VERBOSE: [CONTROLLER]: LCM:  [ Skip   Set      ]  [[xADUser]Iainbrighton]

It works?! Aren't these supposed to be the defaults anyway that should be used in the first (and existing) example?! Confused!


Comments from Reviewable

@TravisEz13
Copy link
Contributor

Review status: all files reviewed at latest revision, 1 unresolved discussion.


DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 976 [r2] (raw file):

Previously, iainbrighton (Iain Brighton) wrote…

Right - riddle me this, Batman. I've installed ADCS in a single DC domain. When attempting to set user passwords with the xADUser resource and ADCS installed,

Here's the original call to ValidateCredentials where I do get the errors this PR is supposed to fix:

return $principalContext.ValidateCredentials(
    $UserName,
    $Password.GetNetworkCredential().Password
);

Here's the output:

VERBOSE: [CONTROLLER]: LCM:  [ Start  Test     ]  [[xADUser]Iainbrighton]
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Importing the module MSFT_xADUser in force mode.
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Retrieving Active Directory user 'IainBrighton' (IainBrighton@lab.local) ...
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Active Directory user 'IainBrighton' (IainBrighton@lab.local) is present.
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Creating connection to Active Directory domain 'lab.local' ...
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Checking Active Directory user 'IainBrighton' password ...
Exception calling "ValidateCredentials" with "2" argument(s): "The server cannot handle directory requests."
    + CategoryInfo          : NotSpecified: (:) [], CimException
    + FullyQualifiedErrorId : DirectoryOperationException
    + PSComputerName        : localhost
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] User 'Password' property is NOT in the desired state. Expected '<Password>', actual '<Password>'.
VERBOSE: [CONTROLLER]: LCM:  [ End    Test     ]  [[xADUser]Iainbrighton]  in 3.3740 seconds.

If I add just the Negotiate option:

return $principalContext.ValidateCredentials(
    $UserName,
    $Password.GetNetworkCredential().Password,
    [System.DirectoryServices.AccountManagement.ContextOptions]::Negotiate 
);

And run this, it does work, but is potentially less secure:

VERBOSE: [CONTROLLER]: LCM:  [ Start  Test     ]  [[xADUser]Iainbrighton]
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Importing the module MSFT_xADUser in force mode.
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Retrieving Active Directory user 'IainBrighton' (IainBrighton@lab.local) ...
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Active Directory user 'IainBrighton' (IainBrighton@lab.local) is present.
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Creating connection to Active Directory domain 'lab.local' ...
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Checking Active Directory user 'IainBrighton' password ...
VERBOSE: [CONTROLLER]: LCM:  [ End    Test     ]  [[xADUser]Iainbrighton]  in 2.4220 seconds.

Now - if I add the "default" context options (as per the documentation):

return $principalContext.ValidateCredentials(
    $UserName,
    $Password.GetNetworkCredential().Password,
    [System.DirectoryServices.AccountManagement.ContextOptions]::Negotiate -bor
        [System.DirectoryServices.AccountManagement.ContextOptions]::Signing -bor
            [System.DirectoryServices.AccountManagement.ContextOptions]::Sealing
);

And run the configuration:

VERBOSE: [CONTROLLER]: LCM:  [ Start  Test     ]  [[xADUser]Iainbrighton]
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Importing the module MSFT_xADUser in force mode.
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Retrieving Active Directory user 'IainBrighton' (IainBrighton@lab.local) ...
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Active Directory user 'IainBrighton' (IainBrighton@lab.local) is present.
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Creating connection to Active Directory domain 'lab.local' ...
VERBOSE: [CONTROLLER]:                            [[xADUser]Iainbrighton] Checking Active Directory user 'IainBrighton' password ...
VERBOSE: [CONTROLLER]: LCM:  [ End    Test     ]  [[xADUser]Iainbrighton]  in 2.2500 seconds.
VERBOSE: [CONTROLLER]: LCM:  [ Skip   Set      ]  [[xADUser]Iainbrighton]

It works?! Aren't these supposed to be the defaults anyway that should be used in the first (and existing) example?! Confused!

Let's discuss offline

Comments from Reviewable

@TravisEz13
Copy link
Contributor

Review status: all files reviewed at latest revision, 2 unresolved discussions.


DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 973 [r2] (raw file):

    }
    Write-Verbose -Message ($LocalizedData.CheckingADUserPassword -f $UserName);
    return $principalContext.ValidateCredentials(

Merge conflicts must be resolved.


DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 976 [r2] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

Let's discuss offline

I suspect this is not the default:
  [System.DirectoryServices.AccountManagement.ContextOptions]::Negotiate -bor
     [System.DirectoryServices.AccountManagement.ContextOptions]::Signing -bor
         [System.DirectoryServices.AccountManagement.ContextOptions]::Sealing

but this is

     [System.DirectoryServices.AccountManagement.ContextOptions]::Signing -bor
         [System.DirectoryServices.AccountManagement.ContextOptions]::Sealing

But,​ I cannot find doc's I believe. What I am more sure of is that negotiate is not part of the default.


Comments from Reviewable

@iainbrighton
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 2 unresolved discussions.


DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 976 [r2] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

I suspect this is not the default:

  [System.DirectoryServices.AccountManagement.ContextOptions]::Negotiate -bor
     [System.DirectoryServices.AccountManagement.ContextOptions]::Signing -bor
         [System.DirectoryServices.AccountManagement.ContextOptions]::Sealing

but this is

     [System.DirectoryServices.AccountManagement.ContextOptions]::Signing -bor
         [System.DirectoryServices.AccountManagement.ContextOptions]::Sealing

But,​ I cannot find doc's I believe. What I am more sure of is that negotiate is not part of the default.

I've added a 'PassswordAuthenticationContext' parameter. Not setting this or setting it to 'Default' is as we were. To drop to NTLM authentication, you'll need to explicitly set the parameter to 'Negotiate'.

Comments from Reviewable

@iainbrighton
Copy link
Contributor Author

iainbrighton commented Jun 9, 2016

@TravisEz13 @KarolKaczmarek Have the PSSA checks been changed recently? Do I need to now fix those PSSA errors in the example files? If so, is PSSA v1.6 now being used so I can suppress the rules in the example configurations?

What's more - why are the [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSAvoidUsingUserNameAndPassWordParams', '')] rule suppressions no longer working (they have been in for a while)? I can no longer suppress them with v1.6 - is this version broken? 😠

@TravisEz13
Copy link
Contributor

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 973 [r2] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

Merge conflicts must be resolved.

builds are failing now

DSCResources/MSFT_xADUser/MSFT_xADUser.schema.mof, line 48 [r3] (raw file):

    [Write, Description("Specifies the Active Directory Domain Services instance to use to perform the task.")] String DomainController;
    [Write, Description("Specifies the user account credentials to use to perform this task"), EmbeddedInstance("MSFT_Credential")] String DomainAdministratorCredential;
    [Write, Description("Specifies the authentication context type used when testing passwords"), ValueMap{"Default","Negotiate"},Values{"Default","Negotiate"}] String PasswordAuthenticationContext;

New-PSSession and most CmdLets I see just call this Authentication


Comments from Reviewable

@iainbrighton
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 973 [r2] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

builds are failing now

Yes - this is due to changes in the PSSA module/build flagging things incorrectly?

DSCResources/MSFT_xADUser/MSFT_xADUser.schema.mof, line 48 [r3] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

New-PSSession and most CmdLets I see just call this Authentication

Agreed. However, using 'Authentication' would imply its the authentication that's used throughout the resource to communicate with AD? Remember, this authentication mechanism is _only_ used if a password is specified for password verification - nothing else.

Comments from Reviewable

@PlagueHO
Copy link
Member

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 973 [r2] (raw file):

Previously, iainbrighton (Iain Brighton) wrote…

Yes - this is due to changes in the PSSA module/build flagging things incorrectly?

I've fixed all the PSSA failures in PR #100.

Comments from Reviewable

@TravisEz13
Copy link
Contributor

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


DSCResources/MSFT_xADUser/MSFT_xADUser.schema.mof, line 48 [r3] (raw file):

Previously, iainbrighton (Iain Brighton) wrote…

Agreed. However, using 'Authentication' would imply its the authentication that's used throughout the resource to communicate with AD? Remember, this authentication mechanism is only used if a password is specified for password verification - nothing else.

remove the word context or reorder to PasswordContextAuthentication.

Comments from Reviewable

…into Issue61

# Conflicts:
#	DSCResources/MSFT_xADUser/MSFT_xADUser.psm1
#	README.md
#	Tests/Unit/MSFT_xADUser.Tests.ps1
…hentication in MSFT_xADUser.schema.mof

Updates manager description
@iainbrighton
Copy link
Contributor Author

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions.


DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 973 [r2] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I've fixed all the PSSA failures in PR #100.

@PlagueHO - Thanks! I've resolved the merge conflicts and now things are building successfully 👍

DSCResources/MSFT_xADUser/MSFT_xADUser.schema.mof, line 48 [r3] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

remove the word context or reorder to PasswordContextAuthentication.

I've removed 'Context' from the parameter so it's now just `PasswordAuthentication`

Comments from Reviewable

@TravisEz13
Copy link
Contributor

Reviewed 4 of 4 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@TravisEz13 TravisEz13 merged commit ae5c3b3 into dsccommunity:dev Jul 8, 2016
@iainbrighton iainbrighton deleted the Issue61 branch July 8, 2016 21:03
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.

4 participants