8000 Duplicate Key Types in `cert_key_types` Passed to `find_cert_chain` · Issue #4763 · randombit/botan · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Duplicate Key Types in cert_key_types Passed to find_cert_chain #4763

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
Open
Seele-Official opened this issue Mar 9, 2025 · 2 comments
Open

Duplicate Key Types in cert_key_types Passed to find_cert_chain #4763

Seele-Official opened this issue Mar 9, 2025 · 2 comments

Comments

@Seele-Official
Copy link

I have encountered an issue where the API function Botan::Credentials_Manager::find_cert_chain is receiving duplicate key types through its cert_key_types parameter.
After some debugging, I tracked the root cause to the two Certificate_13 constructors that invoke the filter_signature_schemes function:

/**
 * Create a Server Certificate message
 */
Certificate_13::Certificate_13(const Client_Hello_13& client_hello,
                               Credentials_Manager& credentials_manager,
                               Callbacks& callbacks,
                               Certificate_Type cert_type) :
      // RFC 8446 4.4.2:
      //    [In the case of server authentication], this field
      //    SHALL be zero length
      m_request_context(), m_side(Connection_Side::Server) {
   BOTAN_ASSERT_NOMSG(client_hello.extensions().has<Signature_Algorithms>());

   const auto key_types = filter_signature_schemes(client_hello.signature_schemes());
   //something else......
}

/**
 * Create a Client Certificate message
 */
Certificate_13::Certificate_13(const Certificate_Request_13& cert_request,
                               std::string_view hostname,
                               Credentials_Manager& credentials_manager,
                               Callbacks& callbacks,
                               Certificate_Type cert_type) :
      m_request_context(cert_request.context()), m_side(Connection_Side::Client) {
   const auto key_types = filter_signature_schemes(cert_request.signature_schemes());
   //something else......
}

In both constructors, filter_signature_schemes converts the provided signature schemes into a std::vector<std::string> of key types. For example, if signature_schemes() returns algorithms such as "RSA_PSS_SHA384" and "RSA_PSS_SHA512", the filtering process results in a vector containing ["RSA", "RSA"], thus creating duplicate entries.

Potential Solution:

One straightforward approach to resolve this issue is to change the container in filter_signature_schemes from a std::vector<std::string> to a std::set<std::string>. But this operation may change the API function Botan::Credentials_Manager::find_cert_chain.
I would appreciate feedback on this approach and am happy to work on a patch if it aligns with the project's direction.
@randombit
Copy link
Owner

We cannot immediately change the API here since that would break compatability with existing clients. We might be able to deduplicate them before invoking the callback though - @reneme what do you think?

@reneme
Copy link
Collaborator
reneme commented Mar 17, 2025

Interesting finding @Seele-Official and thanks for the analysis that seems logical to me. I think deduplicating those entries won't do any harm (and this is the intended behavior anyway). I do like the std::set idea, because it clearly communicates this intent (and the small number of options won't cause performance issues from using a non-contiguous container either). Perhaps we could introduce that new API as an overload and deprecate the old one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@randombit @reneme @Seele-Official
0