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

feat: add rust safe wrapper certificate match #5220

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

johubertj
Copy link
Contributor

@johubertj johubertj commented Mar 24, 2025

Resolved issues:

resolves #5185

Description of changes:

Adds a safe Rust wrapper method, Connection::certificate_match(), to expose the s2n_connection_get_certificate_match C API. Previously, this functionality was only accessible through the low-level FFI

Testing:

Added four unit tests to validate the behavior of the certificate_match Rust wrapper and ensure it correctly reflects the results of the underlying s2n_connection_get_certificate_match C API:

  • test_certificate_match_returns_exact_match: Verifies ExactMatch is returned when the client's SNI exactly matches the certificate.

  • test_certificate_match_returns_wildcard_match: Verifies WildcardMatch is returned when the SNI matches a wildcard domain in the certificate. (format - *.insect.hexapod)

  • test_certificate_match_returns_no_sni_match: Verifies NoSNI is returned when the client does not send an SNI.

  • test_certificate_match_returns_no_match: Verifies NoMatch is returned when the SNI does not match any certificate.

These tests provide coverage for all possible enum outcomes and ensure correct integration with the C layer.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Mar 24, 2025
@johubertj johubertj requested a review from jmayclin March 24, 2025 22:12
@johubertj johubertj requested a review from jmayclin March 25, 2025 22:57
@johubertj johubertj marked this pull request as ready for review March 25, 2025 22:57
@johubertj johubertj requested a review from jmayclin March 27, 2025 23:34
@suresr-aws suresr-aws requested a review from Copilot March 28, 2025 16:04
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a safe Rust wrapper method, Connection::certificate_match(), to expose the underlying C API s2n_connection_get_certificate_match, enabling clients to access certificate matching results through a high-level API.

  • Added a new enum (CertSNIMatch) with variants to represent certificate match outcomes and implemented conversion from the low-level enum.
  • Introduced the certificate_match() method in the Connection struct with corresponding unit tests to verify its behavior.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
bindings/rust/extended/s2n-tls/src/enums.rs Added CertSNIMatch enum and its TryFrom conversion for mapping.
bindings/rust/extended/s2n-tls/src/connection.rs Added certificate_match() method and unit test for certificate matching.
Comments suppressed due to low confidence (1)

bindings/rust/extended/s2n-tls/src/connection.rs:1149

  • [nitpick] The variable name 'version' is ambiguous in this context since it represents the certificate match result. Consider renaming it to 'cert_match' for improved clarity.
let mut version = s2n_cert_sni_match::SNI_NO_MATCH;

@johubertj johubertj requested a review from jmayclin March 29, 2025 00:06
@johubertj johubertj requested a review from maddeleine March 31, 2025 21:19

/// Test that the `certificate_match` Rust wrapper returns NoMatch enum
#[test]
fn test_certificate_match_returns_no_match() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have the test return result types like we do elsewhere, so that we can use ? instead of having all the expect assertions everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bindings: support s2n_connection_get_certificate_match
3 participants