Determine selected client certificate via thumbprint #5187
Conversation
WalkthroughThe PR replaces certificate serial-number comparison with thumbprint-based comparison in the CefSharp certificate callback wrapper, deriving and comparing thumbprints (case-insensitive) and removing the legacy serial-number code path. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Cef (invoker)
participant Wrapper as CefCertificateCallbackWrapper
participant Cert as X509Certificate2
Note over Wrapper: On certificate list received
Caller->>Wrapper: Provide certificate list
Wrapper->>Cert: Select certificate (X509Certificate2)
Cert-->>Wrapper: cert->Thumbprint (DER)
Wrapper->>Wrapper: compute thumbprintStr from DER
Wrapper->>Wrapper: certThumbprint = cert->Thumbprint
alt thumbprints match (case-insensitive)
Wrapper->>Caller: Accept/match certificate
else no match
Wrapper->>Caller: Continue search / reject
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CefSharp.Core.Runtime/Internals/CefCertificateCallbackWrapper.h (1)
55-74: Add error handling for certificate construction.The code lacks error handling for potential failures when constructing
X509Certificate2from DER-encoded bytes (line 66). If the certificate data is malformed or unsupported, an exception could be thrown. Additionally, when no matching certificate is found in the loop,_callback->Selectis never called - verify this is the intended behavior.Consider wrapping the certificate construction in a try-catch block:
for (; it != _certificateList.end(); ++it) { + try + { auto bytes((*it)->GetDEREncoded()); auto byteSize = bytes->GetSize(); auto bufferByte = gcnew cli::array<Byte>(byteSize); pin_ptr<Byte> src = &bufferByte[0]; // pin pointer to first element in arr bytes->GetData(static_cast<void*>(src), byteSize, 0); auto newcert = gcnew System::Security::Cryptography::X509Certificates::X509Certificate2(bufferByte); auto thumbprintStr = newcert->Thumbprint; if (certThumbprint == thumbprintStr) { _callback->Select(*it); break; } + } + catch (System::Exception^) + { + // Continue to next certificate if parsing fails + continue; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CefSharp.Core.Runtime/Internals/CefCertificateCallbackWrapper.h(2 hunks)
🔇 Additional comments (1)
CefSharp.Core.Runtime/Internals/CefCertificateCallbackWrapper.h (1)
53-53: LGTM! Thumbprint comparison is more reliable.Using thumbprints instead of serial numbers is the correct approach for uniquely identifying certificates, especially when dealing with certificates from different CAs or self-signed certificates.
CefSharp.Core.Runtime/Internals/CefCertificateCallbackWrapper.h
Outdated
Show resolved
Hide resolved
|
✅ Build CefSharp 140.1.140-CI5362 completed (commit b17d205070 by @crnzgm) |
2 participants
Fixes: #5185
Summary:
Changes: [specify the structures changed]
How Has This Been Tested?
Used the WinForms.Example project to load a website that requires a client certificate and verified that the selected one was sent to the server.
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes