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

fix: Uses rustls-tls for reqwest instead of requiring openssl #1048

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

mrferris
Copy link
Collaborator

@mrferris mrferris commented Nov 29, 2023

What was wrong?

/usr/bin/trin: error while loading shared libraries: libssl.so.1.1: cannot open shared object file: No such file or directory when running a docker image built from trin's docker/Dockerfile.

I thought it was interesting that @njgheorghita saw this error but his instance kept running normally.

This was caused by this commit on master 00cbbd8

Which started using openssl through our reqwest library

How was it fixed?

By changing reqwest = { version = "0.11.13", features = ["json"] }

to

reqwest = { version = "0.11.13", default-features = false, features = ["json", "rustls-tls"] }
default-features = false disables openssl from being used
features = ["rustls-tls"] enables using the rust native tls library

Kolby tested the docker build on portal-hive and it works

To-Do

Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

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

Instead of downgrading Ubuntu, we can download and install the library from the openssl archive:

FROM ubuntu:22.04

RUN apt update && apt install wget -y
RUN wget http://nz2.archive.ubuntu.com/ubuntu/pool/main/o/openssl/libssl1.1_1.1.1f-1ubuntu2_amd64.deb
RUN dpkg -i libssl1.1_1.1.1f-1ubuntu2_amd64.deb

FROM ubuntu:22.04
FROM ubuntu:20.04

RUN apt-get update && apt-get install libssl-dev -y
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say we consolidate this line with line 53...

RUN apt-get update && apt-get install libcurl4 -y && apt-get install libssl-dev -y

@@ -35,7 +35,9 @@ COPY ./rpc ./rpc
RUN cargo build -p trin -p portal-bridge --release

# final base
FROM ubuntu:22.04
FROM ubuntu:20.04
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's also worth a comment mentioning why we're on an old ubuntu in both dockerfiles

Copy link
Collaborator

Choose a reason for hiding this comment

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

Though if @ogenev 's suggestion works, I agree it's better to keep ubuntu version up to date

Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

I think we should revert these changes and add default-features = false, to the cargo.toml in trin-beacon/light-client/cargo.toml

This way we no longer rely on openssl for TLS. Our default JSON-RPC already doesn't support TLS, so why should trin-beacon/light-client rpc support it

reqwest = { version = "0.11.13", default-features = false, features = ["json", "rustls-tls"] } this is another option which would keep TLS support, but not use openssl

@KolbyML
Copy link
Member

KolbyML commented Nov 30, 2023

Yeah so it seems like our solution should be reqwest = { version = "0.11.13", default-features = false, features = ["json", "rustls-tls"] } after some investigation. This uses a rust native ssl library instead of openssl, which is a win win

@mrferris mrferris changed the title fix: adds libssl-dev and downgrades ubuntu 22->20 fix: Uses rustls-tls for reqwest instead of requiring openssl Nov 30, 2023
@KolbyML KolbyML changed the title fix: Uses rustls-tls for reqwest instead of requiring openssl fix: use rustls instead of openssl to avoid non-static lib Nov 30, 2023
@mrferris mrferris requested a review from KolbyML November 30, 2023 16:06
@KolbyML
Copy link
Member

KolbyML commented Nov 30, 2023

whoops didn't mean to switch your title on you xD

editing each other's comments will lead to a lot of confusion and we should probably just put our thoughts in our own comments

@KolbyML KolbyML changed the title fix: use rustls instead of openssl to avoid non-static lib fix: Uses rustls-tls for reqwest instead of requiring openssl Nov 30, 2023
Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

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

Nice 👍

@mrferris mrferris merged commit adb1979 into master Nov 30, 2023
@mrferris mrferris deleted the fix-libssl branch November 30, 2023 16:49
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.

4 participants