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

The session resumption doesn`t work #182

Closed
bushuan opened this issue Apr 19, 2023 · 13 comments · Fixed by #231
Closed

The session resumption doesn`t work #182

bushuan opened this issue Apr 19, 2023 · 13 comments · Fixed by #231
Labels
duplicate Another issue/PR addressing the same problem exists.

Comments

@bushuan
Copy link

bushuan commented Apr 19, 2023

I tried using HelloChrome_100_PSK to see the session resumption

  1. first request
	config := tls.Config{
		ClientSessionCache: tls.NewLRUClientSessionCache(32),
		ServerName:         requestHostname,
	}
	dialConn, err := net.DialTimeout("tcp", addr, dialTimeout)

	uTlsConn := tls.UClient(dialConn, config, tls.HelloChrome_100)
	uTlsConn.Handshake()
  1. second request to see if session resumption is possible
		cacheKey := config.ServerName
		session, ok := config.ClientSessionCache.Get(cacheKey)
		if !ok || session == nil {
			return
		}
		sessionTicket := session.SessionTicket()

		ticketAge := uint32(time.Now().Sub(session.ReciveAt()) / time.Millisecond)
		obfuscatedTicketAge := ticketAge + session.AgeAdd()


	uTlsConn := tls.UClient(dialConn, config, tls.HelloCustom)
	preset, err := tls.UTLSIdToSpec(tls.HelloChrome_100_PSK) // correct
	if err != nil {
		return "", err
	}
	if pskExt, ok := preset.Extensions[len(preset.Extensions)-1].(*tls.FakePreSharedKeyExtension); ok {
		pskExt.PskIdentities = []tls.PskIdentity{ // must set identity
			{
				Label:               sessionTicket, // change this
				ObfuscatedTicketAge: obfuscatedTicketAge,          // change this
			},
		}
		// each fake binder is 32 bytes of zeros
		pskExt.PskBinders = [][]byte{ // must set psk binders
			{
				0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // change this
			},
		} // byte slices
	}
	uTlsConn.ApplyPreset(&preset) // make sure to apply preset to the connection

uTlsConn.Handshake()

error occurred:
#> HttpGetCustom() failed: uTlsConn.Handshake() error: local error: tls: bad record MAC

image

Is PSK binders is wrong or some other reason?

deep into the code
handshake_client.go -> loadSession

	ticketAge := uint32(c.config.time().Sub(session.receivedAt) / time.Millisecond)
	identity := pskIdentity{
		label:               session.sessionTicket,
		obfuscatedTicketAge: ticketAge + 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.
	psk := cipherSuite.expandLabel(session.masterSecret, "resumption",
		session.nonce, cipherSuite.hash.Size())
	earlySecret = cipherSuite.extract(psk, nil)
	binderKey = cipherSuite.deriveSecret(earlySecret, resumptionBinderLabel, nil)
	transcript := cipherSuite.hash.New()
	helloBytes, err := hello.marshalWithoutBinders()

It seems like the PSK binder calculation is correct, so I cannot find where the issue is

@gaukas gaukas added bug Unexpected behavior confirmed and should be fixed help wanted Calling for community PR/volunteer labels Apr 21, 2023
@gaukas
Copy link
Contributor

gaukas commented Apr 21, 2023

I'm not sure about if it is a bug in uTLS or crypto/tls.

As far as I know, our upstream crypto/tls never fully implemented PSK-based resumption and I have lost track on their progress, see golang/go#6379.

And unfortunately uTLS did not extend any functionality in terms of PSK. You might want to try if you could get crypto/tls working with PSK resumption first.

@gaukas
Copy link
Contributor

gaukas commented Aug 14, 2023

Related to #107.

@gaukas gaukas added duplicate Another issue/PR addressing the same problem exists. and removed bug Unexpected behavior confirmed and should be fixed help wanted Calling for community PR/volunteer labels Aug 14, 2023
@gaukas
Copy link
Contributor

gaukas commented Aug 15, 2023

#231 adds support for PSK.

@let4be
Copy link

let4be commented Sep 1, 2023

HI @gaukas, great job on the PSK

I'm trying to use PSK while restoring client hello from raw bytes(comes from a real browser) via

spec := &utls.ClientHelloSpec{}
if err := spec.FromRaw(RawBytesClientHello, true, true); err != nil {
   return nil, fmt.Errorf("cannot translate ClientHello to ClientHelloSpec: %w", err)
}
if err := utlsClient.ApplyPreset(spec); err != nil {
   return nil, fmt.Errorf("uTlsConn.Handshake() error: %+v", err)
}

everything is well from my logs I see client session being stored and retrieved without a problem(shared client session cache between uTlsClient with a mutex), until the 2nd handshake where it dies with a panic panic: tls: initPskExt failed: the [0] parameter is nil
(first call to the website finishes just fine)

For some reason browser never sends PreSharedKeyExtension in the 2nd ClientHello, which I verified by inserting a bunch of fmt.Println into uTLS here:

extWriter, ok := ext.(TLSExtensionWriter)

first handshake:

EXT =  0 , OK =  true
EXT =  23 , OK =  true
EXT =  65281 , OK =  true
EXT =  10 , OK =  true
EXT =  11 , OK =  true
EXT =  35 , OK =  true
EXT =  16 , OK =  true
EXT =  5 , OK =  true
EXT =  34 , OK =  true
EXT =  51 , OK =  true
EXT =  43 , OK =  true
EXT =  13 , OK =  true
EXT =  45 , OK =  true
EXT =  28 , OK =  true
EXT =  21 , OK =  true

second handshake:

EXT =  0 , OK =  true
EXT =  23 , OK =  true
EXT =  65281 , OK =  true
EXT =  10 , OK =  true
EXT =  11 , OK =  true
EXT =  35 , OK =  true
EXT =  16 , OK =  true
EXT =  5 , OK =  true
EXT =  34 , OK =  true
EXT =  51 , OK =  true
EXT =  43 , OK =  true
EXT =  13 , OK =  true
EXT =  45 , OK =  true
EXT =  28 , OK =  true
EXT =  21 , OK =  true

as you see 2nd handshake doesn't have presharedkeyextension(41)

In normal operation browser(Mozilla/5.0 (X11; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/114.0) always sends PreSharedKeyExtension on resume

I would like to understand what's going on...
Any help greatly appreciated

At the very least there should be no panic...

@let4be
Copy link

let4be commented Sep 1, 2023

If this is any help, I'm using uTLS in a context of proxy server where I'm parsing ClientHello from my browser(firefox) and trying to reproduce it via uTLSClient by calling ClientHelloSpec.FromRaw

I pinned this down to uTLS server being unable to establish PSK, I configured it with the following:

conf.UnwrapSession = func(identity []byte, cs tls.ConnectionState) (*tls.SessionState, error) {
		m.Lock()
		defer m.Unlock()

		index := binary.LittleEndian.Uint64(identity)
		v, ok := sessions[index]
		fmt.Println("Restoring session for index ", index, " = ", ok)

		return v, nil
	}
	conf.WrapSession = func(_ tls.ConnectionState, s *tls.SessionState) ([]byte, error) {
		m.Lock()
		defer m.Unlock()

		index := i
		i = i + 1
		sessions[index] = s

		var a bytes.Buffer
		_ = binary.Write(&a, binary.LittleEndian, index)

		fmt.Println("Saving session for index ", index)

		return a.Bytes(), nil
	}

and then

Conn = tls.Server(conn, conf)

tls package is uTLS here obviously
Firefox refuses to send PreSharedKeyExtension for some reason when connecting to uTLS server, not sure what am I missing

@gaukas
Copy link
Contributor

gaukas commented Sep 1, 2023

@3andne possibly related. When no PSK ext is used, by misconfiguration the client MAY call uconn.sessionController.initPskExt and cause this error.

I could not stably reproduce this panic but I do see it a few times when trying different combinations of configs.

@gaukas gaukas reopened this Sep 1, 2023
@gaukas
Copy link
Contributor

gaukas commented Sep 1, 2023

One of the thing to note when you try PSK-based resumption is you may want to carefully check IF there is a valid session to resume (the version needs to be TLS1.3).

Also, you may want to set OmitEmptyPsk in Config just to allow uTLS drop the PSK extension if no PSK-compatible session is available.

For more detailed usages please check out the examples.

@3andne
Copy link
Contributor

3andne commented Sep 1, 2023

Taking a look.

@let4be
The [0]th parameter refers to the PSK extension itself.

From the assertion failure, it can be inferred that the issue is as follows: The utls is basically complaining that:

  • It has been instructed to perform a session resumption (where SessionTicketsDisabled is false and ClientSessionCache is valid).
  • It has successfully retrieved a TLS 1.3 session.
  • However, the client hello spec does not include a PreSharedKey extension.

In this scenario, the utls is unsure how to proceed. Although the error message could be improved, the intention is to highlight such unintended behaviors explicitly.

@gaukas
Copy link
Contributor

gaukas commented Sep 1, 2023

Not sure if I understood correctly here. But in my opinion, in that case, we may just want to simply skipping attempting on the resumption.

It could be very common (or at least justifiable, if anyone would argue on that) for a user to use a non-PSK ClientHelloSpec with CSC set.

@3andne
Copy link
Contributor

3andne commented Sep 1, 2023

a user to use a non-PSK ClientHelloSpec with CSC set.

It has to have both ClientSessionCache and SessionTicketsDisabled: false, which indicates that the user would prefer a resumption. My concern is that skipping the resumption may not align with their expectations. It is crucial that we inform users about this, as it can be avoided, and it looks like a misuse.

@gaukas
Copy link
Contributor

gaukas commented Sep 1, 2023

SessionTicketsDisabled: false

It is awkward since it is an passive flag and people will most likely just ignore it. But I see your point that simply skipping the resumption may cause other problems...

@3andne
Copy link
Contributor

3andne commented Sep 1, 2023

It is awkward since it is an passive flag and people will most likely just ignore it.

Agreed. I really hate making things verbose. However in this case there are something we have to do. I propose improving the error message, adding another control flag 😂 that enables the skipping behavior. We can also find a way to enable this automatically for predefined fingerprints that doesn't contain PSK extensions.

@let4be
Copy link

let4be commented Sep 4, 2023

So, turns out I was using uTLS wrong - no UtlsPreSharedKeyExtension in the ClientHello I was using to build ClientHello spec from was my fault...

Better error message could be nice to have, so that further users have a better idea about what exactly went wrong

@gaukas gaukas closed this as completed Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate Another issue/PR addressing the same problem exists.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants