Skip to content

Conversation

@nhorman
Copy link
Contributor

@nhorman nhorman commented Oct 25, 2025

QUIC doesn't support the SSL_set_block_padding_ex apis, because QUIC objects always pad at the packet level during packet encryption. However, record padding is a config file option, and setting it there is done without any knoweldge of the kind of SSL objects we are going to create (TCP vs QUIC). As such, it seems to make sense to siliently ignore the option when configuring an SSL/SSL_CTX on QUIC objects.

Checklist
  • documentation is added or updated

@nhorman nhorman self-assigned this Oct 25, 2025
@nhorman nhorman linked an issue Oct 25, 2025 that may be closed by this pull request
@nhorman nhorman marked this pull request as draft October 25, 2025 11:30
@nhorman nhorman marked this pull request as ready for review October 25, 2025 11:35
@github-actions github-actions bot added the severity: ABI change This pull request contains ABI changes label Oct 25, 2025
@nhorman nhorman added branch: master Applies to master branch approval: review pending This pull request needs review by a committer branch: 3.5 Applies to openssl-3.5 branch: 3.6 Applies to openssl-3.6 labels Oct 25, 2025
@nhorman
Copy link
Contributor Author

nhorman commented Oct 25, 2025

backport ci jobs can be ignored, will fixup manually on merge

Copy link
Contributor

@Sashan Sashan left a comment

Choose a reason for hiding this comment

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

Why do we need this change? snippet below comes from SSL_set_block_padding_ex():

6026                                                                                                                                                                                                                                                                                                                        
6027 int SSL_set_block_padding_ex(SSL *ssl, size_t app_block_size,
6028                              size_t hs_block_size)
6029 {
6030     SSL_CONNECTION *sc = SSL_CONNECTION_FROM_SSL(ssl);
6031
6032     if (sc == NULL
6033         || (IS_QUIC(ssl)
6034             && (app_block_size > 1 || hs_block_size > 1)))
6035         return 0;

The SSL_set_block_padding_ex() currently indicate failure (return 0) when it is presented with SSL QUIC object.

I think we can opt for more conservative change without introducing public version of SSL_CTX_is_quic(). We can let SSL_set_block_padding_ex() to always succeed for QUIC objects. Something like

in SSL_set_block_padding_ex(SSL *ssl, size_t app_block_size, size_t hs_block_size)
{
    if (IS_QUIC(ssl))
        return 1;
    ...

I think the one-liner above has the same effect as the change proposed here. I admit I might be missing some detail... thanks.

@nhorman
Copy link
Contributor Author

nhorman commented Oct 27, 2025

@Sashan I specifically opted not to go with that approach because SSL_set_block_padding_ex and SSL_CTX_set_block_padding_ex are unsupported by QUIC, so I wanted applications that make those calls directly to be informed of that lack of support via the error code return. The exception we have to that case is the event in which we set these options via config file (where we have no idea what kind of SSL object we will be applying the padding to), so I figured this was the best compromise to make.

@nhorman nhorman requested review from Sashan and andrewkdinh October 27, 2025 11:57
Sashan
Sashan previously approved these changes Oct 27, 2025
Copy link
Contributor

@Sashan Sashan left a comment

Choose a reason for hiding this comment

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

thanks for clarificaiton. looks good to me.

@Sashan Sashan added the triaged: bug The issue/pr is/fixes a bug label Oct 27, 2025
@nhorman nhorman requested a review from t-j-h October 29, 2025 16:04
npajkovsky
npajkovsky previously approved these changes Oct 31, 2025
Just run the quicapitest (which attempts to create quic connections)
while using a config that specifies recordpadding, which quic should
ignore
@nhorman nhorman added approval: review pending This pull request needs review by a committer and removed hold: needs tests The PR needs tests to be added to it labels Nov 6, 2025
@nhorman nhorman requested a review from Sashan November 6, 2025 14:45
@nhorman
Copy link
Contributor Author

nhorman commented Nov 6, 2025

test added

@t8m t8m added the tests: present The PR has suitable tests present label Nov 6, 2025
t8m
t8m previously approved these changes Nov 6, 2025
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

OK for 4.0.

Copy link
Contributor

@Sashan Sashan left a comment

Choose a reason for hiding this comment

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

looks good to me.

@Sashan Sashan added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Nov 19, 2025
@nhorman nhorman added branch: 3.5 Applies to openssl-3.5 branch: 3.6 Applies to openssl-3.6 labels Nov 20, 2025
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Nov 20, 2025
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Nov 21, 2025
Like SSL_is_quic, it would be helpful to know if SSL_CTX objects create
QUIC SSL's or not.

Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #28992)
openssl-machine pushed a commit that referenced this pull request Nov 21, 2025
QUIC connections always pad data at the packet level during packet
encryption, and so have no ability to do padding at the record level.

We want to be able to inform the user of this condition when
applications call SSL_set_block_padding_ex directly by returning an
error, we have no idea of what kind of SSL objects are created when the
config file is written.

As such, silently ignore this config file option when QUIC objects are created.

Fixes #28953

Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #28992)
openssl-machine pushed a commit that referenced this pull request Nov 21, 2025
Just run the quicapitest (which attempts to create quic connections)
while using a config that specifies recordpadding, which quic should
ignore

Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #28992)
@t8m
Copy link
Member

t8m commented Nov 21, 2025

Merged to the master branch. Please create a backport PR for 3.6 branch without having the SSL_CTX_is_quic() API addition. (I.e. make non-public ossl_ssl_ctx_is_quic() there.)

@t8m t8m closed this Nov 21, 2025
@t8m t8m removed branch: 3.5 Applies to openssl-3.5 branch: 3.6 Applies to openssl-3.6 labels Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch severity: ABI change This pull request contains ABI changes tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RecordPadding option causes QUIC connections to fail

8 participants