sandstorm

sandstorm copied to clipboard

We've talked about this, but I can't find an open issue tracking it, so I'm creating one.

Now that the powerbox exists, we should start working towards replacing the functionality of HackSessionContext, so that we can remove it.

Things that need doing:

• [ ] Right now receiving email and static publishing both make use of a "public id." Instead:
  • [ ] To receive email, apps should make powerbox offers for EmailSendPort.
  • [ ] static publishing should also use some powerbox interface. There's an interface called WebSite that's speced out in web-publishing.capnp, but it does not appear to be referenced from anywhere else in the source tree.
• [ ] The method getUserAddress should be replaced. For most use cases, this can probably be replaced with a powerbox request for EmailSendPort.
• [x] httpGet needs to be removed. Apps should use the powerbox with ApiSession instead.
  • [x] Likewise for getUiViewForEndpoint

@kentonv suggested having a grandfathering process for older apps that use these APIs, so we don't have to fix every existing app before removing an interface. We could start a new capnp file analogous to appid-replacements.capnp, in which we'd whitelist apps permitted to continuing using these APIs.

For each API that we want to remove, once we have implemented the alternative functionality, we should identify popular legacy apps that use these APIs and, if porting is non-trivial, add them to the whitelist.

It would be cool if, for compatibility purposes, we could implement some sort of wrapper that actually runs inside the sandbox that implements HackSessionContext in terms of powerbox requests. sandstorm-supervisor could spawn this wrapper when it detects that it's loading an old app, based on "API version" in the metadata. In this case there would be no need to grandfather old apps.

However, I haven't thought through whether that would actually be practical to re-implement all the old functionality this way. Certainly it's likely that there would be some quirks caused by powerbox requests being injected where they didn't happen before, but maybe we can find a way to do it that works acceptably with all current known apps?

Quoting Kenton Varda (2020-01-26 04:16:01)

It would be cool if, for compatibility purposes, we could implement some sort of wrapper that actually runs inside the sandbox that implements HackSessionContext in terms of powerbox requests. sandstorm-supervisor could spawn this wrapper when it detects that it's loading an old app, based on "API version" in the metadata. In this case there would be no need to grandfather old apps.

However, I haven't thought through whether that would actually be practical to re-implement all the old functionality this way. Certainly it's likely that there would be some quirks caused by powerbox requests being injected where they didn't happen before, but maybe we can find a way to do it that works acceptably with all current known apps?

One thing that jumps to mind: this would probably require implementing SessionContext.request(), as we'd need to trigger powerbox requests from the server.

I suspect there are few enough apps that actually use these APIs that an 80%-solution compatibility shim might be good enough.

It might also just be worth running through existing apps in the market and figuring out what's being used and how. If there's only like one app that's using a particular API maybe we just fix that one apps ourselves and remove the API outright.

We just pushed a release of ttrss that no longer uses hack-session. Do we know of any other users of httpGet() and/or getUiViewForEndpoint()? I can't think of any. I suppose there could be private apps that use these that we don't know about.

Unless there are clear blockers others can think of, I'd like to start pinning down a process and timeline for removing those methods, as they are the most dangerous of the group.

Would it make sense to put up a beacon url which sandstorm can ping every time someone uses one of the to-be-removed apis? At least for those who have opted in to anonymous usage statistics.

I would be hesitant to rely too much on telemetry (and adding more of it). If there was one thing I maybe wish we had in it would be appVersion, so we could see how long it takes for a statistical majority of users to be using the latest TTRSS.

In general though, I'd prefer Sandstorm avoid telemetry-driven development practices though, as they're a generally harmful behavior. If anything, I would rather if we detect deprecated functionality in use, we warn the server admin and inform them of where they can notify us it would be a problem.

The main reason one would be worried about preserving functionality would be for private packages, but I don't think there's any safe way to preserve that for packageIds we don't even know about. If keeping private packages and/or old TTRSS spks functional is important, we'd need to whitelist packageIds.

What about a new flag (or versioning number) inside the spk, to specify that the app doesn't need HackSessionContext? Then each new app version that is uploaded to the market could confirm that it doesn't need HackSessionContext, and any legacy apps could still use it. When the admin is shopping for new apps, we could flag legacy ones that aren't compatible?

Or, what about adding a new setting in sandstorm.conf for disabling/enabling HackSessionContext? Then each server admin could disable it on their server when they know they're ready.

Apps are selected and installed at the user level, so server admins, particularly of large multiuser installs, wouldn't know if they should disable it.

Also, most people are surprised this hole exists in the sandbox. I feel we need to default to blocking, not default to allowing. I'd rather have a table like appReplacements where we have people explicitly provide packageIds that should be exempted.

When there is some security hole in Wekan, I do first fix it and release new version of Wekan. After that, I wait does anyone complain did something else get broken also, and look is fixing that possible. Security goes first. It would be nice to also have Sandstorm prefer security first.

I agree we shouldn't use telemetry for this.

I'm not super inclined to go out of my way to avoid breaking hypothetical apps which use APIs that no public app uses, and that were marked deprecated before they were even introduced. Like, the docs for these state right up front that we planned on removing them.

I think for web publishing there's not that much harm in keeping the APIs around, much more benefit to doing so (because it's somewhat widely used), and I think a compatibility shim is realistic. But I think we should just make a hard break on httpGet & getUiViewForEndpoint.

I would suggest that, when we actually remove these methods, we:

• Rename them to obsoleteHttpGet and obsoleteGetUiViewForEndpoint, like the other removed methods in the file.
• In the implementation, throw an exception whose human-readable message indicates that the API has been removed, perhaps with a link to a longer explanation.

This way, if there are packages out there that use this that we don't know about, when they break, if users visit the debug console they'll likely see a pretty clear error telling them what's up, and if they try to re-compile the app, it they'll get compile errors where they use the relevant functionality (if they're using a statically-typed language).

I don't think it's worth the effort and complexity trying to do more than that.

My concern would be if an organization exists that uses these APIs and how it would impact them if we break a major internal app with no recourse. We could lose whole servers of users that way.

If we did an environment variable or something (like Kenton did for the experimental gateway) they could change to give them transition time, we'd avoid losing an install.

@ocdtrekkie

Is there any info does anyone have internal Sandstorm apps that uses that API ?

@xet7 No. Kenton might know of some anecdotally, but while the opt-in telemetry collects package names, it wouldn't collect API usage.

That API could be turned off, and then there could be some checkbox in Sandstorm Admin Panel for enabling it, and information that if someone really uses that API, they should comment on this issue.

That's pretty much what I'm thinking. I'm okay pushing an update that just breaks these APIs (best way to find out if anyone still uses them, tbh), but I'm worried that if we get a complaint, we should have a way for people to reenable it, at least while they rework their apps.

How about this roadmap:

• We wait a bit so most boxes have updated to the new ttrss
• Then, we add a flag ALLOW_LEGACY_HACK_SESSION_HTTP to sandstorm.conf, disabled by default. If the flag is disabled, the relevant methods throw unimplemented errors, with an error message including a useful explanation of the situation. If it is enabled, they follow their current logic. We also change the method name at this time, so attempts to re-compile against the curent code will have to update the callers, if even to put the word 'obsolete' in the relevant one way or another.
• After some grace period, we remove the flag, and delete the implementation of the APIs entirely.

If that sounds good to people we just need to agree on timeframe.

That sounds good to me. And nobody needs to add to their conf unless they need to reenable the APIs, everything should stay really clean.

Our errors should probably direct people to contact us rather than instructing them how to silently reenable the functionality. Since we need to know if there's a compelling reason to hold out on completing the deprecation or not.

That means we can give people a "for now" resolution, but we'll also know whether or not people are depending on it.

Quoting Jacob Weisz (2020-08-09 13:56:18)

Our errors should probably direct people to contact us rather than instructing them how to silently reenable the functionality. Since we need to know if there's a compelling reason to hold out on completing the deprecation or not.

+1