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.

Sign up for GitHub

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

Jump to bottom

Revert "schannel: reverse the order of certinfo insertions" #11536

Closed
wants to merge 1 commit into from
Closed

Revert "schannel: reverse the order of certinfo insertions" #11536

wants to merge 1 commit into from

Conversation

nmoinvaz
Copy link
Contributor

@nmoinvaz nmoinvaz commented Jul 28, 2023 •
edited

This reverts commit 8986df8.

Windows does not guarantee a particular certificate ordering, even though TLS may have its own ordering/relationship guarantees. Recent versions of Windows 11 reversed the ordering of ceritifcates returned by CertEnumCertificatesInStore, therefore this commit no longer works as initially intended. libcurl makes no guarantees about certificate ordering if the operating system can't.

See issue #9706 for more details.

@nmoinvaz
Revert "schannel: reverse the order of certinfo insertions"
63ec56c 
This reverts commit 8986df8.

Windows does not guarantee a particular certificate ordering, even though TLS
may have its own ordering/relationship guarantees. Recent versions
of Windows 11 reversed the ordering of ceritifcates returned by
CertEnumCertificatesInStore, therefore this commit no longer works as initially
intended. libcurl makes no guarantees about certificate ordering if the
operating system can't.
@github-actions github-actions bot added TLS Windows Windows-specific labels Jul 28, 2023
@jay
Copy link
Member

jay commented Jul 29, 2023

@RoguePointer80 do you have anything to add? the consensus is that order is not guaranteed

@RoguePointer80
Copy link

I'm ok with this revert. I read the linked issue; my understanding is that it is no longer necessary since on newer versions of Windows (22h2) the order is now the same as OpenSSL.
I understand Microsoft's statement that "the order is not guaranteed", but I wouldn't put so much emphasis on it. I believe that statement means they reserve the right to change it, without a contractual binding to backward compatibility. But the order isn't "random", as shown from testing it is consistent.

As for the suggestion of checking Windows version, and choosing to reverse or not the order of certificates: seems too much trouble for something fragile, and since libCurl doesn't guarantee anything that check would amount to over-engineering.

✅ :pr-approved:

@jay jay closed this in f540a39 Jul 29, 2023
@jay
Copy link
Member

jay commented Jul 29, 2023

Thanks

ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
@nmoinvaz @ptitSeb
Revert "schannel: reverse the order of certinfo insertions"
2123cbf 
This reverts commit 8986df8.

Windows does not guarantee a particular certificate ordering, even
though TLS may have its own ordering/relationship guarantees. Recent
versions of Windows 11 reversed the ordering of ceritifcates returned by
CertEnumCertificatesInStore, therefore this commit no longer works as
initially intended. libcurl makes no guarantees about certificate
ordering if the operating system can't.

Ref: curl#9706

Closes curl#11536
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder bagder approved these changes

@MarcelRaad MarcelRaad MarcelRaad approved these changes

Assignees
No one assigned
Labels
TLS Windows Windows-specific
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nmoinvaz @jay @RoguePointer80 @bagder @MarcelRaad