Fix partial tls support#973
Conversation
|
Thank you, looks good. Are you able to write a regression test for this? |
|
Actually we're probably just going to remove the TLS support entirely. Someone else noticed last week that it's not working and probably hasn't for some time - which implies no one's been using it. Thank for the report and PR. I'm happy to review any other changes you put up. |
|
What you say is extremely disheartening to me. It makes sense nobody would use this because peer code to connect with TLS on the other side has not yet been written, and gateways (which use TLS) are now using ar.io's infrastructure in a different repository. But removing this when the fix seems so simple seems like a step backward. Are you saying that Arweave is not interested in transport security, or rather that a larger PR is needed that integrates more features so the functionality can be immediately used by miners? I have a lot of transport layer issues and have been dreaming of Arweave improving its transport security for years( from 2022 ) which many chains have already integrated for a very long time, such as all chains in the cosmos ecosystem. Right now my chunk2 requests are failing for regions within data sync records, and I can't tell if this is a firewall or a bug because there;is no peer authentication. Arweave only works for me if I have a path to using it successfully. Where is this conversation happening? |
|
I'm not sure how to engage with this. I'm not a fan of rhetorical extrapolations like "Are you saying that Arweave is not interested in transport security" - it's unclear whether you want to engage or just argue on the internet. I have no time for the latter. The code that was removed had not worked for quite some time and no one had noticed. That is why it was removed. Full stop. Your PR was an edit against dead code, which is why it was closed. Full stop. As I also said: Thank you for the report and PR. I'm happy to review any other changes you put up. If you want to submit a new PR with tests which adds TLS support, I will review it. If it is unusable because it implements only half of the TLS system (e.g. as the old, removed code did), we may not be able to merge the PR until it is usable. |
|
@xloem Let me try to reset.
#3 is the reason the code was removed. Going forward we would like to support TLS. Ideal would be a client/server implementation that would be regularly used so we don't repeat the "unused -> buggy -> security risk" path again. That said with the new NASA protocol it's possible that there may be a server-only usecase - but we'd want to firm that usecase up before implementing it. I'm not sure the best path forward, and for now it's unclear how much capacity my team has for this feature relative to other tasks in the short term. If you would like to take a shot at implementing TLS that would be awesome and I can review. If you want to start with just server-side that is also helpful, but we might defer merging it until we can either firm up the NASA-usecase or someone else can add the client-side. |
b07b1d8 broke TLS support introduced in a5a0c4f by changing TransportOpts to a map but not updating its use.
This was identified and fixed by perplexity computer .
Note
Medium Risk
Touches the TLS listener startup path; misconfiguration would prevent HTTPS from starting, but the change is small and localized to option wiring.
Overview
Restores working TLS support for the HTTP interface by updating the
cowboy:start_tls/3call to correctly applycertfile/keyfile(and port) via theTransportOptsmap’ssocket_opts.This replaces the previous list-style transport option extension with a map update, aligning with the newer
TransportOptsrepresentation.Reviewed by Cursor Bugbot for commit 909fdbc. Bugbot is set up for automated code reviews on this repo. Configure here.