Fix(git): add warning of the mismatch of git cred and url #3136

FogDong · 2020-08-26T08:16:20Z

Changes

Fix(git): add warning of the mismatch of git cred and url
Fix #3094

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Fix(git): Tekton's credentials initialization now detects when an SSH credential is used with a non-SSH URL (and vice versa) in Git PipelineResources and will log a warning in Step containers.

tekton-robot · 2020-08-26T08:16:35Z

Hi @FogDong. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

withlin · 2020-08-26T09:02:36Z

/kind feature
/ok-to-test

FogDong · 2020-08-27T02:22:37Z

PTAL 😉 @sbwsg

pkg/git/git.go

FogDong · 2020-09-02T02:43:35Z

Fixed. 😆 Please review @sbwsg

FogDong · 2020-09-07T07:48:29Z

Can anybody take a look at this PR? 🤔

/cc @vdemeester

pkg/git/git.go

ghost · 2020-09-08T18:40:35Z

pkg/git/git.go

+func validateGitAuth(logger *zap.SugaredLogger, url string) error {
+	homeenv := os.Getenv("HOME")
+	sshCred := true
+	if _, err := os.Stat(homeenv + "/.ssh"); os.IsNotExist(err) {


Thinking about this some more I'm not sure this is enough to correctly validate SSH credentials. A user can mix SSH and non-SSH git credentials in a single Step. For example, with two Secrets:

apiVersion: v1 kind: Secret metadata: name: example-gitlab-creds annotations: tekton.dev/git-0: https://gitlab.com type: kubernetes.io/basic-auth stringData: username: myuser password: hunter2 --- apiVersion: v1 kind: Secret metadata: name: example-github-creds annotations: tekton.dev/git-0: github.com type: kubernetes.io/ssh-auth stringData: ssh-privatekey: blahblahblah --- apiVersion: v1 kind: ServiceAccount metadata: name: example-service-account secrets: - name: example-gitlab-creds - name: example-github-creds --- apiVersion: tekton.dev/v1beta1 kind: TaskRun metadata: name: foobar spec: serviceAccountName: example-service-account taskSpec: steps: - name: wait image: alpine:latest script: | echo "foo"

In this example YAML both a ~/.ssh directory and a ~/.git-credentials file will exist side by side. ~/.git-credentials is created for the gitlab Secret. So this code will print a warning because the gitlab basic auth URL is not SSH-formatted. But the user never intends to use SSH auth with a gitlab URL.

The full solution to this problem is probably quite a lot more complicated - it would need to read the ~/.ssh/config file and figure out whether the given url is intended to be used with SSH authentication.

We could work around this somewhat by updating the warning messages to be more relaxed. Something like:

logger.Warnf("SSH credentials have been provided but the URL %q is not a valid SSH URL", url)

But this feels like it would lead to more user confusion and I don't think this is a great workaround either.

I met the similar problem when I tested the code: if the given URL is a public repo and the cred is SSH type, the warning log will be shown and the pod can still pull the code successfully.
But it will be more too complex if we check the URL is public or not. So my point is we can just let the warning show since it's only a warning and people will not care about the warning if the job success. 🤔 WDYT?

Hrm, tricky problem! OK, I think we can make this work with an even longer log message:

SSH credentials have been provided but the URL %q is not a valid SSH URL. This warning can be safely ignored if the URL is for a public repo or you are using basic auth

@vdemeester what do you think of this? Is it too vague? The other thing we could do is drop this message and only print a warning if a URL is definitely an SSH url but the user has 0 SSH credentials available in the Step. That is 100% of the time a legitimate error I think?

I'll change the message first but I'm also not quite sure that if we need that message when SSH credentials are provided but the URL is non-SSH. Let me know if there is better solutions!

pkg/git/git.go

tekton-robot · 2020-09-09T03:59:35Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/git/git.go	Do not exist	2.4%

tekton-robot · 2020-09-09T04:02:17Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/git/git.go	Do not exist	2.4%

tekton-robot · 2020-09-09T06:51:15Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/git/git.go	Do not exist	2.4%

tekton-robot · 2020-09-09T08:09:05Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/git/git.go	Do not exist	2.4%

ghost

Sorry, I've left more comments. I'm not totally sure if we want the warning message when SSH credentials are provided but the URL is non-SSH. Might be too vague? Curious what others think about this!

pkg/git/git.go

pkg/git/git_test.go

ghost · 2020-09-10T19:33:21Z

pkg/git/git.go

+func validateGitAuth(logger *zap.SugaredLogger, url string) error {
+	homeenv := os.Getenv("HOME")
+	sshCred := true
+	if _, err := os.Stat(homeenv + "/.ssh"); os.IsNotExist(err) {


Hrm, tricky problem! OK, I think we can make this work with an even longer log message:

SSH credentials have been provided but the URL %q is not a valid SSH URL. This warning can be safely ignored if the URL is for a public repo or you are using basic auth

@vdemeester what do you think of this? Is it too vague? The other thing we could do is drop this message and only print a warning if a URL is definitely an SSH url but the user has 0 SSH credentials available in the Step. That is 100% of the time a legitimate error I think?

tekton-robot · 2020-09-11T02:43:09Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/git/git.go	Do not exist	2.4%

tekton-robot · 2020-09-11T02:56:44Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/git/git.go	Do not exist	2.4%

FogDong · 2020-09-11T03:55:58Z

/retest

tekton-robot · 2020-09-12T17:44:24Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/git/git.go	Do not exist	2.4%

ghost

Thanks @FogDong !

FogDong · 2020-09-16T11:04:06Z

Can I get a lgtm? /cc @vdemeester 😉

withlin

/lgtm

tekton-robot · 2020-09-22T07:31:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg, withlin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sbwsg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bobcatfish · 2020-09-23T14:27:58Z

@FogDong @sbwsg can you update the release note with more information about what this change is doing?

ghost · 2020-09-23T15:00:45Z

@bobcatfish sure thing; updated!

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 26, 2020

tekton-robot requested review from imjasonh and a user August 26, 2020 08:16

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 26, 2020

tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 26, 2020

ghost reviewed Aug 28, 2020

View reviewed changes

pkg/git/git.go Outdated Show resolved Hide resolved

pkg/git/git.go Outdated Show resolved Hide resolved

FogDong force-pushed the git branch 2 times, most recently from 684e542 to d99c607 Compare August 31, 2020 02:59

tekton-robot requested a review from vdemeester September 7, 2020 07:48

vdemeester reviewed Sep 7, 2020

View reviewed changes

pkg/git/git.go Outdated Show resolved Hide resolved

pkg/git/git.go Outdated Show resolved Hide resolved

FogDong force-pushed the git branch from d99c607 to 92a33f4 Compare September 8, 2020 03:14

ghost reviewed Sep 8, 2020

View reviewed changes

FogDong force-pushed the git branch from 92a33f4 to 26abd78 Compare September 9, 2020 03:53

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 9, 2020

FogDong force-pushed the git branch from 26abd78 to 033b893 Compare September 9, 2020 03:57

FogDong force-pushed the git branch from 033b893 to 6a7ffe1 Compare September 9, 2020 06:48

FogDong force-pushed the git branch from 6a7ffe1 to 8c01fe7 Compare September 9, 2020 08:04

ghost reviewed Sep 10, 2020

View reviewed changes

FogDong force-pushed the git branch from 8c01fe7 to d2b8534 Compare September 11, 2020 02:38

FogDong force-pushed the git branch from d2b8534 to 7fb4108 Compare September 11, 2020 02:51

Fix(git): add warning of the mismatch of git cred and url and tests

bc518e2

FogDong force-pushed the git branch from 7fb4108 to bc518e2 Compare September 12, 2020 17:40

ghost approved these changes Sep 14, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2020

withlin approved these changes Sep 22, 2020

View reviewed changes

tekton-robot assigned withlin Sep 22, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2020

tekton-robot merged commit 53f761f into tektoncd:master Sep 22, 2020

FogDong mentioned this pull request Sep 22, 2020

Add FogDong to the org tektoncd/community#214

Merged

vdemeester mentioned this pull request Nov 2, 2020

ssh cred validation is wrong with disable-home-env=true #3481

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix(git): add warning of the mismatch of git cred and url #3136

Fix(git): add warning of the mismatch of git cred and url #3136

FogDong commented Aug 26, 2020 •

edited by ghost

Loading

tekton-robot commented Aug 26, 2020

withlin commented Aug 26, 2020

FogDong commented Aug 27, 2020

FogDong commented Sep 2, 2020

FogDong commented Sep 7, 2020

ghost Sep 8, 2020

FogDong Sep 9, 2020

ghost Sep 10, 2020

FogDong Sep 11, 2020

tekton-robot commented Sep 9, 2020

tekton-robot commented Sep 9, 2020

tekton-robot commented Sep 9, 2020

tekton-robot commented Sep 9, 2020

ghost left a comment

ghost Sep 10, 2020

tekton-robot commented Sep 11, 2020

tekton-robot commented Sep 11, 2020

FogDong commented Sep 11, 2020

tekton-robot commented Sep 12, 2020

ghost left a comment

FogDong commented Sep 16, 2020

withlin left a comment

tekton-robot commented Sep 22, 2020

bobcatfish commented Sep 23, 2020

ghost commented Sep 23, 2020

Fix(git): add warning of the mismatch of git cred and url #3136

Fix(git): add warning of the mismatch of git cred and url #3136

Conversation

FogDong commented Aug 26, 2020 • edited by ghost Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Aug 26, 2020

withlin commented Aug 26, 2020

FogDong commented Aug 27, 2020

FogDong commented Sep 2, 2020

FogDong commented Sep 7, 2020

ghost Sep 8, 2020

Choose a reason for hiding this comment

FogDong Sep 9, 2020

Choose a reason for hiding this comment

ghost Sep 10, 2020

Choose a reason for hiding this comment

FogDong Sep 11, 2020

Choose a reason for hiding this comment

tekton-robot commented Sep 9, 2020

tekton-robot commented Sep 9, 2020

tekton-robot commented Sep 9, 2020

tekton-robot commented Sep 9, 2020

ghost left a comment

Choose a reason for hiding this comment

ghost Sep 10, 2020

Choose a reason for hiding this comment

tekton-robot commented Sep 11, 2020

tekton-robot commented Sep 11, 2020

FogDong commented Sep 11, 2020

tekton-robot commented Sep 12, 2020

ghost left a comment

Choose a reason for hiding this comment

FogDong commented Sep 16, 2020

withlin left a comment

Choose a reason for hiding this comment

tekton-robot commented Sep 22, 2020

bobcatfish commented Sep 23, 2020

ghost commented Sep 23, 2020

FogDong commented Aug 26, 2020 •

edited by ghost

Loading