Send ports forwarded to control server #2392

jagaimoworks · 2024-08-03T12:40:09Z

First timer here. This is a somewhat working implementation of #2369. Hit me with the improvements I can take it 😅

I say somewhat working because the removal of ports from the firewall suffers from #2334 and therefore does not reliably work right now.

The way it works right now is by sending a http PUT request with a body like {ports: [1234, 3456]} to /v1/openvpn/portforwarded.

qdm12

That's great, thanks for the PR 💯 !
I will wait to fix the iptables removals (to create less user frustration and duplicate issues) after v3.39.0 gets released, to merge this though.

qdm12 · 2024-08-03T13:47:40Z

internal/portforward/loop.go

+	err := l.service.SetPortsForwarded(l.runCtx, ports)
+	if err != nil {
+		l.logger.Error(err.Error())
+	}


Perhaps we should return an error here to let the http client know it failed for xyz reason 🤔
And possibly log it as well, as it is now.

Commit f18cdb8 addresses this. It would probably suffice to let control server respond with a more generic error, since the original error message already gets logged anyways. What do you think?

qdm12 · 2024-08-03T13:48:35Z

internal/portforward/loop.go

+	if l.service == nil {
+		return
+	}


We could set the ports somehow, even if the service is not started. The ports could then be injected to the service when we create it. A bit of a futuristic approach about when we could do all kind of modifications live 😄

Yeah, that might be beyond me for now. 😅

qdm12 · 2024-08-03T13:49:22Z

internal/portforward/service/service.go

+	for _, port := range s.ports {
+		err := s.portAllower.RemoveAllowedPort(ctx, port)
+		if err != nil {
+			s.logger.Error(err.Error())


If this fails, we should attempt to re-add the removed ports (ignoring the possible errors) to 'revert' back to how it was, and then return an error (we can log the error and also return it).

See 1d8e3e1. I probably should add comments to the loops, right?

With 52522df it now also won´t reprocess the failed setting or removal.

qdm12 · 2024-08-03T13:52:55Z

internal/portforward/service/service.go

+	for _, port := range ports {
+		err := s.portAllower.SetAllowedPort(ctx, port, s.settings.Interface)
+		if err != nil {
+			s.logger.Error(err.Error())


If this fails, we should, in order to have an atomic state:

attempt to remove the ports just added (ports[0] and up to ports[len(ports)-2]) and ignore errors returned by the removals

attempt to re-add previously removed s.ports and ignore errors returned by the additions.

See 1d8e3e1.

ports[0] and up to ports[len(ports)-2]

The -2 just hit me. I will fix it.

Fixed it: 52522df

qdm12 · 2024-08-03T13:57:57Z

internal/portforward/service/service.go

+	s.ports = make([]uint16, len(ports))
+	copy(s.ports, ports)
+
+	s.logger.Info("Updated: " + portsToString(s.ports))


nit

Suggested change

s.logger.Info("Updated: " + portsToString(s.ports))

s.logger.Info("updated: " + portsToString(s.ports))

See 8fbb43d.

qdm12 · 2024-08-03T13:58:41Z

internal/server/interfaces.go

@@ -21,8 +21,9 @@ type DNSLoop interface {
 	GetStatus() (status models.LoopStatus)
 }

-type PortForwardedGetter interface {
+type PortForwarded interface {


nit whilst we're renaming it, I think PortForwardingService or just PortForwarding would make more sense 😉 !

See b1826bd.

qdm12 · 2024-08-03T13:59:35Z

internal/server/openvpn.go

@@ -11,19 +11,19 @@ import (
 )

 func newOpenvpnHandler(ctx context.Context, looper VPNLooper,
-	pfGetter PortForwardedGetter, w warner) http.Handler {
+	portForwarded PortForwarded, w warner) http.Handler {


nit rename to portForwarding or portForwardingService 😉

See b1826bd.

qdm12 · 2024-08-03T14:00:11Z

internal/server/openvpn.go

+	}
+
+	if len(data.Ports) == 0 {
+		http.Error(w, "invalid request", http.StatusBadRequest)


Suggested change

http.Error(w, "invalid request", http.StatusBadRequest)

http.Error(w, "no port specified", http.StatusBadRequest)

Resolved with 8fbb43d.

qdm12 · 2024-08-03T14:01:49Z

internal/server/openvpn.go

+
+func (h *openvpnHandler) setPortForwarded(w http.ResponseWriter, r *http.Request) {
+	decoder := json.NewDecoder(r.Body)
+	encoder := json.NewEncoder(w)


nit move this above line 164 where it's first used. I know other handler functions are ugly doing this as well, but since it's fresh new code 😸

Your wish is my command 😄 (00dc345)

…d vice versa

qdm12 · 2024-08-09T14:46:34Z

(Sort of) blocked by #1785

qdm12 · 2024-08-23T06:07:37Z

Blocked by #2238 as well.

jagaimoworks added 2 commits August 3, 2024 14:17

added capability to set forwarded ports at runtime

ec3a983

added control server route for dynamic port forwarding

8be9055

jagaimoworks mentioned this pull request Aug 3, 2024

Bug: cannot remove allowed input port from firewall #2334

Closed

qdm12 reviewed Aug 3, 2024

View reviewed changes

qdm12 added Status: 🔴 Blocked Blocked by another issue or pull request Status: 🔒 After next release Will be done after the next release labels Aug 3, 2024

qdm12 force-pushed the master branch from 4d53eb6 to 10f43d7 Compare August 4, 2024 08:36

jagaimoworks added 6 commits August 5, 2024 17:35

addressing renaming nits

b1826bd

try to silently undo changes when port set/remove fail

1d8e3e1

added http error response for failing to set forwarded ports

f18cdb8

changed http error response to accuratly depict the error

8fbb43d

moved statement to improve readability

00dc345

fix: don´t readd failed port forwardings that failed to be removed an…

52522df

…d vice versa

qdm12 mentioned this pull request Aug 9, 2024

Feature request: run a script on interface up/down #1785

Open

qdm12 removed the Status: 🔒 After next release Will be done after the next release label Aug 9, 2024

qdm12 added Status: 🔴 Blocked Blocked by another issue or pull request Status: 🟡 Nearly resolved This might be resolved or is about to be resolved and removed Status: 🔴 Blocked Blocked by another issue or pull request labels Aug 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send ports forwarded to control server #2392

Send ports forwarded to control server #2392

jagaimoworks commented Aug 3, 2024 •

edited

Loading

qdm12 left a comment

qdm12 Aug 3, 2024

jagaimoworks Aug 5, 2024

qdm12 Aug 3, 2024

jagaimoworks Aug 5, 2024

qdm12 Aug 3, 2024

jagaimoworks Aug 5, 2024

jagaimoworks Aug 5, 2024

qdm12 Aug 3, 2024

jagaimoworks Aug 5, 2024

jagaimoworks Aug 5, 2024

qdm12 Aug 3, 2024

jagaimoworks Aug 5, 2024 •

edited

Loading

qdm12 Aug 3, 2024

jagaimoworks Aug 5, 2024

qdm12 Aug 3, 2024

jagaimoworks Aug 5, 2024

qdm12 Aug 3, 2024

jagaimoworks Aug 5, 2024

qdm12 Aug 3, 2024

jagaimoworks Aug 5, 2024

qdm12 commented Aug 9, 2024

qdm12 commented Aug 23, 2024

	s.logger.Info("Updated: " + portsToString(s.ports))
	s.logger.Info("updated: " + portsToString(s.ports))

	http.Error(w, "invalid request", http.StatusBadRequest)
	http.Error(w, "no port specified", http.StatusBadRequest)

Send ports forwarded to control server #2392

Are you sure you want to change the base?

Send ports forwarded to control server #2392

Conversation

jagaimoworks commented Aug 3, 2024 • edited Loading

qdm12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagaimoworks Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 commented Aug 9, 2024

qdm12 commented Aug 23, 2024

jagaimoworks commented Aug 3, 2024 •

edited

Loading

jagaimoworks Aug 5, 2024 •

edited

Loading