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

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

Merged

Conversation

3andne
Copy link
Contributor

@3andne 3andne commented Sep 5, 2023

No description provided.

@gaukas gaukas self-requested a review October 4, 2023 17:18
Copy link
Member

@gaukas gaukas left a comment

Choose a reason for hiding this comment

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

Just my 2 cents. Otherwise this looks good to me.

// 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
Member

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
Member

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.

@gaukas gaukas merged commit 75eb8e9 into refraction-networking:master Oct 5, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants