Skip to content

Add password confirmation in Get-Credential #12782

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.

Sign up for GitHub

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

Jump to bottom
Merged
iSazonov merged 13 commits into from
Jul 22, 2020
Merged

Add password confirmation in Get-Credential #12782

iSazonov merged 13 commits into from
Jul 22, 2020

Conversation

@Shriram0908
Copy link
Contributor

@Shriram0908 Shriram0908 commented May 25, 2020 •
edited
Loading

PR Summary

Get-Credential : Add parameter -ReEnterPassword to resolve #10625

PR Context

By adding -ReEnterPassword parameter the user will be prompted to re-enter the password and performs a comparison. If comparison fails null is returned and user is notified about password mismatch.

PR Checklist

@ghost ghost assigned iSazonov May 25, 2020
@Shriram0908 Shriram0908 mentioned this pull request May 25, 2020
Closed
14 tasks
iSazonov
iSazonov reviewed May 25, 2020
View reviewed changes
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label May 25, 2020
@iSazonov iSazonov added this to the 7.1.0-preview.4 milestone May 25, 2020
@iSazonov iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label May 25, 2020
@Shriram0908 Shriram0908 requested a review from iSazonov May 26, 2020 10:00
@Shriram0908
Copy link
Contributor Author

Shriram0908 commented May 26, 2020

@iSazonov Thank you for the feedback. I have made the appropriate changes. Please review.

I see the below error message in Linux test. I can see the file is present. What could be casing this issue.

Executing all tests in 'test/powershell/Language/Operators/NullConditional.Tests.ps1', 'test/powershell/Language/Parser/Parsing.Tests.ps1', '/test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1' with Tags RequireSudoOnUnix
Resolve-Path: /home/vsts/work/1/a/bins/publish/Modules/Pester/4.10.1/Pester.psm1:1326
Line |
1326 |  …             & $script:SafeCommands['Resolve-Path'] -Path $unresolvedP …
     |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Cannot find path
     | '/test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1'
     | because it does not exist.



Tests completed in 191ms
Tests Passed: 0, Failed: 0, Skipped: 0, Pending: 0, Inconclusive: 0 
WARNING: Tests failed. See the issue below.
Exception: /home/vsts/work/1/s/tools/ci.psm1:672
Line |
 672 |  … ll -ne $_} | ForEach-Object { Test-PSPesterResults -ResultObject $_ }
     |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Parameter cannot be processed because the parameter name
     | 'Error' is ambiguous. Possible matches include: -ErrorAction
     | -ErrorVariable.


##[error]PowerShell exited with code '1'.

@Shriram0908 Shriram0908 requested a review from iSazonov May 26, 2020 15:47
iSazonov
iSazonov reviewed May 26, 2020
View reviewed changes
@Shriram0908 Shriram0908 requested a review from iSazonov May 27, 2020 05:52
iSazonov
iSazonov reviewed May 27, 2020
View reviewed changes
iSazonov
iSazonov reviewed May 27, 2020
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

LGTM with one minor comment.

@Shriram0908 Shriram0908 requested a review from iSazonov May 27, 2020 10:41
@iSazonov iSazonov requested a review from PaulHigin May 27, 2020 10:44
@iSazonov
Copy link
Collaborator

iSazonov commented May 27, 2020 •
edited
Loading

@Shriram0908 Please open new issue in PowerShell-Docs repo and add the reference to the PR description.

@Shriram0908 Shriram0908 mentioned this pull request May 27, 2020
Closed
12 tasks
@iSazonov iSazonov mentioned this pull request May 27, 2020
Closed
14 tasks
@iSazonov iSazonov removed the Documentation Needed in this repo Documentation is needed in this repo label May 28, 2020
@PaulHigin
Copy link
Contributor

PaulHigin commented May 28, 2020

Re-entering a password is normally done when creating a username/password, but not when using a password to access resources. I am not sure how useful this change is, particularly since it is opt-in via a parameter switch. I would like to get the committee's opinion before accepting the change.

@PaulHigin PaulHigin added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label May 28, 2020
PaulHigin
PaulHigin suggested changes May 28, 2020
View reviewed changes
src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceSecurity.cs Outdated
Copy link
Contributor

@PaulHigin PaulHigin May 28, 2020

Choose a reason for hiding this comment

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

Converting passwords to plain text, leaves multiple immutable string objects in memory, indefinitely. I think it would be better to convert to byte array for comparison, and the byte array values can be zeroed out afterwards. Also you can compare byte array length for early out.

Copy link
Contributor Author

@Shriram0908 Shriram0908 May 29, 2020

Choose a reason for hiding this comment

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

@PaulHigin Thank you for the review. I have updated the function to use byte array for comparison and have zeroed it in the finally clause. The memory space where the pointer was pointing had a null value between each byte so I had to double the length to use as size. Please review and provide your feedback.

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceSecurity.cs Outdated
Copy link
Contributor

@PaulHigin PaulHigin May 28, 2020

Choose a reason for hiding this comment

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

I wonder if the user should be prompting multiple times for re-entering password in this loop. It is not known if the password was incorrect the first or second time it was entered. So it may be better to exit with error immediately if the confirmation fails, and let the user try again from the beginning.

Copy link
Contributor Author

@Shriram0908 Shriram0908 May 29, 2020

Choose a reason for hiding this comment

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

I have changed the behavior.

Copy link
Collaborator

@iSazonov iSazonov May 29, 2020

Choose a reason for hiding this comment

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

and let the user try again from the beginning.

What is beginning? Go to user or password re-enter?

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 28, 2020
@ghost ghost removed this from the 7.1.0-preview.4 milestone May 28, 2020
Shriram0908 and others added 10 commits June 26, 2020 19:08
@Shriram0908
Improve code readability
c5e19c1 
Add new private function in ConsoleHostUserInterface to compare secure stings
Print "Passwords do not match." in red
Fix CodeFactor issues
@Shriram0908
Get-Credential: Add Pester test case
2cf47f0
@Shriram0908
Fix code formating
b8916a8
@Shriram0908
Remove confirm password loop
77331ed 
the cmdlet return null if the confirm password does not match
uses byte array for comparison of password
@Shriram0908
Use bit comparison for secure sting
deb354b 
Remove byte array comparison method
@Shriram0908 @iSazonov
Fix code formating
0e00f4f 
Co-authored-by: Ilya <darpa@yandex.ru>
@Shriram0908
Add early return if comparison fails
91361d8
@Shriram0908
Fix code formating
6688841
@Shriram0908
Rename from ReenterPassword to ConfirmPassword
65af943
@Shriram0908
Fix code factor issue
7bef04a
@ghost ghost added the Review - Needed The PR is being reviewed label Jul 3, 2020
@ghost
Copy link

ghost commented Jul 3, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@iSazonov iSazonov removed the Review - Needed The PR is being reviewed label Jul 22, 2020
@iSazonov iSazonov changed the title Get-Credential password confirmation Add password confirmation in Get-Credential Jul 22, 2020
@iSazonov iSazonov merged commit a6bd47f into PowerShell:master Jul 22, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Jul 22, 2020

@Shriram0908 Thanks for your contribution!

@Shriram0908
Copy link
Contributor Author

Shriram0908 commented Jul 23, 2020

Thank you @iSazonov and @PaulHigin for your feedback.

@Shriram0908 Shriram0908 deleted the Get-Credential-#10625 branch July 23, 2020 11:06
@daxian-dbw daxian-dbw added this to the 7.1.0-preview.7 milestone Jul 29, 2020
rjmholt
rjmholt reviewed Aug 4, 2020
View reviewed changes
src/System.Management.Automation/engine/hostifaces/MshHostUserInterface.cs
Comment on lines +946 to +953
public abstract PSCredential PromptForCredential(
string caption,
string message,
string userName,
bool confirmPassword,
string targetName,
PSCredentialTypes allowedCredentialTypes,
PSCredentialUIOptions options);
Copy link
Collaborator

@rjmholt rjmholt Aug 4, 2020

Choose a reason for hiding this comment

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

@TylerLeonhardt this may be why the PowerShell daily build has been causing CI issues.

There's a new abstract method in the host which has broken me:

An error has occurred that was not properly handled. Additional information is shown below. The PowerShell process will exit.
Unhandled exception. System.TypeLoadException: Method 'PromptForCredential' in type 'Microsoft.PowerShell.EditorServices.Services.PowerShell.Host.EditorServicesConsolePSHostUserInterface' from assembly 'Microsoft.PowerShell.EditorServices, Version=2.2.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.
   at Microsoft.PowerShell.EditorServices.Services.PowerShell.Host.EditorServicesConsolePSHost..ctor(ILoggerFactory loggerFactory, String name, Version version, PSHost internalHost, ConsoleReadLine readline)
   at Microsoft.PowerShell.EditorServices.Services.PowerShell.PowerShellExecutionService.Initialize() in C:\Users\Robert Holt\Documents\Dev\Microsoft\PowerShellEditorServices\src\PowerShellEditorServices\Services\PowerShell\PowerShellExecutionService.cs:line 286
   at Microsoft.PowerShell.EditorServices.Services.PowerShell.PowerShellExecutionService.RunTopLevelConsumerLoop() in C:\Users\Robert Holt\Documents\Dev\Microsoft\PowerShellEditorServices\src\PowerShellEditorServices\Services\PowerShell\PowerShellExecutionService.cs:line 244
   at System.Threading.ThreadHelper.ThreadStart_Context(Object state)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.ThreadHelper.ThreadStart()

I'll open an issue to track fixing this

Copy link
Collaborator

@iSazonov iSazonov Aug 4, 2020

Choose a reason for hiding this comment

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

Can we add new CI for ESS in the repo?

Copy link
Contributor Author

@Shriram0908 Shriram0908 Aug 4, 2020

Choose a reason for hiding this comment

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

@rjmholt , @iSazonov Anything I could do to help ?

Copy link
Collaborator

@iSazonov iSazonov Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@Shriram0908 Thanks! It is a question for MSFT team - they could add optional CIs for dependent tools so we could see possible regressions early. It could be on-demand CIs we run before merge.

@rjmholt rjmholt mentioned this pull request Aug 4, 2020
Closed
@TravisEz13 TravisEz13 modified the milestones: 7.1.0-preview.7, 7.1.0-preview.6 Aug 5, 2020
rjmholt added a commit that referenced this pull request Aug 6, 2020
@rjmholt
Revert "Add password confirmation in Get-Credential (#12782)"
fd09f72 
This reverts commit a6bd47f.
@rjmholt rjmholt mentioned this pull request Aug 6, 2020
Merged
@rjmholt rjmholt added CL-NotInBuild Indicates that a PR is reverted and not part of the build. and removed CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Aug 6, 2020
iSazonov pushed a commit that referenced this pull request Aug 7, 2020
@rjmholt
Revert "Add password confirmation in Get-Credential (#12782)" (#13372)
ef7c7d6 
This reverts commit a6bd47f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13 TravisEz13 is a code owner

+2 more reviewers

@rjmholt rjmholt rjmholt left review comments

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov iSazonov

Labels

CL-NotInBuild Indicates that a PR is reverted and not part of the build. Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

7.1.0-preview.6

Development

Successfully merging this pull request may close these issues.

Get-Credential password confirmation

7 participants

@Shriram0908 @iSazonov @PaulHigin @SteveL-MSFT @rjmholt @daxian-dbw @TravisEz13