Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
fix for issue 3476 #3486
fix for issue 3476 #3486
Changes from 3 commits
c3db7a8
f69f351
5860b37
f4f31d0
ce38bd7
8743a71
63a9897
d6a95df
0aea637
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this conditional, as the purpose of this function is to pass in the intermediates that should be used for chain building.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is important and should be present.
we will reach the new function ValidateAndUnpackCertWithIntermediates() via two paths
verifyInternal is called in some cases where we dont have an intermediate present in signature but we pass that as an argument in verification in those cases co.IntermediateCerts will have a valid intermediate cert but we will not be able to set the intermediatePool argument. one scenario is verification of Blob signatures.
so we need to have this condition if the intermediate Argument is Null that set the value from co.IntermediateCerts
I tested this by removing this condition and i could see verify blob signature test cases were failing so i put this condition back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I think I understand the issue. Correct me if I misunderstand, you're saying that co.IntermediateCerts may have been set by one of the functions that calls verifyInternal.
I think the fix would just be to pass
co.IntermediateCerts
toValidateAndUnpackCertWithIntermediates
on line 745 ifco.IntermediateCerts != nil
. Then you could remove this check.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be sufficient to create a pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed i will incorporate this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line needs to be updated too, we can't overwrite
co.IntermediateCerts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed i will incorporate this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mukuls77 What is the purpose of this extra set of certificates? It looks like this is testing the same behavior, are these e2e test changes needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case creates the scenario when multiple signatures are attached to a artifact with different root chains and we want to verify the artifact with any one of the root certificate. because there are more than one signatures attached so multiple threads will be created during verification and each thread will verify the allocated signature. During this operation previously when threads were modifying co.intermediates sometime verification use to fail, now with this fix it will not fail. so in my view this is an important test cases of verifying multiple signatures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that makes sense. Can you add a comment about this in the test file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we modify this test so that we still test verification with a single attached cert, then after verification, attach another and try verification?