Skip to content

Change PKCS12 password to assist with credscan - #58807

Merged
bartonjs merged 2 commits into
dotnet:mainfrom
vcsjones:change-cert-pass
Sep 9, 2021
Merged

Change PKCS12 password to assist with credscan#58807
bartonjs merged 2 commits into
dotnet:mainfrom
vcsjones:change-cert-pass

Conversation

@vcsjones

@vcsjonesvcsjones commented Sep 8, 2021

Copy link
Copy Markdown
Member

@ghostghost added community-contribution Indicates that the PR has been added by a community member area-System.Security labels Sep 8, 2021
@ghost

ghost commented Sep 8, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

/cc @aik-jahoda

Author:vcsjones
Assignees:-
Labels:

area-System.Security, community-contribution

Milestone:-

@vcsjonesvcsjones added the test-enhancement Improvements of test source code label Sep 8, 2021
@vcsjones
vcsjones marked this pull request as draft September 8, 2021 15:38
@vcsjones

vcsjones commented Sep 8, 2021

Copy link
Copy Markdown
MemberAuthor

Converting to draft because apparently the password was supposed to be PLACEHOLDER. Will change it again.

@vcsjones
vcsjones marked this pull request as ready for review September 8, 2021 15:58
@bartonjs

bartonjs commented Sep 8, 2021

Copy link
Copy Markdown
Member

If we already have it suppressed, why are we changing it?

Does the new PKCS12 use the same encryption and MAC parameters, or are we possibly losing some edge coverage on DSA?

@vcsjones

vcsjones commented Sep 8, 2021•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

If we already have it suppressed, why are we changing it?

I got an email I should change it. I'm guessing the comment suppression wasn't enough and there maybe needed to be another above the .ctor for X509Certificate2 for the password itself.

Does the new PKCS12 use the same encryption and MAC parameters

Yes. Same PBE parameters, same key (DSA). Just changing the PBE password.

@aik-jahoda

aik-jahoda commented Sep 9, 2021

Copy link
Copy Markdown
Contributor

The problem is not in the certificate itself, but with

return new X509Certificate2(SampleDsaPfx, "velleity");

It complains about plaintext password. We decided to prefer PLACEHOLDER as a safe well known "password" which at first sight confirm it is test only password.

@aik-jahoda
aik-jahoda self-requested a review September 9, 2021 08:54
aik-jahoda
aik-jahoda approved these changes Sep 9, 2021
View reviewed changes

@aik-jahodaaik-jahoda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@bartonjs
bartonjs merged commit e55968b into dotnet:mainSep 9, 2021
@vcsjones
vcsjones deleted the change-cert-pass branch September 9, 2021 16:21
@aik-jahoda

aik-jahoda commented Sep 14, 2021

Copy link
Copy Markdown
Contributor

/backport to release/6.0

@github-actions

github-actionsBot commented Sep 14, 2021

Copy link
Copy Markdown
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1233560600

@github-actionsgithub-actionsBot mentioned this pull request Sep 14, 2021
Merged
@ghostghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bartonjsbartonjsbartonjs approved these changes
+1 more reviewer
@aik-jahodaaik-jahodaaik-jahoda approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Securitycommunity-contributionIndicates that the PR has been added by a community membertest-enhancementImprovements of test source code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vcsjones@bartonjs@aik-jahoda