Avoid exposing password and token from git repositories #105220

tuliom · 2024-08-20T18:29:48Z

Try to detect if the git remote URL has a password or a Github token and
return an error teaching the user how to avoid leaking their password or
token.

Restrict the URL that is exposed to the official LLVM repository at Github in order to avoid exposing usernames, passwords or even private URLS unintentionally. Users willing to expose different Git repositories can continue to do so by setting LLVM_FORCE_VC_REPOSITORY or CLANG_REPOSITORY_STRING.

kbeyls · 2024-08-21T07:55:12Z

I added @tstellar and @petrhosek as reviewers. Would one of you be able to help review this? Or do you otherwise have suggestions for who would be good to review this PR?

llvm/cmake/modules/VersionFromVCS.cmake

petrhosek · 2024-08-21T08:11:57Z

While I understand the concern, I'm a bit concerned that this could be undesirable to many vendors and there should be an RFC to raise the awareness and get more feedback. If we decide to go forward with this change, I also think there should be a CMake option controlling this behavior rather than relying on LLVM_FORCE_VC_REPOSITORY.

Currently the output of dexter --version contains the raw output of `git remote get-url origin`, which may contain a username and password. This patch adds a small change to remove these from the output string. A similar patch for LLVM's default version string[0] also removes the git URL altogether unless opted-in to; it's not clear whether this is a necessary or desirable step yet, but if so we can trivially remove the URL from Dexter as well. [0]: llvm#105220

…105533) Currently the output of dexter --version contains the raw output of `git remote get-url origin`, which may contain a username and password. This patch adds a small change to remove these from the output string. A similar patch for LLVM's default version string* also removes the git URL altogether unless opted-in to; it's not clear whether this is a necessary or desirable step yet, but if so we can trivially remove the URL from Dexter as well. *PR here: #105220

…lvm#105533) Currently the output of dexter --version contains the raw output of `git remote get-url origin`, which may contain a username and password. This patch adds a small change to remove these from the output string. A similar patch for LLVM's default version string* also removes the git URL altogether unless opted-in to; it's not clear whether this is a necessary or desirable step yet, but if so we can trivially remove the URL from Dexter as well. *PR here: llvm#105220

tuliom · 2024-08-29T12:40:53Z

While I understand the concern, I'm a bit concerned that this could be undesirable to many vendors and there should be an RFC to raise the awareness and get more feedback.

@petrhosek I've just created the RFC at https://discourse.llvm.org/t/rfc-avoid-exposing-unknown-git-repositories/80962

If we decide to go forward with this change, I also think there should be a CMake option controlling this behavior rather than relying on LLVM_FORCE_VC_REPOSITORY.

Could you please share your proposal in the Discourse topic, please?
I would also appreciate if you could elaborate how this new option would work, because it isn't clear to me.

joker-eph · 2024-08-29T13:58:13Z

LGTM, but please get more approvals.

tuliom · 2024-09-03T21:44:43Z

I updated this PR based on the feedback received on Discourse.

Title and description have been updated accordingly.

llvm/cmake/modules/VersionFromVCS.cmake

tuliom · 2024-09-10T13:28:01Z

As mentioned in the Forum, the discussion on this has been going down.
I believe the current change doesn't change the behavior as the original one did.
So, I plan to merge it later today if nobody objects.

llvm-ci · 2024-09-12T03:30:32Z

LLVM Buildbot has detected a new failure on builder lld-x86_64-win running on as-worker-93 while building llvm at step 7 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/1129

Here is the relevant piece of the build log for the reference

Step 7 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests.exe/43/86' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe-LLVM-Unit-10868-43-86.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=86 GTEST_SHARD_INDEX=43 C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe
--

Script:
--
C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe --gtest_filter=ProgramEnvTest.CreateProcessLongPath
--
C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(160): error: Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(163): error: fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied



C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:160
Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:163
fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied




********************

danilaml · 2024-09-16T15:15:38Z

Adding -DLLVM_FORCE_VC_REPOSITORY doesn't achieve the intended effect. It overrides the repository but also no revision is generated either, i.e. VCSRevision.h becomes

#undef LLVM_REVISION
#define LLVM_REPOSITORY "test"

Considering LLVM_REPOSITORY is only really used by lld, clang and flang, it seems to create unnecessary friction with no easy workaround for other projects.

tuliom · 2024-09-17T17:58:53Z

@danilaml could you elaborate, please? What kind of friction was created by the merged commit?

danilaml · 2024-09-17T22:29:02Z

@tuliom the error is triggered in certain configs/machines we use but

we do not use anything that'd require LLVM_REPOSITORY define so those configs were not affected by the security issue anyway
using -DLLVM_FORCE_VC_REPOSITORY to override the error unsets LLVM_REVISION which is actually used by llvm.

tuliom · 2024-09-18T13:27:17Z

@danilaml I'm afraid I still don't fully understand the impact you're seeing, but let me try to suggest a solution anyway: do you think that setting -DLLVM_FORCE_VC_REPOSITORY=<url> -DLLVM_FORCE_VC_REVISION=<revision> helps?
Depending on what you're trying to achieve, you may only need -DLLVM_FORCE_VC_REVISION=<revision>.

danilaml · 2024-09-18T13:38:28Z

@tuliom setting them both (or just LLVM_FORCE_VC_REVISION) means we'd have to manually duplicate the logic the old cmake was already doing to get the revision (and hope it matches whatever is used on other machines).

llvmbot added the cmake Build system in general and CMake in particular label Aug 20, 2024

kbeyls requested review from tstellar and petrhosek August 21, 2024 07:53

petrhosek reviewed Aug 21, 2024

View reviewed changes

llvm/cmake/modules/VersionFromVCS.cmake Outdated Show resolved Hide resolved

petrhosek reviewed Aug 21, 2024

View reviewed changes

llvm/cmake/modules/VersionFromVCS.cmake Outdated Show resolved Hide resolved

SLTozer mentioned this pull request Aug 21, 2024

[Dexter] Sanitize user details from git repo URL in dexter --version #105533

Merged

fixup! Avoid exposing unknown git repositories

0d18a3c

fixup! Avoid exposing unknown git repositories

be6a044

tuliom changed the title ~~Avoid exposing unknown git repositories~~ Avoid exposing password and token from git repositories Sep 3, 2024

jyknight approved these changes Sep 4, 2024

View reviewed changes

llvm/cmake/modules/VersionFromVCS.cmake Outdated Show resolved Hide resolved

fixup! Avoid exposing unknown git repositories

20d6fe3

petrhosek reviewed Sep 5, 2024

View reviewed changes

llvm/cmake/modules/VersionFromVCS.cmake Outdated Show resolved Hide resolved

petrhosek approved these changes Sep 5, 2024

View reviewed changes

fixup! Avoid exposing unknown git repositories

0de9f98

tuliom merged commit 5904448 into llvm:main Sep 11, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Avoid exposing password and token from git repositories #105220

Avoid exposing password and token from git repositories #105220

Uh oh!

tuliom commented Aug 20, 2024 •

edited

Loading

Uh oh!

kbeyls commented Aug 21, 2024

Uh oh!

Uh oh!

Uh oh!

petrhosek commented Aug 21, 2024

Uh oh!

tuliom commented Aug 29, 2024

Uh oh!

joker-eph commented Aug 29, 2024

Uh oh!

tuliom commented Sep 3, 2024

Uh oh!

Uh oh!

Uh oh!

tuliom commented Sep 10, 2024

Uh oh!

Uh oh!

llvm-ci commented Sep 12, 2024

Uh oh!

danilaml commented Sep 16, 2024

Uh oh!

tuliom commented Sep 17, 2024

Uh oh!

danilaml commented Sep 17, 2024 •

edited

Loading

Uh oh!

tuliom commented Sep 18, 2024

Uh oh!

danilaml commented Sep 18, 2024

Uh oh!

Uh oh!

Avoid exposing password and token from git repositories #105220

Avoid exposing password and token from git repositories #105220

Uh oh!

Conversation

tuliom commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbeyls commented Aug 21, 2024

Uh oh!

Uh oh!

Uh oh!

petrhosek commented Aug 21, 2024

Uh oh!

tuliom commented Aug 29, 2024

Uh oh!

joker-eph commented Aug 29, 2024

Uh oh!

tuliom commented Sep 3, 2024

Uh oh!

Uh oh!

Uh oh!

tuliom commented Sep 10, 2024

Uh oh!

Uh oh!

llvm-ci commented Sep 12, 2024

Uh oh!

danilaml commented Sep 16, 2024

Uh oh!

tuliom commented Sep 17, 2024

Uh oh!

danilaml commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tuliom commented Sep 18, 2024

Uh oh!

danilaml commented Sep 18, 2024

Uh oh!

Uh oh!

tuliom commented Aug 20, 2024 •

edited

Loading

danilaml commented Sep 17, 2024 •

edited

Loading