nodeSolidServer / node-solid-server

Solid server on top of the file-system in NodeJS
https://solidproject.org/for-developers/pod-server
Other
1.78k stars 303 forks source link

Basic prep #1792

Closed CxRes closed 2 weeks ago

CxRes commented 2 months ago

This PR implements PREP/Solid-PREP as an alternative to the existing notifications mechanism in NSS which was deprecated for security reasons.

PREP/Solid-PREP bring many improvements to the existing notifications mechanism in NSS, as illustrated in this table: WebSockets API Solid Notifications PREP/Solid-PREP
Transport WebSockets Multiple HTTP Streaming
Discovery Headers Discovery Resource
Link Headers
Headers
Initialization
AuthN/Z No Yes Yes
RoundTrips 2 1-2 0[^1]-1
Features
State No Yes Yes
Rate No Optional No
Termination
End Time Undefined Negotiated Negotiated
Closing Undefined Automatic Automatic
Notifications
Format Custom RDF/AS2 Any[^2]
Target Yes Yes Yes
Activity No Yes Yes
Object URL No Yes Yes
Time No Yes Yes
Content Negotiation No Yes Yes
Deltas No In Development Optional
Error Messages No In Development In Development

The last published version of Solid Protocol and the ED mentions that WebSockets-API that NSS uses does not include an authentication mechanism. That was the main reasons why Solid-CG's notifications panel deprecated the WebSockets-API and transitioned Solid to the Solid Notifications Protocol. Solid-PREP is part of this incubation work in the CG.

[^1]: PREP can fetch notifications along with the representation body in the response. [^2]: Solid-PREP serves notifications that are identical to Solid Notifications Protocol, with suitable content negotiation requesting an RDF media-type.

CxRes commented 2 months ago

I do not understand why nyc is failing with EEXIST. Will need some help here as I am blind testing on Windows.

CxRes commented 2 months ago

If I disable nyc (now replaced with c8), I get just one other error https://github.com/CxRes/node-solid-server/actions/runs/10639483327/job/29497511727#step:12:702

I am not sure if this error is an artifact of running tests in my own repo or actually a bug!

CxRes commented 2 months ago

Tests pass. Phew!

I would need guidance for writing tests specific to notifications.

Also need to check with reviewers if they are happy migrating to Node 20 (which is the active LTS).

bourgeoa commented 2 months ago

Tests pass. Phew!

Well done.

I would need guidance for writing tests specific to notifications.

I don't know what to say.

CxRes commented 1 month ago

I added tests as well (coverage, though not 100%, covers all relevant functionality), which means that this PR should have been ready to merge.

However, one surface test, which has absolutely nothing to do with this PR, fails! The failure only happens with the commit that adds tests.

melvincarvalho commented 1 month ago

Hi @CxRes, during the call today you mentioned wanting a review. I'd be happy to try and help. Just a quick question—do you have an issue or a high-level overview tied to this, or a bit of context?

melvincarvalho commented 1 month ago

Could this be a switch for NSS. For example Websocket uses options.live for live updates. Perhaps options.prep could go in the config for prep (if it isnt already)?

CxRes commented 1 month ago

@melvincarvalho Thanks for the offer!

The high level overview are the specifications:

https://cxres.github.io/prep/draft-gupta-httpbis-per-resource-events.html and https://solid.github.io/solid-prep/protocol/

Re Switch: There is no switch right now. I don't think one is even needed, because everything is HTTP and unless the request has the specific header (Accept-Events), the GET response is not modified. OTOH, I am happy to add one, if one can show me how to access the switch variables in the code. Its basically wrapping the middleware invocation in 3 files in if-else statements.

The other enhancement that I would like to investigate (not for this PR) is a way for the user to specify how to turn on and off PREP for a resource and globally for a POD or specify which media-types to use for notifications, maybe using description resource or ACLs.

I don't want to rush you, but I would really like to merge the code soon, though, because I want to make the case for it working at the next IETF and those discussions have already started (and maybe even coax one or two developers to implement clients for it, which IMHO is very easy with the libraries I provide). This was meant to be ready at the last IETF, and as you are already aware, been held up for many months. If the code is not breaking anything, I am happy to make improvements over the winter.

melvincarvalho commented 1 month ago

I am happy to add one, if one can show me how to access the switch variables in the code

Thanks! Here's how it's handled for websockets—might be a similar approach for this.

https://github.com/nodeSolidServer/node-solid-server/blob/f5b2b5cd88e9201138ac4e65c498e0dc5c2c6539/bin/lib/options.js#L141

melvincarvalho commented 1 month ago

This also needs a version bump, consistent with semver

CxRes commented 1 month ago

This also needs a version bump, consistent with semver

Bumping is the job of the maintainers ;-) !

CxRes commented 1 month ago

Yay! Tests are passing again. Thanks for the flag tip @melvincarvalho!

melvincarvalho commented 1 month ago

Thanks, that’s great news. Let’s definitely review this in our next meeting.

I think it’d be helpful to cover a few things, including:

express-prep, express-negotiate-events, express-accept-events (if I’m remembering correctly).

https://github.com/CxRes?tab=repositories

Also, worth discussing semver while we’re at it.

Another point—when using a PREP client on an HTTP/1.1 server, every GET is keeping the connection alive, so we should probably think about that.

One more thing: it hasn't been tested beyond localhost yet. I’d suggest we aim for at least one working demonstration on a live URI to make sure OIDC is all functioning as expected.

And really pleased to see the switch added—great move. Maybe an idea would be to have the default set to OFF initially, and after some winter testing, we could think about switching it to ON.

CxRes commented 1 month ago

Another point—when using a PREP client on an HTTP/1.1 server, every GET is keeping the connection alive, so we should probably think about that.

This is unfortunately a node thing. I ran the server without PREP and still you get the Connection and Keep-Alive headers.

CxRes commented 1 month ago

I’d suggest we aim for at least one working demonstration on a live URI to make sure OIDC is all functioning as expected.

There is nothing in the code that can remotely interfere with OIDC. Only once the resource has been authorized and is already ready to send, does the PREP logic even kick in (And that too is limited to RDF resources for now).

Maybe an idea would be to have the default set to OFF initially, and after some winter testing, we could think about switching it to ON.

I would really like for PREP to be in the hands of as many NSS users as possible as soon as possible. The (absence of) 'Accept-Events' header is also an OFF switch. If server deployers so feel inclined, they can turn it off.

jeff-zucker commented 1 month ago

I would really like for PREP to be in the hands of as many NSS users as possible as soon as possible.

I agree. I do not see a downside to putting PREP in the next release of NSS with a notation that it is experimental. If this is not possible, at the least it should be available on the :8443 test server. There will be no progress if we can't try out new things.

melvincarvalho commented 1 month ago

I was going through the PREP spec and came across this point:

Due to the limits on connections per domain, the Per Resource Events Protocol is not suitable for use with HTTP/1.1

Since NSS is currently running on HTTP/1.1, maybe it's not ideal to make this the default right now. This could be a good moment to kick off a 3.x branch of NSS that works with HTTP/2 or HTTP/3, along with the necessary tests and reverse proxy considerations. Meanwhile, the 2.x branch can keep things steady for those still on HTTP/1.1.

I imagine some people will always prefer sticking with HTTP/1.1 for simplicity or performance reasons, so keeping both branches around could be useful in the long run.

Personally, I'd hold off on merging until we've seen it in action on at least one live instance. Since this PR is already in a branch, it can easily be deployed and tested from there.

CxRes commented 1 month ago

This is me being through and transparent. The entire paragraph is:

Due to the limits on connections per domain, the Per Resource Events Protocol is not suitable for use with HTTP/1.1, especially when providing notifications for multiple resource from the same domain that might be accessed simultaneously. This limitation is identical to that of other HTTP based streaming based protocols such as Server-Sent Events.

This has not been a problem in practice, for example, with CSS already providing Solid Notifications over streaming HTTP with HTTP/1.1 (which has identical limitations). SSE, which is well established and widely deployed, also has the same limitations.

As it is, we have a paucity of NSS maintainers, a HTTP 2.x version of NSS is a going to be a long while. If we were to wait for a 3.x branch, PREP will never get deployed.

Again, no user is obligated or forced to use PREP, which triggers only when they add the specific request header during GET. This is not even a breaking change. Therefore, I see no reason for this onerous standard of testing.

melvincarvalho commented 1 month ago

Thanks for the clarification. While I appreciate the work on PREP, I’m concerned that Solid, as it transitions to a working group, needs stability and well-tested implementations over experimental features that could shift its architecture or REST principles.

Given that PREP is being introduced as the default, and considering it’s only been tested locally by its author, I believe this is premature. The NSS team is small with limited resources, and it’s already a challenge to maintain what we have and fix existing bugs. We may be too thinly spread to properly review, test, and maintain this feature.

That said, this seems like a good technical topic for discussion, but I don’t think it can be merged before it’s tested on live servers to show it works reliably. It can be deployed from the PR branch to the community test server on port 8443, and perhaps some other servers as well.

From my point of view, keeping the 2.0 branch focused on stability and HTTP/1.1, while starting a 3.0 branch for more experimental features like PREP—potentially with HTTP/2 and requiring TLS everywhere—might be ideal.

For my part, I’m keen to focus on the HTTP/1.1 version of NSS and get it as bug-free as possible.

jeff-zucker commented 1 month ago

Again, no user is obligated or forced to use PREP, which triggers only when they add the specific request header during GET. This is not even a breaking change.

I agree. Since it is not a breaking change and impacts no one except those who want to experiment with it, we should implement it ASAP. Talk of future possibilities like HTTP/2 is a total red-herring and not relevant to adoption of the hard work that exists now.

elf-pavlik commented 1 month ago

Since NSS is currently running on HTTP/1.1, maybe it's not ideal to make this the default right now.

What stops someone from running NSS behind a reverse proxy that handles HTTP/2?

If the problem is with having PREP on by default, I suggest merging this PR without the default and creating a separate PR that makes it on by default, dealing with that detail with input from people who will update their deployments.

melvincarvalho commented 1 month ago

@csarven , @bourgeoa Before approving, please take into account this limitation from the spec:

'Due to the limits on connections per domain, the Per Resource Events Protocol is not suitable for use with HTTP/1.1.'

I strongly agree with this point. The assumption that the best place to get updates is directly from the resource is, in practice, problematic. It can lead to an excessive number of open connections without efficient multiplexing. A dedicated updater service is a far more scalable and efficient approach, as it avoids the overhead and connection limits that PREP introduces.

I also recommend thoroughly reviewing the code and demonstrating a working example before moving forward with approval, as this hasn't been done yet.

csarven commented 1 month ago

I'd just like to mention that one of Solid CG's positions on https://github.com/solid/solid-spec/blob/master/api-websockets.md can be found in https://solidproject.org/ED/protocol#notifications-protocol :

The following is non-normative.

The Solid WebSockets API (Unofficial Draft) [SOLID-WEBSOCKETS-API] has been the common notification protocol for many years. That draft does not include an authentication mechanism, and therefore this Protocol has transitioned to require the Solid Notifications Protocol.

Existing client and server implementations should begin providing support for the new notification protocol while supporting backwards compatibility, as appropriate.

In fact, the Solid CG kicked off the Solid Notifications Panel to address the shortcomings with SOLID-WEBSOCKETS-API. One of the outputs was the Solid Notifications Protocol, and a number of channel types. (Aside: The Solid Notifications Protocol was listed as one of the input documents for the LWS WG charter.)

With respect to WebSockets, the work on WebSocketChannel channel type that works part of Solid Notifications Protocol is what's deemed to be the way to approach WebSockets in Solid Protocol.

And just to clarify, I mention all of this not because NSS (or its maintainers, we, anyone) "must" follow the above but that we/they should be mindful of the path that's been in the works for a number of years now by the CG. So, all things considered, there should be more energy put into adopting the most recent work while working on deprecating the older work. This shouldn't be a debate at this point. We're past that. If there is conflicting or stronger data, that can be shared (publicly linkable) and reviewed.

That aside, Solid-PREP is part of the incubation work in the CG: https://solidproject.org/TR/ and it is not proposed to replace WebSockets in the Solid Protocol (AFAIK) but that it can be adopted by implementers (servers, applications..) that wish to support it in their products.


Regarding:

Due to the limits on connections per domain

As I understand it, minor clarification: the browser limits apply to per host, not domain.

best place to get updates is directly from the resource is, in practice, problematic.

Can we study those problems based on practice? As I understand it, Server-sent Events is one example where it uses the resource directly to get updates. But if some problematic practices are not known to WHATWG, perhaps it'd be best to bring up to them.

CxRes commented 1 month ago

@csarven Thanks for bringing your wealth of knowledge about Solid to provide this historic retrospective.

I have also updated the topmost comment to explain the reason we need another notifications mechanism in NSS, along with a table comparing the various notification mechanisms available in Solid.

As I understand it, minor clarification: the browser limits apply to per host, not domain.

Thanks for the correction, I shall update have updated the specification document accordingly.

Can we study those problems based on practice?

That is why this PR is so important to merge. Having working servers imho is the best way to compare different approaches in the real world.

melvincarvalho commented 1 month ago

I have also updated the topmost comment

Just a quick note on the comparison table.

It’s worth pointing out that WebSockets inherits its AuthN/AuthZ from HTTPS, since WSS is an HTTP upgrade protocol. So, the suggestion that it lacks these by design isn’t quite accurate. It’s more a matter of aligning it properly or fixing the bugs to ensure everything works as expected, security-wise.

Given that we’re quite thinly spread at the moment, I think it’s important we focus on getting the current system fully functional and bug-free.

melvincarvalho commented 1 month ago

Please add to the comparison table which versions of HTTP each column is suitable for.

This limitation from the spec should also be included in the comparison, especially since NSS runs on HTTP/1.1:

"Due to the limits on connections per domain, the Per Resource Events Protocol is not suitable for use with HTTP/1.1."

elf-pavlik commented 1 month ago

especially since NSS runs on HTTP/1.1

https://github.com/nodeSolidServer/node-solid-server/pull/1792#issuecomment-2408642218

What stops someone from running NSS behind a reverse proxy that handles HTTP/2?

@melvincarvalho, I see you keep claiming that NSS runs on HTTP/1.1 could you please clarify what stops one from deploying it behind a reverse proxy Nginx/HAproxy/Traefik/ you name it, which will handle the HTTP/2?

melvincarvalho commented 4 weeks ago

@elf-pavlik, just to clarify, NSS does run on HTTP/1.1. This is a fact, not a claim.

Regarding reverse proxying for HTTP/2:

  1. Reverse Proxy Doesn’t Fix the Core Issue:

    • Even with a reverse proxy in front, NSS still operates on HTTP/1.1. The underlying server hasn’t changed, and the reliance on HTTP/1.1 remains. Switching NSS to HTTP/2 requires proper testing and a major version bump, as everyone using NSS today is running it on HTTP/1.1.
  2. Added Complexity:

    • Requiring a reverse proxy makes the installation process much harder. Instead of a simple NSS setup, users now have to configure and maintain a separate reverse proxy, increasing the burden on those without dedicated DevOps teams. This added complexity particularly impacts hobbyists, those running NSS on low-powered devices, or even on phones. What was once a lightweight, easy-to-install server becomes a multi-layer system that requires significant additional setup and knowledge.
  3. Increased Debugging and Moving Parts:

    • Introducing a reverse proxy adds more moving parts, increasing the chances of bugs or misconfigurations across the system. Each additional layer requires more effort to debug, monitor, and maintain. Additionally, debugging HTTP/2 is much more complex than HTTP/1.1 due to its multiplexed streams and different handling of connections. If something breaks between the reverse proxy and NSS, identifying the issue is more challenging with HTTP/2, especially for users unfamiliar with its intricacies.

In short, reverse proxies don’t solve the issue. There will always be users who need NSS to remain lightweight and simple, running on HTTP/1.1 without the overhead of added layers, more complex installations, or potential issues.

melvincarvalho commented 4 weeks ago

This also needs a version bump, consistent with semver

Bumping is the job of the maintainers ;-) !

Just a quick note about versioning. 😊 While maintainers typically handle the final bump, semantic versioning does ask that a PR proposes the right version change, especially if there are any breaking changes or big shifts involved.

In this case, if there’s any shift towards HTTP/2 or other significant updates, it’s probably worth suggesting a major version bump (moving to 6.x) to reflect that. API changes or anything that might affect existing users would definitely fall under that category.

For stability, I’d suggest keeping the current 5.x branch free of these changes. Maybe experimental work could go in a new branch or 6.x? That way, we keep things smooth for users on 5.x who depend on everything working just as it does today.

elf-pavlik commented 4 weeks ago

Even with a reverse proxy in front, NSS still operates on HTTP/1.1. The underlying server hasn’t changed, and the reliance on HTTP/1.1 remains.

This doesn't matter for the Notification Receiver in a web browser since they will connect over HTTP/2

Requiring a reverse proxy makes the installation process much harder. Instead of a simple NSS setup, users now have to configure and maintain a separate reverse proxy, increasing the burden on those without dedicated DevOps teams.

This needs more details, including the auto-renewal of certificates. Modern reverse proxies handle that automatically. Two complete setups would need to be compared side by side.

This added complexity particularly impacts hobbyists, those running NSS on low-powered devices, or even on phones.

They don't need to enable PERP. If the argument is about having it on by default, I think a reasonable step, which I already suggested, would be to merge it with PREP disabled by default. At the same time, someone could provide deployment instructions, playbook, flake, compose file (you name it), which would make it easy to deploy it behind a reverse proxy and use config that enables PREP.

Looking at https://github.com/nodeSolidServer/node-solid-server/graphs/contributors It seems like @bourgeoa should have the final say on this PR. Until then, I think there is no point on further reiterating the same opinions.

CxRes commented 3 weeks ago

Can you please try the conformance test at the https://github.com/nodeSolidServer/node-solid-server/pull/1792/commits/33997ae5ff2487b7f03273d93eab63fa553935f8 commit. Given this is a container 404, the only change in the code that even could cause this failure is the commit after that.

Also, is the the only test that is failing, or are there others? Can you direct me to the repo where the test is set up?

melvincarvalho commented 2 weeks ago

They don't need to enable PERP. If the argument is about having it on by default, I think a reasonable step, which I already suggested, would be to merge it with PREP disabled by default.

I think this may have been merged prematurely. This part also still needs attention. Perhaps a revert would be best.