Archived Module Improvement Arc

[Immortius]

Name: Module++
Summary: A number of improvements to the core module system, to help for the basis for future modding support
Scope: Engine
Current Goal: Improved shaping during downloading
Phase: Implementation
Curator: Immortius


The goal of this arc is to lay the foundation for future mod support, by firming up module support. Tasks falling under this arc are:

• Some initial cleanup to the metadata systems for components/events. DONE
• Identify Components and Events with URIs, to prevent potential name conflicts between packages. DONE
• Add an improved name resolution system. This will work in the context of a module and its dependencies, so if a prefab in "testmod" has a "location" component this will be resolved to "engine:location", ignoring the presence of "someOtherMod:location" if it is not a dependency of "testmod". If "testmod" did have a dependency on "someOtherMod", then "location" would not resolve fully, and "testmod"'s prefab would have to use a full uri. DONE
• Fix anything that is loaded or save to support full and partial uris appropriately. DONE
• Module versioning support. Modules will have three-part versions (MAJOR.minor.revision), and when declaring dependencies will specify a dependency range. When connecting to a server the versions will also be propagated. Then when loading modules the latest available version that meets all requirements will be used. In some cases this will be impossible (Module A asks for Module C 3.*.*, Module B asks for Module C 1.0.2), which will need to be detected and reported. This will also mean that multiple versions being present (but not running) needs to be supported. DONE
• Prefab extension system - The ability to add extra components into prefabs as part of a module. This would need to be available both programatically, and through some sort of asset.
• Module sandboxing. Modules should have restricted access to many features, such as file system access and network access. This will be handled by restricting modules to accessing only approved classes and classes from other modules, as well as installing a java security manager to prevent access to reflection capabilities. DONE
• Add support for modules requesting higher access, which requires approval by the user.
• Module download from server. Given sandboxing, enable clients to download mods from the server as needed. DONE
• Improved shaping of connections during downloads.
• Updating the modules used by a saved game.

Optional tasks:

• Implement byte code generation based support to replace our current reflection based system for interacting with components/events/etc.

Out of Scope/Future work:

• Allow servers to point to another location to download mods from (so their bandwidth isn't vampirised by connecting clients)
• Any improvements to block definitions (that will be a separate incubator)

 

[Immortius]

I have switched Components and Events over to using URIs (e.g. "engine:location") to identify them. To go with this I've implemented the improved resolution support. This means that prefabs can now either be written like:

{
    "engine:location" : {
    }
}

or

{
    "location" : {
    }
}

In the latter case, the component will be resolved by looking up any location component defined in the module declaring the prefab or its dependencies (including "engine" implicitly). This protects the module from any non-dependency modules that introduce their own location component. It doesn't protected them from a new version of a dependency introducing a location component though - that sort of thing will have to be managed through versioning and version-ranged dependencies.

 

[Immortius]

Module versioning has been added. Modules now have a three-part version, and when declaring dependencies specify a minVersion and optionally a maxVersion (which defaults to the next major version). Each module will only work with the versions between the min (inclusive) and max (exclusive) version.

This means multiple versions of the same module may be present.

Additionally, a little bit of work has been done to the UIDialogModule - it shows the latest versions of modules, and reports incompatibilities. Not too much work has been done here, because ultimately users will not be directly selecting modules (they'll select gametypes, mods, and other features).

 

[Immortius]

Module sandbox is now well underway. Securing modules has two elements:

• Installing a security manager. This security manager is invoked whenever code calls System.getSecurityManager().checkPermission(), which is done by a number of core Java classes to check for things like accessing the network, reading or writing files, or accessing a classloader. The Terasology security manager checks the call stack resulting in the security check, and if it contains a module class ensures that it is calling an approved API - so it is ok if a file is being read if it is caused by a module triggering an asset load, but not if a module is directly reading a file.
• Using a custom class loader for modules with a white list on what classes modules have access to. This means we can limit modules to a subset of classes and interfaces out of Terasology, its supporting libraries like vecmath, and the Java API. If the class doesn't fall on the white list, trying to use it in a module will result in an exception.

This is a whitelist approach, so every permission and class has to be approved for module usage. This means there will be teething issues as we find features that modules should have access to but don't - when sandboxing is activated, let me know if you come across such issues. They will manifest as errors in the logs, likely as a NoClassDefFoundError.

I'm still putting together the initial whitelist - for engine classes this will use an @API annotation to mark the classes and interfaces to expose. For library and java api classes the individual classes will be added, or full packages.

 

[Cervator]

That @API annotation is going to be gold for auto-generating API docs or at least doc stubs. Javadoc for everything, then a way to highlight the API pieces in particular, I'm thinking. Maybe after javadoc gen step scan for @API and put links to those on an API doc page on GitHub. Or maybe we can use the annotation to mark those classes in the Javadoc itself?

Anyway - not really a right-now item of importance. I added an issue in the restructure tracker to not forget.

 

[Cervator]

Bump - at this point it looks like both Marcin Sciesinski and nh_99 have run into needed whitelist entries for their modules mainly related to GUI components

They might be on top of it already but I wanted to mention it in here as well

 

[Skaldarnar]

Will it be possible for modules to introduce new asset types, such as grammar files, quest dialogues and so on? Or is there a security issue with it?

Edit: Found the post I was searching for half an our in the engine road map thread:

Assets work - Further work improving the asset system

• Removing the AssetType enumeration and replacing with something more flexible.
• Adding support for new asset types in modules, including automatically hooking them up.

Have you already started working on this?

 

[Immortius]

No. In general it should be considered whether prefebs can be used in the first instance.

My thinking is that the AssetData classes would be annotated and that would replace AssetType. Some of the information currently provided by AssetType would instead be moved to being part of registering the asset loaders.

There is a security issue in that the modules shouldn't be able to open files directly, so the api for asset loaders will need tweaking.

We can have those assets in the engine in the meantime.

 

[Immortius]

On module sandboxing, I will be rolling out a change to the checking of permissions shortly, so please report any issues encountered by modules.

Under the new system, the check logic is:
1. If the permission has been globally allowed, the check passes.
2. Determine whether module code is in the execution stack (is the permission check ultimately invoked from module code).
3. If no module is involved the check passes.
4. Each class involved between the module code and the permission check is checked to see if it has been granted the permission. If so the check passes.
5. The check fails.

This system allows us the more finely control module permissions - previously as soon as the called into api code they had full permissions. Now we have to explicitly specify when an API allows a module to do something. On the down side it means that really low level activities in the engine can fail if the permission hasn't been granted to an intermediate class.

To aid managing this, @API now accepts a list of permission classes to grant the api class or package, and there are new methods for granting permissions to specific classes (which don't have to be API classes).

 

[Cervator]

Reporting issues as requested!

Hit a few, including on trying to spawn a creature in Portals. The logging seemed pretty useful from its level of detail:

java.security.AccessControlException: Module class 'org.terasology.portals.DefaultMobFactory' calling into 'org.terasology.entitySystem.entity.internal.PojoEntityManager' requiring permission '("java.lang.RuntimePermission" "accessClassInPackage.sun.reflect")'

From that I puzzled out that a visit to TerasologyEngine's initModuleManager was in order. I found a similar looking entry to clone with the stuff from the log statement:

moduleSecurityManager.addAllowedPermission(ClassMetadata.class, new RuntimePermission("createClassLoader"));
moduleSecurityManager.addAllowedPermission(PojoEntityManager.class, new RuntimePermission("accessClassInPackage.sun.reflect"));

That seemed to clear up the initial permission issue, but left me with a different NPE seemingly from the BehaviorSystem, which makes sense since the LASR creatures have Behavior details added - even if Portals haven't previously cared about that. And there are probably other things to clear out there.

Anyway, that still leaves me wondering if that addition in initModuleManager is right, or if there should be a new flavor @API entry directly on PojoEntityManager instead. What's a good way to think about it, and could you point out an example or two?

Another current issue is any game created with the height map generator crashes (the Core module lacking the permission):

java.security.AccessControlException: Module class 'org.terasology.core.world.generator.chunkGenerators.BasicHMTerrainGenerator' calling into 'org.terasology.utilities.procedural.HeightmapFileReader' requiring permission '("java.util.PropertyPermission" "user.dir" "read")'

Which also crashes WoodAndStone and so on since it relies on the HM

 

[Immortius]

Basically what happens is:
Some module code X calls API code Y. Several layers lower an internal class Z triggers a permission check. The correct location for allowing the access could be anywhere between Y and Z inclusive - and there are probably a few we should allow globally. So the stack trace is really useful for working this out.

As for where to add the permissions... @API certainly makes no sense on PojoEntityManager, since it isn't something we want to expose to users (not an API class). Actually granting these permissions is a separate issue to API really. Perhaps a new annotation could be added, although I'm happy with adding things in TerasologyEngine at the moment - an annotation wouldn't add that much, and having this centralized aids management.

How to work out where to grant permissions - I guess it should be the highest sensible location for that permission to appear. So for instance it makes sense that code under AssetManager should be granted FilePermission, since AssetManager loads assets. By having it as high up as possible it avoid potential abuse from modules somehow accessing the lower levels directly and working straight through them. In a way granting permissions to ClassMetadata is an example of something that might be too low - if a module somehow gains access to the ClassMetadata class, it could use it to access the fields of any class. But at the same time it is tricky tracking down all the locations that make use of ClassMetadat. Perhaps the solution there is to add a permission check on creating ClassMetadata instances, since we may be happy with modules using class metadata (even directly), but only on types determined by the engine.

Looking at what you posted...

Basically HeightMapGenerator failing is correct. Modules shouldn't have file direct io access, and it directly accesses a file. Working as intended. The only reason it was working previously was the actual method for accessing the file was in API, but given that it takes a file path string this represents security hole. We possibly need to move the generator back into engine, or have it load a texture asset and use that as the height map instead.

 

[Mike Kienenberger]

Caused by: java.lang.NoClassDefFoundError: org/terasology/rendering/assets/TextureRegion
    at OpenUiAction.onActivate(OpenUiAction.java:54) ~[na:na]
    ... 33 common frames omitted

I think this one is easy -- UIImage requres TextureRegion, along with a few other widgets.

 

[Mike Kienenberger]

Caused by: java.security.AccessControlException: Module class 'OpenUiAction' calling into 'org.terasology.rendering.nui.internal.NUIManagerInternal' requiring permission '("java.lang.RuntimePermission" "accessDeclaredMembers")'
    at org.terasology.engine.module.ModuleSecurityManager.checkAPIPermissionsFor(ModuleSecurityManager.java:179) ~[bin/:na]
    at org.terasology.engine.module.ModuleSecurityManager.checkModAccess(ModuleSecurityManager.java:160) ~[bin/:na]
    at org.terasology.engine.module.ModuleSecurityManager.checkPermission(ModuleSecurityManager.java:188) ~[bin/:na]
    at java.lang.Class.checkMemberAccess(Class.java:2237) ~[na:1.7.0_45]
    at java.lang.Class.getDeclaredFields(Class.java:1805) ~[na:1.7.0_45]
    at org.reflections.ReflectionUtils.getFields(ReflectionUtils.java:125) ~[reflections-0.9.9-RC1.jar:na]
    at org.reflections.ReflectionUtils.getAllFields(ReflectionUtils.java:119) ~[reflections-0.9.9-RC1.jar:na]
    at org.terasology.rendering.nui.internal.NUIManagerInternal.inject(NUIManagerInternal.java:292) ~[bin/:na]
    at org.terasology.rendering.nui.internal.NUIManagerInternal.prepare(NUIManagerInternal.java:285) ~[bin/:na]
    at org.terasology.rendering.nui.internal.NUIManagerInternal.pushScreen(NUIManagerInternal.java:123) ~[bin/:na]
    at OpenUiAction.onActivate(OpenUiAction.java:56) ~[na:na]
    ... 33 common frames omitted

Not sure yet how to fix it, so I guess I better read the previous posts more carefully

EDIT: Fixed by adding

moduleSecurityManager.addAllowedPermission(NUIManagerInternal.class, new RuntimePermission("accessDeclaredMembers"));

to TerasologyEngine.initModuleManager()

 

[Immortius]

I've addressed these, although I probably went a bit further/too far by refactoring things to help.

I moved TextureRegion and the SubTexture classes to live under the texture package, which naturally adds them to the API.

A much bigger change was that I created a new org.terasology.registry package and moved the CoreRegistry and In/Share annotations to live there. Then I added an InjectionHelper class to centralize injection and sharing. I left deprecated copies of CoreRegistry, In and Share, and those are still functional, so modules are not immediately broken from this change.

I also put a permission check into CoreRegistry's putPermanently() method, as this should not be available to modules (it is intended for core engine systems that exist for an entire run of the game).

 

[Mike Kienenberger]

Caused by: java.security.AccessControlException: Module class 'org.terasology.miniion.componentsystem.action.OpenUiAction' calling into 'org.terasology.logic.manager.GUIManager' requiring permission '("java.lang.reflect.ReflectPermission" "suppressAccessChecks")'
    at org.terasology.engine.module.ModuleSecurityManager.checkAPIPermissionsFor(ModuleSecurityManager.java:179) ~[bin/:na]
    at org.terasology.engine.module.ModuleSecurityManager.checkModAccess(ModuleSecurityManager.java:160) ~[bin/:na]
    at org.terasology.engine.module.ModuleSecurityManager.checkPermission(ModuleSecurityManager.java:188) ~[bin/:na]
    at java.lang.reflect.AccessibleObject.setAccessible(AccessibleObject.java:128) ~[na:1.7.0_45]
    at java.lang.Class$1.run(Class.java:353) ~[na:1.7.0_45]
    at java.lang.Class$1.run(Class.java:351) ~[na:1.7.0_45]
    at java.security.AccessController.doPrivileged(Native Method) ~[na:1.7.0_45]
    at java.lang.Class.newInstance(Class.java:350) ~[na:1.7.0_45]
    at org.terasology.logic.manager.GUIManager.loadWindow(GUIManager.java:275) ~[bin/:na]
    at org.terasology.logic.manager.GUIManager.openWindow(GUIManager.java:240) ~[bin/:na]
    at org.terasology.miniion.componentsystem.action.OpenUiAction.onActivate(OpenUiAction.java:58) ~[na:na]

The workaround for this is to do the window creation in the module and hardcode the window-id to window instance creation. As long as we call uiWindowInstance.open() the window will be registered with the window manager. UIWindow.open() is the only safe public alternative to openWindow(String).

 

[Mike Kienenberger]

In Behavior, we are generically creating a component for a behavior actor if the component does not already exist.

Do we need to remove this convenience method? Or is there a security fix for it?

I guess one possible alternative would be to add a ComponentFactory to the behavior node child.

14:56:17.350 [main] ERROR o.terasology.engine.TerasologyEngine - Uncaught exception
java.security.AccessControlException: Module class 'org.terasology.jobSystem.FindJobNode$FindJobTask' calling into 'org.terasology.logic.behavior.tree.Actor' requiring permission '("java.lang.reflect.ReflectPermission" "suppressAccessChecks")'
    at org.terasology.engine.module.ModuleSecurityManager.checkAPIPermissionsFor(ModuleSecurityManager.java:179) ~[bin/:na]
    at org.terasology.engine.module.ModuleSecurityManager.checkModAccess(ModuleSecurityManager.java:160) ~[bin/:na]
    at org.terasology.engine.module.ModuleSecurityManager.checkPermission(ModuleSecurityManager.java:188) ~[bin/:na]
    at java.lang.reflect.AccessibleObject.setAccessible(AccessibleObject.java:128) ~[na:1.7.0_45]
    at java.lang.Class$1.run(Class.java:353) ~[na:1.7.0_45]
    at java.lang.Class$1.run(Class.java:351) ~[na:1.7.0_45]
    at java.security.AccessController.doPrivileged(Native Method) ~[na:1.7.0_45]
    at java.lang.Class.newInstance(Class.java:350) ~[na:1.7.0_45]
    at org.terasology.logic.behavior.tree.Actor.component(Actor.java:65) ~[bin/:na]
    at org.terasology.jobSystem.FindJobNode$FindJobTask.onInitialize(FindJobNode.java:42) ~[na:na]
    at org.terasology.logic.behavior.tree.Task.tick(Task.java:56) ~[bin/:na]
    at org.terasology.logic.behavior.tree.Interpreter.step(Interpreter.java:135) ~[bin/:na]
    at org.terasology.logic.behavior.tree.Interpreter.tick(Interpreter.java:112) ~[bin/:na]
    at org.terasology.logic.behavior.BehaviorSystem.update(BehaviorSystem.java:105) ~[bin/:na]
    at org.terasology.engine.modes.StateIngame.update(StateIngame.java:148) ~[bin/:na]
    at org.terasology.engine.TerasologyEngine.mainLoop(TerasologyEngine.java:692) ~[bin/:na]
    at org.terasology.engine.TerasologyEngine.run(TerasologyEngine.java:298) ~[bin/:na]
    at org.terasology.engine.Terasology.main(Terasology.java:55) [bin/:na]

 

[Immortius]

I've changed the permission check mechanism again. This time I've aligned it more closely with Java's existing mechanisms, particularly the AccessController. This has a feature where code can be run through AccessController.doPrivileged(), which causes it to have the permissions of the code source in which it is defined rather than the intersection of the permissions of all the code in the call stack. Because a lot of the existing Java code already makes use of this feature, this should help avoid a lot of permission granting required due to low level stuff like classloading.

We also have the option of using doPrivileged() blocks ourselves, which is nice.

I've wiped all the granted permissions for now, so some previously resolved issues may crop up again.

 

[Cervator]

Got an example or two of how something looked in the old way and then the new way? I didn't trigger anything despite messing around with WoodAndStone, Miniions, and Portals, but would be good to know if we do run into something soon

 

[Mike Kienenberger]

How can I do the equivalent for this from a module?

                    WorldAtlas worldAtlas = CoreRegistry.get(WorldAtlas.class);
                    float tileSize = worldAtlas.getRelativeTileSize();

Right now I get

14:17:02.995 [main] ERROR o.t.engine.module.ModuleClassLoader - Denied access to class (not available to modules): org.terasology.world.block.loader.WorldAtlas
14:17:03.218 [main] ERROR o.terasology.engine.TerasologyEngine - Uncaught exception
java.lang.NoClassDefFoundError: org/terasology/world/block/loader/WorldAtlas
    at org.terasology.minimap.rendering.nui.layers.MinimapCell$1.get(MinimapCell.java:92) ~[na:na]
    at org.terasology.minimap.rendering.nui.layers.MinimapCell$1.get(MinimapCell.java:1) ~[na:na]
    at org.terasology.minimap.rendering.nui.layers.MapLocationIcon.getIcon(MapLocationIcon.java:49) ~[na:na]
    at org.terasology.minimap.rendering.nui.layers.MapLocationIcon.onDraw(MapLocationIcon.java:34) ~[na:na]

I'm trying to build a texture to draw for minimap block icons, so I figured I'd reuse what was working in AwtCanvasRenderer:

        textureAtlas = Assets.getTexture("engine:terrain");
[...]
                    BlockAppearance primaryAppearance = block.getPrimaryAppearance();
 
                    // TODO: get from map grid
                    BlockPart blockPart = BlockPart.TOP;
 
                    WorldAtlas worldAtlas = CoreRegistry.get(WorldAtlas.class);
                    float tileSize = worldAtlas.getRelativeTileSize();
 
                    Vector2f textureAtlasPos = primaryAppearance.getTextureAtlasPos(blockPart);
 
                    TextureRegion textureRegion = new BasicTextureRegion(textureAtlas, textureAtlasPos, new Vector2f(tileSize, tileSize));

 

[Mike Kienenberger]

This looks like oversight.

15:23:31.331 [main] ERROR o.t.engine.module.ModuleClassLoader - Denied access to class (not available to modules): org.terasology.rendering.nui.layers.hud.CoreHudWidget
15:23:31.338 [main] ERROR o.terasology.engine.TerasologyEngine - Uncaught exception
org.reflections.ReflectionsException: could not get type for name org.terasology.minimap.rendering.nui.layers.MinimapHUDElement

Same thing with HUDScreenLayer