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

ooniprobe: new packages #11116

Merged
merged 1 commit into from Aug 5, 2020
Merged

ooniprobe: new packages #11116

merged 1 commit into from Aug 5, 2020

Conversation

ja-pa
Copy link
Contributor

@ja-pa ja-pa commented Jan 25, 2020

Maintainer: me
Compile tested: Turris Omnia (TOS5), OpenWrt master
Run tested: Turris Omnia (TOS5), OpenWrt master

Description:
This PR adds two packages probe-cli ooniprobe package related to network measurements for OONI (Open Observatory of Network Interference).
It helps researchers collect information about malicious behavior in the public networks (us of DPI, middleboxes, censorship,etc.)

Project website https://ooni.org/post/ https://ooni.org

@ja-pa ja-pa changed the title probe-cli & probe-engine: new packages WIP: probe-cli & probe-engine: new packages Jan 25, 2020
@ja-pa
Copy link
Contributor Author

ja-pa commented Jan 25, 2020

The compilation is failing because https://github.com/Psiphon-Labs/bolt doesn't detect mipsel arch. (related issue boltdb/bolt#656) Will try to fix that in probe-engine pkg

@BKPepe
Copy link
Member

BKPepe commented Jan 25, 2020

Wouldn't help if you are going to create an issue in both repositories of the packages, which you want to add to ask if they are going to use bbolt instead of a bolt?

In probe-cli/go.sum and same for probe-engine/go.sum , they are using:
github.com/Psiphon-Labs/bolt v0.0.0-20190731171712-94750aa2185e h1:2zxppKeojJOuiO2aRjvVTD13hTPb+0RCia+TbGZX4Dg= and
github.com/boltdb/bolt v1.3.1 h1:JQmyP4ZBrce+ZQu0dY660FMfatumYDLun9hBCUVIkF4=

In repository Psiphon-Labs/bolt, I see the last commit on 31st July 2019. However, in bolt repository, I was able to found this:

If you are interested in using a more featureful version of Bolt, I suggest that you look at the CoreOS fork called bbolt.

So, you can create an issue in Psiphon-Labs/bolt to add support for mips. As I would like to see handled this in upstream instead of patching it here.

@ja-pa ja-pa force-pushed the ooni branch 3 times, most recently from 82b7b72 to 6aaa83a Compare January 27, 2020 09:16
@ja-pa
Copy link
Contributor Author

ja-pa commented Jan 28, 2020

Thx, I created PR to fix bolt Psiphon-Labs/bolt#1 But still, it will take time when developers will integrate that to upstream version of the package because probe-cli depends on quite an old version of bolt.
I also squash probe-engine into probe-cli, because there is no need to compile it separately and it also fixes some issue with linking library.

@ja-pa ja-pa changed the title WIP: probe-cli & probe-engine: new packages probe-cli: new packages Feb 3, 2020
@ja-pa
Copy link
Contributor Author

ja-pa commented Feb 3, 2020

I squashed commits into a single one. The package contains src for probe-engine and bolt package, which needs to be patched for OpenWrt. After a new probe-cli release, I believe it will be possible to remove these patches.

@BKPepe
Copy link
Member

BKPepe commented Feb 3, 2020

@jefferyto may I ask you to review this Go package?

net/probe-cli/Makefile Outdated Show resolved Hide resolved
net/probe-cli/Makefile Outdated Show resolved Hide resolved
net/probe-cli/Makefile Outdated Show resolved Hide resolved
net/probe-cli/Makefile Outdated Show resolved Hide resolved
+ // #cgo LDFLAGS: -levent_core
+ // #cgo LDFLAGS: -levent_extra
+ // #cgo LDFLAGS: -levent_pthreads
+ // #cgo LDFLAGS: -lz
Copy link
Member

Choose a reason for hiding this comment

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

Can this patch be upstreamed?

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 created an issue , but it's not their priority right now.

Copy link
Member

Choose a reason for hiding this comment

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

Would it work if you added these flags to TARGET_LDFLAGS instead of patching this file?

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 tried that, but it failed in the liking phase. arch Linux has similar patch

https://github.com/ooni/probe-engine/blob/master/measurementkit/task_cgo.go#L29

Copy link
Member

Choose a reason for hiding this comment

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

I've added CGO_LDFLAGS in #11255. That should allow you to set TARGET_LDFLAGS and have it affect cgo compilation (instead of having to patch the flags into the file).

I suggest patching out the #cgo linux,amd64 LDFLAGS: lines though, since that will affect x86-64 builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bassosimone Ok, so I replaced default sentry url with my own url and get the same error as you.

Copy link
Member

Choose a reason for hiding this comment

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

I got this error when testing in malta-be (but not armvirt-64, although I didn't let it finish the whole test). I don't have a stack trace to compare though.

Perhaps you can try running upstream's pre-built (static) binary to see if you get the same error? If it doesn't happen with their binary then it's probably something in our build.

Copy link
Contributor Author

@ja-pa ja-pa Feb 13, 2020

Choose a reason for hiding this comment

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

I just tested version rc9 to rc3 on OpenWrt. The same error is in 9,8,7,6,5,4 but version 3 works as expected. Upstream version for amd64 works without panic.

Copy link
Member

Choose a reason for hiding this comment

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

Just want to confirm: Did you also change the version of probe-engine used when testing the different versions?

Copy link
Contributor Author

@ja-pa ja-pa Feb 13, 2020

Choose a reason for hiding this comment

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

Yes (more or less) I removed the part with probe-engine, since after your changes to golang its no longer necessary. I will push the updated package.

@ja-pa ja-pa changed the title probe-cli: new packages WIP probe-cli: new packages Feb 6, 2020
@jefferyto
Copy link
Member

Also, perhaps this package can be named ooniprobe or ooniprobe-cli (or something with "ooni" in it)? "probe-cli" seems a bit too generic to me.

@bassosimone
Copy link

Wouldn't help if you are going to create an issue in both repositories of the packages, which you want to add to ask if they are going to use bbolt instead of a bolt?

I asked the same question some time ago. I was told that Psiphon prefers to use their own fork of bolt because they have developed a bunch of patches for robustness that they'd rather keep.

(OONI does not use bolt directly.)

@bassosimone
Copy link

bassosimone commented Feb 10, 2020

Also, perhaps this package can be named ooniprobe or ooniprobe-cli (or something with "ooni" in it)? "probe-cli" seems a bit too generic to me.

Yes, please! (FWIW, I like ooniprobe the most, given that it seems to me unlikely that OpenWrt will package the Desktop app, and also the binary is called ooniprobe.)

@hellais
Copy link

hellais commented Feb 10, 2020

Let me also join the package naming game party 🎉

Maybe I suggest calling the package: ooniprobe?

That would be consistent with the naming OONI Probe packages for Command Line Interface users have always had.
Since the CLI binary tool is still invoked via ooniprobe I think that should also be the name of the package.

The reason why the source code repo is called ooni/probe-cli, is that we now have version of OONI Probe for Desktop and Mobile as well, hence the distinctive naming.

Thanks so much for helping out with packaging it!

@hellais
Copy link

hellais commented Feb 10, 2020

Another very useful thing to add to the package would be some way for people to run OONI Probe automatically on a regular basis.

I am not very familiar with what is generally the best practice for doing this in OpenWRT land, but something like a simple cronjob should be enough.

This can either be enabled by default or something the users opt-in to doing.

Check the optional section of the probe-cli readme for a linux and macOS example for this: https://github.com/ooni/probe-cli#user-setup.

@jefferyto
Copy link
Member

You still need to patch out these lines in probe-engine (or somehow set CGO_LDFLAGS_DISALLOW to exclude those flags), otherwise they will affect compilation for x86-64 (linux,amd64).

@bassosimone
Copy link

@ja-pa I have addressed the issue and now you probably don't need to patch sources anymore. Could you perhaps see if you are satisfied with the new code? ooni/probe-engine#333 🙏

@ja-pa ja-pa changed the title WIP probe-cli: new packages WIP ooniprobe ~~probe-cli~~: new packages Feb 17, 2020
@ja-pa ja-pa changed the title WIP ooniprobe ~~probe-cli~~: new packages WIP ooniprobe: new packages Feb 17, 2020
@ja-pa ja-pa force-pushed the ooni branch 2 times, most recently from c922571 to 6750b86 Compare April 29, 2020 12:28
@ja-pa ja-pa changed the title WIP ooniprobe: new packages ooniprobe: new packages Apr 29, 2020
@ja-pa
Copy link
Contributor Author

ja-pa commented May 7, 2020

@jefferyto I think that measurement-kit is not necessary for runtime (I just checked that on my Omnia).

@ja-pa
Copy link
Contributor Author

ja-pa commented May 12, 2020

Updated to version 3.0.0

@neheb
Copy link
Contributor

neheb commented May 13, 2020

measurement_kit needs a dependency on libevent2-openssl.

@ja-pa
Copy link
Contributor Author

ja-pa commented May 14, 2020

measurement_kit needs a dependency on libevent2-openssl.
Yes, but here compilation fails without it

Package ooniprobe is missing dependencies for the following libraries:
libevent_openssl-2.1.so.7

DEPENDS:=\
$(GO_ARCH_DEPENDS) +libevent2-openssl \
+libevent2-core +libevent2-extra +libevent2-pthreads \
+libmaxminddb +libnghttp2 +libopenssl \
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure libnghttp2 is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without it I get

Package ooniprobe is missing dependencies for the following libraries:
libnghttp2.so.14

Copy link
Member

@jefferyto jefferyto May 19, 2020

Choose a reason for hiding this comment

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

I have compiled this package without +libnghttp2 in DEPENDS (and without the -lnghttp2 in TARGET_LDFLAGS) in armvirt-32 and x86-generic, and I haven't encountered this error... Is there a specific target arch I should try compiling for to see this error?

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 use mvebu which should be arm32 . I think it could be somehow related to ooni/probe-engine@ae59c05#diff-38f5d43a0af2601dc205a09e9e321845

Copy link

Choose a reason for hiding this comment

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

When we compile for Linux without the "ooni" flag (which does what we want in our strange Linux setup) we just -lcurl and assume the dynamic linker will figure out.

When I did that, I assumed a distro would have preferred dynamic linking. Is there a reason not to use dynamic linking of C/C++ dependencies here?

Copy link
Member

Choose a reason for hiding this comment

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

Dynamic linking to curl here is fine; TARGET_LDFLAGS has -lcurl.

I was asking about libnghttp2 - it doesn't appear to be necessary anymore in the latest release.

Choose a reason for hiding this comment

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

So, MK never depended directly from nghttp2. AFAIK, however, curl uses nghttp2 to implement http2. Hence, whether it's needed or not I think depends on libcurl.so?

@neheb
Copy link
Contributor

neheb commented May 31, 2020

What's the status on this?

@jefferyto
Copy link
Member

I'm don't believe libnghttp2 is a run-time dependency, or perhaps it should be selected if LIBCURL_NGHTTP2 is selected (similar to libcurl).

DEPENDS:=\
$(GO_ARCH_DEPENDS) +libevent2-openssl \
+libevent2-core +libevent2-extra +libevent2-pthreads +libnghttp2 \
+libmaxminddb +libopenssl +LIBCURL_NGHTTP2:libnghttp2 \
Copy link
Member

Choose a reason for hiding this comment

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

libnghttp2 appears twice in the above 2 lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thx


TARGET_LDFLAGS:=\
-lmeasurement_kit -lmaxminddb -lcurl \
-lnghttp2 -levent_openssl -lssl \
Copy link
Member

Choose a reason for hiding this comment

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

I believe -lnghttp2 also depends on LIBCURL_NGHTTP2 being set.

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 removed that and build/run test worked file

@jefferyto
Copy link
Member

I think the CI error is because the measurement-kit package needs to be updated to 0.10.12.

@ja-pa
Copy link
Contributor Author

ja-pa commented Jul 22, 2020

@jefferyto Thx, I rebased ooni branch to master

@bassosimone
Copy link

@ja-pa @jefferyto as a heads-up, this is one of the latest ooni/probe-cli that uses Measurement Kit as a dependency. Also, as another heads-up: we've discovered an issue with 3.0.4 and we're cutting 3.0.5 in ~1 hour to fix the issue.

@ja-pa ja-pa force-pushed the ooni branch 2 times, most recently from 91f754c to 3b740b0 Compare August 3, 2020 16:54
@ja-pa
Copy link
Contributor Author

ja-pa commented Aug 3, 2020

I just updated ooniprobe to version 3.0.6 and removed dependency on libnghttp2

Runtested with im tests

@jefferyto
Copy link
Member

I'd like to have this added:

define Package/ooniprobe/config
  select MEASUREMENT_KIT_BUILD_DEPENDS
endef

but otherwise this looks good to me.

@ja-pa
Copy link
Contributor Author

ja-pa commented Aug 4, 2020

I'd like to have this added:

define Package/ooniprobe/config
  select MEASUREMENT_KIT_BUILD_DEPENDS
endef

but otherwise this looks good to me.

Added, thanks !

Signed-off-by: Jan Pavlinec <jan.pavlinec@nic.cz>
Copy link
Member

@jefferyto jefferyto left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this!

@neheb neheb merged commit c4254a3 into openwrt:master Aug 5, 2020
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

6 participants