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

feature: extensionPreSharedKey support #107

Closed
ghost opened this issue Jun 4, 2022 · 18 comments · Fixed by #231
Closed

feature: extensionPreSharedKey support #107

ghost opened this issue Jun 4, 2022 · 18 comments · Fixed by #231
Labels
enhancement Feature with low severity but good value help wanted Calling for community PR/volunteer

Comments

@ghost
Copy link

ghost commented Jun 4, 2022

Hey,
is there a way in Utls to add PSK/pre_shared_key/extensionPreSharedKey,
all i can find in utls is that somewhere you guys say: TODO: wait for someone who cares about PSK to implement
Thanks.

@gaukas gaukas pinned this issue Aug 14, 2023
@gaukas gaukas added enhancement Feature with low severity but good value help wanted Calling for community PR/volunteer labels Aug 14, 2023
@gaukas
Copy link
Contributor

gaukas commented Aug 14, 2023

Seemingly our upstream crypto/tls is claimed to have enough support implemented for pre_shared_key.

Since all changes up to Go 1.21.0 has been integrated into uTLS, it should be possible to implement support for PreSharedKeyExtension based off FakePreSharedKeyExtension.

@gaukas
Copy link
Contributor

gaukas commented Aug 14, 2023

We would welcome report for any use-cases for how to (programmatically) use a pre_shared_key extension with crypto/tls, which would greatly help us in implementing this TLS Extension for uTLS.

Currently, we still don't fully understand how crypto/tls supports PSK, therefore could not tackle all the necessary internal states.

@gaukas gaukas changed the title UTLS add extensionPreSharedKey Call For Examples: extensionPreSharedKey support Aug 14, 2023
@jjsaunier
Copy link

  • device/user authentication through PSK (out of band)
  • In proxy app, leverage 0-RTT resumption
  • Still in proxy app, leverage server-side state https://datatracker.ietf.org/doc/html/rfc5077
  • In TLS mitm, replicate 1:1 the incoming fingerprint (this is already ok) and n+1 handshake behavior (PSK behavior is not replicated and forwarded, it redo a full handshake)

@gaukas
Copy link
Contributor

gaukas commented Aug 15, 2023

So yesterday I spent some time on crypto/tls to play with it a bit. Seemingly simply setting ClientSessionCache would work but you will need to do a full handshake plus reading at least one post-handshake message from the TLSConn to trigger the handling of a NewSessionTicket.

@gaukas gaukas changed the title Call For Examples: extensionPreSharedKey support feature: extensionPreSharedKey support Aug 15, 2023
@gaukas
Copy link
Contributor

gaukas commented Aug 15, 2023

#231 adds support for PSK.

@3andne
Copy link
Contributor

3andne commented Aug 16, 2023

Hi @gaukas,
Nice work on the PSK support. However I got some questions:

FakePreSharedKeyExtension uses CipherSuiteID, SessionSecret and Identities to calculate the corresponding binders and send them, without setting the internal states. Therefore if the server accepts the PSK and tries to resume, the connection fails.

Why doesn't it set the internal states? Is this a non-goal or just not being a high priority?
We recently did some work regarding this. On a private branch we have modified the FakePSKExt and made utls possible to provide PSK based session resumption for tls 1.3 just like crypto/tls. We also identified a bunch of bugs in the process.
We are finalizing our work. This could result in a PR if it's appreciated.

@gaukas
Copy link
Contributor

gaukas commented Aug 16, 2023

Why doesn't it set the internal states? Is this a non-goal or just not being a high priority?

If real PSK is needed, for now the only way to support it is via ClientSessionCache. I haven't learned enough on what is needed vs not for an external PSK -- crypto/tls isn't great in terms of documentations.

And For a FakePreSharedKeyExtension, what we want here is actually "advertising PSK but not expecting server to accept it". Otherwise it should be just UtlsPreSharedKeyExtension.

We also identified a bunch of bugs in the process. We are finalizing our work. This could result in a PR

Of course! Any PR addressing bugs and pending features are greatly appreciated.

Btw while you are here, @3andne, you know more about TLS than I do so I'd rather just confirm this with you: Is the current FakePreSharedKeyExtension (calculating the binder with parameters) providing more safeness guarantee than HardcodedPreSharedKeyExtension (just send whatever specified Identities and Binders are), or a censor is unable to validate the binders without the pre-shared keys?

@gaukas
Copy link
Contributor

gaukas commented Aug 16, 2023

If

a censor is unable to validate the binders without the pre-shared keys

Then we would consider removing the current FakePreSharedKeyExtension and adding an ExternalPreSharedKeyExtension -- if you would love to raise a PR for the implementation it would be great.

@3andne
Copy link
Contributor

3andne commented Aug 16, 2023

I might add some details tomorrow, but in short:

If real PSK is needed, for now the only way to support it is via ClientSessionCache. I haven't learned enough on what is needed vs not for an external PSK -- crypto/tls isn't great in terms of documentations.

Exactly. Real PSK based session resumption requires the access to ClientSessionCache. That means some structural change to u_conn.go. On a high level you need to do exactly the same as:

utls/handshake_client.go

Lines 415 to 437 in 8199306

ticketAge := c.config.time().Sub(time.Unix(int64(session.createdAt), 0))
identity := pskIdentity{
label: cs.ticket,
obfuscatedTicketAge: uint32(ticketAge/time.Millisecond) + session.ageAdd,
}
hello.pskIdentities = []pskIdentity{identity}
hello.pskBinders = [][]byte{make([]byte, cipherSuite.hash.Size())}
// Compute the PSK binders. See RFC 8446, Section 4.2.11.2.
earlySecret = cipherSuite.extract(session.secret, nil)
binderKey = cipherSuite.deriveSecret(earlySecret, resumptionBinderLabel, nil)
transcript := cipherSuite.hash.New()
helloBytes, err := hello.marshalWithoutBinders()
if err != nil {
return nil, nil, nil, err
}
transcript.Write(helloBytes)
pskBinders := [][]byte{cipherSuite.finishedHash(binderKey, transcript)}
if err := hello.updateBinders(pskBinders); err != nil {
return nil, nil, nil, err
}
return

And For a FakePreSharedKeyExtension, what we want here is actually "advertising PSK but not expecting server to accept it". Otherwise it should be just UtlsPreSharedKeyExtension.

I see. The naming doesn't really matter. It's possible to have a real PSK extension.

Is the current FakePreSharedKeyExtension (calculating the binder with parameters) providing more safeness guarantee than HardcodedPreSharedKeyExtension (just send whatever specified Identities and Binders are), or a censor is unable to validate the binders without the pre-shared keys?

I'd say a censor is not likely to distinguish between real or fake PSK exts. There are 3 elements in the PSK extension: labels (tickets), obfsAge and binders. Binders are generated by session secret and hash function. If you don't have the secret they are just random bytes. obfsAge is age + salt, and the salt is sent from the server in encrypted records. Tickets are sent from the server in encrypted records as well. Theoretically no one will know the plain text and therefore they should be unpredictable.

@gaukas
Copy link
Contributor

gaukas commented Aug 16, 2023

It's possible to have a real PSK extension.

Right. In the PR I mentioned above we are already providing UtlsPreSharedKeyExtension which utilizes a ClientSessionCache to do real PSK resumption. It is done. What's not done is just the external PSK part. i.e., ClientSessionCache involves some un-exported field which we may need to export, if we are supporting PSK distributed not via a previous NewSessionTicket message.

Theoretically no one will know the plain text and therefore they should be unpredictable.

Okay, that makes sense. I may push another commit to my PR tomorrow at some point to replace the

type FakePreSharedKeyExtension struct {
	CipherSuite   uint16 `json:"cipher_suite"`   // this is used to compute the binder
	SessionSecret []byte `json:"session_secret"` // this is used to compute the binder
	Identities []PskIdentity `json:"identities"`
}

With

type HardcodedPreSharedKeyExtension struct {
	Identities []PskIdentity `json:"identities"`
	Binders    [][]byte      `json:"binders"`
}

to further reduce the ambiguity. Thanks for confirming @3andne!

See u_pre_shared_key.go for the implementations of all 3 (will be 2 after I push the change) implementations of our new PreSharedKeyExtension interface, if anyone is at all interested.

@3andne
Copy link
Contributor

3andne commented Aug 16, 2023

The CipherSuite might be required to parrot Binders, since it's generated from the hash function of the cipher suite. A passive observer may be able to detect errors or inconsistencies in length.

@3andne
Copy link
Contributor

3andne commented Aug 16, 2023

Btw can you share you plan for the next steps? I may start working on a PR later today based off your implementation of UtlsPreSharedKeyExtension. It could take up to a few days since I may as well update restls-client-go for testing. It would be the best if I don't need to chase a moving target.

@gaukas
Copy link
Contributor

gaukas commented Aug 16, 2023

inconsistencies in length

Interesting observation. Will a censor be able to know which cipher suite does a PSK Identity associate with? If not possibly we could just check for "any valid length of binder".

the next steps

I can't think of any major updates in addition to the pending ones (kyberX and PSK). So once these two PRs merge, I will be less proactive in adding features to uTLS for a while. With that said my focus in the near future will be on fixing bugs.

@gaukas
Copy link
Contributor

gaukas commented Aug 16, 2023

I will tag a new version once the discussed changes are implemented later today. Then it should be safe to base any PRs off it.

@gaukas
Copy link
Contributor

gaukas commented Aug 16, 2023

New version tagged as v1.5.0-beta.3. It is safe to base any of your changes on this tag: we will rebase & merge the pull request #231.

We had a bug regarding UtlsPreSharedKeyExtension: setting tls.Config.ClientSessionCache will cause PSK-resumption to fail. Currently users must set only tls.UtlsPreSharedKeyExtension.ClientSessionCacheOverride. Please see the added example.

Bug will be discussed here: #233 (comment)

@3andne
Copy link
Contributor

3andne commented Aug 16, 2023

Interesting observation. Will a censor be able to know which cipher suite does a PSK Identity associate with? If not possibly we could just check for "any valid length of binder".

Quote from rfc8446

Each PSK is associated with a single Hash algorithm. For PSKs
established via the ticket mechanism (Section 4.6.1), this is the KDF
Hash algorithm on the connection where the ticket was established.
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. The server MUST ensure that it selects a compatible PSK
(if any) and cipher suite.

Technically, yes. A censor has that ability if they capture all previous server hello records. But they probably aren't able to tell from a single client hello, if we use the 256-bit safe bet.

@gaukas
Copy link
Contributor

gaukas commented Aug 16, 2023

if they capture all previous server hello records

Then we do not need to worry too much about it! Plus there are also external PSK available. For now let's just check if the binder is set to a valid length:

utls/u_pre_shared_key.go

Lines 302 to 312 in 3d7eea3

LOOP_BINDERS:
for _, binder := range e.Binders {
// check if binder size is valid
for _, cipherSuite := range cipherSuitesTLS13 {
if len(binder) == cipherSuite.hash.Size() {
bindersLength += len(binder) + 1 // binder length + binder
continue LOOP_BINDERS
}
}
return 0, errors.New("tls: invalid binder size")
}

@gaukas
Copy link
Contributor

gaukas commented Aug 17, 2023

Fixed the bug in the latest commit (v1.5.0-beta.4). Finally eliminating the existence of ClientSessionCacheOverride.

@gaukas gaukas unpinned this issue Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature with low severity but good value help wanted Calling for community PR/volunteer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants