Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve The Security Model #268

Closed

Conversation

newtoallofthis123
Copy link
Contributor

@newtoallofthis123 newtoallofthis123 commented Aug 24, 2024

Below PR aims to simplify and improve swhkd's security model.
Please refer to the changelog for full details

Changes made

It would be difficult to layout all of the changes made in the codebase in a single changelog, however,
the following are the major changes that can be easily noticable in the codebase.
For the full changes, please refer to the Github repository.

  1. Seperate Env Module: refer:
    All of the env parsing and feed has been categorized into a seperate module called environ.rs.
    This module is responsible for parsing the environment variables and providing them to the daemon.
    The env from the server is just a newline separated string of key=value pairs.
    These are parsed, deduped and then fed to the daemon.

  2. Retire SWHKS command execution: refer:
    The SWHKS command execution has been retired and instead, the daemon now uses su to execute the commands in the user space.
    This means that the daemon is now less reliant on the server and hence the security concerns are mitigated.

  3. Daemonize the server and env sending: refer:
    The server has been daemonized and all of it's output has been redirected to /dev/null.
    Moreover, the actual function to send the env to the daemon has been added.
    Any connection to the server now results in the server sending the env.

  4. Event based env refresh: refer:
    The server now has an event based cron job that sends the env to the daemon every 650ms by default.
    However, the user can specify a custom timeout using the -r flag.

  5. Config file better defaults: refer:
    The config file location now correctly defaults to ~/.config/swhkd/swhkdrc thanks to the environ refresh from the server.
    The problem was that the daemon was running in root space with the root user's env, so it was not able to find the config file
    that was stored in the user space.
    So, just for the config file location, the daemon now requests the server for the env and then uses su to read the config file.

  6. Channel Based Communication: refer:
    The daemon now uses a channel based communication to communicate with the thread.
    The thread is spawned at the beginning and is valid throughout the lifetime of the daemon.
    A mpsc channel of a good default of 100 is used to communicate between the daemon and the thread.
    This means that the daemon can now execute the commands in the user space without spawning a new thread everytime.

  7. Server Instance Tracking: refer:
    The server now detects if it is already running and if it is, it doesn't start a new instance.
    This is usefull because it has been daemonized and hence can be running in the background when a new connection is made.

Final Flow

The final flow of the daemon is as follows:
The daemon is launched in the root space and the server is launched in the user space.
This is reminiscent of the old IPC model as such:

./swhks && doas ./swhkd

The doas or sudo can be skipped by making the swhkd binary a setuid binary.
This can be done by running the following command:

sudo chown root:root swhkd
sudo chmod u+s swhkd

Right after this is done, the first connection to the server is made and the server sends the env to the daemon.
This information is stored in the env struct instance and this is exchanged and valid throughout the process life cycle.
The XDG_CONFIG_HOME is also set to ~/.config/swhkd and the config file is read from there if it exists.
A thread is spawned that is valid throughout the lifetime of the daemon.
The thread is also de-escalated to the user space.
The thread can communicate with the daemon through a channel.

Next, the daemon starts listening for the key events.
When a key event is detected, the daemon just sends it to the thread through the channel.
Concurrently, there is a cron job that sends the env to the daemon every 650ms by default.
This ensures that the env is always fresh and the daemon can always execute the commands in the user space.

swhkd/src/daemon.rs Fixed Show fixed Hide fixed
@newtoallofthis123 newtoallofthis123 marked this pull request as ready for review August 24, 2024 14:12
@Shinyzenith
Copy link
Member

Hi! Ci is broken. Can you resolve the issue?

@newtoallofthis123
Copy link
Contributor Author

Yep I messed up a small line :)

@newtoallofthis123
Copy link
Contributor Author

@Shinyzenith It should be all fixed now :)

@Shinyzenith
Copy link
Member

I still do not understand why we are adding 120ms, we were gonna subtract 10% as delta right?

@newtoallofthis123
Copy link
Contributor Author

I'm sorry I had not updated what I was working on
This is the latest commit

@newtoallofthis123
Copy link
Contributor Author

@Shinyzenith this should work :)

@Shinyzenith
Copy link
Member

Shinyzenith commented Aug 28, 2024

Hi, have you documented all your changes? Some parts of the code still feel awkward to me, we'll speak personally on that later

@Shinyzenith
Copy link
Member

Shinyzenith commented Aug 28, 2024

Eg:

    let _ = daemon(true, false);

Why? This line doesn't make any sense, the function name doesn't tell me what it is intuitively by just looking at it once. What are the arguments you're passing? It doesn't make much sense.

Edit: ah this is unistd::daemon, I was searching for fn daemon and that led to the confusion. Probably should just call it unistd::daemon as it adds more context.

@newtoallofthis123
Copy link
Contributor Author

I can leave more comments if that is helpful.
Especially near the areas where the tokio spawns and env exchanges happen.
You are right it should have more context.

@newtoallofthis123
Copy link
Contributor Author

As for documentation, the description for the pr is up to date, however I will also leave some helpful comments in the places i made changes to

@Shinyzenith
Copy link
Member

@newtoallofthis123 In terms of documentation, I mean updating readme and man pages with relevant information. The comments are an important part too. Thanks for taking note.

@newtoallofthis123
Copy link
Contributor Author

That makes sense 😅
I'll update them accordingly

@newtoallofthis123
Copy link
Contributor Author

@Shinyzenith Updated the docs :)

@Shinyzenith
Copy link
Member

Why did you remove logging from swhks?

@newtoallofthis123
Copy link
Contributor Author

Ever since the command execution was taken out from it, the log file is no longer used in swhks.
I can probably add it back and use it for logging when the environment is sent?

swhkd/src/daemon.rs Show resolved Hide resolved
swhks/src/main.rs Outdated Show resolved Hide resolved
@Shinyzenith
Copy link
Member

Also your commit messages are horrendous sadly. We need to rebase / work on getting those improved.

@newtoallofthis123
Copy link
Contributor Author

Yeah I am very bad at commit messages 😅
I'll try rebasing and stacking them so that they make sense in a chronological way.

swhkd/src/daemon.rs Dismissed Show dismissed Hide dismissed
@newtoallofthis123
Copy link
Contributor Author

Closed and ported to #270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants