-
Notifications
You must be signed in to change notification settings - Fork 11k
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
Enable CORS requests on "/health" server endpoint. #5756
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add the headers to all endpoints?
Yes, I think we should use the post routing to set header to all the endpoints: https://github.com/yhirose/cpp-httplib?tab=readme-ov-file#post-routing-handler In high-level web frameworks, this idea is called "middleware":
This way, we can remove the handler for OPTION method. @StrangeBytesDev If you have time, can you try this approach? |
Be careful! These headers disable a security mechanism! Modern browsers have an additional layer of protection that do not allow Internet websites to access local websites. But let's assume that layer is not there (e.g. because the browser is old or there is a misconfiguration): If the headers are added, this will allow any website you open in your web browser to access your |
Aren't there privacy concerns? Some endpoints include sensitive information like other users' prompts. |
@CyberShadow Sorry because I'm working on cybersecurity sector and I always get mad when people talk about security while they do not understand what they are talking about. In the image below, you see that even big company like OpenAI do CORS on their API: Not only OpenAI, but all modern RESTful API work that way (Facebook API, Google Cloud API,...): Why? Because these API are stateless, meaning there is no cookies attached to them. But now how can I secure my API? Good question, that's why there is API key. In llama.cpp, there an option to specify the API key that is accepted. If you want extra security, you can do that. Otherwise, I never use API key option, because I never leave the llama.cpp server open more than 30 minutes on my computer. And if someone manage to call the API, I can see in the log and just shutdown the server. |
Good for you, but please try to be less arrogant, we are trying to do what is right. There is a big difference here: With OpenAI, it is impossible to access the website without an API key. But llama.cpp does not require an API key by default. As a professional in this sector, you should understand the importance of secure defaults, and also that the specifics of your personal setup and workflow is completely irrelevant! I think allowing CORS iff an API key is specified is a good idea. |
@CyberShadow If you really care about security, I'd suggest you use something other than llama.cpp server example. There are good server implementation out there, for example ollama. The server code is not that secured IMO, as we care more about performance and not security, there are many occasions that you can see memory corruption (segfault,...). They can lead to remote code execution. In short, llama.cpp server is not for production, but rather be an example to show how developer integrate llama.cpp into their own server implementation. |
Well, maybe we should tell that to the |
You can open a PR if you want, but pay attention: Removing CORS by default will break the integration of llama.cpp with some existing frontend implementation. Eventually, many users will open issue regarding that and someone will eventually ask to enable CORS by default. Security is good, but is it worth to worsen the user experience in order to be more secure? The answer totally depends on how user want to use it. |
cd3ec7b
to
87c91c0
Compare
@ggerganov @ngxson I've opened a new PR with CORS enabled on all routes. Partly so that the PR matches the actual changes better, but also because I'm terrible at handling merge conflicts and that was easier. |
@CyberShadow I appreciate your concern towards ensuring that the project is mindful of security best practices. I agree with you that security concerns should be considered seriously. CORS is of course a security feature, so disabling it outright at least feels like a reduction in overall robustness. I do think though, that in this case, CORS is more security theater than practical security. The potential mitigation that CORS would eliminate is browser based requests to the server, from browsers that respect CORS. Non browser based requests won't be prevented, so CORS itself isn't effective at preventing any unauthorized access. Browser based simple requests are also not prevented by CORS, so even if CORS was enabled on all routes by default, browsers could still access the GET and POST routes just by creating an appropriate request with a 'multipart/form-data' content type. The server doesn't require an API key by default, but it does bind to localhost by default. That method is effective at blocking unauthorized access to both browsers and non browser based requests. For those reasons, I think that enabling CORS doesn't add any additional protection in this specific application. But security is hard, and it's more than likely that I haven't thought of every angle. I'd love to hear if there's something important that I hadn't considered. |
I just want to clarify a few things in your response:
I'm not sure it's useful to say this, as it implies that every website a user visits has equal footing as software that they have downloaded and installed to their machine. I think most users expect a higher level of isolation for a visited website.
The request won't be prevented, but it would not be possible for the website to access the response. So, this changes the worst outcome of the threat model from "theft of resources" to "denial of service", which is a lot less enticing to exploit. Disabling CORS allows actually using the result, thus enabling the theft of resources.
I hope I helped clarify some confusion about this, but I'll leave the final say to the project maintainers. |
Whats the threat vector you're imagining exactly? Are you talking about protecting users who are running llama.cpp server locally from random websites accessing it with requests to localhost? Because those are already blocked by all the browsers. I have been assuming you're talking about protecting a publicly available deployment of the server, but if you're talking about users running it locally, then I can't figure out what exactly disabling CORS would benefit. |
FYI, browser does not block requests to localhost, but that's also why I what to keep CORS: Most of users out there are already familiar with running LLM in local without any config (without API key, and with CORS enable). If one day we decide to disable CORS by default and force users to set an API key, they will not understand and will open a bunch of issues asking us "why it worked yesterday but today it no longer working", then "why we need to add such complexity while other programs out there (ollama for example) don't care about?", and then finally someone will just bring back the CORS by default, then someone complains that it's bad for security, then here we go again... |
@ngxson My bad. I was under the impression that Chrome and Firefox did from something I read, but I didn't test it myself. At least Brave does actually block requests to localhost from random websites. |
Well what we're talking about here is all about risks. There are methods to deal with a risk. What I'm doing here is to accept it, the term is risk acceptance and it's not only applied to cybersecurity, but pretty much any other domains. If the impact is low compared to the effort to prevent it, then we don't do. The impact of this risk if say, for example Google, they may lost millions of $$$ and lost client's trust. In that such case, there's no reason not to fix the problem. But for a literally a code example (yes, the The effort that we make to prevent that risk is not just doing some lines of code, but to process all the issues that will be raised by users who are impacted by us disabling CORS. Thus to me it's not worth to fix the issue. Also, the vector attack that @CyberShadow raised is correct, but the chance of such attack happens in the wild is too small. If this bug is in a normal consumer program like let's say Zoom or MS Teams, then that's a real risk. But for a code example like this one, I don't think the chance is higher than winning lottery. |
Currently, cross origin requests to the server are allowed for any of the endpoints that use POST requests.
Additionally, some of the GET endpoints allow for cors requests, but not all. Notably, /props and /v1/models are allowed, but /health /slots and /metrics are not.
Checking "/health" from a frontend application to see if the server is ready would provide some useful additional information over "/v1/models" or "/props".