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

Initial HTTPS support via Naett (partial platform support) #17744

Merged
merged 16 commits into from
Jul 21, 2023
Merged

Conversation

hrydgard
Copy link
Owner

@hrydgard hrydgard commented Jul 20, 2023

Naett is a little C wrapper library on top of multiple OS-specific HTTPS request implementations. By far the most lightweight dependency we can take to support these.

Might switch to something more independent later, see #17583 , but this looks like it will suffice for now to avoid shipping an Android release without RetroAchievements https support.

This enables naett for HTTPS on:

  • Windows + Windows/UWP
  • Android
  • Mac
  • iOS (untested)

The importance of this is because RetroAchievements doesn't have a policy on how long they'll keep http support alive, and new clients are expected to understand https. So I don't want to ship official binaries on Win/Android/Mac that don't support https.

This doesn't yet enable naett on Linux since it requires curl with openssl support, and I'm a bit unclear about how to properly set up that dependency from cmake. And if we're gonna use openssl anyway (which we really, really don't want to do on Windows) we could wrap it around our own http client instead, though will be some extra work.

On platforms not supported by naett, we'll have to avoid https, still. Added a temporary HTTPS_NOT_AVAILABLE build flag for this, so that the upcoming PRs that enable HTTPS for RA and the store can check it. Initially, I only plan to use HTTPS selectively for RetroAchievements, and for the homebrew store too mostly as a test case.

Missing before merge:

Next steps after this:

  • Optional linux support via curl/openssl deps (which may later be replaced with something else)

#17750 should be merged before this.

@hrydgard hrydgard added this to the v1.16.0 milestone Jul 20, 2023
Core/Util/PortManager.cpp Outdated Show resolved Hide resolved
@hrydgard
Copy link
Owner Author

I don't understand why the check is failing. Locally on my linux machine, it now avoids building naett.c (due to the curl dependency on linux) just fine.

@anr2me
Copy link
Collaborator

anr2me commented Jul 20, 2023

/home/runner/work/ppsspp/ppsspp/ext/naett/naett.c:23:10: fatal error: curl/curl.h: No such file or directory 23 | #include <curl/curl.h>

@hrydgard
Copy link
Owner Author

@anr2me yes, I know. but naett.c isn't supposed to be building at all on linux yet, that's the problem. Somehow the logic to prevent it works on my laptop but not on CI.

hrydgard added a commit that referenced this pull request Jul 20, 2023
Following up comment by @anr2me in #17744, to be merged before that one.
@hrydgard hrydgard changed the title HTTPS support via Naett (wip) Initial HTTPS support via Naett (partial platform support) Jul 20, 2023
@hrydgard hrydgard marked this pull request as ready for review July 20, 2023 22:33
Copy link
Collaborator

@unknownbrackets unknownbrackets left a comment

Choose a reason for hiding this comment

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

Outside of cancelling and progress issues, it might be nice even to use this for all requests once ready. In theory, using WinHTTP or etc. will use system proxy settings, which might be better.

The web debugger can't use https easily on the server side (which naett and probably winhttp don't even support), but in theory even the HTTPFileLoader could use naett. The https problem there is about certificates, in theory of course it could serve using a self-signed certificate but that would cause people to get security alerts (or in most cases, just errors they can't easily skip) trying to use it.

-[Unknown]

UI/Store.cpp Outdated Show resolved Hide resolved
Common/Net/HTTPNaettRequest.cpp Outdated Show resolved Hide resolved

const char *methodStr = method_ == RequestMethod::GET ? "GET" : "POST";
req_ = naettRequest_va(url_.c_str(), !postMime_.empty() ? 3 : 2, naettMethod(methodStr), naettHeader("accept", "application/json, text/*; q=0.9, */*; q=0.8"), naettHeader("Content-Type", postMime_.c_str()));
res_ = naettMake(req_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like this is missing any timeout setting, which naett seems to support.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added (though will make more configurable in a followup with a RequestDesc struct

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, there's SetDataTimeout() currently, which I guess Download doesn't currently call.

-[Unknown]

Common/Net/HTTPNaettRequest.cpp Outdated Show resolved Hide resolved
Common/Net/HTTPNaettRequest.cpp Outdated Show resolved Hide resolved
@hrydgard
Copy link
Owner Author

I've patched naett with two PRs: erkkah/naett#18 and erkkah/naett#17 , temporarily, to achieve progress reporting and user agent setting.

@hrydgard
Copy link
Owner Author

I'm gonna get this in as-is, so it gets tested properly ASAP. I'll move naett back to upstream once my PRs are merged.

@hrydgard hrydgard merged commit d4c0d1d into master Jul 21, 2023
19 checks passed
@hrydgard hrydgard deleted the naett-https branch July 21, 2023 16:56
@anr2me
Copy link
Collaborator

anr2me commented Jul 22, 2023

Since Naett can't set user-agent before, i guess it doesn't support custom headers too, right? (since user-agent is a part of header too)
I guess i won't be able to replace my sceHttpDeleteHeader / sceHttpAddExtraHeader implementation using Naett (yet?)

Edit: nevermind, naett's example did shows the ability to add custom header(s), so i guess it won't be an issue as long i use naett directly.

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.

3 participants