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

Fix test sensitivity to PKCS7 certificate ordering #59818

Merged
merged 1 commit into from
Oct 11, 2021

Conversation

vcsjones
Copy link
Member

Android exports PKCS7 certificates in a different order, and the xunit assertion is sensitive to order. So we sort them first.

Fixes #59777.

After this PR and #59812 is merged, the X509Certificates test suite back to fully passing on Android.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 30, 2021
@ghost
Copy link

ghost commented Sep 30, 2021

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

Issue Details

Android exports PKCS7 certificates in a different order, and the xunit assertion is sensitive to order. So we sort them first.

Fixes #59777.

After this PR and #59812 is merged, the X509Certificates test suite back to fully passing on Android.

Author: vcsjones
Assignees: -
Labels:

area-System.Security

Milestone: -

@vcsjones vcsjones added os-android and removed area-System.Security community-contribution Indicates that the PR has been added by a community member labels Sep 30, 2021
@ghost
Copy link

ghost commented Sep 30, 2021

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

Android exports PKCS7 certificates in a different order, and the xunit assertion is sensitive to order. So we sort them first.

Fixes #59777.

After this PR and #59812 is merged, the X509Certificates test suite back to fully passing on Android.

Author: vcsjones
Assignees: -
Labels:

os-android

Milestone: -

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

ghost commented Sep 30, 2021

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

Issue Details

Android exports PKCS7 certificates in a different order, and the xunit assertion is sensitive to order. So we sort them first.

Fixes #59777.

After this PR and #59812 is merged, the X509Certificates test suite back to fully passing on Android.

Author: vcsjones
Assignees: -
Labels:

area-System.Security, os-android, community-contribution

Milestone: -

@vcsjones
Copy link
Member Author

vcsjones commented Sep 30, 2021

and removed area-System.Security community-contribution labels 5 minutes ago

Quietly files an internal bug ticket.

@bartonjs
Copy link
Member

Android exports PKCS7 certificates in a different order

My initial thought was "is it just reversed? can we just feed them in in a different order on Android?", but a few more seconds thought makes me guess that our other platforms end up sorting the certs by size as part of a DER SET OF ordering, and that Android is probably just taking them in the order presented.

If it's the DER sort thing, then only like 20 people on the planet can reason about it, so it's not worth "normalizing" the Android behavior.

@vcsjones
Copy link
Member Author

If it's the DER sort thing, then only like 20 people on the planet can reason about it, so it's not worth "normalizing" the Android behavior.

CMS/PKCS#7 as a specification talks a lot about BER, and only requires DER where canonical encoding is required, liked SignedAttributes. I don't think the SET OF CertificateChoices needs to be DER sorted. We already know that macOS uses indefinite length encoding where possible.

@akoeplinger akoeplinger merged commit 5f9fec9 into dotnet:main Oct 11, 2021
@vcsjones vcsjones deleted the 59777-fix branch October 11, 2021 14:04
@ghost ghost locked as resolved and limited conversation to collaborators Nov 10, 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 os-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android] System.Security.Cryptography.X509Certificates ExportCertificatePems_MultiCert Failure
3 participants