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

pomerium: init at 0.13.3 #108745

Merged
merged 5 commits into from Mar 31, 2021
Merged

pomerium: init at 0.13.3 #108745

merged 5 commits into from Mar 31, 2021

Conversation

lukegb
Copy link
Contributor

@lukegb lukegb commented Jan 8, 2021

Motivation for this change

Packages the Pomerium authenticating HTTP proxy.

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.

Hi. I have left reviewed the module portion of this PR and left some comments I hope you will find useful. Let me know if you have any questions, etc...

nixos/modules/services/web-servers/pomerium.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-servers/pomerium.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-servers/pomerium.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-servers/pomerium.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-servers/pomerium.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-servers/pomerium.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-servers/pomerium.nix Outdated Show resolved Hide resolved
@lukegb lukegb force-pushed the pomerium branch 5 times, most recently from e3c0b5e to add12a3 Compare January 9, 2021 00:46
@lukegb
Copy link
Contributor Author

lukegb commented Jan 9, 2021

@ofborg eval
@ofborg test pomerium
@ofborg build pomerium

@aanderse
Copy link
Member

@lukegb looks like a failed test.

@lukegb
Copy link
Contributor Author

lukegb commented Jan 16, 2021

@aanderse I'm parking this until #108741 is merged, which should make this PR a little simpler

@lukegb
Copy link
Contributor Author

lukegb commented Jan 19, 2021

@aanderse Thanks! Should be ready for review again.

@aanderse
Copy link
Member

@lukegb please see above comment: #108745 (comment)

@lukegb
Copy link
Contributor Author

lukegb commented Jan 24, 2021

@lukegb please see above comment: #108745 (comment)

done! sorry for the delay

@aanderse
Copy link
Member

@lukegb this module is awesome! Great work on it. I'm not familiar with the software at all, though... Can we find anyone else to chip in with review? Maybe @marvin-mk2 can help us out? How about you @m1cr0man - are you familiar with this at all?

/marvin opt-in
/status needs_reviewer

Copy link
Contributor

@m1cr0man m1cr0man left a comment

Choose a reason for hiding this comment

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

This is some great work. I'm not familiar with pomerium, but the test suite looks solid and the systemd service has an impressive amount of hardening 😃

My only gripe is with the use of LoadCredential. If pomerium does support hot reloads via process signals then I would prefer to see CERTIFICATE[_KEY]_FILE env vars set to the full path to the cert. This creates a new problem in that you would also need to change how DynamicUser is used. From what I understand of the DynamicUser docs, setting Group to "acme" should be sufficient. As it is now your solution is more secure and in reality Apache has worked similarly in the past and no one complained 😉

Also see my note about the secretsFile permissions. It might make sense to add it to the LoadCredential option too, so that it can be owned and readable only by root.

};
};

# postRun hooks on cert renew can't be used to restart Nginx since renewal
Copy link
Contributor

Choose a reason for hiding this comment

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

Says "Nginx" here ;)

type = format.type;
};

secretsFile = mkOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

With the use of DynamicUser in the systemd config, what would be the proper way to secure a secrets file on the local filesystem from other users?

@contrun
Copy link
Contributor

contrun commented Jan 30, 2021

Was just about to try out pomerium. This looks great to me. Thank you.

@contrun
Copy link
Contributor

contrun commented Mar 2, 2021

I think this pr is ready to be merged. @lukegb can you please update pomerium to v0.13.2 ?

Copy link
Contributor

@contrun contrun left a comment

Choose a reason for hiding this comment

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

@lukegb I just tried pomerium 0.13.3. The proposed changes build on my machine. I also tried the test case. It works great. Thank you for your contribution.

pkgs/servers/http/pomerium/default.nix Outdated Show resolved Hide resolved
pkgs/servers/http/pomerium/default.nix Outdated Show resolved Hide resolved
pkgs/servers/http/pomerium/default.nix Outdated Show resolved Hide resolved
@lukegb lukegb changed the title pomerium: init at 0.11.1 pomerium: init at 0.13.3 Mar 29, 2021
Copy link
Contributor

@contrun contrun left a comment

Choose a reason for hiding this comment

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

Thanks

@adisbladis adisbladis merged commit f5a14a3 into NixOS:master Mar 31, 2021
};
varFlags = concatStringsSep " " (mapAttrsToList (name: value: "-X github.com/pomerium/pomerium/internal/version.${name}=${value}") setVars);
in [
"-ldflags=${varFlags}"
Copy link
Member

Choose a reason for hiding this comment

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

Missing -s -w.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't seem to be set in pkgs/development/go-modules/generic/default.nix as a default, so it's not like we're overriding it off, I think?

Copy link
Member

Choose a reason for hiding this comment

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

It probably does not work for every package. Should be set anyway when specifying ldflags.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe does not work with all packages. Should be set anyway when specifying ldflags.

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

6 participants