-
Notifications
You must be signed in to change notification settings - Fork 7
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
Adding SNI config #40
Conversation
… it if there's a enabled SNIConfig; also adding function for verifying the certificate
…w for using the go version from go.mod
…o *Masquerade instead of masquerade and add function for finding provider from a given masquerade
…es for using a nil SNIConfig
…sed for testing purposes
…t maches with masquerade
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WendelHime this is really great work. Clean, minimal, easy to follow. I appreciate the updates to modernize this code as well!
I left a few minor comments, plus one new requirement - sorry about that 😬. Let me know if you have any questions!
direct.go
Outdated
// selecting a random SNI | ||
randomSNIIndex := rand.IntN(len(provider.SNIConfig.ArbitrarySNIs)) | ||
sniDomain := provider.SNIConfig.ArbitrarySNIs[randomSNIIndex] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we actually want this to be deterministic - and deterministic across clients too. The ideal scenario IMO is if any two clients in region R both send requests to IP X, they both use the same SNI. So perhaps we hash the masquerade IP and use the result modulo the set of SNIs to choose an SNI. Does that make sense?
A couple of minor points with my scheme: (1) If two clients have different lists, they will not end up with the same SNI. That is okay though; I think clients should only end up with different lists if they're in different regions. In that case, we don't need them to use the same SNI. (2) If the SNI list changes, then SNI-to-IP mappings will change. This is okay too; I don't expect us to change the SNI often.
Sorry, I know this is a new requirement added after the fact. I realize that can be frustrating. This didn't occur to me earlier =/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a crc32.ChecksumIEEE
func call for hashing and made some changes for distributing SNIs per masquerade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of things. Otherwise it's good.
…v by %w and unused test log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor request, but otherwise this looks great! Awesome work 😁
…ashing for consistently retrieving a SNI for the masquerade IP address
This pull request implements the SNI aribitrary configuration which allows flashlight to send a SNI on requests depending on the provider SNI configuration (and country at flashlight). Currently our requests doesn't send a SNI domain when making TLS connections (if you open wireshark
sudo wireshark -i <interface> -k
, scan and filter forClientHello
s -_ws.col.info contains "Client Hello"
- , you should be able to see our requests without specifying the SNI domain. e.g. image with empty client hello below.Here's a example of how it should look like now when sending requests with the SNI config.
Testing
$ go test -run=TestDirectDomainFrontingWithSNIConfig ./...
Note
This pull request is required to be merged before getlantern/flashlight#1396