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

Add check to see if key already exists locally before querying keyservers #209

Merged
merged 4 commits into from
Mar 11, 2021
Merged

Add check to see if key already exists locally before querying keyservers #209

merged 4 commits into from
Mar 11, 2021

Conversation

vadave
Copy link
Contributor

@vadave vadave commented Mar 5, 2021

Addresses issue #208. Checks to see if the key exists in the keyring. If the check is successful, it bypasses the remote call. This vastly reduces the time needed to execute this script on subsequent runs (which is helpful when you're using something like ansible to manage asdf/plugin configs).

$gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} --no-tty --keyserver "hkp://$server" $OPTIONS --display-charset utf-8 --recv-keys "$key" && break
done
for server in $SERVERS; do
$gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} -k $key -with-colons > /dev/null 2>&1 || \
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a missing - in -with-colons as documented in gpg manpage

Suggested change
$gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} -k $key -with-colons > /dev/null 2>&1 || \
$gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} -k $key --with-colons > /dev/null 2>&1 || \

@augustobmoura
Copy link
Member

Also, after this merge I think we can try to close #138, running the import before every install should be quick enough to don't bother anyone. @vadave can you consider adding the auto import feature in this PR? Just invoking the import script in the install script file should be enough

@augustobmoura
Copy link
Member

augustobmoura commented Mar 9, 2021

As of the check by itself, I ran it in my machine and it worked perfectly

@vadave
Copy link
Contributor Author

vadave commented Mar 9, 2021

Added the automatic import to the install script and tweaked the usage of --with-colons (which turned out to be slightly more than just adding -, as that would produce an error saying --with-colons wasn't supported. Changing the order of the args caused it to work as expected)

bin/install Outdated
@@ -241,6 +241,9 @@ download_and_verify_checksums() {
export GNUPGHOME="${ASDF_DIR:-$HOME/.asdf}/keyrings/nodejs"
fi

# Automatically add needed PGP keys
source "$(dirname "$0")/import-release-team-keyring"
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: no need for source here. "$(dirname "$0")/import-release-team-keyring" is sufficient on its own.

$gnugp_verify_command_name --with-colons --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} -k $key > /dev/null 2>&1 || \
$gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} --no-tty --keyserver "hkp://$server" $OPTIONS --display-charset utf-8 --recv-keys "$key" && break
done
# fi
Copy link
Member

Choose a reason for hiding this comment

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

commented out code. Please remove.

$gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} --no-tty --keyserver "hkp://$server" $OPTIONS --display-charset utf-8 --recv-keys "$key" && break
done
for server in $SERVERS; do
$gnugp_verify_command_name --with-colons --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} -k $key > /dev/null 2>&1 || \
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this line be moved above line 42?

Something like this:

for key in $KEYS; do
  # Halt fetching this key if we already have it
  $gnugp_verify_command_name --with-colons --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} -k $key > /dev/null 2>&1 && break

  # Otherwise loop through servers until we find it
  for server in $SERVERS; do
    $gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} --no-tty --keyserver "hkp://$server" $OPTIONS --display-charset utf-8 --recv-keys "$key" && break
  done
done

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'd originally tried that but got a weird gnupg_verify_command_name: unbound variable error that I didn't get when I had it in the inner loop. I futzed with it a bit but ultimately opted to go with working code vice spending more time down the rathole troubleshooting further.

@Stratus3D
Copy link
Member

Thanks for the PR @vadave ! I think this is a nice improvement. I left a few comments, let me know if you have any questions.

@Stratus3D
Copy link
Member

Tested this locally and it worked perfectly. Merging. We may want to add some tests around the logic in the bin/import-release-team-keyring script, it has become fairly complicated.

@Stratus3D Stratus3D merged commit a0ce61a into asdf-vm:master Mar 11, 2021
@Stratus3D
Copy link
Member

Thanks for the PR @vadave !

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.

3 participants