Skip to content

Update CmsCommands to use Store vs cert provider #11643

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
TravisEz13 merged 25 commits into from
Feb 6, 2020
Merged

Update CmsCommands to use Store vs cert provider #11643

TravisEz13 merged 25 commits into from
Feb 6, 2020

Conversation

@mikeTWC1984
Copy link
Contributor

@mikeTWC1984 mikeTWC1984 commented Jan 21, 2020 •
edited
Loading

PR Summary

This PR replaces cert provider (windows only) with X509Store class to resolve certificate by name/thumbprint on CmsUtils.

PR Work items:

  • ResolveFromSubjectName and ResolveFromThumbprint - remove cert: provider call and merge into single method ( both lookups can utilize X509Certificate2Collection.Find )
  • Remove GetCertEKU (that uses native calls). Use X509Certificate2 Extensions property instead in CertHasOId method.
  • LocalMachine/My store - Windows only
  • replace List<509Certificate2> with X509Certificate2Collection on related methods.
  • Update tests
  • test on Mac
  • add wildcard for subjectname lookup

PR Checklist

@ghost ghost assigned iSazonov Jan 21, 2020
@msftclas
Copy link

msftclas commented Jan 21, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@mikeTWC1984 mikeTWC1984 mentioned this pull request Jan 21, 2020
Closed
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jan 22, 2020
@iSazonov iSazonov added this to the 7.1.0-preview.1 milestone Jan 22, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Jan 22, 2020 •
edited by unfurl-links bot
Loading

@mikeTWC1984 We should export the cmdlets on Unix-s:

Also please review tests https://github.com/PowerShell/PowerShell/tree/master/test/powershell/Modules/Microsoft.PowerShell.Security

GitHub
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.
GitHub
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

@iSazonov iSazonov changed the title updated CmsCommands to use Store vs cert provider WIP: Update CmsCommands to use Store vs cert provider Jan 23, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Jan 23, 2020

I added WIP until the PR is ready to review.

@mikeTWC1984 mikeTWC1984 changed the title WIP: Update CmsCommands to use Store vs cert provider Update CmsCommands to use Store vs cert provider Jan 23, 2020
@mikeTWC1984 mikeTWC1984 changed the title Update CmsCommands to use Store vs cert provider WIP: Update CmsCommands to use Store vs cert provider Jan 23, 2020
@mikeTWC1984
Copy link
Contributor Author

mikeTWC1984 commented Jan 23, 2020

OK. It's pretty much ready for review, but somehow Powershell-CI pester test is failing. I ran 3 tests today, all failed for linux, 2 for Mac and 1 for Windows. I also tried Start-PSPester locally for my branch and Powershell's master and got ~30 errors. A the moment it's not quite clear what's wrong. . Probably some temp issue, will keep experimenting. The build itself goes through, CMS commands seem to work, but I haven't done thorough testing yet.

@mikeTWC1984 mikeTWC1984 changed the title WIP: Update CmsCommands to use Store vs cert provider Update CmsCommands to use Store vs cert provider Jan 24, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Jan 24, 2020

@mikeTWC1984 You should enable the cmdlets in DefaultCommands.Tests.ps1 too (See link above).

@xtqqczze
Copy link
Contributor

xtqqczze commented Jan 25, 2020

This PR does not appear to conflict with #11590

@mikeTWC1984 mikeTWC1984 changed the title WIP: Update CmsCommands to use Store vs cert provider Update CmsCommands to use Store vs cert provider Feb 4, 2020
@mikeTWC1984
Copy link
Contributor Author

mikeTWC1984 commented Feb 4, 2020

OK, tests finally pass on Mac. Turns out certs got added to the store automatically during creation (X509Store.add actually doesn't work ) . I'll work on the Doc today or tomorrow.

iSazonov
iSazonov reviewed Feb 4, 2020
View reviewed changes
iSazonov
iSazonov reviewed Feb 4, 2020
View reviewed changes
PaulHigin
PaulHigin suggested changes Feb 4, 2020
View reviewed changes
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Feb 4, 2020
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Feb 4, 2020
@mikeTWC1984
Copy link
Contributor Author

mikeTWC1984 commented Feb 4, 2020 •
edited
Loading

Ok, applied all the latest changes. I was back and forth between using CertificateCollection.Find and foreach loop to match SubjectName, and I'm finally sticking with foreach, so it's the same approach as original version had. I realized find will always do "contains" match, but I guess mostly user would need exact match.

PaulHigin
PaulHigin approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

LGTM
We still need to do a security review, but the code changes look good.

iSazonov
iSazonov reviewed Feb 5, 2020
View reviewed changes
src/System.Management.Automation/security/SecuritySupport.cs Outdated
Comment on lines 1273 to 1276
if (subjectNamePattern.IsMatch(cert.Subject))
{
certificatesToProcess.Add(cert);
}
Copy link
Collaborator

@iSazonov iSazonov Feb 5, 2020

Choose a reason for hiding this comment

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

@mikeTWC1984 If you ask about this IsMatch() it is ok to preserve the old behavior until we get a negative feedback.

Copy link
Contributor Author

@mikeTWC1984 mikeTWC1984 Feb 5, 2020

Choose a reason for hiding this comment

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

This one is OK. It's bit counterintuitive to use wildcard (and directory) when expecting single item, but helps when typing those names manually. After going back and forth with this, I realized omitting CN= prefix in subject name would be very useful, so I added one extra IsMatch for simple name too

iSazonov
iSazonov reviewed Feb 6, 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 two minor comments.

@mikeTWC1984 mikeTWC1984 mentioned this pull request Feb 6, 2020
Closed
11 tasks
@mikeTWC1984
Copy link
Contributor Author

mikeTWC1984 commented Feb 6, 2020

@TravisEz13 I think this is ready for security review. I also filed Documentation issue.

@TravisEz13
Copy link
Member

TravisEz13 commented Feb 6, 2020

@mikeTWC1984 Yes, I've already schedule the review. The reviews are Confidential and we have to decide how to communicate the results after the review.

@TravisEz13
Copy link
Member

TravisEz13 commented Feb 6, 2020

@mikeTWC1984 Security review summary: we don't think the security footing of the cmdlet has changed.

@TravisEz13 TravisEz13 changed the title Update CmsCommands to use Store vs cert provider Update CmsCommands to use Store vs cert provider Feb 6, 2020
@TravisEz13 TravisEz13 merged commit 69bf704 into PowerShell:master Feb 6, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Feb 7, 2020

@mikeTWC1984 Thanks for your contribution!

@mikeTWC1984
Copy link
Contributor Author

mikeTWC1984 commented Feb 7, 2020

Cool, thanks all. There are few enhancements we can add to those commands, I'll probably submit an issue or PR sometime soon. I guess it's also the time to update documentation, will work on that.

@iSazonov
Copy link
Collaborator

iSazonov commented Feb 7, 2020

@mikeTWC1984 Open new issue(s) if there is a field for discussion.

@ghost
Copy link

ghost commented Mar 26, 2020

🎉v7.1.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR Indicates that a PR is out for the issue label Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

+1 more reviewer

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@TravisEz13 TravisEz13

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log In-PR Indicates that a PR is out for the issue

Projects

None yet

Milestone

7.1.0-preview.1

Development

Successfully merging this pull request may close these issues.

6 participants

@mikeTWC1984 @msftclas @iSazonov @xtqqczze @TravisEz13 @PaulHigin