Conversation
5f54234 to
c37bd2f
Compare
|
Retest this please AgentOfflineException |
622feb1 to
1814bce
Compare
1814bce to
9192db1
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements fixes for nginx 1.28.1 compatibility, primarily addressing session ticket handling and certificate management. The changes enable peer certificates to be stored in session tickets and add support for streaming BIO sources.
Changes:
- Modified session ticket structures to support variable-length peer certificates
- Enhanced PEM reading to handle streaming sources (pipes/FIFOs)
- Updated stack operations for proper shallow/deep copying semantics
- Fixed memory handling in certificate and key management
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfssl/internal.h | Made session ticket structures variable-length to store peer certificates; moved MAX_PSK_ID_LEN definition |
| src/internal.c | Added peer certificate serialization/restoration in tickets; improved ticket encryption/decryption |
| src/x509.c | Added streaming BIO support; implemented TRUSTED_CERT_TYPE parsing; refactored AIA string handling |
| wolfssl/openssl/ssl.h | Changed sk_X509_dup and sk_SSL_CIPHER_dup to shallow copy semantics |
| src/ssl_sk.c | Implemented X509_CRL deep copy support in stack duplication |
| src/ssl_api_cert.c | Added certificate pointer compatibility check for nginx OCSP stapling |
| src/bio.c | Handled WOLFSSL_BAD_FILETYPE for pipe/FIFO sources |
| wolfssl/ssl.h | Added X509_CRL_new_null and X509_CRL_up_ref function declarations |
| src/crl.c | Implemented X509_CRL_up_ref for reference counting |
| tests/test-maxfrag*.conf | Removed DHE-RSA-AES256-GCM-SHA384 tests that don't fit in max fragment |
| .github/workflows/nginx.yml | Updated workflow for nginx 1.28.1 testing with sanitizers |
| configure.ac | Added nginx to OPENSSLALL enable condition |
Comments suppressed due to low confidence (1)
src/internal.c:1
- Consider extracting the compression extra calculation into a named constant or inline function for better readability and reusability, as this pattern appears twice in consecutive blocks.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
### `wolfssl/internal.h`
- **`InternalTicket` struct gains a flexible array member**: A new `peerCert[]` field (with a preceding `peerCertLen[2]`) is added to `InternalTicket`. This allows the peer's DER-encoded certificate to be stored directly inside the session ticket.
- **`ExternalTicket` struct becomes variable-length**: The `enc_ticket` field is changed from a fixed-size array to a flexible array member (`byte enc_ticket[]`). The `mac` field is removed from the struct — the MAC is now placed dynamically after the encrypted data in `enc_ticket`.
### `src/internal.c`
- The `GetRecordHeader` function now only adds `MAX_COMP_EXTRA` to the maximum allowed record size when `ssl->options.usingCompression` is true, tightening the length validation. The max fragment length extension check is now much stricter.
- **Peer certificate is serialized into the ticket**: During ticket creation, the code attempts to find the peer certificate from `ssl->peerCert` or from `ssl->session->chain` (fallback). If found and within `MAX_TICKET_PEER_CERT_SZ`, it's copied into `it->peerCert`. DTLS is explicitly excluded (peer cert length set to 0) to keep ticket size small for MTU constraints. If `HAVE_MAX_FRAGMENT` is defined and max fragment is not `MAX_RECORD_SIZE` for TLS 1.3, the cert is also skipped since `SendTls13NewSessionTicket` doesn't support fragmentation yet.
- **Peer certificate restoration from ticket**: On successful ticket decryption, if the ticket contains a peer certificate (`peerCertLen > 0`), it is decoded back into `ssl->peerCert` via `ParseCertRelative`/`CopyDecodedToX509`, and also added to `ssl->session->chain` via `AddSessionCertToChain`.
- The `CLEAR_ASN_NO_PEM_HEADER_ERROR` macro was rewritten to loop and remove all consecutive PEM no-start-line errors (not just the last one), wrapped in a `do { ... } while(0)` for safety.
- The `SendTicket` function is simplified to use `SendHandshakeMsg` to support fragmenting the larger ticket.
---
### `src/x509.c`
- `loadX509orX509REQFromPemBio` now accepts `TRUSTED_CERT_TYPE` in addition to `CERT_TYPE` and `CERTREQ_TYPE`.
- **Streaming BIO support**: When `wolfSSL_BIO_get_len()` returns ≤ 0 (e.g., pipes/FIFOs), the function no longer returns an error. Instead, it sets an initial buffer of `MAX_X509_SIZE` and dynamically grows (doubling) up to `MAX_BIO_READ_BUFFER` (`MAX_X509_SIZE * 16`) as data is read byte-by-byte.
- **Alternate footer detection**: For `TRUSTED_CERT_TYPE`, the PEM reader also checks for the regular `CERT_TYPE` footer (`-----END CERTIFICATE-----`) in addition to the trusted cert footer (`-----END TRUSTED CERTIFICATE-----`), so it can parse either format.
- Removed two lines that set `cert->srcIdx` to `SIGALGO_SEQ` offset. This makes `cert->srcIdx` reflect the end of parsed certificate data. This is used by `loadX509orX509REQFromBuffer` to detect where auxiliary trust data begins in trusted certificates.
---
### `src/ssl_sk.c`
- Added a `STACK_TYPE_X509_CRL` case to `wolfssl_sk_dup_data` that calls `wolfSSL_X509_CRL_dup` for deep-copying CRL stack elements. Previously, `STACK_TYPE_X509_CRL` fell through to the unsupported default case.
---
### `wolfssl/openssl/ssl.h`
- `sk_X509_dup` now maps to `wolfSSL_shallow_sk_dup` (was `wolfSSL_sk_dup`/deep copy). This matches OpenSSL's behavior where `sk_X509_dup` does a shallow copy.
- `sk_SSL_CIPHER_dup` similarly changed to `wolfSSL_shallow_sk_dup`.
---
### `src/ssl_api_cert.c`
- When `ssl->ourCert` is `NULL` and the SSL owns its cert, the function now checks if `ssl->ctx->ourCert` points to the same certificate (by comparing DER buffers). If so, it returns the ctx's `X509` pointer directly. This maintains pointer compatibility for applications (like nginx OCSP stapling) that use the `X509*` from `SSL_CTX_use_certificate` as a lookup key.
### `src/bio.c`
- When `wolfssl_file_len` returns `WOLFSSL_BAD_FILETYPE` (now returned for pipes/FIFOs), `wolfSSL_BIO_get_len` treats it as length 0 instead of propagating the error.
---
### `tests/test-maxfrag.conf` and `tests/test-maxfrag-dtls.conf`
- Removed `DHE-RSA-AES256-GCM-SHA384` test entries because the ClientKeyExchange doesn't fit in the selected max fragment length.
9192db1 to
3ff948c
Compare
3ff948c to
e9a2f27
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
wolfcrypt/test/test.c:1
- The macro now uses ERROR_OUT which requires a label 'out' to exist in the function. Verify that the 'out' label is defined later in random_bank_test, otherwise this will cause a compilation error.
src/internal.c:1 - The conditional addition of MAX_COMP_EXTRA is duplicated in both the HAVE_MAX_FRAGMENT and else branches (lines 12227-12228 and 12234-12235). Consider extracting this into a variable to reduce duplication and improve maintainability.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Depends on wolfSSL/wolfssl-nginx#31
wolfssl/internal.hInternalTicketstruct gains a flexible array member: A newpeerCert[]field (with a precedingpeerCertLen[2]) is added toInternalTicket. This allows the peer's DER-encoded certificate to be stored directly inside the session ticket.ExternalTicketstruct becomes variable-length: Theenc_ticketfield is changed from a fixed-size array to a flexible array member (byte enc_ticket[]). Themacfield is removed from the struct — the MAC is now placed dynamically after the encrypted data inenc_ticket.src/internal.cGetRecordHeaderfunction now only addsMAX_COMP_EXTRAto the maximum allowed record size whenssl->options.usingCompressionis true, tightening the length validation. The max fragment length extension check is now much stricter.ssl->peerCertor fromssl->session->chain(fallback). If found and withinMAX_TICKET_PEER_CERT_SZ, it's copied intoit->peerCert. DTLS is explicitly excluded (peer cert length set to 0) to keep ticket size small for MTU constraints. IfHAVE_MAX_FRAGMENTis defined and max fragment is notMAX_RECORD_SIZEfor TLS 1.3, the cert is also skipped sinceSendTls13NewSessionTicketdoesn't support fragmentation yet.peerCertLen > 0), it is decoded back intossl->peerCertviaParseCertRelative/CopyDecodedToX509, and also added tossl->session->chainviaAddSessionCertToChain.CLEAR_ASN_NO_PEM_HEADER_ERRORmacro was rewritten to loop and remove all consecutive PEM no-start-line errors (not just the last one), wrapped in ado { ... } while(0)for safety.SendTicketfunction is simplified to useSendHandshakeMsgto support fragmenting the larger ticket.src/x509.cloadX509orX509REQFromPemBionow acceptsTRUSTED_CERT_TYPEin addition toCERT_TYPEandCERTREQ_TYPE.wolfSSL_BIO_get_len()returns ≤ 0 (e.g., pipes/FIFOs), the function no longer returns an error. Instead, it sets an initial buffer ofMAX_X509_SIZEand dynamically grows (doubling) up toMAX_BIO_READ_BUFFER(MAX_X509_SIZE * 16) as data is read byte-by-byte.TRUSTED_CERT_TYPE, the PEM reader also checks for the regularCERT_TYPEfooter (-----END CERTIFICATE-----) in addition to the trusted cert footer (-----END TRUSTED CERTIFICATE-----), so it can parse either format.cert->srcIdxtoSIGALGO_SEQoffset. This makescert->srcIdxreflect the end of parsed certificate data. This is used byloadX509orX509REQFromBufferto detect where auxiliary trust data begins in trusted certificates.src/ssl_sk.cSTACK_TYPE_X509_CRLcase towolfssl_sk_dup_datathat callswolfSSL_X509_CRL_dupfor deep-copying CRL stack elements. Previously,STACK_TYPE_X509_CRLfell through to the unsupported default case.wolfssl/openssl/ssl.hsk_X509_dupnow maps towolfSSL_shallow_sk_dup(waswolfSSL_sk_dup/deep copy). This matches OpenSSL's behavior wheresk_X509_dupdoes a shallow copy.sk_SSL_CIPHER_dupsimilarly changed towolfSSL_shallow_sk_dup.src/ssl_api_cert.cssl->ourCertisNULLand the SSL owns its cert, the function now checks ifssl->ctx->ourCertpoints to the same certificate (by comparing DER buffers). If so, it returns the ctx'sX509pointer directly. This maintains pointer compatibility for applications (like nginx OCSP stapling) that use theX509*fromSSL_CTX_use_certificateas a lookup key.src/bio.cwolfssl_file_lenreturnsWOLFSSL_BAD_FILETYPE(now returned for pipes/FIFOs),wolfSSL_BIO_get_lentreats it as length 0 instead of propagating the error.tests/test-maxfrag.confandtests/test-maxfrag-dtls.confDHE-RSA-AES256-GCM-SHA384test entries because the ClientKeyExchange doesn't fit in the selected max fragment length.