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

Align how http proxies are handled when fetching gpg keys for download verification #1125

Closed

Conversation

rbambrough-intel
Copy link

I noticed while trying to run a dev-container setup behind a corporate proxy that git-lfs was getting stuck waiting for gpg keys.

It downloads the collateral file from, mostly, github and then checks to make sure the hashes match up with what is expected. Due to how the proxy was being handled through the install scripts it would either failure to download the collateral (http proxy not correctly setup in container environment) or would fail to verify the collateral (http proxy setup correctly, but gpg not being passed proxy values).

To solve this I went through and aligned all of the features which use gpg checking with how the python feature does it.

I also removed the httpProxy parameter from the features which had it as anyone using it would have already had to have had the proxy correctly setup in order to download the collateral initially. I was one the fence about this change and would remove it if it is deemed no backwards compatible. I didn't see any documentation on how this type of removal should be handled.

Affected features:

git-lfs
git
github-cli
kubectl-helm-minikube
python
ruby
terraform

…d verification

  * Copy receive_gpg_keys in python/install.sh to other features which use gpg verification
  * Ensure http proxy url is passed to gpg when receiving keys from servers
    * Could connect to key servers but not receive keys
  * Remove httpProxy from features as it would have been broken from the issue above
  * Fix proxy arg mismatch between downloading collateral and verifying collateral
    * Should be both or none
  * Curl should automatically pick up proxy config
@rbambrough-intel rbambrough-intel requested a review from a team as a code owner September 19, 2024 22:55
@@ -15,6 +15,7 @@ GIT_LFS_ARCHIVE_GPG_KEY_URI="https://packagecloud.io/github/git-lfs/gpgkey"
GIT_LFS_ARCHIVE_ARCHITECTURES="amd64 arm64"
GIT_LFS_ARCHIVE_VERSION_CODENAMES="stretch buster bullseye bionic focal jammy"
GIT_LFS_CHECKSUM_GPG_KEYS="0x88ace9b29196305ba9947552f1ba225c0223b187 0x86cd3297749375bcf8206715f54fe648088335a9 0xaa3b3450295830d2de6db90caba67be5a5795889"
KEYSERVER_PROXY="${HTTP_PROXY:-""}"
Copy link
Member

Choose a reason for hiding this comment

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

@rbambrough-intel Thank you so much for taking the time to contribute this PR.

Instead of these changes, how about we making similar changes #1119 ?

Copy link

Choose a reason for hiding this comment

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

It seems the link is not pointing to the right PR and I don't understand your statement as it seems to be the common way to define a default variable value in this project (e.g.:

ADDITIONAL_VERSIONS="${ADDITIONALVERSIONS:-""}"
,
ADDITIONAL_VERSIONS="${ADDITIONALVERSIONS:-""}"
,
POWERSHELL_MODULES="${MODULES:-""}"
). However I think this following will be enough:

Suggested change
KEYSERVER_PROXY="${HTTP_PROXY:-""}"
KEYSERVER_PROXY="${HTTP_PROXY}"

But maybe I missed something.

Copy link
Member

Choose a reason for hiding this comment

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

Apologies, I pasted the incorrect link.

I was recommending #1124 which tests the servers before using them to download the keys.

KEYSERVER_PROXY="${HTTP_PROXY:-""}"

This simply assigns the KEYSERVER_PROXY to "" if HTTP_PROXY is not defined.

Copy link
Author

Choose a reason for hiding this comment

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

@Jarsop If we are fine with removing the httpProxy setting from some of the features then it can be simplified into:

KEYSERVER_PROXY="$HTTP_PROXY"

Copy link
Author

Choose a reason for hiding this comment

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

@samruddhikhandale It does do curl checking of the key servers, just not the same as the one you linked (map instead of multiple function calls). I wasn't sure touching the function one so I adjusted all the others that looked closee

@Jarsop
Copy link

Jarsop commented Sep 26, 2024

Thanks for your PR, I came to the same conclusion and it avoids duplicate work.

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