Skip to content

Conversation

@vcsjones
Copy link
Member

@vcsjones vcsjones commented Sep 8, 2021

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

ghost commented Sep 8, 2021

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: -

@vcsjones vcsjones 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
Copy link
Member Author

vcsjones commented Sep 8, 2021

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
Copy link
Member

bartonjs commented Sep 8, 2021

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
Copy link
Member Author

vcsjones commented Sep 8, 2021

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
Copy link
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
Copy link
Contributor

@aik-jahoda aik-jahoda left a comment

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:main Sep 9, 2021
@vcsjones vcsjones deleted the change-cert-pass branch September 9, 2021 16:21
@aik-jahoda
Copy link
Contributor

/backport to release/6.0

@github-actions
Copy link
Contributor

@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Security community-contribution Indicates that the PR has been added by a community member test-enhancement Improvements of test source code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants