Fix statefulness #8
Conversation
|
This fixes the shared state problem I was seeing in gotapdance |
u_parrots.go
Outdated
| @@ -208,6 +209,7 @@ func (uconn *UConn) applyPresetByID(id ClientHelloID) (err error) { | |||
| } | |||
|
|
|||
| // ApplyPreset should only be used in conjunction with HelloCustom to apply custom specs. | |||
| // Separate ClientHelloSpec for each UConn is advised, to avoid state sharing. | |||
There was a problem hiding this comment.
Is this because ApplyPreset modifies p? If so, that could be made explicit here.
There was a problem hiding this comment.
It doesn't modify state of p anymore.
| @@ -250,15 +252,17 @@ func (uconn *UConn) ApplyPreset(p *ClientHelloSpec) error { | |||
| uconn.greaseSeed[ssl_grease_extension2] ^= 0x1010 | |||
| } | |||
|
|
|||
| hello.CipherSuites = p.CipherSuites | |||
| hello.CipherSuites = make([]uint16, len(p.CipherSuites)) | |||
There was a problem hiding this comment.
This is confusing. Are you deep-copying the input (in which case it's safe to use the same input for many calls) or are you requiring the input to be independent for each call (in which case there's no need to make a copy).
There was a problem hiding this comment.
The only reliable way to deepcopy all the extensions I found is to add Clone() method to TLSExtension interface. Which I didn't quite want to do.
Current solution simply makes a new preset for built-in parrots. For custom ones, only shared state is pointers and slices in fields of TLSExtensions, which I indeed could be more explicit about.
With last refactoring, that enabled flexible external configs(and ultimately enabled effortless use of auto-generated code for any fingerprint, including recently integrated iOS 11 one), we introduced a regression:
Various connections, using same ClientHelloSpec may share state.
To provide more info on what state was erroneously shared, here are some examples:
SupportedCurvesExtension- should only change, if manually changed by user, which I don't think anyone does.h2looks weird and sticks out!). Such change would persist for all subsequent usages of same fingerprint spec.SetSNI.This PR fixes statefulness by making utls generate a new
ClientHelloSpecfor each usage of defaultClientHelloSpecs. CustomClientHelloSpecs (which no one uses atm as far as I know), may still share some state, so I added a disclaimer for that to advise users to create a new custom ClientHelloSpec for each UConn.