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.

Sign up for GitHub

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

Jump to bottom

Iodine: ipv6 support, updates, hardening, nixos test.... #79120

Merged
merged 9 commits into from Mar 16, 2020
Merged

Iodine: ipv6 support, updates, hardening, nixos test.... #79120

merged 9 commits into from Mar 16, 2020

Conversation

symphorien
Copy link
Member

Motivation for this change
  • update iodine for ipv6 support
  • update nm-iodine because it is buggy (100% cpu usage after the vpn disconnects)
  • add some hardening flags to the nixos service
  • add a nixos test to check that the hardening didn't break anything

I ran nixpkgs-fmt on the nixos module, so best reviewed commit by commit :)

Things done

Tested on a real setup.

  • 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.

@worldofpeace
Copy link
Contributor

@symphorien The commits for 73b8fdb and b2b6645 should be like

$package: 1.2.0 -> YYYY-MM-DD

for the unstable version.

@worldofpeace
Copy link
Contributor

@GrahamcOfBorg test iodine

@symphorien
iodine: 1.2.0 -> 2019-09-27
2a04e69 
Adds ipv6 support
@symphorien
network-manager-iodine: 1.2.0 -> 2019-11-05
683fa30 
the released version is old and cannot reconnect after the first
connection is closed. This is fixed in master.
@symphorien
nixos/iodine: nixpkgs-fmt
7437bff
@symphorien
nixos/iodine: hardening
00a91d9
@symphorien
nixos/iodine: add test
b7f27cb
@symphorien
Copy link
Member Author

Reworded the commits as required.
... And I forgot to have a look at ofborg's output before so we lost the results. Sorry.

@flokli flokli requested a review from mweinelt February 4, 2020 23:51
mweinelt
mweinelt reviewed Feb 5, 2020
View reviewed changes
nixos/modules/services/networking/iodine.nix
example = "8.8.8.8";
};

extraConfig = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

Sounds more like extraArgs to me.

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'd like not to do this renaming:

  1. it's a breaking change for an arguably low improvement
  2. iodine only means of configuration are arguments file so it's not ambiguous
  3. This appears in the diff because I reformatted the file with nixpkgs-fmt, but this is outside the scope of this PR. I welcome "en passant" improvements, but let's restrain ourselves to trivial changes.

mweinelt
mweinelt reviewed Feb 5, 2020
View reviewed changes
nixos/modules/services/networking/iodine.nix
};

passwordFile = mkOption {
type = types.str;
Copy link
Member

Choose a reason for hiding this comment

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

Can probably be a path instead of a str, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

see details at #78640

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I understand, it should be a non-path string, to the contrary. Please correct me if I'm wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

See how it's done here

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 put uses of these options under toString. Was that what you meant? (your link is 404)

symphorien and others added 4 commits February 5, 2020 19:42
@symphorien @mweinelt
nixos/iodine: improve description of some options
1addf1f 
Co-Authored-By: Martin Weinelt <mweinelt@users.noreply.github.com>
@symphorien @mweinelt
nixos/iodine: fix typo in description
dfa6763 
Co-Authored-By: Martin Weinelt <mweinelt@users.noreply.github.com>
@symphorien @mweinelt
nixos/iodine: improve wording of some descriptions
d2d5d89 
Co-Authored-By: Martin Weinelt <mweinelt@users.noreply.github.com>
@symphorien
nixos/iodine: protect passwordFiles with toString
44fd320 
It should prevent copying the files to a store path
@symphorien
Copy link
Member Author

Can I request a new round of review now that most comments have been taken into account I think?

@flokli flokli requested a review from mweinelt March 2, 2020 22:58
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/127

@Ekleog
Copy link
Member

Ekleog commented Mar 16, 2020

@ofborg test iodine

@Ekleog Ekleog merged commit a0307ba into NixOS:master Mar 16, 2020
@symphorien symphorien deleted the iodine branch March 21, 2020 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flokli flokli flokli left review comments

@obadz obadz Awaiting requested review from obadz

@Phreedom Phreedom Awaiting requested review from Phreedom

@domenkozar domenkozar Awaiting requested review from domenkozar

@worldofpeace worldofpeace Awaiting requested review from worldofpeace

@mweinelt mweinelt Awaiting requested review from mweinelt

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@symphorien @worldofpeace @nixos-discourse @Ekleog @mweinelt @flokli