Add Socket #2618

SoniEx2 · 2016-11-21T22:47:46Z

Contains

Adds Sockets. Use

@In
SocketManager socket;

In a System to get access to sockets. It has TCP clients, but can be expanded later.

How to test

This is a badly written IRC client that just barely works: https://github.com/SoniEx2/TeraIRC

Outstanding before merging

Separate whitelists.
Whitelists need to be user-configurable somehow.
Rename to "Socket".
~~Move DynamicInstanceProvider to a separate PR.~~ Easier to just keep it here.
~~Disallow internet access on the client.~~ HARD. Depends on the whole CoreRegistry deprecation/removal thing and splitting the internal server from the client. Not high-priority.
~~HTTP, with separate whitelists. Anything with TCP access also has HTTP access, but HTTP access does not give you TCP access.~~ Should be an external "HTTP" module, not part of this PR.
~~Global whitelist that any module can use?~~ Would avoid some duplication, but can easily be misused.
Make the SocketManager thread-safe.

It is deliberately allowed to hand your SocketManager (and thus your whitelist) to another module. After all, it's trivial to wrap SocketManager for cross-module use, with or without being able to share SocketManager itself.

Also, if I knew how, I'd use Netty for this. The idea was to make an implementation-agnostic API, maybe with events, but I'm not very good at that.

GooeyHub · 2016-11-21T22:47:47Z

Can one of the admins please verify this patch?

Cervator · 2016-11-22T06:24:22Z

@GooeyHub: ok to test

@SoniEx2: good to see some actual code!

Hoping to see some good review and discussion here, especially for the change to InjectionHelper - I am not familiar with that enough to be comfortable reviewing that piece myself. Maybe @immortius @flo @Josharias @msteiger @skaldarnar ... somebody please! :-)

GooeyHub · 2016-11-22T06:41:05Z

Hooray Jenkins reported success with all tests good!

oniatus

I like the idea of having an option to open up connections (something similar for a database backend would be awesome too ;) ).

The injection with one instance per module is also a handy feature but I would prefer a different way of registration, e.g. something like @In(scope=Scope.MODULE) with a registration of a dynamic instance provider (interface is fine IMHO) in the registry or a similar place. We can then check in the injection helper if a provider for the module is registered (i like the access to the module name in this place) and then we can ask the provider for an instance for the current module (or throw an exception for whatever reasons).

oniatus · 2016-11-22T08:03:19Z

engine/src/main/java/org/terasology/internet/InternetManager.java

+     * @param port     The target port.
+     * @return The socket.
+     * @throws IOException If an I/O error occurs.
+     */


I would prefer to return the actual socket and leave the in/out-handling to the caller.

I don't think exposing java.net to the sandbox is very, uh, sandboxed... It'd allow the sandbox to make its own sockets, defeating the purpose of the sandbox.

oniatus · 2016-11-22T08:08:39Z

engine/src/main/java/org/terasology/internet/internal/InternetManagerImpl.java

+/**
+ * @author soniex2
+ */
+public class InternetManagerImpl implements InternetManager, DynamicInstanceProvider<InternetManager> {


Please separate the provider from the implementation (i will add an additional comment about the per-module instance on the review)

There are 2 1 reasons why I did it this way.

I'm lazy

~~Good luck doing it any other way.~~

Uh I forgot the other reason but this feels more natural. We still want a global instance that everyone can use, even if that global instance does nothing, because other parts of the engine can see and access it, and we need something to return from CoreRegistry.get().

SoniEx2 · 2016-11-22T08:44:18Z

@oniatus This way is user-friendly and backwards compatible as we can just add permission sets (#1703) without modules needing to know or care about it. It's less things for the programmer to worry about ("does this thing I wanna use have permissions so I need to use that 2nd variant of the dependency injection? or can I use it directly?").

skaldarnar

Shouldn't there be only one instance of the provider, managing the cache? What prevents creating multiple providers?
Imho @oniatus comment to separate provider and implementation is sensible in this context.

SoniEx2 · 2016-11-22T13:33:18Z

@skaldarnar You're meant to have a provider for the client and a provider for the (internal/integrated) server (in the same JVM), but that's not fully implemented yet (part of another issue). And in practice there really is only one instance of the provider: the private final Map<Name, InternetManager> permCache;.

I designed this to be future-proof in a way compatible with the current implementation.

GooeyHub · 2016-12-04T18:27:01Z

Hooray Jenkins reported success with all tests good!

SoniEx2 · 2016-12-04T22:42:11Z

@oniatus Still not gonna do that @In(scope=Scope.MODULE) thing. Being able to sandbox old modules is a nice feature to have.

oniatus · 2016-12-05T08:12:40Z

@SoniEx2 I'm fine with that, but we should definitley document the feature somewhere after the PR is merged 😃 Maybe you want to have a look at the checkstyle report in the meantime, there is probably something more to clean up.

Two more open questions from my side:

What would I have to implement, if i want sandboxed database access (e.g. giving access to a wrapped Connection) for some modules?
As far as I see, this would be:
1. Implement a DatabaseManager
2. Implement a DatabaseManagerProvider
3. Add a new Subsystem to the engine which puts the manager and the provider in the root context
I see some potential code duplication, especially in the Manager and Provider classes if we add more sandboxed content (probably also an ocp violation in the engine with the raising number of subsystems). If we only need one or two (Internet and Database are the only one coming to my mind), this is okay for me. If we need more, we should think about a way to make this open for extension of more modules without touching more classes (especially the engine subsystems).
(Maybe out of scope for this change): Where would the actual sandboxing happen?
Let's say we implement a DynamicInstanceProvider which checks if the gameplay module allowed the access of internet for a used module/the user accepted the required extra permissions for the modules somehow, then we would have to replace all existing dynamic instance providers with the new permission strategy.

Just some short ideas about the extensibility of the new feature. Maybe @skaldarnar or @Cervator are the better candidates for such questions, as I do not have the architectual farsight in this scope 😉

In any case I think that both (Internet Access and general Sandboxing of some API) are very valuable features, keep going 👍

SoniEx2 · 2016-12-05T08:23:41Z

@oniatus

For DB:
1. Create a new module, "Database".
2. Use the Internet subsystem to open database connections. This requires you to write it yourself but eh w/e.
3. Implement a DatabaseManagerProvider to control access. DynamicInstanceProvider registration is available to modules (however, they can't retrieve them from the registry/Context).
For updating instances: Look at the InternetManagerProvider. Note how it contains a Name->InternetManager map. Just access that map, then update the instances. But yes, that is definitely out of scope. Right now you can't even modify the whitelist without recompiling.

GooeyHub · 2016-12-05T10:44:19Z

Hooray Jenkins reported success with all tests good!

GooeyHub · 2016-12-08T22:17:40Z

Hooray Jenkins reported success with all tests good!

GooeyHub · 2016-12-08T22:35:23Z

Hooray Jenkins reported success with all tests good!

SoniEx2 · 2016-12-09T12:15:33Z

@oniatus How do the configs look?

rzats · 2016-12-09T14:09:55Z

engine/src/main/java/org/terasology/internet/internal/InternetManagerImpl.java

 import org.terasology.internet.InternetManager;
 import org.terasology.internet.TCPSocket;
 import org.terasology.naming.Name;
+import org.terasology.registry.CoreRegistry;


Looks like this is unused.

(Disregard that, I accidentally commented on the previous commit)

rzats · 2016-12-09T14:15:20Z

engine/src/main/java/org/terasology/internet/internal/InternetManagerImpl.java

+        // TODO We have many options when both a wildcard entry and a specific entry exist. Which one should we pick?
+        // 1. Merge allowed ports.
+        // 2. Wildcard overrides specific hostname.
+        // 3. Specific hostname overrides wildcard. This option pleases me the most. (It also enables blacklists.)


So if I'm reading this correctly, in a whitelist containing *.example.com foo.example.com will no longer be whitelisted after bar.example.com is added to the config? That doesn't sound too intuitive - the second option makes more sense.

No, wildcards don't actually work (yet). *.example.com would only match a literal *.example.com. I only added * as an option so a dedicated "HTTP" module (not an engine subsystem) is a possibility.

But if you had * set to all ports allowed, and you added example.com with only port 80 allowed, then example.com would match first and override the * match - only port 80 would be valid for example.com.

If I do get around to implementing wildcards, then *.example.com will match both foo.example.com and bar.example.com. If bar.example.com exists on the whitelist, then bar.example.com would only match against it, and foo.example.com would still use the *.example.com entry.

This allows the "whitelist" to act as both a whitelist and a blacklist.

That sounds like a far better idea, thanks for the detailed explanation 🙂

rzats · 2016-12-09T14:24:02Z

engine/src/main/java/org/terasology/internet/internal/InternetManagerImpl.java

+        Map<String, TIntSet> allowedHosts = this.allowedHosts;
+        if (!(allowedHosts.containsKey(hostname) || allowedHosts.containsKey("*"))) {
+            // neither hostname nor wildcard in allowedHosts
+            // TODO maybe use a different exception type? (SandboxException or something?)


Sounds good - IllegalArgumentException is IMO best used in Preconditions-esque checks

public int foo (int a) { if (a % 2 == 2) { throw new IllegalArgumentException ("wow you messed up"); } /* ... */ }

and I can't find a good default exception for this usecase.

rzats · 2016-12-09T14:27:24Z

engine/src/main/java/org/terasology/config/InternetConfig.java

+    /**
+     * All the TCP whitelists. Keyed by module ID.
+     */
+    private Map<Name, Hosts> tcpWhitelists = Maps.newTreeMap();


AFAIK there's some sort of TObjectObjectMap class. Might be worth using alongside TIntSet.

There's no benefit in this case.

Socket is more accurate.

GooeyHub · 2016-12-13T21:03:42Z

Hooray Jenkins reported success with all tests good!

GooeyHub · 2016-12-13T23:22:39Z

Hooray Jenkins reported success with all tests good!

rzats · 2016-12-20T19:01:01Z

@SoniEx2: looks like the TODO list in the description is fully finished - is this ready for a final review?

SoniEx2 · 2016-12-21T02:26:06Z

@rzats Not yet, looks like I overlooked something. (Sorry!)

SoniEx2 · 2016-12-27T03:55:49Z

@rzats Fixed.

GooeyHub · 2016-12-27T04:02:30Z

Hooray Jenkins reported success with all tests good!

flo

I first was very doubtful when I saw the PR originally that we want to do this at all.Since it is not really that an important feature and it adds a risk. I also know that I am not the only one thinking that way. However you put good effort in thinking of a way to make it secure. So I went ahead and checked if this can be already merged or not.

I think some additional work is required before this can be merged, but I am no longer that sceptical as I was first.

To sum it up for others, the basic idea to make it secure is that at injection time the module of the class that gets injected gets determined. Via that class a module specific instance will be injected. So the way for creating a module specific instance can be protected against access. So as long as the module keeps it's field private no other module will be able to call the module specific instance. The module specific instance can then do permission checks and allow for example only the access to certain hosts for that module.

flo · 2017-01-11T22:14:25Z

engine/src/main/java/org/terasology/socket/internal/SocketSubsystem.java

+
+    @Override
+    public void initialise(GameEngine engine, Context rootContext) {
+        rootContext.putInstanceProvider(SocketManager.class, new SocketManagerProvider());


Root context is wrong place to put something that stores something for modules. e.g. At the end of the game, everything that belonged to modules used in that game should be gone.

Something like sockets should also be closed implicitly at the end of the game.

Where should it go? We don't have separate contexts yet...

Also, if sockets are implicitly closed, how do we give modules enough time to gracefully shutdown?

Actually there are 2 Context objects usually. One engine context that exists until the program terminates. Besides that there is the context that lives for the duration of the game that inherits from the engine context.

if you explicitly want to give modules a chance to do something before the disconnect you can send an event (e.g. to world entity) which they can then listen to.

flo · 2017-01-11T22:17:05Z

engine/src/main/java/org/terasology/registry/CoreRegistry.java

+     * @return A Context object representing this class.
+     */
+    public static Context asContext() {
+        return CoreRegistryContext.INSTANCE;


this method isn't needed. CoreRegistory.get(Context.class) can be used if really needed

This returns a Context that behaves like CoreRegistry - if you change the CoreRegistry backing context, it reflects on the CoreRegistryContext.

As suggested above, I recommend that the thing you register at the context should have the same live span as the context. Thus using something that lives over multiple context lifespans does not make sense.

flo · 2017-01-11T22:19:37Z

engine/src/main/java/org/terasology/exception/SandboxException.java

+ * Used when a resource access has been blocked by the sandbox.
+ */
+@API
+public class SandboxException extends Exception {


Checked exceptions are good for things where you want to force each caller to implement a handling. e.g. typically when something can happen even if the program is written correctly.

If something wrong gets detected that is caused by a programming mistake, then it should be a RuntimeException as it is really not something that must be mentioned in each signature till there is somewhere a handler.

Actually, SandboxException is to be used when the user chooses to restrict what a module can do. So, e.g. a clipboard API would throw SandboxException instead of returning null, if the user disallows clipboard access.

flo · 2017-01-11T22:20:45Z

engine/src/main/java/org/terasology/registry/CoreRegistry.java

-     * @param type   The interface which the system fulfils
-     * @param object The system itself
-     * @param <T>
+     * @param type   The interface which the system fulfils.


it's not necessarly an interface. You could put a class in it.

flo · 2017-01-11T22:21:08Z

engine/src/main/java/org/terasology/registry/CoreRegistry.java

-     * @param object The system itself
-     * @param <T>
+     * @param type   The interface which the system fulfils.
+     * @param object The system itself.


It's not necessarily a system

flo · 2017-01-11T22:23:56Z

engine/src/main/java/org/terasology/registry/CoreRegistry.java

+        return CoreRegistryContext.INSTANCE;
+    }
+
+    private static final class CoreRegistryContext implements Context {


This class can then also be removed as it is not needed for asContext

flo · 2017-01-11T23:02:59Z

engine/src/main/java/org/terasology/context/internal/ContextImpl.java

+    public <T> DynamicInstanceProvider<T> getInstanceProvider(Class<? extends T> type) {
+        if (System.getSecurityManager() != null) {
+            System.getSecurityManager().checkPermission(new RuntimePermission("permGetInstanceProvider"));
+        }


I would like to keep the Context interface and the injection concept as simple as possible.

Some idea I could think of to simplify it a little bit would be to have Context have a inject method so that we don't need to have a method that we must protect against all but one caller. Although I am not that happy with that idea.

flo · 2017-01-11T23:03:24Z

engine/src/main/java/org/terasology/registry/DynamicInstanceProvider.java

+/**
+ * A DynamicInstanceProvider provides an instance dynamically.
+ */
+public interface DynamicInstanceProvider<T> {


DynamicInstanceProvider is kind of a rendundant name as a Provider is already someting that provides you dyamically with instances. Also the javadoc currently is kind of redundant to the class name.

When you just look at this class as another developer you would do very hard to guess why we need it and what to do with it.

I think the idea behind it should be described at one of the classes like this one and other API parts should refer to this class for an explanation. Also the class name should give a better indication what the class is good for. For example the class could be called ModuleSecurityBoundInstanceProvider.

Example for javadoc:

Some managers need to be able to check, if a using module has actually the permission to use a certain feature. The idea is to do this check at injection time. At this point of time the module of the class that gets injected can be determined. With this information it is possible to inject a module specific instance to the class. The creation or determination of this module specific instance gets done via this interface. Please note that implementors of this class must make sure that there is no way for other modules to obtain a instance of this interface and by pass thus the injection.

Of course modules that let a instance be injected should ensure that there is no way to access this instance from outside. This should be document in each type that can be obtained via this interface.

flo · 2017-01-11T23:05:19Z

engine/src/main/java/org/terasology/registry/DynamicInstanceProvider.java

+     * @param context The current context.
+     * @return An instance.
+     */
+    T getInstanceForModule(Name moduleId, Context context);


I think Context should not be an argument, but something that gets passed to the constructor when the object gets created. Reasoning: The provider will live for the same duration like the objects it provides.

flo · 2017-01-11T23:06:35Z

engine/src/main/java/org/terasology/socket/internal/SocketManagerImpl.java

+    /**
+     * The allowed hosts.
+     */
+    //@Nonnull


Comment and redundant javadoc (to name) can be removed.

SoniEx2 · 2017-01-11T23:27:08Z

Lots of the javadocs were added so my IDE wouldn't complain about missing javadocs.

GooeyHub · 2017-02-17T18:20:46Z

Can one of the admins please verify this patch?

GooeyHub · 2017-02-17T18:29:44Z

Hooray Jenkins reported success with all tests good!

skaldarnar · 2017-04-09T15:50:31Z

@SoniEx2 This has been open for some time now, and some of the comments have not been addressed yet (neither have been changed nor commented on further discussion). What is the current state of this PR?

@flo @rzats @oniatus Any comments on whether we can get this merged _soonish_™️ or if it needs more work and/or discussion.

SoniEx2 · 2017-04-09T21:02:35Z

@skaldarnar The PR is uh... dead, mostly.

It needs to be rewritten from scratch and we need a better system than @Ins for cross-module sandboxing. The internet manager provider wouldn't have to be registered as part of the engine/into the core context, if we had such better system. (Basically, it'd allow for better sandboxing of built-in systems in addition to cross-module sandboxing.)

skaldarnar · 2017-04-10T05:38:26Z

Closing in favor of (hopefully soonish) new PR on the matter. We still made good way with this one and all the discussion around it.

@SoniEx2 Thanks for checking back and letting me know. Feel free to open a new PR whenever ready.

oniatus · 2017-04-10T06:41:12Z

For the interested: The discussion point on the forums would be http://forum.terasology.org/threads/config-context-and-security.1761/

Add Internet

b15be0e

GooeyHub added the in progress label Nov 21, 2016

oniatus suggested changes Nov 22, 2016

View reviewed changes

skaldarnar reviewed Nov 22, 2016

View reviewed changes

Clean up

7bfdb6f

Clean up Checkstyle

cab1b31

SoniEx2 mentioned this pull request Dec 5, 2016

Expose @DoNotAutoRegister to modules #2645

Open

oniatus approved these changes Dec 8, 2016

View reviewed changes

Separate whitelists, configurable.

c2c8939

Cleanup static analysis warnings

6edad37

rzats reviewed Dec 9, 2016

View reviewed changes

Rename Internet -> Socket

1afca03

Socket is more accurate.

SoniEx2 changed the title ~~Add Internet~~ Add Socket Dec 13, 2016

Add SandboxException

52c3b24

rzats removed the in progress label Dec 20, 2016

rzats added API Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. labels Dec 20, 2016

Fix thread safety issues

f92ccbd

SoniEx2 mentioned this pull request Jan 10, 2017

FlexibleConfig #2668

Closed

flo suggested changes Jan 11, 2017

View reviewed changes

SoniEx2 closed this Jan 12, 2017

SoniEx2 reopened this Feb 17, 2017

spadaval mentioned this pull request Mar 19, 2017

Add RESTful API option to the game server #2436

Closed

skaldarnar closed this Apr 10, 2017

SoniEx2 mentioned this pull request Apr 19, 2017

Is it possible to use KICL in a sandboxed environment? KittehOrg/KittehIRCClientLib#186

Closed

skaldarnar removed the Api label May 15, 2021

Add Socket #2618

Add Socket #2618

Conversation

SoniEx2 commented Nov 21, 2016 • edited Loading

Contains

How to test

Outstanding before merging

GooeyHub commented Nov 21, 2016

Cervator commented Nov 22, 2016

GooeyHub commented Nov 22, 2016

oniatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoniEx2 Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoniEx2 Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

SoniEx2 commented Nov 22, 2016

skaldarnar left a comment

Choose a reason for hiding this comment

SoniEx2 commented Nov 22, 2016

GooeyHub commented Dec 4, 2016

SoniEx2 commented Dec 4, 2016

oniatus commented Dec 5, 2016

SoniEx2 commented Dec 5, 2016 • edited Loading

GooeyHub commented Dec 5, 2016

GooeyHub commented Dec 8, 2016

GooeyHub commented Dec 8, 2016

SoniEx2 commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GooeyHub commented Dec 13, 2016

GooeyHub commented Dec 13, 2016

rzats commented Dec 20, 2016

SoniEx2 commented Dec 21, 2016 • edited Loading

SoniEx2 commented Dec 27, 2016

GooeyHub commented Dec 27, 2016

flo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoniEx2 commented Jan 11, 2017

GooeyHub commented Feb 17, 2017

GooeyHub commented Feb 17, 2017

skaldarnar commented Apr 9, 2017

SoniEx2 commented Apr 9, 2017 • edited Loading

skaldarnar commented Apr 10, 2017

oniatus commented Apr 10, 2017

SoniEx2 commented Nov 21, 2016 •

edited

Loading

SoniEx2 Nov 22, 2016 •

edited

Loading

SoniEx2 Nov 22, 2016 •

edited

Loading

SoniEx2 commented Dec 5, 2016 •

edited

Loading

SoniEx2 commented Dec 21, 2016 •

edited

Loading

SoniEx2 commented Apr 9, 2017 •

edited

Loading