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

Support QUIC with the msg_callback and -trace in s_client #20914

Closed
wants to merge 26 commits into from

Conversation

mattcaswell
Copy link
Member

We extend the msg_callback to be able to support QUIC related events and also add QUIC support to the pre-built SSL_trace() callback function. This means that the s_client "-trace" option can now be used to dump QUIC tracing data related to send/receipt of datagrams, packets and frames.

This PR does not teach SSL_trace() how to parse the quic_transport_parameters extension. That is an exercise for future improvement.

@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member tests: present The PR has suitable tests present labels May 9, 2023
ssl/quic/quic_impl.c Outdated Show resolved Hide resolved
ssl/quic/quic_txp.c Outdated Show resolved Hide resolved
@hlandau
Copy link
Member

hlandau commented May 11, 2023

Other than that looks good.

At this stage we just support msg_callback on receipt of a datagram.
Extend SSL_trace so that it knows how to dump information about the
receipt of a QUIC datagram.
We enable SSL_trace support for when we receive QUIC Packets. This is
called after header protection is removed, but before the packet is
decrypted.
Extend the existing QUIC tracing capability for frames.
Previously we were only doing tracing of frames received from the peer.
Now we do that for transmitted frames as well.
Extend the tracing capability to also trace when we have a datagram to the
peer.
Prior to this commit we were just printing the fact that we had received
or sent a frame of a particular type. We now provide more details about
those frames.
We provide information about the new QUIC support related to the
msg_callback. We also document SSL_trace() which was previously missing
from the man pages.
Ensure that SSL_trace can print certificate data even with a non-default
libctx.
@t8m t8m added the triaged: feature The issue/pr requests/adds a feature label May 18, 2023
We create setter functions for the msg_callback and msg_callback_arg so
that these values can be properly propagated to the QRX/QTX/TXP even
after the channel has been created.
@mattcaswell
Copy link
Member Author

I rebased this and added some commits to address the feedback from @hlandau. Please take another look.

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.

Just minor things

Comment on lines +243 to +247
# define SSL3_RT_QUIC_DATAGRAM 0x200
# define SSL3_RT_QUIC_PACKET 0x201
# define SSL3_RT_QUIC_FRAME_FULL 0x202
# define SSL3_RT_QUIC_FRAME_HEADER 0x203
# define SSL3_RT_QUIC_FRAME_PADDING 0x204
Copy link
Member

Choose a reason for hiding this comment

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

The SSL3_RT_ prefix sounds quite awkward for something that has no relation to SSL3 at all. Perhaps OSSL_RT_.... would be better?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. But the problem here is that the SSL3_RT_ prefix is well established for this namespace and is legacy. It would be quite weird to have some values in the same space to have different prefixes. Plausibly we could introduce OSSL_RT_* everywhere and have some compatibility defines for old usage of SSL3_RT_* but this is a significant change with knock-on impacts throughout libssl. The SSL3_RT_ values are used beyond just the msg_callback. I'd rather not do that in this PR.

doc/man3/SSL_CTX_set_msg_callback.pod Outdated Show resolved Hide resolved
include/internal/quic_wire_pkt.h Outdated Show resolved Hide resolved
ssl/quic/quic_channel.c Outdated Show resolved Hide resolved
ssl/quic/quic_impl.c Outdated Show resolved Hide resolved
ssl/quic/quic_record_rx.c Outdated Show resolved Hide resolved
ssl/quic/quic_record_tx.c Outdated Show resolved Hide resolved
ssl/quic/quic_trace.c Outdated Show resolved Hide resolved
ssl/quic/quic_trace.c Outdated Show resolved Hide resolved
ssl/t1_trce.c Outdated Show resolved Hide resolved
@t8m
Copy link
Member

t8m commented May 19, 2023

CI is also relevant

@arapov arapov linked an issue May 19, 2023 that may be closed by this pull request
openssl-machine pushed a commit that referenced this pull request May 24, 2023
We enable SSL_trace support for when we receive QUIC Packets. This is
called after header protection is removed, but before the packet is
decrypted.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
openssl-machine pushed a commit that referenced this pull request May 24, 2023
Extend the existing QUIC tracing capability for frames.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
openssl-machine pushed a commit that referenced this pull request May 24, 2023
Previously we were only doing tracing of frames received from the peer.
Now we do that for transmitted frames as well.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
openssl-machine pushed a commit that referenced this pull request May 24, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
openssl-machine pushed a commit that referenced this pull request May 24, 2023
Extend the tracing capability to also trace when we have a datagram to the
peer.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
openssl-machine pushed a commit that referenced this pull request May 24, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
openssl-machine pushed a commit that referenced this pull request May 24, 2023
Prior to this commit we were just printing the fact that we had received
or sent a frame of a particular type. We now provide more details about
those frames.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
openssl-machine pushed a commit that referenced this pull request May 24, 2023
We provide information about the new QUIC support related to the
msg_callback. We also document SSL_trace() which was previously missing
from the man pages.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
openssl-machine pushed a commit that referenced this pull request May 24, 2023
Ensure that SSL_trace can print certificate data even with a non-default
libctx.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
openssl-machine pushed a commit that referenced this pull request May 24, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
openssl-machine pushed a commit that referenced this pull request May 24, 2023
We create setter functions for the msg_callback and msg_callback_arg so
that these values can be properly propagated to the QRX/QTX/TXP even
after the channel has been created.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
openssl-machine pushed a commit that referenced this pull request May 24, 2023
We change to use %llu when printing uint64_t types for consistency with
what we've done elsewhere.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
openssl-machine pushed a commit that referenced this pull request May 24, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
openssl-machine pushed a commit that referenced this pull request May 24, 2023
We create the internal/ssl.h header file and move the typedef for
ossl_msg_cb. This is needed by both the QUIC code (which generally doesn't
include ssl_local.h) and the rest of libssl.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
At this stage we just support msg_callback on receipt of a datagram.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
Extend SSL_trace so that it knows how to dump information about the
receipt of a QUIC datagram.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
We enable SSL_trace support for when we receive QUIC Packets. This is
called after header protection is removed, but before the packet is
decrypted.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
Extend the existing QUIC tracing capability for frames.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
Previously we were only doing tracing of frames received from the peer.
Now we do that for transmitted frames as well.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
Extend the tracing capability to also trace when we have a datagram to the
peer.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
Prior to this commit we were just printing the fact that we had received
or sent a frame of a particular type. We now provide more details about
those frames.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
We provide information about the new QUIC support related to the
msg_callback. We also document SSL_trace() which was previously missing
from the man pages.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
Ensure that SSL_trace can print certificate data even with a non-default
libctx.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
We create setter functions for the msg_callback and msg_callback_arg so
that these values can be properly propagated to the QRX/QTX/TXP even
after the channel has been created.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
We change to use %llu when printing uint64_t types for consistency with
what we've done elsewhere.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
We create the internal/ssl.h header file and move the typedef for
ossl_msg_cb. This is needed by both the QUIC code (which generally doesn't
include ssl_local.h) and the rest of libssl.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#20914)
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 Merge to master branch tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Extend SSL_trace capability for QUIC
4 participants