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

Use fixed list of ciphers #35

Merged
merged 1 commit into from
Apr 4, 2021
Merged

Conversation

KapJI
Copy link
Contributor

@KapJI KapJI commented Apr 3, 2021

Compare to DEFAULT_CIPHERS I removed ECDH+AESGCM and DH+AESGCM, they don't seem to be required. My tests show that all other ciphers are required.

We've got many people who are unable to authorize and get master token: leikoilja/ha-google-home#95

It turns out that other libraries may alter DEFAULT_CIPHERS: example. This has already been fixed but I think gpsoauth shouldn't depend on DEFAULT_CIPHERS which may also change in the new releases of urllib3.

This will make gpsoauth more robust.

@simon-weber
Copy link
Owner

Interesting. When this got merged my original motivation was to try and avoid maintaining a copy of DEFAULT_CIPHERS, since I figured the urllib3 people are more likely to be on top of what's secure than we are. Mostly, I didn't want to be responsible for people's Google accounts getting hacked 😬

Thinking about it again, Google can change what they accept at any point, and will probably be on top of blocking anything that's insecure. So, I think I'm ok with keeping our own list. Maybe I'll make a note to check it against urllib3's list every now and then.

@simon-weber simon-weber merged commit fc79242 into simon-weber:master Apr 4, 2021
@KapJI KapJI deleted the fixed-ciphers branch April 4, 2021 23:54
@KapJI
Copy link
Contributor Author

KapJI commented Apr 4, 2021

@simon-weber Thanks for reviewing all my PRs! I think this is all I wanted to do here, can you release the new version please? 🙂

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.

2 participants