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

openconnect: fix paths to systemd tools in vpnc-script #121917

Closed
wants to merge 2 commits into from

Conversation

liff
Copy link
Contributor

@liff liff commented May 6, 2021

Motivation for this change

The vpnc-script looks for a bunch of tools in fixed FHS paths. Since it won’t find them it’ll use the fallback mechanism to update DNS information, which means it’ll just change /etc/resolv.conf. This doesn’t work too well with systemd.

There’s also #105020, which this should fix.

Things done
  • Updated vpnc-scripts to the latest version from Git.
  • Changed the vpnc-scripts build to just install vpnc-script and fix the paths in it.
  • Changed license to lgpl21Only per Robot Robert’s suggestion.
    The license headers in OpenConnect sources have some occurrences of “or later version” but most of them do not contain that text. So, I am assuming the stricter(?) interpretation applies.

Didn’t run nix-shell -p nixpkgs-review --run "nixpkgs-review wip" since that would build a rather sizable portion of the desktop (GNOME/KDE) world.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@r-rmcgibbo
Copy link

r-rmcgibbo commented May 6, 2021

Result of nixpkgs-review pr 121917 at cb9b5689 run on aarch64-linux 1

124 packages marked as broken and skipped:
  • gnome3.gnome-books
  • gnome3.gnome-documents
  • libsForQt512.akonadi-calendar
  • libsForQt512.akonadiconsole
  • libsForQt512.alkimia
  • libsForQt512.baloo-widgets
  • libsForQt512.bomber
  • libsForQt512.bovo
  • libsForQt512.calendarsupport
  • libsForQt512.discover
  • ...
441 packages skipped due to time constraints:
  • adapta-gtk-theme
  • almanah
  • apostrophe
  • appgate-sdp
  • arc-theme
  • arx-libertatis
  • asciidoc-full
  • asciidoc-full-with-plugins
  • astroid
  • balsa
  • ...
12 packages built successfully:
  • connman
  • connman-gtk
  • connman-ncurses
  • connmanFull
  • connman_dmenu
  • networkmanager-openconnect (gnome3.networkmanager-openconnect)
  • libsForQt5.kdelibs4support (libsForQt515.kdelibs4support ,plasma5Packages.kdelibs4support)
  • libsForQt5.networkmanager-qt (libsForQt515.networkmanager-qt ,plasma5Packages.networkmanager-qt)
  • libsForQt5.plasma-nm (libsForQt515.plasma-nm ,plasma5Packages.plasma-nm)
  • networkmanager
  • openconnect (openconnect_gnutls)
  • openconnect_openssl

Result of nixpkgs-review pr 121917 at cb9b5689 run on x86_64-linux 1

130 packages marked as broken and skipped:
  • gimpPlugins.exposureBlend
  • gimpPlugins.texturize
  • glimpsePlugins.exposureBlend
  • glimpsePlugins.texturize
  • gnome3.gnome-books
  • gnome3.gnome-documents
  • googleearth-pro
  • libsForQt512.akonadi-calendar
  • libsForQt512.akonadiconsole
  • libsForQt512.alkimia
  • ...
513 packages skipped due to time constraints:
  • adapta-gtk-theme
  • almanah
  • apostrophe
  • appgate-sdp
  • arc-theme
  • areca
  • arx-libertatis
  • asciidoc-full
  • asciidoc-full-with-plugins
  • astroid
  • ...
12 packages built successfully:
  • connman
  • connman-gtk
  • connman-ncurses
  • connmanFull
  • connman_dmenu
  • networkmanager-openconnect (gnome3.networkmanager-openconnect)
  • libsForQt5.kdelibs4support (libsForQt515.kdelibs4support ,plasma5Packages.kdelibs4support)
  • libsForQt5.networkmanager-qt (libsForQt515.networkmanager-qt ,plasma5Packages.networkmanager-qt)
  • libsForQt5.plasma-nm (libsForQt515.plasma-nm ,plasma5Packages.plasma-nm)
  • networkmanager
  • openconnect (openconnect_gnutls)
  • openconnect_openssl
1 suggestion:
  • warning: unclear-gpl

    lgpl21 is a deprecated license, please check if project uses lgpl21Plus or lgpl21Only and change meta.license accordingly.

    Near pkgs/tools/networking/openconnect/default.nix:60:5:

       |
    60 |     license = licenses.lgpl21;
       |     ^
    

liff added 2 commits May 7, 2021 16:09
The license headers in OpenConnect sources have some
occurrences of “or later version” but most of them do
not contain that text. So, I am assuming the stricter
interpretation applies.
@symphorien
Copy link
Member

Your approach looks good, but if you feel adventurous this looks like a perfect use case for resholve, see pkgs/development/misc/resholve/README.md

@liff
Copy link
Contributor Author

liff commented May 9, 2021

I attempted to use resholve but this is as far as I got.

$ resholve --interpreter `which bash` --path /run/current-system/bin --keep '.:/etc/functions.sh;$script' --keep '$IPROUTE' vpnc-script
Traceback (most recent call last):
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1990, in <module>
    sys.exit(punshow())
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 622, in punshow
    epilogue=args.epilogue,
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 486, in resolve_script
    script_path, shebang=shebang, prologue=prologue, epilogue=epilogue
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1401, in __init__
    self.Visit(node)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1965, in Visit
    self.VisitChildren(node)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1980, in VisitChildren
    self.Visit(item)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1965, in Visit
    self.VisitChildren(node)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1984, in VisitChildren
    self.Visit(child)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1965, in Visit
    self.VisitChildren(node)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1980, in VisitChildren
    self.Visit(item)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1964, in Visit
    self._Visit(node)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1958, in _Visit
    self._visit_command_Simple(node)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1668, in _visit_command_Simple
    node,
Exception: ('Trying to handle exec but it lacks a required argument', (command.Simple
  words: [(compound_word parts:[(Token id:Id.Lit_Chars span_id:4287 val:exec)])]
  redirects: [
    (redir
      op: (Token id:Id.Redir_Less span_id:4289 val:'6<')
      loc: (redir_loc.Fd fd:6)
      arg: 
        (compound_word
          parts: [
            (double_quoted
              left: (Token id:Id.Left_DoubleQuote span_id:4291 val:'"')
              parts: [
                (simple_var_sub
                  token: (Token id:Id.VSub_DollarName span_id:4292 val:'$RESOLV_CONF_BACKUP')
                )
              ]
              multiline: F
              spids: [4291 4293]
            )
          ]
        )
    )
  ]
  do_fork: T
))

Does the command look right? Seems to me like the keep directive $script shouldn’t be needed since it’s a local variable in a for loop.

@tricktron
Copy link
Member

tricktron commented May 9, 2021

@liff Could you check if the changes from this pr #106465 using openresolv solve the same problem?

@symphorien
Copy link
Member

I attempted to use resholve but this is as far as I got.

$ resholve --interpreter `which bash` --path /run/current-system/bin --keep '.:/etc/functions.sh;$script' --keep '$IPROUTE' vpnc-script
Traceback (most recent call last):
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1990, in <module>
    sys.exit(punshow())
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 622, in punshow
    epilogue=args.epilogue,
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 486, in resolve_script
    script_path, shebang=shebang, prologue=prologue, epilogue=epilogue
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1401, in __init__
    self.Visit(node)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1965, in Visit
    self.VisitChildren(node)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1980, in VisitChildren
    self.Visit(item)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1965, in Visit
    self.VisitChildren(node)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1984, in VisitChildren
    self.Visit(child)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1965, in Visit
    self.VisitChildren(node)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1980, in VisitChildren
    self.Visit(item)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1964, in Visit
    self._Visit(node)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1958, in _Visit
    self._visit_command_Simple(node)
  File "/nix/store/7m6r5wzmpc3fs148prq9dimqi7w64h12-resholve-0.5.1/bin/.resholve-wrapped", line 1668, in _visit_command_Simple
    node,
Exception: ('Trying to handle exec but it lacks a required argument', (command.Simple
  words: [(compound_word parts:[(Token id:Id.Lit_Chars span_id:4287 val:exec)])]
  redirects: [
    (redir
      op: (Token id:Id.Redir_Less span_id:4289 val:'6<')
      loc: (redir_loc.Fd fd:6)
      arg: 
        (compound_word
          parts: [
            (double_quoted
              left: (Token id:Id.Left_DoubleQuote span_id:4291 val:'"')
              parts: [
                (simple_var_sub
                  token: (Token id:Id.VSub_DollarName span_id:4292 val:'$RESOLV_CONF_BACKUP')
                )
              ]
              multiline: F
              spids: [4291 4293]
            )
          ]
        )
    )
  ]
  do_fork: T
))

Does the command look right? Seems to me like the keep directive $script shouldn’t be needed since it’s a local variable in a for loop.

hum I don't know, cc @abathur maybe?

@abathur
Copy link
Member

abathur commented May 9, 2021

@symphorien @liff This is abathur/resholve#23. I'll be brief since I'm working on the fix at the moment, but the TL;DR:

  • You (probably) aren't holding it wrong. I wasn't thinking about exec's FD behavior when I added it to a list of command-executing-commands that are all handled by the same code.
  • The fix is tied up in a general refactor to separate out individual command handlers. I'm ~close, and have this issue fixed locally, but I'm very focused on the trees at the moment and don't have a great sense of how much longer it'll take to land. (I'm certainly hoping to make 21.05).

@liff
Copy link
Contributor Author

liff commented May 10, 2021

@tricktron #106465 looks good, though it wouldn’t work for me with systemd-resolved. I’ll continue on that one.

@liff liff closed this May 10, 2021
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.

None yet

5 participants