New server function getServerEnvironmentValue #1937

pentaflops · 2020-12-21T19:52:24Z

A useful function for storing passwords and other settings if using docker images

str getServerEnvironmentValue( str envName )

AlexTMjugador · 2020-12-22T11:35:38Z

I like the idea of this PR. Environment variables are a nice way to pass parameters to processes in an easy to automate way, and without requiring command line parameters. I don't know why someone downvoted this, as you can pass a hash of sensitive data, like you would do in a database, and already use modules to retrieve this information.

Anyway, I'd use the new argument parser for this function. I'd also make sure that it is restricted in the ACL by default, so that no random resource can get environment variables unless the admin explicitly allows it, for privacy concerns. Finally, I'd rename the function to getServerEnvironmentVariable, but this is up to discussion.

Pirulax · 2020-12-24T17:24:33Z

I'd rename it to getServerEnvironmentVariable (in case we decide to add some kind of support to the client in the future)

Pirulax · 2020-12-24T17:31:52Z

Server/mods/deathmatch/logic/lua/CLuaFunctionDefs.h

@@ -111,6 +111,7 @@ class CLuaFunctionDefs
    LUA_DECLARE(SetServerPassword);
    LUA_DECLARE(GetServerConfigSetting);
    LUA_DECLARE(SetServerConfigSetting);
+    LUA_DECLARE(GetServerEnvironmentValue);


static std::string_view CLuaFunctionDefs::GetServerEnvironmentValue(std::string name) ;

Pirulax · 2020-12-24T17:34:53Z

Server/mods/deathmatch/logic/lua/CLuaManager.cpp

@@ -233,6 +233,7 @@ void CLuaManager::LoadCFunctions()
        {"getServerPassword", CLuaFunctionDefs::GetServerPassword},
        {"setServerPassword", CLuaFunctionDefs::SetServerPassword},
        {"getServerConfigSetting", CLuaFunctionDefs::GetServerConfigSetting},
+        {"getServerEnvironmentValue", CLuaFunctionDefs::GetServerEnvironmentValue},


{"getServerEnvironmentValue", ArgumentParser<GetServerEnvironmentValue>}

Also, for this to work, you need to move it to CLuaUtilDefs.
The new parser doesn't work in CLuaManager.

Pirulax · 2020-12-24T17:34:56Z

Server/mods/deathmatch/logic/lua/CLuaFunctionDefs.Server.cpp

+int CLuaFunctionDefs::GetServerEnvironmentValue(lua_State* luaVM)
+{
+    SString strName;
+
+    CScriptArgReader argStream(luaVM);
+    argStream.ReadString(strName);
+
+    if (!argStream.HasErrors())
+    {
+        lua_pushstring(luaVM, getenv(strName));
+        return 1;
+    }
+    else
+        m_pScriptDebugging->LogCustom(luaVM, argStream.GetFullErrorMessage());
+
+    lua_pushboolean(luaVM, false);
+    return 1;
+}
+


std::string_view CLuaFunctionDefs::GetServerEnvironmentValue(std::string name) { return getenv(name.c_str()); |

One thing im not sure about: What if getenv returns null? Does Lua handle it correctly?

Pirulax · 2020-12-24T17:37:38Z

Also, there is a native Lua function called os.getenv. We could enable it on the server, instead of adding a non-native function. It calls the same function we do, since Lua is pure ANSI C.

prnxdev · 2020-12-26T15:37:12Z

What is difference between this nad get and set functions? I once wrote a script that was connecting to MySQL database and I had different auth credentials for local and production so I used those functions to manage those data. Problem is that get function read data correctly but It adds * to every key (as I remember).

In other way there is also fileWrite and fileOpen functions. I don't know how exactly this function works but if it reads data from .env file I think it's not so important to have one in MTA API.

patrikjuvonen · 2020-12-29T00:47:37Z

What are the security concerns regarding getenv and the scope of variables one could have access to?

Disinterpreter · 2020-12-29T07:31:38Z

What are the security concerns regarding getenv and the scope of variables one could have access to?

It is only server side feature and I don't see the problems with accessing to the scope in this feature. If we think about unnecessary security of serverside we risky to lost all connections with OS as it was in: #1782

I can find thousand programs which work with ENV and it's okay.

patrikjuvonen · 2020-12-29T13:33:13Z

I believe we could try implementing the ACL system from the Lua modules branch and then allow getenv through the native Lua os function as suggested by @Pirulax

AlexTMjugador · 2020-12-30T12:15:49Z

What is difference between this nad get and set functions? I once wrote a script that was connecting to MySQL database and I had different auth credentials for local and production so I used those functions to manage those data. Problem is that get function read data correctly but It adds * to every key (as I remember).

In other way there is also fileWrite and fileOpen functions. I don't know how exactly this function works but if it reads data from .env file I think it's not so important to have one in MTA API.

Those functions solve a different problem: allowing the server administrator to set up configuration values for the resources. Environment variables are not meant to store resource configuration; instead, they represent more generic data about the OS environment that may be useful to processes as a whole.

Of course storing the environment variables in files via external programs and reading them back in the MTA: SA Lua VM is a workaround, but that implies writes to a physical block device, which contributes to reducing the lifespan of SSDs (unless they are done in a RAM filesystem, but using such filesystems in Windows is awkward). And, honestly, this function is simple enough to be merged quickly and successfully, so it's not like that workaround is even needed.

Also, there is a native Lua function called os.getenv. We could enable it on the server, instead of adding a non-native function. It calls the same function we do, since Lua is pure ANSI C.

Good catch! I was concerned about how we can ACL native Lua functions, though, but @patrikjuvonen proposed a solution for that, so it's perfect.

It is only server side feature and I don't see the problems with accessing to the scope in this feature. If we think about unnecessary security of serverside we risky to lost all connections with OS as it was in: #1782

I can find thousand programs which work with ENV and it's okay.

As @sbx320 said in the discussion you linked to, backdoored and compiled server side resources are a concern. Allowing these functions by default might expose the server to be fingerprinted in undesirable ways that may not be easily noticed by the server owner, and, if the server owner improperly uses environment variables to store secrets like deployment keys, it may even hand down a potential attacker with enough information to compromise the system. Denying this function by default is a way of making the server owner aware of this, and if some random compiled DirectX GUI resource insists in needing access to functions like os.getenv, then it's easy to conclude that something is wrong with it.

Disinterpreter · 2020-12-30T12:38:26Z

Denying this function by default is a way of making the server owner aware of this, and if some random compiled DirectX GUI resource insists in needing access to functions like os.getenv, then it's easy to conclude that something is wrong with it.

I have no complaints of some alerts or ACL lists. It is good, really. But I hate when superfluous security on the server restrict me. I think os.getenv must be in MTA server. If you want to mark this function as dangerous and implement it with alerts of security - let it be so, this feature is useful.

LosFaul · 2020-12-30T12:44:33Z

in general I think unsafe Lua functions should be enabled/disabeled via meta.xml for server side
(default is always disabled)
to have it per resource rather than for whole server

botder · 2020-12-30T12:55:48Z

Enable os.getenv but provide a custom C function to check if the executing resource has the ACL permission to do so and default to false if not given.

Gallardo994 · 2020-12-31T08:40:24Z

Those functions solve a different problem: allowing the server administrator to set up configuration values for the resources. Environment variables are not meant to store resource configuration; instead, they represent more generic data about the OS environment that may be useful to processes as a whole.

Well, that's unless you're running an immutable Docker image and want to use different configurations for each instance, without any need to pass volumes or store any local data (totally stateless MTA server instance). This is totally normal for containers to have ENVs as configurations.

But yeah ACL would be required for other usecases, e.g. if you use third-party resources which you may not have sources of, or if ENVs have some sensitive data required only for a selected resource (read: passwords)

patrikjuvonen · 2020-12-31T13:49:02Z

@pentaflops Are you willing to work on the os.getenv + ACL implementation or should this PR be closed and a new issue be created?

github-actions · 2022-01-07T12:38:15Z

This draft pull request is stale because it has been open for at least 90 days with no activity. Please continue on your draft pull request or it will be closed in 30 days automatically.

github-actions · 2022-02-07T01:11:42Z

This draft pull request was closed because it has been marked stale for 30 days with no activity.

pentaflops added 4 commits February 27, 2020 17:57

Merge remote-tracking branch 'multitheftauto/master'

8925ee0

Merge tag '1.5.8'

4baa578

Merge remote-tracking branch 'multitheftauto/master'

fbad18d

New function getServerEnvironmentValue

497c7c5

Pirulax requested changes Dec 24, 2020

View reviewed changes

patrikjuvonen added the enhancement New feature or request label Dec 29, 2020

Disinterpreter mentioned this pull request Jan 2, 2021

Unsafe functions #1972

Open

patrikjuvonen marked this pull request as draft January 2, 2021 15:11

patrikjuvonen added the feedback Further information is requested label Jan 2, 2021

Disinterpreter mentioned this pull request Jan 4, 2021

Unexpected result of trying to deny function in the table in ACL.xml #1983

Open

github-actions bot added the stale Inactive for over 90 days, to be closed label Jan 7, 2022

github-actions bot closed this Feb 7, 2022

Uh oh!

New server function getServerEnvironmentValue #1937

New server function getServerEnvironmentValue #1937

Uh oh!

Conversation

pentaflops commented Dec 21, 2020

Uh oh!

AlexTMjugador commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pirulax commented Dec 24, 2020

Uh oh!

Pirulax Dec 24, 2020

Choose a reason for hiding this comment

Uh oh!

Pirulax Dec 24, 2020

Choose a reason for hiding this comment

Uh oh!

Pirulax Dec 24, 2020

Choose a reason for hiding this comment

Uh oh!

Pirulax commented Dec 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prnxdev commented Dec 26, 2020

Uh oh!

patrikjuvonen commented Dec 29, 2020

Uh oh!

Disinterpreter commented Dec 29, 2020

Uh oh!

patrikjuvonen commented Dec 29, 2020

Uh oh!

AlexTMjugador commented Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Disinterpreter commented Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LosFaul commented Dec 30, 2020

Uh oh!

botder commented Dec 30, 2020

Uh oh!

Gallardo994 commented Dec 31, 2020

Uh oh!

patrikjuvonen commented Dec 31, 2020

Uh oh!

github-actions bot commented Jan 7, 2022

Uh oh!

github-actions bot commented Feb 7, 2022

Uh oh!

Uh oh!

AlexTMjugador commented Dec 22, 2020 •

edited

Loading

Pirulax commented Dec 24, 2020 •

edited

Loading

AlexTMjugador commented Dec 30, 2020 •

edited

Loading

Disinterpreter commented Dec 30, 2020 •

edited

Loading