Augment RETRY validation token #26048

andrewkdinh · 2024-11-25T00:04:19Z

Adds fields to the QUIC RETRY packet validation token: timestamp, remote_addr, odcid, & rscid.

Also adds functionality to validate the token once returned by the client. Checks that the client address matches and that the token isn't expired (10 seconds for tokens from RETRY packets and 60 minutes for tokens from NEW_TOKEN packets, which may be configurable in the future).

Loosely based on quic-go: https://github.com/quic-go/quic-go/blob/master/internal/handshake/token_generator.go

Currently, a few tests never complete because the server receives the initial request but is unable to fetch the remote address of the client to create the RETRY token (the remote address of the peer is always NULL, which I'm assuming is an issue with the test itself). I'm currently working on resolving this: openssl/project#933

Note that this does not encrypt the token yet (openssl/project#928). Also, NEW_TOKEN_packets aren't sent by the server yet (openssl/project#929).

Fixes openssl/project#912

Checklist

documentation is added or updated
tests are added or updated

andrewkdinh · 2024-11-25T00:23:46Z

I believe the failing coding style check is a known issue. It complains whether or not I have a { for the else if statement.

    if (hdr.token == NULL) {
        port_send_retry(port, &e->peer, &hdr);
        goto undesirable;
    } else if (port_validate_token(&hdr, port, &e->peer, &odcid, &scid) != 1)
        goto undesirable;

andrewkdinh · 2024-11-26T23:59:47Z

Most recent force-push rebases this branch on top of the changes from #26066. CI will not pass yet, as I need to update the data files for quicapitest.

Also adds a check to check the new DCID against one in the header to completely replace #26067. @nhorman

nhorman · 2024-11-27T00:23:58Z

I believe the failing coding style check is a known issue. It complains whether or not I have a { for the else if statement.
    if (hdr.token == NULL) {
        port_send_retry(port, &e->peer, &hdr);
        goto undesirable;
    } else if (port_validate_token(&hdr, port, &e->peer, &odcid, &scid) != 1)
        goto undesirable;

The failure is because of the lack of braces around the else clause. Coding style has a rule that says, in an of/else pair, if you need curly braces around either branch, you should add them around both

andrewkdinh · 2024-11-27T01:04:34Z

@nhorman The style check fails whether or not I have the curly braces around the else if statement (which is what I have currently)

ssl/quic/quic_port.c

Sashan

I think changes are heading in right direction. Some of my suggestion can be done in follow-up PR.

ssl/quic/quic_port.c

andrewkdinh · 2024-11-27T20:39:30Z

Implemented all the suggested changes. Please re-review @Sashan @nhorman

ssl/quic/quic_port.c

t8m · 2024-12-09T15:01:28Z

@mattcaswell @nhorman please reconfirm

nhorman · 2024-12-09T15:05:56Z

ack, approval holds

Sashan

I think this should go in. looks good to me.

openssl-machine · 2024-12-10T17:00:26Z

This pull request is ready to merge

t8m · 2024-12-10T19:33:57Z

Squashed and merged to the feature branch. Thank you.

Adds fields to the QUIC RETRY packet validation token: timestamp, remote_addr, odcid, & rscid. Also adds functionality to validate the token once returned by the client. Note that this does not encrypt the token yet. Also check that the RSCID stored in the RETRY validation token matches the DCID in the header. Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #26048)

Adds fields to the QUIC RETRY packet validation token: timestamp, remote_addr, odcid, & rscid. Also adds functionality to validate the token once returned by the client. Note that this does not encrypt the token yet. Also check that the RSCID stored in the RETRY validation token matches the DCID in the header. Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#26048)

Adds fields to the QUIC RETRY packet validation token: timestamp, remote_addr, odcid, & rscid. Also adds functionality to validate the token once returned by the client. Note that this does not encrypt the token yet. Also check that the RSCID stored in the RETRY validation token matches the DCID in the header. Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #26048)

Adds fields to the QUIC RETRY packet validation token: timestamp, remote_addr, odcid, & rscid. Also adds functionality to validate the token once returned by the client. Note that this does not encrypt the token yet. Also check that the RSCID stored in the RETRY validation token matches the DCID in the header. Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#26048)

Adds fields to the QUIC RETRY packet validation token: timestamp, remote_addr, odcid, & rscid. Also adds functionality to validate the token once returned by the client. Note that this does not encrypt the token yet. Also check that the RSCID stored in the RETRY validation token matches the DCID in the header. Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #26048)

Adds fields to the QUIC RETRY packet validation token: timestamp, remote_addr, odcid, & rscid. Also adds functionality to validate the token once returned by the client. Note that this does not encrypt the token yet. Also check that the RSCID stored in the RETRY validation token matches the DCID in the header. Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#26048)

andrewkdinh requested review from Sashan, nhorman, t8m and mattcaswell November 25, 2024 00:05

andrewkdinh force-pushed the naive-token branch 2 times, most recently from d4170c8 to e74f02d Compare November 25, 2024 00:22

andrewkdinh mentioned this pull request Nov 26, 2024

Update QUIC tests to allow server to retrieve remote address of client openssl/project#933

Closed

andrewkdinh marked this pull request as ready for review November 26, 2024 01:06

mattcaswell mentioned this pull request Nov 26, 2024

Add the ablity to set a local address for BIO_dgram_pair #26066

Closed

andrewkdinh force-pushed the naive-token branch from e74f02d to dbdeac8 Compare November 26, 2024 23:56

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Nov 27, 2024

andrewkdinh force-pushed the naive-token branch 3 times, most recently from a482921 to aaff6d4 Compare November 27, 2024 00:17

andrewkdinh force-pushed the naive-token branch 2 times, most recently from 118bae4 to 19d4fc7 Compare November 27, 2024 00:59

nhorman requested changes Nov 27, 2024

View reviewed changes

ssl/quic/quic_port.c Outdated Show resolved Hide resolved

ssl/quic/quic_port.c Outdated Show resolved Hide resolved

ssl/quic/quic_port.c Outdated Show resolved Hide resolved

ssl/quic/quic_port.c Show resolved Hide resolved

Sashan requested changes Nov 27, 2024

View reviewed changes

nhorman added the style: waived exempted from style checks label Nov 27, 2024

nhorman mentioned this pull request Nov 27, 2024

Use retry token to improve corruption resistance #26067

Closed

1 task

nhorman reviewed Nov 27, 2024

View reviewed changes

ssl/quic/quic_port.c Show resolved Hide resolved

andrewkdinh force-pushed the naive-token branch from 8fa470c to 13e67c2 Compare November 27, 2024 20:35

andrewkdinh requested review from Sashan and nhorman November 27, 2024 20:39

nhorman requested changes Nov 27, 2024

View reviewed changes

ssl/quic/quic_port.c Outdated Show resolved Hide resolved

ssl/quic/quic_port.c Outdated Show resolved Hide resolved

ssl/quic/quic_port.c Outdated Show resolved Hide resolved

andrewkdinh force-pushed the naive-token branch from 021ca36 to 25e99da Compare December 9, 2024 06:49

andrewkdinh requested a review from t8m December 9, 2024 06:50

t8m approved these changes Dec 9, 2024

View reviewed changes

nhorman approved these changes Dec 9, 2024

View reviewed changes

andrewkdinh 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 Dec 9, 2024

Sashan approved these changes Dec 9, 2024

View reviewed changes

mattcaswell approved these changes Dec 10, 2024

View reviewed changes

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 Dec 10, 2024

t8m closed this Dec 10, 2024

andrewkdinh deleted the naive-token branch February 14, 2025 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Augment RETRY validation token #26048

Augment RETRY validation token #26048

andrewkdinh commented Nov 25, 2024 •

edited

Loading

andrewkdinh commented Nov 25, 2024

andrewkdinh commented Nov 26, 2024 •

edited

Loading

nhorman commented Nov 27, 2024

andrewkdinh commented Nov 27, 2024

Sashan left a comment

andrewkdinh commented Nov 27, 2024

t8m commented Dec 9, 2024

nhorman commented Dec 9, 2024

Sashan left a comment

openssl-machine commented Dec 10, 2024

t8m commented Dec 10, 2024

Augment RETRY validation token #26048

Augment RETRY validation token #26048

Conversation

andrewkdinh commented Nov 25, 2024 • edited Loading

Checklist

andrewkdinh commented Nov 25, 2024

andrewkdinh commented Nov 26, 2024 • edited Loading

nhorman commented Nov 27, 2024

andrewkdinh commented Nov 27, 2024

Sashan left a comment

Choose a reason for hiding this comment

andrewkdinh commented Nov 27, 2024

t8m commented Dec 9, 2024

nhorman commented Dec 9, 2024

Sashan left a comment

Choose a reason for hiding this comment

openssl-machine commented Dec 10, 2024

t8m commented Dec 10, 2024

andrewkdinh commented Nov 25, 2024 •

edited

Loading

andrewkdinh commented Nov 26, 2024 •

edited

Loading