Skip to content
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

[release/6.0] Fix SignedCms certificate collection modification with attribute certificates #80209

Merged

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Jan 4, 2023

Backport of #79940 and #80195 to release/6.0

Customer Impact

Reported by a partner for dotnet/sign at #79935. When using SignedCms with a CMS that contains an attribute certificate, such as an RFC3161 timestamp issued by Azure Codesigning, the AddCertificate and RemoveCertificate APIs would raise an exception when adding or removing an X.509 certificate because they did not know how to process the attribute certificate. The impact of this is that these two APIs cannot function in the presence of an attribute certificate.

Testing

This introduces tests to validate behavior of a CMS that contains attribute certificates to prevent regressions.

Risk

Low. The changes are isolated and well understood.

IMPORTANT: Is this backport for a servicing release? Yes.

…ificates.

When adding or removing certificates from the certificateSet collection, we assumed that the collection would
only contain X.509 certificates. This changes the implementation so that when looking for duplicates, we skip
over choices that are not an X.509 certificate when looking for a duplicate.

The tests peek in to the SignedData ASN.1 to ensure that the attribute certificates are preserved during a round
trip when encoding and decoding a CMS.
@ghost
Copy link

ghost commented Jan 4, 2023

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #79940 and #80195 to release/6.0

Customer Impact

Reported by a partner for dotnet/sign at #79935. When using SignedCms with a CMS that contains an attribute certificate, such as an RFC3161 timestamp issued by Azure Codesigning, the AddCertificate and RemoveCertificate APIs would raise an exception when adding or removing an X.509 certificate because they did not know how to process the attribute certificate. The impact of this is that these two APIs cannot function in the presence of an attribute certificate.

Testing

This introduces tests to validate behavior of a CMS that contains attribute certificates to prevent regressions.

Risk

Low. The changes are isolated and well understood.

IMPORTANT: Is this backport for a servicing release? Yes.

Author: vcsjones
Assignees: -
Labels:

area-System.Security

Milestone: -

@vcsjones vcsjones changed the title Backport/pr 79940 to release/6.0 [release/6.0] Fix SignedCms certificate collection modification with attribute certificates Jan 4, 2023
@carlossanlop
Copy link
Member

@bartonjs if this is ready, please add the servicing-consider label and send an email to Tactics requesting approval.

@vcsjones how is the CI looking? are any of the failures related?

@vcsjones
Copy link
Member Author

vcsjones commented Jan 5, 2023

@carlossanlop the CI failures look unrelated. I think those failures are caused by the upgrade to macOS 12 in the release/6.0 branch which was merged yesterday: #80030

In that PR, the same tests failed.

@bartonjs bartonjs added Servicing-consider Issue for next servicing release review Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 5, 2023
@carlossanlop carlossanlop added this to the 6.0.14 milestone Jan 9, 2023
@carlossanlop
Copy link
Member

Approved by Tactics (6.0.14).
Signed off by area owner.
Required OOB changes look good.
CI failure unrelated: #80252
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit fb31012 into dotnet:release/6.0 Jan 9, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 8, 2023
@vcsjones vcsjones deleted the backport/pr-79940-to-release/6.0 branch May 3, 2023 21:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants