[2.1] Credscan second round - #43052
Conversation
danmoseley
left a comment
danmoseley
left a comment
There was a problem hiding this comment.
LGTM, let's get crypto/networking signoff and the tests passing.
|
@aik-jahoda shouldn't tests be running? It looks like it's building, but not testing. |
|
seems like GH is broken @danmoseley ;( I don't know if there is way how to kick it manually. |
1 similar comment
|
seems like GH is broken @danmoseley ;( I don't know if there is way how to kick it manually. |
|
Ah, I guess we restart after the GH outage... |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
wfurt
left a comment
wfurt
left a comment
There was a problem hiding this comment.
LGTM.
I like the PLACEHOLDER approach.
|
Branches got closed yesterday, we might have to wait for the may release to merge this one. |
|
@Anipik is there a way to allow test-only changes like this to get merged anyway? The reason is is that it's a high priority task to clear out all these issues. I don't think we can hit deadline if we can't commit to the branch - or at least it would be a lot easier. If the changes are test only, they won't flow anywhere - is the changing SHA the problem? I wonder if we can work around that somehow. |
|
restarting CI as github is back - in the hope it makes tests run this time (?) |
|
@krwq could you please take a look as well, so we can merge this? CI doesn't run tests in 2.1, but @aik-jahoda has run them locally, and also will look at the official build results after merge. |
|
or @bartonjs maybe |
|
Still needs update to runtime-assets version to make tests pass (although CI doesn't run them..) |
|
Please do not change any of the passwords/etc used in the crypto tests. Get them exempted instead. |
|
@bartonjs can you say more? My assumption is that the passwords were arbitrary here. |
|
I'm guessing the reason is - non zero chance of no longer verifying whatever the test was verifying with the previous arbitary passwords. That makes sense and we can exempt those. |
|
Looking again at these specific instances, my initial thought that it was deviating from published test vectors is incorrect (e.g. the Rfc2898DeriveBytes tests the changes are in the "we added these for variation" section, not the "this is from a public spec" case. The X509 ExportTests are OK to change, the password there is definitely arbitrary. The ones that are changing a PFX are harder. There are subtle changes:
Neither of them changed to having a default persisted key on Windows, but it took me 5-10 minutes to verify that. Since I've done the work to verify, I guess it's OK, but to me a better default position is "crypto tests need lots of things that look like creds, exempt them." |
|
Makes sense. I don't want any chance of defeating our test coverage. We're already going to have lots of exemptions. @aik-jahoda would it be possible to revert the changes relating to crypto? Going forward for new tests let's use PLACEHOLDER though! |
|
@bartonjs I have reverted the files you mentioned, can you please take another look? |
|
The Rfc2898Tests.cs was not reverted as if I understand well, the changes there are safe as it is in "we added these for variation" section. Am I right @bartonjs ? |
|
Just need package version and sigonff, then can merge. |
|
Still LGTM... |
* Credscan second round * Remove crypto changes * Update system.net.testdata * fix UriRelativeResolutionTest.cs * Add fix for HttpClientHandlerTest.cs
* Remove crypto changes * Update system.net.testdata * fix UriRelativeResolutionTest.cs * [2.1] Credscan second round (dotnet#43052) * Credscan second round * Remove crypto changes * Update system.net.testdata * fix UriRelativeResolutionTest.cs * Add fix for HttpClientHandlerTest.cs * more credscan * Add more fixes Co-authored-by: Dan Moseley <danmose@microsoft.com>
5 participants
Second round of Credscan effort
First we need merge dotnet/runtime-assets#124