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

Network hardening using sysctl #856

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Network hardening using sysctl #856

wants to merge 1 commit into from
+200 −0

Conversation

gngram
Copy link
Contributor

@gngram gngram commented Oct 18, 2024

Description of changes

sysctl configuration for network hardening.
Uses configuration recommended in different security audits.
Configuration and it's recommendation is available in this document:

Recommended sysctl settings

Sysctl network setting audit report available:
https://github.com/gangaram-tii/ghaf-debug-tools/blob/main/report/hardened_network_audit_report.md

Network Performance impact:
https://github.com/gangaram-tii/ghaf-debug-tools/blob/main/report/perf-iperf3.png
Note: ghaf+ label is with sysctl hardened network settings.

Checklist for things done

  • [-] Summary of the proposed changes in the PR description
  • [-] More detailed description in the commit message(s)
  • [-] Commits are squashed into relevant entities - avoid a lot of minimal dev time commits in the PR
  • [-] Contribution guidelines followed
  • Ghaf documentation updated with the commit - https://tiiuae.github.io/ghaf/
  • PR linked to architecture documentation and requirement(s) (ticket id)
  • [-] Test procedure described (or includes tests). Select one or more:
    • Tested on Lenovo X1 x86_64
    • Tested on Jetson Orin NX or AGX aarch64
    • Tested on Polarfire riscv64
  • Author has run make-checks and it passes
  • All automatic Github Action checks pass - see actions
  • Author has added reviewers and removed PR draft status
  • Change requires full re-installation
  • Change can be updated with nixos-rebuild ... switch

Instructions for Testing

  • [-] List all targets that this applies to: all platform targets
  • Is this a new feature: yes
    • List the test steps to verify:
  • [-] If it is an improvement how does it impact existing functionality?
    • Test network related functionality e.g. (internet access and inter vm communication).

@gngram gngram temporarily deployed to internal-build-workflow October 18, 2024 12:02 — with GitHub Actions Inactive
Mic92
Mic92 reviewed Oct 19, 2024
View reviewed changes
modules/common/security/networking.nix
https://linux-audit.com/linux-security-guide-for-hardening-ipv6/
'';
type = lib.types.bool;
default = true;
Copy link
Collaborator

@Mic92 Mic92 Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should this be default to true? We have now almost 50% ipv6 deployment world-wide: https://www.google.com/intl/en/ipv6/statistics.html#tab=ipv6-adoption
So one cannot say that ipv6 is not used. Especially in VPN it has the advantage that hosts never have to be renumbered again as they are too many collisions between private ipv4 addresses spaces i.e. when merging company networks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I set default to false. In Ghaf by default IPv6 is disabled.
https://github.com/tiiuae/ghaf/blob/main/modules/microvm/virtualization/microvm/common/vm-networking.nix#L24C23-L24C24

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we enable IPv6 we should include hardening settings as well. I agree we should support IPv6, but enabling it indiscriminately in all VMs and interfaces may not be the best strategy with our current setup, and afaik enabling it for specific interfaces is still possible with these settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have included some IPv6 configs in the updated PR which falls in some of current categories. There are more recommendations for IPv6 hardening, I will include them later after discussion.

Mic92
Mic92 reviewed Oct 19, 2024
View reviewed changes
modules/common/security/networking.nix Outdated
default = true;
};

enable-rp-filter = lib.mkOption {
Copy link
Collaborator

@Mic92 Mic92 Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Doesn't linux has enabled this by default anyway? If someone ever disable this, they are most likely doing so intentionally i.e. they want to do BGP routing. Having this option just makes things more complex.

Copy link
Contributor Author

@gngram gngram Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is not enabled by default in Linux. Since it is recommended to enable rp_filter in many Linux security audits, so I thought to include it. Now it defaults to false.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems this flag is not used? Also, NixOS seems to set it to 2 (loose mode), whereas this option sets it to 1 (strict mode). Not entirely sure about the implications within Ghaf, especially internal network, can you elaborate if strict mode makes sense as a default or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I studied around this. As far as what I understood, loose mode(default) is fine for VMs which are communicating through internal network. Strict mode should be beneficial in net-vm specially if some one tries to spoof an VM address. In strict mode these packets will be dropped while in loose mode these may be accepted as there is always a valid route for these.

Mic92
Mic92 reviewed Oct 19, 2024
View reviewed changes
modules/common/security/networking.nix Outdated
default = true;
};

disable-ping-request = lib.mkOption {
Copy link
Collaborator

@Mic92 Mic92 Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This breaks ipv6. It's also a problem for debugging cases where the MTU is too high.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also defaults to false now.

@gngram gngram force-pushed the pr__sysctl__network branch from 7f10c3c to aba64ea Compare October 21, 2024 13:21
@gngram gngram temporarily deployed to internal-build-workflow October 21, 2024 13:21 — with GitHub Actions Inactive
@gngram gngram force-pushed the pr__sysctl__network branch from aba64ea to 5ee28d4 Compare October 22, 2024 10:33
@gngram gngram temporarily deployed to internal-build-workflow October 22, 2024 10:33 — with GitHub Actions Inactive
@gngram gngram force-pushed the pr__sysctl__network branch from 5ee28d4 to a70f0fe Compare October 23, 2024 13:38
@gngram gngram temporarily deployed to internal-build-workflow October 23, 2024 13:38 — with GitHub Actions Inactive
@gngram gngram force-pushed the pr__sysctl__network branch from a70f0fe to 13dac01 Compare November 27, 2024 14:09
@gngram gngram temporarily deployed to internal-build-workflow November 27, 2024 14:09 — with GitHub Actions Inactive
mbssrc
mbssrc reviewed Dec 5, 2024
View reviewed changes
modules/common/security/networking.nix Outdated
{
## Options to enable IP security
options.ghaf.security.sysctl.network = {
disable-all = lib.mkOption {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not the typical enable flag that defaults to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done.

@gngram gngram force-pushed the pr__sysctl__network branch from 13dac01 to 8cd2099 Compare December 5, 2024 11:58
@gngram gngram temporarily deployed to internal-build-workflow December 5, 2024 11:58 — with GitHub Actions Inactive
vunnyso
vunnyso suggested changes Dec 9, 2024
View reviewed changes
modules/common/security/networking.nix
...
}:
let
cfg = config.ghaf.security.sysctl.network;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can add

  inherit (lib) mkOption mkForce mkDefault mkIf types;

For better readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@gngram gngram force-pushed the pr__sysctl__network branch from 8cd2099 to a09ae92 Compare December 9, 2024 12:56
@gngram gngram temporarily deployed to internal-build-workflow December 9, 2024 12:56 — with GitHub Actions Inactive
vunnyso
vunnyso approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@vunnyso vunnyso left a comment

Choose a reason for hiding this comment

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

Approved with prejudice.

@gngram gngram force-pushed the pr__sysctl__network branch from a09ae92 to 7f1ff93 Compare December 17, 2024 11:43
@gngram gngram temporarily deployed to internal-build-workflow December 17, 2024 11:43 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbssrc mbssrc mbssrc left review comments

@Mic92 Mic92 Mic92 left review comments

@vunnyso vunnyso vunnyso approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gngram @Mic92 @mbssrc @vunnyso