Swap out iframely #1706
Swap out iframely #1706
Conversation
src/main.rs
Outdated
| // Required because docker installs try to use TLS, which is disabled as a reqwest feature flag | ||
| let client = reqwest::ClientBuilder::new() | ||
| .danger_accept_invalid_certs(true) | ||
| .build()?; |
There was a problem hiding this comment.
We should only use this for testing, not in prod.
There was a problem hiding this comment.
This fails for both dev and prod. The only way to not do this would be to turn on an SSL feature flag of reqwest.
There was a problem hiding this comment.
And thats enabled by default. Its also required by activitypub, so its impossible to disable it. Not to mention that it would be a security vulnerability for open graph fetching as well.
So I really dont understand why you think it is necessary to accept invalid certs.
There was a problem hiding this comment.
It took me a while to find the ssl bug, but the docker post creates will fail without this flag. I'll redo it then post the error.
There was a problem hiding this comment.
Yep, just tested removing it, and it gets this error every time:
error sending request for url (https://www.marxists.org/archive/lenin/works/1921/apr/21.htm): error trying to connect: error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed:../ssl/statem/statem_clnt.c:1913: (unable to get local issuer certificate)
I remember googling it, its due to SSL not being correctly installed on the system that uses reqwest, IE using the native SSL.
There was a problem hiding this comment.
If we merge this without using .danger_accept_invalid_certs(true), all post creates with a URL will be broken, and this feature won't work on any official install.
There was a problem hiding this comment.
But the SSL problem doesnt happen in prod, otherwise federation wouldnt work either.
There was a problem hiding this comment.
Ohhh okay I can maybe guess the issue. We never use or generate self-signed certs (which apparently reqwest needs ) , we use letsencrypt for the ansible deploys, which docker is somehow smart enough to be able to insert into from the system into our docker container, which rust can then use. I have no idea how it does this without any configuring, because we haven't touched a thing or passed these forward in any way.
The best way would be to check if it exists using openssl_probe: https://docs.rs/openssl-probe/0.1.4/openssl_probe/fn.has_ssl_cert_env_vars.html , and use the dangerous flag if there are no certs, and output a warning in the console on lemmy startup.
Reqwest works in our system unit tests by using the local system generated certs, but our docker images don't do this.
There was a problem hiding this comment.
Reqwest as http client doesnt need access to website certificates, those are sent by the server. All it needs are the CA certificates in order to verify the tls certificate. Did you try apt-get install ca-certificates and checking CURL_CA_BUNDLE env var as described in my link above?
There was a problem hiding this comment.
Ah that appeared to do it, I forgot that the volume_mount uses a different DockerFile.
I wonder why this doesn't need to be done with the musl images.
dessalines
force-pushed
the
swap_out_iframely
branch
from
4e72057
to
804c385
Compare
This reverts commit dbf7d1b.
dessalines
force-pushed
the
swap_out_iframely
branch
from
d75b497
to
bc3a8ee
Compare
crates/utils/src/request.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub async fn fetch_iframely_and_pictrs_data( | ||
| /// Both are options, since the URL might be either an html page, or an image | ||
| pub async fn fetch_site_metadata_and_pictrs_data( |
There was a problem hiding this comment.
Imo the thumbnail is also part of metadata, so no need to mention pictrs explicitly here. This method should definitely log errors (under verbose). A more detailed description of what it does would also be nice.
There was a problem hiding this comment.
There's already a fetch_site_metadata function, which refers specifically to this SiteMetadata struct I just added. I can change the name to fetch_site_data tho.
| context: &Data<LemmyContext>, | ||
| _websocket_id: Option<ConnectionId>, | ||
| ) -> Result<GetSiteMetadataResponse, LemmyError> { | ||
| let data: &Self = self; |
There was a problem hiding this comment.
Why do you rename self in every API method? For me its quite confusing, because it doesnt actually do anything.
There was a problem hiding this comment.
Just history, these used to be very different impls. If someone wanted to go through and change all these to self, it wouldn't hurt anything, but data to me is more clear as its describing that its the Form data.
This is obviously missing embed codes, IE
embed_htmlin our DB, but I greatly doubt they were used much anyway (and they're a security risk).