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

Changes to helper script example. #1

Merged
merged 4 commits into from
Jul 25, 2020
Merged

Changes to helper script example. #1

merged 4 commits into from
Jul 25, 2020

Conversation

tetov
Copy link
Contributor

@tetov tetov commented Jul 24, 2020

Thanks for this script, it is the only gpg-bridge I've found that works without hiccups.

However, while adapting the helper script to my setup I ran into some problems with unrecognized arguments errors from the ruby part. This was probably because of something I did wrong, but it prompted me to rewrite the script.

Do you think it is clear and usable?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved

if [ "$WSL2" = true ]
then
cmd+=("--remote-address $(ip route | awk '/^default via / {print $3}')")
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be two separate arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be, it works as is but that might be more readable?

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, maybe it works because of the eval line, that will cause all arguments to be split at whitespace.


printf -v _cmd '%s ' "${cmd[@]}"

eval "$_cmd"
Copy link
Owner

Choose a reason for hiding this comment

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

I'm thinking the reason you are using eval here is so that the redirection to /dev/null will work. Am I right that you don't want to see the message about an existing gpg bridge already running? Maybe it would be better to add a --quiet option to the script to make it silent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using eval because I ran into issues where ruby would either complain about not finding a file named "path/to/bridge arg1 arg2 arg3" or not recognized an argument because it was single-quoted..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I solved the quiet part with a lazy env variable, I didn't find a good way to parse non positional arguments.

Copy link
Owner

Choose a reason for hiding this comment

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

OK!

Copy link
Owner

@mhyllander mhyllander left a comment

Choose a reason for hiding this comment

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

Hej Anton! Thanks for the contribution, nice that someone has use for the script!

I assume the reason for putting the helper script in Windows is so that it can be used from different WSL distributions. I'm fine with that. There's just one thing though, the WSL2 variable need to be set by the caller instead of in the helper script to allow it to be used from both WSL1 and WSL2 distributions. I suggested one way that will allow the caller to set WSL2=true before sourcing the script.

@tetov
Copy link
Contributor Author

tetov commented Jul 24, 2020

Hej!

I assume the reason for putting the helper script in Windows is so that it can be used from different WSL distributions.

That wasn't my intention actually, I just installed my second distro outside of the distros Docker keeps. I source it from my dotfiles/bin, so that would be how I reuse it from different distros.

Your suggestion about sourcing WSL2 from env makes sense in whatever way it's used anyways. I'll add that to the instructions.

--

If this grows more in complexity it should probably:

  1. Become its own file in the repo
  2. Be written in something that's not bash. If I were to write it it would be in Python but that would make things a bit confusing, given that the rest is in ruby, haha!

Copy link
Owner

@mhyllander mhyllander left a comment

Choose a reason for hiding this comment

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

OK, let's merge this now! I agree the helper script should be in a separate file, I can fiddle around with that later.

@mhyllander mhyllander merged commit 39d1b61 into mhyllander:master Jul 25, 2020
@tetov
Copy link
Contributor Author

tetov commented Jul 25, 2020

Hah, I just saw this. I actually worked a bit more on it, I got interested ZSH coding so I rewrote it with that. I think there's just a few lines that need to change to make it bash compatible. What do you think?

Also, do you have a WSL1 distro setup? Could you check if you have wslpath?

#!/usr/bin/env zsh -eu
#--------------------------------------------------------------------------
# GPG bridging from WSL gpg to gpg4win gpg-agent.exe
# (needed to use a Yubikey, since WSL cannot access USB devices)

SCRIPT_DIR_WSL=/mnt/c/Users/a/.gpgbridge
# shellcheck disable=SC1003
SCRIPT_DIR_WIN=$(wslpath -wa $SCRIPT_DIR_WSL)

PIDFILE_WSL=$SCRIPT_DIR_WSL/gpgbridge_wsl.pid
LOGFILE_WSL=$SCRIPT_DIR_WSL/gpgbridge_wsl.log

_pidfile_win=$SCRIPT_DIR_WSL/gpgbridge_win.pid
_logfile_win=$SCRIPT_DIR_WSL/gpgbridge_win.log
touch $_pidfile_win $_logfile_win
PIDFILE_WIN=$(wslpath -wa $_pidfile_win)
LOGFILE_WIN=$(wslpath -wa $_logfile_win)

SCRIPT_FILE_NAME=gpgbridge.rb

SCRIPT_PATH_WSL=$SCRIPT_DIR_WSL/$SCRIPT_FILE_NAME

function start_gpgbridge
{
    _parse_opts $@

    if (( ! $+commands[ruby.exe] )); then
        echo 'No ruby.exe found in path'
        return
    fi

    typeset cmd
    cmd=( ruby \
          $SCRIPT_PATH_WSL \
          --daemon \
          --pidfile $PIDFILE_WSL \
          --logfile $LOGFILE_WSL \
          --windows-pidfile ${(q-)PIDFILE_WIN} \  # (q-) escapes chars
          --windows-logfile ${(q-)LOGFILE_WIN} \
   )

    if ${SSH:=false} ; then
        cmd+=( --enable-ssh-support )
        SSH_AUTH_SOCK=$(gpgconf --list-dirs agent-ssh-socket)
        export SSH_AUTH_SOCK
    fi

    if ${WSL2:=false} ; then
       cmd+=( --remote-address "$(ip route | awk '/^default via / {print $3}')" )
    fi

    if ${QUIET:=false} ; then
        cmd+=( '>/dev/null 2>&1' )
    else
        cmd+=( --verbose )
    fi

    eval ${(z)cmd}
}

function stop_gpgbridge
{
    pkill -TERM -f 'ruby.*gpgbridge\.rb'
}

function restart_gpgbridge
{
    stop_gpgbridge
    sleep 1
    start_gpgbridge $@
}

function _parse_opts {
   for a in $@ ; do
       a=${a//-/} # remove leading dashes
       a=${a:l} # make lovercase

       case $a in
           ssh )
               export SSH=true
               ;;
           wsl2)
               export WSL2=true
               ;;
           quiet|q)
               export QUIET=true
       esac
   done
}

start_gpgbridge $@

@mhyllander
Copy link
Owner

Hi! Yes, wslpath is available in all WSL distributions as far as I know.
I'm not familiar with zsh, so I can't say if you can make it compatible with bash, but it looks like there are some differences.

@tetov
Copy link
Contributor Author

tetov commented Jul 25, 2020

Yeah it might have been stupid to rewrite for ZSH instead of bash. On the other hand isn't zsh included in most distros now?

I can rewrite it, the hard part is doing quoting (of windows paths) and splitting the right way.

The cleanest way might be to rewrite it in Ruby, since it's a dependency anyways, but that I'll have to leave to you.

@mhyllander
Copy link
Owner

Maybe it's better to have separate scripts for bash and zsh of the syntax is too different.

@tetov
Copy link
Contributor Author

tetov commented Jul 27, 2020

Maybe it's better to have separate scripts for bash and zsh of the syntax is too different.

I'll write a posix compatible one so we can be sure it works :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants