Skip to content

pomerium: init at 0.13.3 #108745

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

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.

Sorry, something went wrong.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 8, 2021
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...

@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

@lukegb lukegb requested a review from aanderse January 9, 2021 00:48
@lukegb lukegb marked this pull request as ready for review January 9, 2021 00:51
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jan 9, 2021
@ofborg ofborg bot requested a review from kalbasit January 9, 2021 01:13
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels Jan 9, 2021
@lukegb lukegb marked this pull request as draft January 13, 2021 00:14
@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 lukegb marked this pull request as ready for review January 19, 2021 16:35
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 and removed 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels Jan 19, 2021
@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.

@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
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants