Skip to content

Conversation

@cydu-cloud
Copy link
Contributor

@cydu-cloud cydu-cloud commented Dec 18, 2019

For cache to store change:

  • By default, cache only last 900 seconds, gitsync will break after
    that for password setup. See https://git-scm.com/docs/git-credential-cache.
  • The test won't work with cache since the test don't have access to
    the default unix socket location; XDG_CACHE_HOME override also cann't
    pre-create a socket in advance.
  • store put the credential into a file, much easier to debug than cache.
  • Considering anyone have access to the pod already able to get the
    credential via environment variables or yaml configs, so put it in
    file won't make it less secure.

For the new password test:

  • askpass_git.sh provided to simulate a git with password challenge.
  • Need and only need to challenge for "clone" action. Need to bypass all the other
    actions like config/credential setup.
  • See credential fill is the official git action to ask password,
    see https://git-scm.com/docs/git-credential.

Fixes #196.

username/password case.

For cache to store change:
* By default, cache only last 900 seconds, gitsync will break after
  that. See https://git-scm.com/docs/git-credential-cache.
* The test won't work with cache since the test don't have access to
  the default unix socket location; XDG_CACHE_HOME override also can
  pre-create a socket in advance.
* `store` put the credential into a file, much easier to debug than cache.
* Considering anyone have access to the pod already able to get the
  credential via environment variables or yaml configs, so put it in
  file won't make it less secure.

For the new password test:
1. askpass_git.sh provided to simulate a git with password challenge.
2. Need and only need to similate "clone" action, need to bypass other
  actions like config/credential setup.
3. See `credential fill` is the official git action to ask password,
  see https://git-scm.com/docs/git-credential.

This change resolved issue kubernetes#196.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 18, 2019
@cydu-cloud
Copy link
Contributor Author

/assign @thockin

cydu-cloud added a commit to cydu-cloud/git-sync that referenced this pull request Dec 19, 2019
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 19, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 19, 2019
@k8s-ci-robot k8s-ci-robot merged commit 5911997 into kubernetes:master Dec 19, 2019
@cydu-cloud cydu-cloud deleted the passwordtest branch December 19, 2019 06:08
@thockin
Copy link
Member

thockin commented Dec 19, 2019 via email

@cydu-cloud
Copy link
Contributor Author

Yeah, that’s possible in theory,
I can file a follow up PR to add the username check if you have strong opinion about it.

@thockin
Copy link
Member

thockin commented Dec 19, 2019 via email

@cydu-cloud cydu-cloud restored the passwordtest branch December 20, 2019 00:32
@cydu-cloud cydu-cloud deleted the passwordtest branch December 20, 2019 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Need a test for username/password

3 participants