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

Customable weights used in generateRandomizedSpec() #163

Merged
merged 5 commits into from
Feb 7, 2023

Conversation

RPRX
Copy link
Contributor

@RPRX RPRX commented Feb 6, 2023

As described in #157 (comment), allowing users to set their own weights would be helpful (and convenient for some purposes too).

CC @gaukas

@RPRX
Copy link
Contributor Author

RPRX commented Feb 6, 2023

Maybe we should add a field to struct ClientHelloID instead, like Seed.

@gaukas gaukas self-assigned this Feb 6, 2023
@gaukas
Copy link
Member

gaukas commented Feb 6, 2023

Thanks for PR and all the help!

It looks quite complicated to me and might not be intuitive enough to use. Would you like to add documentation and a few examples?

u_common.go Outdated Show resolved Hide resolved
u_common.go Outdated
// Please make a copy first instead of modifying them directly.
var DefaultWeights = Weights{
Weight_Extensions_Append_ALPN: 0.7,
Weight_TLSVersMax_Set_VersionTLS13: 0.4,
Copy link
Member

Choose a reason for hiding this comment

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

As discussed if we could update this list of weights it would be great. For example, perhaps it is a good time to have TLS 1.3 to weigh more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should update these preset weights in another PR?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. I was about to contact Sergey to learn how did he yield all these parameters.

@RPRX
Copy link
Contributor Author

RPRX commented Feb 7, 2023

complicated

Yes, I feel the same. I will extend struct ClientHelloID instead.

@RPRX
Copy link
Contributor Author

RPRX commented Feb 7, 2023

Upgraded, and fixed a bug introduced by #155.

u_parrots.go Show resolved Hide resolved
@@ -2076,11 +2076,11 @@ func (uconn *UConn) ApplyPreset(p *ClientHelloSpec) error {
}

func (uconn *UConn) generateRandomizedSpec() (ClientHelloSpec, error) {
return generateRandomizedSpec(uconn.ClientHelloID, uconn.serverName, uconn.HandshakeState.Session, uconn.config.NextProtos)
return generateRandomizedSpec(&uconn.ClientHelloID, uconn.serverName, uconn.HandshakeState.Session, uconn.config.NextProtos)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -1847,7 +1847,7 @@ func utlsIdToSpec(id ClientHelloID) (ClientHelloSpec, error) {
default:
if id.Client == helloRandomized || id.Client == helloRandomizedALPN || id.Client == helloRandomizedNoALPN {
// Use empty values as they can be filled later by UConn.ApplyPreset or manually.
return generateRandomizedSpec(id, "", nil, nil)
return generateRandomizedSpec(&id, "", nil, nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the original ClientHelloID won't be changed, and it's expected too.

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.

This might break someone's code if they are declaring their own ClientHelloIDs using struct literal without fields name (just like us), but it looks good to me otherwise.

@gaukas
Copy link
Member

gaukas commented Feb 7, 2023

If no other thing coming to your mind at the moment let's get this merged. We can always make changes later, before the next tagged version (expected in a few weeks).

@RPRX
Copy link
Contributor Author

RPRX commented Feb 7, 2023

It's ready to be merged.

@gaukas gaukas merged commit a75a4b4 into refraction-networking:master Feb 7, 2023
@gaukas
Copy link
Member

gaukas commented Feb 7, 2023

Thanks a ton! Merged.

@RPRX
Copy link
Contributor Author

RPRX commented Feb 7, 2023

This might break someone's code if they are declaring their own ClientHelloIDs using struct literal without fields name

I agree. But declaring variables using an exported struct without fields name is a very bad practice and is expected to be broken.

As v1.2.1 is a little buggy, it's suggested to tag v1.2.2 ASAP once these preset weights are updated (by you, of course).

@gaukas
Copy link
Member

gaukas commented Feb 7, 2023

Once sergey gets back to me 😉
Otherwise we might need to come up with our own numbers.

@gaukas
Copy link
Member

gaukas commented Feb 7, 2023

@RPRX Actually a covert channel between us would be beneficial given that our areas are largely overlapping with each other.

If you wouldn't mind?

@RPRX
Copy link
Contributor Author

RPRX commented Feb 7, 2023

@gaukas I've sent an email to you, please check.

@gaukas
Copy link
Member

gaukas commented Feb 10, 2023

I don't think Sergey is responding to inquiries about uTLS. I will get v1.2.2 released as a hotfix to v1.2.1, then we can discuss a smarter way to assign these default weights.

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