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.

Sign up for GitHub

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

Jump to bottom

feat: add an option to skip resumption on nil ext & update examples #239

Merged
merged 2 commits into from
Oct 5, 2023
Merged

feat: add an option to skip resumption on nil ext & update examples #239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c245cc
feat: add an option to skip resumption on nil ext
3andne Sep 5, 2023
0032a39
fix: clone unit test
3andne Sep 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,19 @@ type Config struct {
// This field is ignored when InsecureSkipVerify is true.
InsecureServerNameToVerify string // [uTLS]

// PreferSkipResumptionOnNilExtension controls the behavior when session resumption is enabled but the corresponding session extensions are nil.
//
// To successfully use session resumption, ensure that the following requirements are met:
// - SessionTicketsDisabled is set to false
// - ClientSessionCache is non-nil
// - For TLS 1.2, SessionTicketExtension is non-nil
// - For TLS 1.3, PreSharedKeyExtension is non-nil
//
// There may be cases where users enable session resumption (SessionTicketsDisabled: false && ClientSessionCache: non-nil), but they do not provide SessionTicketExtension or PreSharedKeyExtension in the ClientHelloSpec. This could be intentional or accidental.
//
// By default, utls throws an exception in such scenarios. Set this to true to skip the resumption and suppress the exception.
PreferSkipResumptionOnNilExtension bool // [uTLS]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to use a more comprehensible name for this field and make it a positive flag: user sets it so it will do something (rather than "not to do something").

How about MustResumeIfSessionAvailable in this case. Most of people will NOT edit the config from an older version, and using a positive flag here keeps the behavior consistent with older versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the point of using a positive flag for consistency with older versions and user expectations. However, my main concern is that a positive flag might inadvertently be overlooked by users. Given the possibility of users making unintentional mistakes when using custom fingerprints, as demonstrated by the user who experienced the panic, I believe we need to prioritize safety and foolproofing over forward behavior consistency.

If a proxy app fails to properly configure this, it could introduce unintended characteristics. It's not typical for a browser to repeatedly visit a website without using resumption. While I don't want to be stubborn, if we don't, by default, throw an exception here, I can't think of another way to let users aware of such mistakes. We're the last line of defense.

Note that we won't throw exceptions for pre-defined fingerprints. For most users this option is transparent.

I agree it's challenging to come up with a descriptive name. We already have several flags for "not to do something", such as OmitEmptyPsk and InsecureSkipVerify. I'm open to naming options that makes the intent clear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are intentionally alerting users about the incompatibility between the ClientHelloSpec (which includes no resumption related extension) and the ClientSessionCache, then I guess you are right we should definitely keep this. Thanks for clarification.

we won't throw exceptions for pre-defined fingerprints

Yeah I noticed that. I guess then that's fine, since ClientHelloSpec customization is never considered an intro-level feature to use.


// CipherSuites is a list of enabled TLS 1.0–1.2 cipher suites. The order of
// the list is ignored. Note that TLS 1.3 ciphersuites are not configurable.
//
Expand Down Expand Up @@ -906,6 +919,8 @@ func (c *Config) Clone() *Config {
KeyLogWriter: c.KeyLogWriter,
sessionTicketKeys: c.sessionTicketKeys,
autoSessionTicketKeys: c.autoSessionTicketKeys,

PreferSkipResumptionOnNilExtension: c.PreferSkipResumptionOnNilExtension, // [UTLS]
}
}

Expand Down
60 changes: 49 additions & 11 deletions examples/tls-resumption/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,16 @@ func (csc *ClientSessionCache) Put(sessionKey string, cs *tls.ClientSessionState
}
}

func runResumptionCheck(helloID tls.ClientHelloID, serverAddr string, retry int, verbose bool) {
type ResumptionType int

const (
noResumption ResumptionType = 0
pskResumption ResumptionType = 1
ticketResumption ResumptionType = 2
)

func runResumptionCheck(helloID tls.ClientHelloID, getCustomSpec func() *tls.ClientHelloSpec, expectResumption ResumptionType, serverAddr string, retry int, verbose bool) {
fmt.Printf("checking: hello [%s], expectResumption [%v], serverAddr [%s]\n", helloID.Client, expectResumption, serverAddr)
csc := NewClientSessionCache()
tcpConn, err := net.Dial("tcp", serverAddr)
if err != nil {
Expand All @@ -55,6 +64,10 @@ func runResumptionCheck(helloID tls.ClientHelloID, serverAddr string, retry int,
OmitEmptyPsk: true,
}, helloID)

if getCustomSpec != nil {
tlsConn.ApplyPreset(getCustomSpec())
}

// HS
err = tlsConn.Handshake()
if err != nil {
Expand Down Expand Up @@ -96,6 +109,7 @@ func runResumptionCheck(helloID tls.ClientHelloID, serverAddr string, retry int,
}
tlsConn.Close()

resumption := noResumption
for i := 0; i < retry; i++ {
tcpConnPSK, err := net.Dial("tcp", serverAddr)
if err != nil {
Expand All @@ -108,6 +122,10 @@ func runResumptionCheck(helloID tls.ClientHelloID, serverAddr string, retry int,
OmitEmptyPsk: true,
}, helloID)

if getCustomSpec != nil {
tlsConnPSK.ApplyPreset(getCustomSpec())
}

// HS
err = tlsConnPSK.Handshake()
if verbose {
Expand All @@ -133,27 +151,47 @@ func runResumptionCheck(helloID tls.ClientHelloID, serverAddr string, retry int,

if tlsVer == tls.VersionTLS13 && tlsConnPSK.HandshakeState.State13.UsingPSK {
fmt.Println("[PSK used]")
return
resumption = pskResumption
break
} else if tlsVer == tls.VersionTLS12 && tlsConnPSK.DidTls12Resume() {
fmt.Println("[session ticket used]")
return
resumption = ticketResumption
break
}
}
time.Sleep(700 * time.Millisecond)
}
panic(fmt.Sprintf("PSK or session ticket not used for a resumption session, server %s, helloID: %s", serverAddr, helloID.Client))

if resumption != expectResumption {
panic(fmt.Sprintf("Expecting resumption type: %v, actual %v; session, server %s, helloID: %s", expectResumption, resumption, serverAddr, helloID.Client))
} else {
fmt.Println("[expected]")
}
}

func main() {
tls13Url := "www.microsoft.com:443"
tls12Url1 := "spocs.getpocket.com:443"
tls12Url2 := "marketplace.visualstudio.com:443"
runResumptionCheck(tls.HelloChrome_100_PSK, tls13Url, 1, false) // psk + utls
runResumptionCheck(tls.HelloGolang, tls13Url, 1, false) // psk + crypto/tls

runResumptionCheck(tls.HelloChrome_100_PSK, tls12Url1, 10, false) // session ticket + utls
runResumptionCheck(tls.HelloGolang, tls12Url1, 10, false) // session ticket + crypto/tls
runResumptionCheck(tls.HelloChrome_100_PSK, tls12Url2, 10, false) // session ticket + utls
runResumptionCheck(tls.HelloGolang, tls12Url2, 10, false) // session ticket + crypto/tls
runResumptionCheck(tls.HelloChrome_100, nil, noResumption, tls13Url, 3, false) // no-resumption + utls
func() {
defer func() {
if err := recover(); err == nil {
panic("must throw")
}
}()

runResumptionCheck(tls.HelloCustom, func() *tls.ClientHelloSpec {
spec, _ := tls.UTLSIdToSpec(tls.HelloChrome_100)
return &spec
}, noResumption, tls13Url, 3, false) // no-resumption + utls custom + no psk extension
}()
runResumptionCheck(tls.HelloChrome_100_PSK, nil, pskResumption, tls13Url, 1, false) // psk + utls
runResumptionCheck(tls.HelloGolang, nil, pskResumption, tls13Url, 1, false) // psk + crypto/tls

runResumptionCheck(tls.HelloChrome_100_PSK, nil, ticketResumption, tls12Url1, 10, false) // session ticket + utls
runResumptionCheck(tls.HelloGolang, nil, ticketResumption, tls12Url1, 10, false) // session ticket + crypto/tls
runResumptionCheck(tls.HelloChrome_100_PSK, nil, ticketResumption, tls12Url2, 10, false) // session ticket + utls
runResumptionCheck(tls.HelloGolang, nil, ticketResumption, tls12Url2, 10, false) // session ticket + crypto/tls

}
2 changes: 1 addition & 1 deletion tls_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,7 +854,7 @@ func TestCloneNonFuncFields(t *testing.T) {
f.Set(reflect.ValueOf("b"))
case "ClientAuth":
f.Set(reflect.ValueOf(VerifyClientCertIfGiven))
case "InsecureSkipVerify", "InsecureSkipTimeVerify", "SessionTicketsDisabled", "DynamicRecordSizingDisabled", "PreferServerCipherSuites", "OmitEmptyPsk":
case "InsecureSkipVerify", "InsecureSkipTimeVerify", "SessionTicketsDisabled", "DynamicRecordSizingDisabled", "PreferServerCipherSuites", "OmitEmptyPsk", "PreferSkipResumptionOnNilExtension":
f.Set(reflect.ValueOf(true))
case "InsecureServerNameToVerify":
f.Set(reflect.ValueOf("c"))
Expand Down
6 changes: 6 additions & 0 deletions u_conn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ type UConn struct {

omitSNIExtension bool

// skipResumptionOnNilExtension is copied from `Config.PreferSkipResumptionOnNilExtension`.
//
// By default, if ClientHelloSpec is predefined or utls-generated (as opposed to HelloCustom), this flag will be updated to true.
skipResumptionOnNilExtension bool

// certCompressionAlgs represents the set of advertised certificate compression
// algorithms, as specified in the ClientHello. This is only relevant client-side, for the
// server certificate. All other forms of certificate compression are unsupported.
Expand All @@ -58,6 +63,7 @@ func UClient(conn net.Conn, config *Config, clientHelloID ClientHelloID) *UConn
uconn.handshakeFn = uconn.clientHandshake
uconn.sessionController = newSessionController(&uconn)
uconn.utls.sessionController = uconn.sessionController
uconn.skipResumptionOnNilExtension = config.PreferSkipResumptionOnNilExtension || clientHelloID.Client != helloCustom
return &uconn
}

Expand Down
19 changes: 16 additions & 3 deletions u_session_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,12 @@ func (s *sessionController) assertNotLocked(caller string) {
}
}

func (s *sessionController) assertCanSkip(caller, extensionName string) {
if !s.uconnRef.skipResumptionOnNilExtension {
panic(fmt.Sprintf("tls: %s failed: session resumption is enabled, but there is no %s in the ClientHelloSpec; Please consider provide one in the ClientHelloSpec; If this is intentional, you may consider disable resumption by setting Config.SessionTicketsDisabled to true, or set Config.PreferSkipResumptionOnNilExtension to true to suppress this exception", caller, extensionName))
}
}

// finalCheck performs a comprehensive check on the updated state to ensure the correctness of the changes.
// If the checks pass successfully, the sessionController's state will be locked.
// Any failure in passing the tests indicates incorrect implementations in the utls, which will result in triggering a panic.
Expand All @@ -141,7 +147,11 @@ func (s *sessionController) initSessionTicketExt(session *SessionState, ticket [
s.assertNotLocked("initSessionTicketExt")
s.assertHelloNotBuilt("initSessionTicketExt")
s.assertControllerState("initSessionTicketExt", NoSession)
panicOnNil("initSessionTicketExt", s.sessionTicketExt, session, ticket)
panicOnNil("initSessionTicketExt", session, ticket)
if s.sessionTicketExt == nil {
s.assertCanSkip("initSessionTicketExt", "session ticket extension")
return
}
initializationGuard(s.sessionTicketExt, func(e ISessionTicketExtension) {
s.sessionTicketExt.InitializeByUtls(session, ticket)
})
Expand All @@ -155,8 +165,11 @@ func (s *sessionController) initPskExt(session *SessionState, earlySecret []byte
s.assertNotLocked("initPskExt")
s.assertHelloNotBuilt("initPskExt")
s.assertControllerState("initPskExt", NoSession)
panicOnNil("initPskExt", s.pskExtension, session, earlySecret, pskIdentities)

panicOnNil("initPskExt", session, earlySecret, pskIdentities)
if s.pskExtension == nil {
s.assertCanSkip("initPskExt", "pre-shared key extension")
return
}
initializationGuard(s.pskExtension, func(e PreSharedKeyExtension) {
publicPskIdentities := mapSlice(pskIdentities, func(private pskIdentity) PskIdentity {
return PskIdentity{
Expand Down