New issue
Advanced search Search tips

Issue 787578 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Feature



Sign in to add a comment

Move firewalld into permission_broker

Project Member Reported by jorgelo@chromium.org, Nov 21 2017

Issue description

The only reason we had firewalld in the first place was because permission_broker couldn't launch 'iptables' with the necessary capabilities. We can use ambient caps for that now.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/787aa7d01defae92185b441e473144caf1e354d4

commit 787aa7d01defae92185b441e473144caf1e354d4
Author: Jorge Lucangeli Obes <jorgelo@chromium.org>
Date: Wed Dec 13 20:33:37 2017

permission_broker: Add firewall functionality.

With ambient capabilities we can launch 'iptables' from permission_broker,
so we don't really need firewalld anymore. Having two processes where one
suffices is silly.

This code is:

1-A subset of the IpTables code in firewalld, and
2-Not yet used in permission_broker.

There was a lot of duplication between permission_broker::PortTracker and
firewalld:IpTables so this version of the code removes all of this duplication.
The new Firewall class knows how to call the 'iptables' and 'ip' binaries, and
that's about it.

I have a draft change that modifies PortTracker to use the Firewall class. The
current Firewall class interface should be enough to implement all of firewalld
in permission_broker.

BUG= chromium:787578 
TEST=New unit tests pass.

Change-Id: I16b82befb9a1eba703142689f52c5b6490928ea9
Reviewed-on: https://chromium-review.googlesource.com/783264
Commit-Ready: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>

[add] https://crrev.com/787aa7d01defae92185b441e473144caf1e354d4/permission_broker/firewall_unittest.cc
[add] https://crrev.com/787aa7d01defae92185b441e473144caf1e354d4/permission_broker/firewall.h
[modify] https://crrev.com/787aa7d01defae92185b441e473144caf1e354d4/permission_broker/permission_broker.gyp
[add] https://crrev.com/787aa7d01defae92185b441e473144caf1e354d4/permission_broker/mock_firewall.cc
[add] https://crrev.com/787aa7d01defae92185b441e473144caf1e354d4/permission_broker/mock_firewall.h
[add] https://crrev.com/787aa7d01defae92185b441e473144caf1e354d4/permission_broker/firewall.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/a7b6938f31ea5254747d1452612158f9599e20bc

commit a7b6938f31ea5254747d1452612158f9599e20bc
Author: Jorge Lucangeli Obes <jorgelo@chromium.org>
Date: Fri Dec 15 02:11:49 2017

permission_broker: Use in-process firewall implementation.

Change firewall rules locally instead of calling firewalld. Once this
lands we can stop building firewalld into our images, remove a process,
and a lot of API duplication between permission_broker and firewalld.

BUG= chromium:787578 
TEST=New unit tests pass.
TEST=platform_Firewall passes.

Change-Id: I9c80aefab517cb80026d4bc43b740e6ba1cc60b3
Reviewed-on: https://chromium-review.googlesource.com/825783
Commit-Ready: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>

[modify] https://crrev.com/a7b6938f31ea5254747d1452612158f9599e20bc/permission_broker/port_tracker.h
[modify] https://crrev.com/a7b6938f31ea5254747d1452612158f9599e20bc/permission_broker/port_tracker.cc
[modify] https://crrev.com/a7b6938f31ea5254747d1452612158f9599e20bc/permission_broker/permission_broker.conf
[modify] https://crrev.com/a7b6938f31ea5254747d1452612158f9599e20bc/permission_broker/permission_broker.gyp
[modify] https://crrev.com/a7b6938f31ea5254747d1452612158f9599e20bc/permission_broker/permission_broker_main.cc
[modify] https://crrev.com/a7b6938f31ea5254747d1452612158f9599e20bc/permission_broker/permission_broker.cc
[modify] https://crrev.com/a7b6938f31ea5254747d1452612158f9599e20bc/permission_broker/permission_broker.h
[modify] https://crrev.com/a7b6938f31ea5254747d1452612158f9599e20bc/permission_broker/port_tracker_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/8cb378263a7c2a34457178d7c89614ffbf62076e

commit 8cb378263a7c2a34457178d7c89614ffbf62076e
Author: Jorge Lucangeli Obes <jorgelo@chromium.org>
Date: Fri Dec 15 23:52:46 2017

permission_broker: Don't depend on firewalld anymore.

BUG= chromium:787578 
TEST=emerge-clapper permission_broker
CQ-DEPEND=CL:825783

Change-Id: I2b2b50c688f386a52328fd2b111506c26c2a69f4
Reviewed-on: https://chromium-review.googlesource.com/826448
Commit-Ready: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/8cb378263a7c2a34457178d7c89614ffbf62076e/chromeos-base/permission_broker/permission_broker-9999.ebuild

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/ce8de257e2547701f2194e43d7d42db5a21a6168

commit ce8de257e2547701f2194e43d7d42db5a21a6168
Author: Jorge Lucangeli Obes <jorgelo@chromium.org>
Date: Tue Dec 19 04:33:10 2017

permission_broker: Close outstanding firewall holes on exit.

This small piece of functionality got lost in the refactor.

Also, clean up LOG(INFO) log messages and an outdated TODO in the init
script.

BUG= chromium:787578 
TEST=Run with --v=1, 'restart permission_broker', see log message.

Change-Id: I2f4c0a0a99892c226a6bf7fdcede56ed90d95cdb
Reviewed-on: https://chromium-review.googlesource.com/829754
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/ce8de257e2547701f2194e43d7d42db5a21a6168/permission_broker/port_tracker.h
[modify] https://crrev.com/ce8de257e2547701f2194e43d7d42db5a21a6168/permission_broker/permission_broker.conf
[modify] https://crrev.com/ce8de257e2547701f2194e43d7d42db5a21a6168/permission_broker/port_tracker.cc

This is almost done. We're only missing removing firwalld from the manifest.
Cc: jorgelo@chromium.org derat@chromium.org
 Issue 715621  has been merged into this issue.
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/c0cd905bffffe4281814c41cb2106d2415bfe304

commit c0cd905bffffe4281814c41cb2106d2415bfe304
Author: Jorge Lucangeli Obes <jorgelo@chromium.org>
Date: Wed Dec 20 14:55:37 2017

firewalld: Remove ebuild.

BUG= chromium:787578 
TEST='emerge-caroline firewalld' finds no ebuilds.

Change-Id: Ib5b9526ea90d67630c2b1844960385937e4d422b
Reviewed-on: https://chromium-review.googlesource.com/834291
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Trybot-Ready: Jorge Lucangeli Obes <jorgelo@chromium.org>

[delete] https://crrev.com/e76d170fc5b3c34d309d2dfe9a1cd9652d1ce27e/chromeos-base/firewalld/firewalld-0.0.1-r1980.ebuild
[delete] https://crrev.com/e76d170fc5b3c34d309d2dfe9a1cd9652d1ce27e/chromeos-base/firewalld/firewalld-9999.ebuild

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 20 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/manifest-internal/+/5565de6e97fc24b21db222000e0139b64e2b5cf9

commit 5565de6e97fc24b21db222000e0139b64e2b5cf9
Author: Jorge Lucangeli Obes <jorgelo@chromium.org>
Date: Wed Dec 20 20:53:49 2017

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/manifest/+/66dea99db2483f0f7769f46416f585e2d34ec4f3

commit 66dea99db2483f0f7769f46416f585e2d34ec4f3
Author: Jorge Lucangeli Obes <jorgelo@chromium.org>
Date: Wed Dec 20 20:54:06 2017

firewalld: Remove from manifest.

Not needed anymore ('equery-caroline d firewalld' is empty).

BUG= chromium:787578 
TEST=PreCQ
CQ-DEPEND=CL:834291

Change-Id: I7f53ddef9faaba07869ff51ca29c1406e469b327
Reviewed-on: https://chromium-review.googlesource.com/833257
Commit-Ready: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/66dea99db2483f0f7769f46416f585e2d34ec4f3/full.xml

Status: Fixed (was: Started)
Whew.

Sign in to add a comment