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

feat: allow tmate ip addresses to firewall. #209

Merged
merged 18 commits into from
Jan 29, 2024
Merged

Conversation

yanksyoon
Copy link
Collaborator

Applicable spec: N/A

Overview

To allow tmate-ssh-server IP addresses from relationdata to be excluded from denylist by filtering them if from denylist if it belongs to one of the networks defined by the denylist.

Rationale

Since many internal subnets can be blocked, including the one that the charm is related to (tmate-ssh-server), it is necessary to allow traffic between related units even though the subnet they belong in might be covered by the denylist.

Juju Events Changes

On ssh-debug relation data change, the firewall is now updated.

Module Changes

firewall.py now splits denylist if one of the ip ranges in allowlist is found within the range of denylist. (Allowlist has priority.)
charm.py now refreshes firewall on ssh-debug relation data changed.

Library Changes

None.

Checklist

@yanksyoon yanksyoon added enhancement New feature or request complex labels Jan 26, 2024
@yanksyoon yanksyoon requested a review from a team as a code owner January 26, 2024 04:14
cbartz
cbartz previously approved these changes Jan 26, 2024
Copy link
Collaborator

@cbartz cbartz left a comment

Choose a reason for hiding this comment

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

LGTM, left some nitpicky comments.

src/charm.py Outdated Show resolved Hide resolved
src/firewall.py Outdated Show resolved Hide resolved
src/firewall.py Outdated Show resolved Hide resolved
Copy link

@gtrkiller gtrkiller left a comment

Choose a reason for hiding this comment

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

Out of interest, is there a reason why this PR only addresses IPv4 and not IPv6? otherwise it LGTM

@yanksyoon
Copy link
Collaborator Author

@gtrkiller Good point, added a generic type - but for concrete types - like host address, it's because we're using the command on L67: "/snap/bin/lxc", "network", "get", self._network, "ipv4.address" to get ipv4 addresses. Then everything has to be IPv4Network type to use the overlaps and address_exclude functions, since they can only be compared to network of same types. Thanks for the review! :D

gtrkiller
gtrkiller previously approved these changes Jan 26, 2024
cbartz
cbartz previously approved these changes Jan 26, 2024
Copy link
Collaborator

@cbartz cbartz left a comment

Choose a reason for hiding this comment

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

LGTM, one nitpicky comment

src/firewall.py Outdated Show resolved Hide resolved
@yanksyoon yanksyoon dismissed stale reviews from cbartz and gtrkiller via ea26b9b January 26, 2024 14:38
cbartz
cbartz previously approved these changes Jan 26, 2024
yhaliaw
yhaliaw previously approved these changes Jan 29, 2024
Copy link
Collaborator

@yhaliaw yhaliaw left a comment

Choose a reason for hiding this comment

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

Left a minor comment

tests/unit/test_firewall.py Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/firewall.py Outdated Show resolved Hide resolved
src/firewall.py Outdated Show resolved Hide resolved
src/firewall.py Outdated Show resolved Hide resolved
tests/unit/test_firewall.py Outdated Show resolved Hide resolved
tests/unit/test_firewall.py Outdated Show resolved Hide resolved
tests/unit/test_firewall.py Show resolved Hide resolved
@yanksyoon yanksyoon dismissed stale reviews from yhaliaw and cbartz via 391eae0 January 29, 2024 06:48
Copy link
Contributor

Test coverage for b386768

Name                         Stmts   Miss Branch BrPart  Cover   Missing
------------------------------------------------------------------------
src/charm.py                   455    110    117     25    74%   86-88, 112-113, 117-119, 157-159, 169->181, 179, 214-233, 247-249, 250->254, 278-282, 313, 393-398, 421, 438-439, 448-471, 491-496, 502-505, 512, 515-516, 532, 536-541, 586->589, 602-603, 605-606, 640-647, 671-672, 684-686, 701-702, 730-731, 733-734, 736-737, 811->813, 874-875, 891, 912-914, 918-921, 925
src/charm_state.py             138     18     22      3    86%   131-138, 221-224, 266-268, 279-280, 286-291, 302-304
src/errors.py                   39      0      0      0   100%
src/event_timer.py              45      9      6      1    73%   80, 100-103, 123-126
src/firewall.py                 52     18     20      0    61%   42-43, 66-69, 110-184
src/github_client.py            68      6     32      3    87%   124, 146, 158-165, 183->216
src/github_metrics.py           14      0      0      0   100%
src/github_type.py              50      0      0      0   100%
src/lxd_type.py                 37      0      2      0   100%
src/metrics.py                  73      2     10      1    96%   61->64, 170-171
src/metrics_type.py              6      0      0      0   100%
src/runner.py                  325     70     96     24    73%   45->47, 129, 256-257, 298-307, 331-336, 341, 361, 365-375, 424->427, 430-432, 439, 453, 463, 467, 469, 484, 518-523, 539-552, 563-602, 607, 645, 698-700, 704, 722, 757, 783, 788-800, 814, 819->821, 824-826
src/runner_logs.py              35      2      6      1    93%   62->61, 66-67
src/runner_manager.py          274     47     90      8    82%   122, 167-169, 182-183, 195-197, 203-208, 212-213, 223-224, 243, 286-287, 331-333, 398, 418-422, 447, 499-502, 538, 645-658, 668, 673-684
src/runner_manager_type.py      33      0      6      0   100%
src/runner_metrics.py          123      8     20      2    93%   147-148, 160, 191-192, 307-311
src/runner_type.py              47      0     10      0   100%
src/shared_fs.py               116     16     20      0    88%   108-109, 133-134, 217-218, 241-242, 254-255, 260-261, 267-268, 292-293
src/utilities.py                68      5     20      7    86%   74->76, 78->84, 91, 121, 135, 173, 226
------------------------------------------------------------------------
TOTAL                         1998    311    477     75    82%

Static code analysis report

Run started:2024-01-29 08:40:55.950802

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 4372
  Total lines skipped (#nosec): 0
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 12

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@yanksyoon yanksyoon merged commit d8066b9 into main Jan 29, 2024
57 checks passed
@yanksyoon yanksyoon deleted the feat/allow_tmate_firewall branch January 29, 2024 10:35
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.

5 participants