external PSK mode #481

kazuho · 2023-08-07T08:26:52Z

ToDo:

add end-to-end test against openssl?

Design of picotls is based on the assumption that PSK-based handshake will not be used together with HRR.

…cept (ec)dhe + external_psk + resumption

From RFC 8446 section 4.2.11: For externally established PSKs, the Hash algorithm MUST be set when the PSK is established or default to SHA-256 if no such algorithm is defined.

Still default to SHA-256 for the hash, but make it configurable. Be sure to use the same cipher suite on both the client and the server (note technically the client only uses the hash alg of it, unless there is early data).

Server should not send early data indicator unless the client sent an early data indicator.

Use configured cipher, or default to a SHA-256 one. Note: here we've hard-coded PTLS_CIPHER_SUITE_AES_128_GCM_SHA256 for the default, but any SHA256 one could be chosen (unless the client is sending early data, but in that case both endpoints need to specify it explicitly, not rely on default).

From TLS 1.3 RFC 8446 section 4.2.10: The PSK used to encrypt the early data MUST be the first PSK listed in the client's "pre_shared_key" extension. I noticed that later on in the code it only sets up the tls->pending_handshake_secret when accept_early_data && tls->ctx->max_early_data_size != 0 && psk_index == 0, so perhaps we don't need to do a check here, but I think it is still good to check it in the psk handshake.

sshock · 2024-02-16T22:15:06Z

Hi @kazuho. I'm seeing an unexpected behavior for no matching PSK identity in a scenario where there is no key share.

In my testing earlier with mismatching PSK identity, the server would fail with a 115 UNKNOWN_PSK_IDENTITY as expected, as we fixed it here to do.

However, in my latest testing I have no key share (ptls_context_t key_exchanges is set to NULL), and now instead of failing with 115 as expected, the server sends a HelloRetryRequest.

This is happening around line 4633 in picotls.c, right after the /* try external psk handshake */ section, where it does the HelloRetryRequest:

    /* send HelloRetryRequest if enforced by config or upon key-share mismatch, unless PSK has already been selected */
    if (!is_second_flight && psk_index == SIZE_MAX &&
        (key_share.algorithm == NULL || (properties != NULL && properties->server.enforce_retry))) {

I'm not sure what the right solution is, but it seems like maybe this block of code to fail with UNKNOWN_PSK_IDENTITY needs to be moved up (to right after the /* try external psk handshake */ section)?

    /* If the server was setup to use an external PSK but failed to agree, abort the handshake. Because external PSK is a form of
     * mutual authentication, we should abort continue the handshake upon negotiation failure, at least by default. */
    if (tls->ctx->pre_shared_key.identity.base != NULL && psk_index == SIZE_MAX) {
        ret = PTLS_ALERT_UNKNOWN_PSK_IDENTITY;
        goto Exit;
    }

I'm not confident though because that is currently placed after a /* try psk handshake */ section (is this second one for handling resumption psk?), so if we moved it up would it break resumption psk?

I'm hoping you can understand the proper way to fix this 🤞, thanks!

sshock · 2024-02-29T16:43:28Z

Since my primary concern is security, I can live with this inconsistent behavior as long as it doesn't present a security vulnerability. That is, as long as the 2nd ClientHello attempt will fail, both with or without a key share. I just don't want a client to be able to trick a server into allowing an anonymous connection.

If you can help me confirm this, that would make me feel a lot better.

Then we can take our time thinking about the proper way to fix it, which I need your help with.

Or if you believe this behavior is correct, please explain. The RFC says Hello Retry is when "it is able to find an acceptable set of parameters but the ClientHello does not contain sufficient information to proceed with the handshake." I just don't see how "no matching PSK identity" falls into this category, but only when key share is missing.

…schedule

…r handshake, and binder_key is derived from null message

…ntly

kazuho · 2024-04-05T07:48:44Z

@sshock

I'm seeing an unexpected behavior for no matching PSK identity in a scenario where there is no key share.

In my testing earlier with mismatching PSK identity, the server would fail with a 115 UNKNOWN_PSK_IDENTITY as expected, as we fixed it here to do.

However, in my latest testing I have no key share (ptls_context_t key_exchanges is set to NULL), and now instead of failing with 115 as expected, the server sends a HelloRetryRequest.

Thank you for the patience.

I've made some changes reducing differences from master so that this PR can be merged. I think I've fixed the bug alongside that. We also have tests that involve PSK + HRR.

sshock · 2024-04-07T23:10:55Z

I've made some changes reducing differences from master so that this PR can be merged. I think I've fixed the bug alongside that. We also have tests that involve PSK + HRR.

Thanks @kazuho ! I was able to confirm that it now fails properly with 115 on wrong psk identity, even with no key share.

sshock · 2024-04-07T23:24:22Z

I also noticed that cli.c now helps set the psk_hash (a522cd9), thanks!

kazuho and others added 30 commits July 18, 2023 10:29

zero-length identity is not permitted

9592b5b

it works

1c38583

but not if ptls_context_t::key_exchanges is set to NULL

c2ddeee

Never send a Retry if handshake using external PSK is possible.

db5af12

Design of picotls is based on the assumption that PSK-based handshake will not be used together with HRR.

remove "only"; client can offer (ec)dhe + external_psk. server can ac…

4031113

…cept (ec)dhe + external_psk + resumption

Fix mistake during rebase

75f7dfa

Fix mistake during rebase

d0451d3

Add a todo comment about renaming

92d9832

Use correct binder key label on client

fba3d96

Default to SHA-256 with external PSK

b6b3c04

From RFC 8446 section 4.2.11: For externally established PSKs, the Hash algorithm MUST be set when the PSK is established or default to SHA-256 if no such algorithm is defined.

Make cipher suite to use with PSK configurable

b6e55b8

Still default to SHA-256 for the hash, but make it configurable. Be sure to use the same cipher suite on both the client and the server (note technically the client only uses the hash alg of it, unless there is early data).

Use correct binder key label on server

9912af6

Fix bug where server was sending early data indicator

7782e0a

Server should not send early data indicator unless the client sent an early data indicator.

Fix warning

6e37a7e

Fix warning

46cbf7a

Merge branch 'master' into tmp0045

63c4724

identity should be part of ptls_context_t

291b329

ECH is orthogonal to PSK

a984308

rename

38c37a0

let psk be a composition of identity and secret (rather than key)

4022142

[cli] -p option to specify PSK identity

35f648a

send session ticket only when ECDHE is used

bf31de4

restore guard

cba2448

retain the pointer to hash algorithm in psk object

374fe53

adjust now that slot for psk has been added

65e77c9

clients can send KeyShare extension, we are not accepting them

ff788d7

remove function no longer used (amends 374fe53)

fc41e2f

revert unneccesary change

fce9b09

Merge branch 'master' into kazuho/psk2

d4987ca

kazuho added 18 commits April 1, 2024 14:07

Merge branch 'master' into kazuho/psk2

90f1ab9

follow the changes made to other backend tests

48ffce5

simplify, reducing diff from master

c7a825a

update picotest

859ba9d

refactor

12b2453

test various forms of psk handshakes (incl. retry)

ddb2121

[cli] add -P option for setting the PSK hash

a522cd9

PSK+HRR works on the client side

de44a18

[cli] add option to enforce PSK handshake rather than PSK-DHE

5655ee1

fix compile error when PTLS_DEBUG is set

dfaa338

[psk] when HRR is involved at the server side, supply PSK to the key …

9547f80

…schedule

more logging

694dd69

when HRR is involved, make sure called HKDF-Extract(PSK) only once pe…

a8e1af5

…r handshake, and binder_key is derived from null message

fix test

436e433

for readability, use one struct and initialize the variables consiste…

388b26e

…ntly

consistently refer to ctx->ctx->psk

1fb276f

refine comments

96826a4

[cli] update help following the changes

3d9b469

kazuho added 2 commits April 6, 2024 10:09

refine comment

5643819

omit typename as it is unused

9eb094c

sshock mentioned this pull request Apr 7, 2024

External PSK mode #321

Closed

kazuho added 2 commits April 8, 2024 10:10

check consistency of ptls_context_t upon instantiation

71d4dfb

more tests

2af3d24

kazuho merged commit 8b52beb into master Apr 8, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

external PSK mode #481

external PSK mode #481

kazuho commented Aug 7, 2023

sshock commented Feb 16, 2024

sshock commented Feb 29, 2024

kazuho commented Apr 5, 2024

sshock commented Apr 7, 2024

sshock commented Apr 7, 2024

external PSK mode #481

external PSK mode #481

Conversation

kazuho commented Aug 7, 2023

sshock commented Feb 16, 2024

sshock commented Feb 29, 2024

kazuho commented Apr 5, 2024

sshock commented Apr 7, 2024

sshock commented Apr 7, 2024