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

Fix Automagic for TCP based OpenVPN and for port aliases #528

Merged
merged 3 commits into from
May 30, 2019

Conversation

RolfWojtech
Copy link
Contributor

I stumbled upon two issues with the wonderful Automagic feature:

  1. It currently does not create a remote line at all even if you have NAT rules that point to the TCP server.
  2. It behaves unexpectedly when dealing with multiple ports inside an alias: it will only use the first one.
    This merge request fixes both issues.

$targetproto becomes tcp-client when creating a "Most clients" config for a TCP server. This caused the comparison $natent['protocol'] == $targetproto) to fail (tcp vs tcp-client). Also it seems like the old version would write "tcp" instead of the correct "tcp-client" into the config file, $targetproto contains the right string.
When you have a Multi-WAN OpenVPN server or multiple ports NATing to a single OpenVPN server, many people recommend using an alias for the ports. Unfortunately the old implementation only creates a remote line for the first port of the alias and discards the rest. Generating multiple remote lines in this case is the expected behaviour.
Copy link
Contributor

@jim-p jim-p left a comment

Choose a reason for hiding this comment

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

Looks OK just needs one small change

$dports = is_port($natent['destination']['port']) ? array($natent['destination']['port']) : filter_expand_alias_array($natent['destination']['port']);
$dest['port'] = $dports[0];

$proto_short = strtolower(substr($targetproto, 0, 3));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this variable is only used once, skip setting it this way and use the contents in the next line directly instead.

@RolfWojtech
Copy link
Contributor Author

Thanks for your review, I made the change you requested (personally I prefer having the variable for the next person that stumbles upon this code, but I concede that it wastes a line).

@RolfWojtech
Copy link
Contributor Author

By the way I made the observation that the network manager in Ubuntu 16.04 cannot handle multiple multiple remotes. However the existing code already generates multiple remotes if you use multi-WAN or two ports for the same server.

Copy link
Member

@rbgarga rbgarga left a comment

Choose a reason for hiding this comment

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

Also bump PORTVERSION or PORTREVISION on Makefile to trigger a new pkg build


if (!isset($natent['disabled']) && ($natent['target'] == $targetip) && ($natent['local-port'] == $targetport) && ($natent['protocol'] == strtolower(substr($targetproto, 0, 3)))) {
$dest['proto'] = $targetproto;

Copy link
Member

Choose a reason for hiding this comment

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

Please keep the indentation style using hard TABs instead of replacing it by spaces

@@ -1033,7 +1029,13 @@ function openvpn_client_export_find_port_forwards($targetip, $targetport, $targe
}
}

$destinations[] = $dest;
$dports = is_port($natent['destination']['port']) ? array($natent['destination']['port']) : filter_expand_alias_array($natent['destination']['port']);
Copy link
Member

Choose a reason for hiding this comment

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

Same here, keep TABs

@rbgarga rbgarga requested a review from jim-p July 17, 2018 11:10
Copy link
Contributor

@jim-p jim-p left a comment

Choose a reason for hiding this comment

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

Aside from the issues noted by @rbgarga this looks OK

@netgate-git-updates netgate-git-updates merged commit 20d0713 into pfsense:devel May 30, 2019
netgate-git-updates pushed a commit that referenced this pull request May 21, 2022
3.1.0 (2022-05-18)
   * Introduce basic support for OpenSSL version 3 (#492)
   * Update regex in grep to be POSIX compliant (#556)
   * Introduce status reporting tools (#555 & #557)
   * Display certificates using UTF8 (#551)
   * Allow certificates to be created with fixed date offset (#550)
   * Add 'verify' to verify certificate against CA (#549)
   * Add PKCS#12 alias 'friendlyName' (#544)
   * Disallow use of '--vars=FILE init-pki' (#566)
   * Support multiple IP-Addresses in SAN (#564)
   * Add option '--renew-days=NN', custom renew grace period (#557)
   * Add 'nopass' option to the 'export-pkcs' functions (#411)
   * Add support for 'busybox' (#543)
   * Add option '--tmp-dir=DIR' to declare Temp-dir (Commit f503a22)

3.0.9 (2022-05-17)
   * Upgrade OpenSSL from 1.1.0j to 1.1.1o (#405, #407)
      - We are buliding this ourselves now.
   * Fix --version so it uses EASYRSA_OPENSSL (#416)
   * Use openssl rand instead of non-POSIX mktemp (#478)
   * Fix paths with spaces (#443)
   * Correct OpenSSL version from Homebrew on macOs (#416)
   * Fix revoking a renewed certificate (Original PR #394)
     Follow-up commit: ef22701878bb10df567d60f2ac50dce52a82c9ee
   * Introduce 'show-crl' (d1993892178c5219f4a38d50db3b53d1a972b36c)
   * Support Windows-Git 'version of bash' (#533)
   * Disallow use of single quote (') in vars file, Warning (#530)
   * Creating a CA uses x509-types/ca and COMMON (#526)
   * Prefer 'PKI/vars' over all other locations (#528)
   * Introduce 'init-pki soft'  option (#197)
   * Warnings are no longer silenced by --batch (#523)
   * Improve packaging options (#510)
   * Update regex for POSIX compliance (#556)
   * Correct date format for Darwin/BSD (#559)
netgate-git-updates pushed a commit that referenced this pull request Sep 26, 2022
Changelog
=========

0.5.3
-----

 - Int casts due to python 3.10 extension interface changes
 - Pycodestyle changes

0.5.2
-----

 - Using more integer divisions to get right type for QPainter
   points

0.5.1
-----

 - fixed crashing polar charts on python3.10  #528 (#539)
netgate-git-updates pushed a commit that referenced this pull request Feb 4, 2023
Changes since 1.1.0:

v1.3.0

This release contains some features and enhancements + upgrades all
dependencies.

- feat: Allow to set reporter on issue create by @ankitpokhrel in #539
- feat: Use single char ellipsis instead of triple dot by @ankitpokhrel in #540
- ehc: Make assignee operation atomic on create by @ankitpokhrel in #531
- ehc: Auto fallback to plain output on notty by @ankitpokhrel in #538
- ehc: Add warning for invalid custom field by @ankitpokhrel in #528 (Original work by @martinpovolny on #525)
- fix(build): Invalid commit hash in docker image by @ankitpokhrel in #535

- dep: Upgrade all packages by @ankitpokhrel in #532
- dep: Upgrade golang to v1.19 by @ankitpokhrel in #534
- ci: Upgrade golangci-lint to v1.50.1 by @ankitpokhrel in #536

Full Changelog: ankitpokhrel/jira-cli@v1.2.0...v1.3.0

v1.2.0

This release adds support for Jira v9, a serverinfo command to quickly check
your Jira server build info, lets you set resolution, assignee and comment on
issue move, and many more.

- feat: Add serverinfo command by @ankitpokhrel in #440
- feat: Support for Jira v9 by @ankitpokhrel in #447
- feat: Allow to set start datetime on worklog add by @ankitpokhrel in #453
- feat: Make date time input in worklog flexible by @ankitpokhrel in #465
- feat: Add support for project datatype in custom fields by @oveaurs in #482
- feat: Add weblink to issue (#446) by @Syd7 in #483
- feat: Resolution, assignee & comment on issue move by @ankitpokhrel in #492
- feat: Filter issues by the absence of label(s) by @martinpovolny in #505
- feat: Add labels to the issue listing by @martinpovolny in #506
- feat: Allow setting of fixed columns in the list of issues, epics and sprints
  by @martinpovolny in #509

- fix: Option to show issues from all projects in sprint list by @ankitpokhrel
  in #475
- fix: Discrepancy in --insecure flag by @ankitpokhrel in #507
- fix: Make board selection optional by @ankitpokhrel in #502
- fix: Improve support for pager by @ankitpokhrel in #503
- fix: Respect editor env vars in Windows by @ankitpokhrel in #524

- ci: Multi-arch docker image by @ankitpokhrel in #508
- doc: Add link to project in help by @ankitpokhrel in #456
- doc: Add Nix package by @bryanasdev000 in #458
- doc: Update help for completion cmd by @ankitpokhrel in #491
- doc: Add scoop installation process by @alkuzad in #497

- @bryanasdev000 made their first contribution in #458
- @oveaurs made their first contribution in #482
- @Syd7 made their first contribution in #483
- @alkuzad made their first contribution in #497
- @martinpovolny made their first contribution in #505

Full Changelog: ankitpokhrel/jira-cli@v1.1.0...v1.2.0
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