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

oocrypto: AES not used on darwin/arm64 #2122

Closed
bassosimone opened this issue Jun 2, 2022 · 3 comments
Closed

oocrypto: AES not used on darwin/arm64 #2122

bassosimone opened this issue Jun 2, 2022 · 3 comments
Assignees
@bassosimone
Labels
bug Something isn't working data quality ooni/probe-engine platform/android platform/ios platform/linux platform/macos platform/windows priority/high

Comments

@bassosimone
Copy link
Member

This issue is about oocrypto being broken and not using AES on darwin/arm64. Most likely, this means that the way in which we're trying to pick up CPU features does not work. So, for example this measurement using the 3.15 release candidate uses TLS_AES_128_GCM_SHA256. Conversely, this measurement using the development branch uses TLS_CHACHA20_POLY1305_SHA256. We should probably stop using oocrypto until we understand why it's not working as intended and possibly consider reverting back to ooni/go instead.
@bassosimone bassosimone self-assigned this Jun 2, 2022
@bassosimone bassosimone mentioned this issue Jun 2, 2022
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue Jun 2, 2022
@bassosimone
hotfix: disable oocrypto until we investigate #2122
3f856a4 
See ooni/probe#2122 for context.
@bassosimone bassosimone mentioned this issue Jun 2, 2022
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue Jun 2, 2022
@bassosimone
hotfix: disable oocrypto until we investigate ciphers selection (#784)
b58cfad 
See ooni/probe#2122 for context.
@ainghazal ainghazal mentioned this issue Jun 17, 2022
Closed
@ainghazal
Copy link

ainghazal commented Jun 17, 2022 •
edited
Loading

Turns out x/sys/cpu currently lacks support for proper AES detection in Apple Silicon, and go gives priority to the chacha20 suite if it cannot detect hardware support on both sides. The fix is trivial: just hardcode AES=true for darwin/arm64, which is exactly what internal/cpu is doing. See the linked upstream issues in the MR for the whole discussion.

@bassosimone
Copy link
Member Author

We may also want to consider https://github.com/klauspost/cpuid

bassosimone added a commit to ooni/probe-cli that referenced this issue Aug 17, 2022
@bassosimone
fix(Makefile): re-enable Android builds
4eb5c5b 
It turns out the problem described in ooni/probe#2122
only affects darwin/arm64, so we should be good to re-enable Android.
@bassosimone bassosimone mentioned this issue Aug 17, 2022
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue Aug 17, 2022
@bassosimone
fix(Makefile): re-enable Android builds (#865)
b7cc22b 
It turns out the problem described in ooni/probe#2122
only affects darwin/arm64, so we should be good to re-enable Android.
bassosimone added a commit to ooni/oocrypto that referenced this issue Aug 18, 2022
@bassosimone @ainghazal
fix: add support for darwin/arm64
f4eef75 
This diff ensures we hardcode the capabilities of darwin/arm64.

While there, strive to make the packages naming slightly less confusing.

Reference issue: ooni/probe#2122.

Surpersedes (and draws from) #5.

Co-authored-by: ain ghazal <ainghazal42@gmail.com>
@bassosimone bassosimone mentioned this issue Aug 18, 2022
Merged
@bassosimone
Copy link
Member Author

I'm going to merge the fix at ooni/oocrypto#7, which is based on the previous diff by @ainghazal at ooni/oocrypto#5 and includes improvements in naming and documentation that should dispel some ambiguity that @ainghazal mentioned in private to me while we were discussing this issue.

bassosimone added a commit to ooni/oocrypto that referenced this issue Aug 18, 2022
@bassosimone @ainghazal
fix: add support for darwin/arm64 (#7)
d01c7ff 
This diff ensures we hardcode the capabilities of darwin/arm64.

While there, strive to make the packages naming slightly less confusing.

Reference issue: ooni/probe#2122.

Surpersedes (and draws from) #5.

Co-authored-by: ain ghazal ainghazal42@gmail.com

Co-authored-by: ain ghazal <ainghazal42@gmail.com>
bassosimone added a commit to ooni/oocrypto that referenced this issue Aug 22, 2022
@bassosimone
fix: backport darwin/arm64 patches from main
24a810b 
This diff backports code to correctly handle darwin/arm64 from
`main` (see #7).

While there, this diff aims at reducing unnecessary differences with
the main branch, by removing code we also removed in there.

The reference issues are:

1. ooni/probe#2122

2. ooni/probe#2223
@bassosimone bassosimone mentioned this issue Aug 22, 2022
Merged
bassosimone added a commit to ooni/oocrypto that referenced this issue Aug 22, 2022
@bassosimone
fix: backport darwin/arm64 patches from main
bbcc762 
This diff backports code to correctly handle darwin/arm64 from
`main` (see #7).

While there, this diff aims at reducing unnecessary differences with
the main branch, by removing code we also removed in there.

The reference issues are:

1. ooni/probe#2122

2. ooni/probe#2223
bassosimone added a commit to ooni/probe-cli that referenced this issue Aug 22, 2022
@bassosimone
fix: use oocrypto@v0.1.1
5d8e243 
This ensures we keep ooni/probe#2122 fixed.
bassosimone added a commit to ooni/probe-cli that referenced this issue Aug 22, 2022
@bassosimone
chore: use {go,oohttp,oocrypto} v1.18.5 (#872)
fe6d378 
* chore: use {go,oohttp,oocrypto} v1.18.5

This diff pins OONI to use go1.18.5 and oohttp and oocrypto at
go1.18.5 as the version of go with which we build releases.

The same codebase also works with go1.19 although this configuration
cannot include Psiphon (see ooni/probe#2222).

Closes ooni/probe#2223.

* fix: use oocrypto@v0.1.1

This ensures we keep ooni/probe#2122 fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bassosimone bassosimone

Labels
bug Something isn't working data quality ooni/probe-engine platform/android platform/ios platform/linux platform/macos platform/windows priority/high
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

bug: add aes support for darwin/arm64 ainghazal/oocrypto
2 participants
@bassosimone @ainghazal