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

Add matrix corporal #93777

Closed
wants to merge 15 commits into from
Closed

Add matrix corporal #93777

wants to merge 15 commits into from

Conversation

dali99
Copy link
Member

@dali99 dali99 commented Jul 24, 2020

Motivation for this change

matrix-corporal is a proxy and reconciliator for matrix-synapse letting you allow/deny API endpoints, and automatically join users to rooms/communities according to a configuration policy

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some comments I hope you find useful. Note that I don't know this software so if I've said anything entirely off base please feel free to correct or ignore me. If you have any questions please don't hesitate to ask.

@@ -1792,6 +1792,12 @@
githubId = 245394;
name = "Hannu Hartikainen";
};
dandellion = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have this in a separate commit.

nixos/modules/services/misc/matrix-corporal.nix Outdated Show resolved Hide resolved
default = pkgs.matrix-corporal;
};

settings = lib.mkOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every option needs a type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that would be lib.types.attrs ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends what you need out of the file. That might be a good fit, or type = with types; attrsOf (oneOf [ bool int str ]);. Or better yet take a look at #75584.

nixos/modules/services/misc/matrix-corporal.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/matrix-corporal.nix Outdated Show resolved Hide resolved
cfg = config.services.matrix-corporal;
in
{
options.services.matrix-corporal = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add some options (like authSharedSecretFile) for secrets so they aren't placed in settings and end up in the nix store.
If you do this you can have a preStart script to replace the secrets with jq at runtime and keep your secrets safe.

Let me know if you need any details on this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, hmm that is pretty tricky, especially the AuthorizationBearerToken stuff that can be part of the config file but aren't necessarily.

I'm actually not sure how to deal with that in a sensible way

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make the option type = with types; nullOr path; and set default = null;. From here you add to the systemd service:

  systemd.services.matrix-corporal = {
    ...
    preStart = optionalString (cfg.authorizationBearerTokenFile) ''
      use jq to insert AuthorizationBearerToken: 'cat ${cfg.authorizationBearerTokenFile}' into a new copy of the file under /var/lib/matrix-corporal
    '';
  };

Or some variation of that.

nixos/modules/services/misc/matrix-corporal.nix Outdated Show resolved Hide resolved
@dali99
Copy link
Member Author

dali99 commented Jul 27, 2020

This wasn't really ready for review, but I still appreciate it very much! I have fixed some of the things brought up!

I will fix the commits properly once this is closer to being mergeable. For now I don't want to lose the review comments by a force push 😅

@aanderse
Copy link
Member

aanderse commented Aug 3, 2020

#75584 has been merged and makes creating settings options easier. Right from the PR description you can see how adding a generic settings option becomes pretty straightforward:

{ lib, config, pkgs, ... }:
let
  format = pkgs.formats.json {};
in {
  options.settings = lib.mkOption {
    type = format.type;
    default = {};
  };

  config = {
    settings.foo = 10;
    settings.bar.baz = "Hello";
    environment.etc.test.source = format.generate "config.json" config.settings;
  };
}

I would recommend using this approach here. If interested and you have any questions, please let me know.

@dali99
Copy link
Member Author

dali99 commented Aug 3, 2020

Thanks! I've already mostly used the settings options from the RFC but I've now changed the file generator and types to match the new format goodness!

@infinisil
Copy link
Member

Doesn't seem ready to review just yet (current version wasn't tested), I'll wait until it's not a draft anymore for a review

@dali99
Copy link
Member Author

dali99 commented Aug 20, 2020

@infinisil Yeah, sorry I'll refrain from opening a PR before it's ready for review next time. I thought the Draft status would be enough to not bother anyone :F

@aanderse I think I need some help in figuring out the secrets stuff. I'm not entirely sure how I can use DynamicUser while also reading files with correct permissions and putting them in the right places.

With the current state of the module, /var/lib/matrix-corporal isn't created, and I'm not sure why

@aanderse
Copy link
Member

@dali99 I pinged @infinisil so not your fault. Feel free to open draft PRs whenever you want, it won't bother anyone.

You specified StateDirectory so the directory should be created once the service starts. As far as secrets management with DynamicUser goes there is a new feature being added to systemd to fix the existing shortcomings.

@dali99 dali99 closed this Sep 29, 2020
@dali99 dali99 mentioned this pull request Sep 29, 2020
10 tasks
@infinisil
Copy link
Member

No worries about pinging even when not ready for review yet btw, I'd love to review once it's ready and am glad @aanderse pinged me :)

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

Successfully merging this pull request may close these issues.

None yet

4 participants