Always run as given user, even if identity set #473

bigpresh · 2020-11-18T14:44:01Z

Previously, if the resource specified user, then git would be executed as that user... except if you also provided identity to control which SSH key to use, in which case the code did the setup required for the git helper script, then called git directly (bypassing the bit that ran it as the desired user).

This changes it by wrapping up the execution of git in a new method which contains the logic to run as the specified user if they asked for that, and uses that wrapper from both places in git_with_identity.

bigpreshkt · 2020-11-18T17:16:44Z

Ah - one more fix needed for this; the fact that git is now actually running as the requested user, even if you used 'identity, now means it'll fail because thegit-helper script is written into Puppet[:statedir] which the user running git probably can't access.

I suspect the solution is just to let Tempfile write the file in the OS's temp directory - although it would probably need to chown the file to the user who we're going to run git as, otherwise they still won't be able to read it...

EDIT: actually, it already gets chmod'd to 755, meaning other users can read it... but not execute it, which is what git needs to do. Changing that to 777 seems reasonable, as already readable... but it just occurred to me that if it is placed in the system temp dir, that may not work as the temp dir may well be mounted with noexec. Hmm.

Perhaps the whole "write a shell script to wrap SSH" could just be done away with in favour of passing options directly, e.g.

git ... -c core.sshCommand="/usr/bin/ssh -i identity" git@github.com:me/repo.git

(As an aside, looking at the file that's written, I find it a little alarming that requesting a specific SSH key be used also disables StrictHostKeyChecking without having been asked to do that... is that intentional and documented?)

carabasdaniel · 2020-11-23T11:17:02Z

Hi @bigpreshkt,

This change seems really helpful and we would love to get this merged as soon as the test are green.

As you mentioned in the second comment I believe that having StrictHostKeyChecking disabled might have been the initial workaround when this functionality was implemented, however addressing this issue is really important and we would definitely appreciate your fix here.

Please let us know if you need a bit of help getting this fix over the line.

Thanks!

bigpresh · 2020-11-23T12:10:39Z

I've made stylistic tweaks to please the linter; if it passes now, excellent. If there are test updates needed, I may need a little help getting it over the line, as I'm not a Ruby coder (I know just about enough Ruby to blunder my way through - I'm a Perl guy at heart) and don't have a huge amount of time available.

As an aside, some of the complaints I, personally, think are a bit weird - e.g.:

lib/puppet/provider/vcsrepo/git.rb:610:5: C: Style/RedundantReturn: Redundant return detected. (https://github.com/bbatsov/ruby-style-guide#no-explicit-return)

    return Puppet::Util::Execution.execute([:git, args], **exec_args)

    ^^^^^^

You could argue it's redundant at present, because it'll be implicitly returned, but (a) making it explicitly clear what you intend to return is a good thing, and (b) it protects someone who comes along in future to change that code, and accidentally changes what's returned by adding code after that line. That said - if that's what the style guide for the project says, then so be it.

codecov-io · 2020-11-23T12:22:06Z

Codecov Report

Merging #473 (f34fa8f) into main (7ba3a5e) will increase coverage by 0.15%.
The diff coverage is 37.93%.

@@            Coverage Diff             @@
##             main     #473      +/-   ##
==========================================
+ Coverage   72.88%   73.03%   +0.15%     
==========================================
  Files          10       10              
  Lines        1088     1083       -5     
==========================================
- Hits          793      791       -2     
+ Misses        295      292       -3

Impacted Files	Coverage Δ
lib/puppet/provider/vcsrepo/git.rb	`74.21% <37.93%> (+0.58%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ba3a5e...f34fa8f. Read the comment docs.

bigpresh · 2020-11-23T12:26:58Z

Alright, help with the coverage fail would be welcomed - I can't see any reason that the changed code wouldn't be covered sufficiently by existing tests.

sanfrancrisko · 2020-12-07T14:31:39Z

Hi @bigpresh - we're looking in to some of the test failures on this PR and will look to address and try and provide some additional coverage, if required. Sorry for the delay - it's on our list to get to. Thanks again for the fix - we'll get it merged soon 👍

carabasdaniel · 2021-02-08T15:57:25Z

Hi @bigpreshkt,
I've push a commit to your branch with the fixes for the unit tests, can you please update this PR to reflect those changes and see if we can get these changes merged.

Thanks

bigpresh · 2021-02-08T16:15:39Z

I've push a commit to your branch with the fixes for the unit tests, can you please update this PR to reflect those changes and see if we can get these changes merged.

Took me a moment to locate, but I see it, 41ac50a ... just figuring out how to get GitHub to incorporate it in this PR, as it appears to be already pushed to the branch...

bigpreshkt · 2021-02-08T20:40:39Z

Right, I have updated my main branch from upstream, then interactive-rebased this branch onto it, squashing a couple of pointless commits in the process. Your fixes to the tests (thanks!) is now here, but there's a conflict - not sure how, given that I rebased my changes onto main.... hmm.

bigpresh · 2021-02-08T20:56:02Z

OK, resolved the conflict (I'd not correctly updated main from upstream, I'd updated master) - it should be all good now.

bigpresh · 2021-02-08T21:32:51Z

And now, failing with key verification errors, e.g.:

Error: /Stage[main]/Main/Vcsrepo[/tmp/vcsrepo/testrepo_user_ssh_id]/ensure: change from 'absent' to 'present' failed: Execution of 'git clone testuser-ssh@localhost:/tmp/vcsrepo/testrepo.git /tmp/vcsrepo/testrepo_user_ssh_id' returned 128: Cloning into '/tmp/vcsrepo/testrepo_user_ssh_id'...
       Host key verification failed.
       fatal: Could not read from remote repository.
       
       Please make sure you have the correct access rights
       and the repository exists.

I'd hazard a guess that this was masked by the previous "disable SSH host key checking if we're using an identity even if we weren't asked to" code, which no longer exists.

Previously, if the resource specified `user`, then git would be executed as that user... except if you also provided `identity` to control which SSH key to use, in which case the code did the setup required for the git helper script, then called git directly (bypassing the bit that ran it as the desired user). This changes it by wrapping up the execution of git in a new method which contains the logic to run as the specified user if they asked for that, and uses that wrapper from both places in git_with_identity.

If you use `identity`, at present a shell script to wrap `ssh` was written out (to the Puppet state dir) then git told to use it by pointing the GIT_SSH env var at it. That failed after my changes to run git as the user requested, because the user probably doesn't have access to the script in the Puppet state dir. The script is unnecessary IMO, anyway - setting the options it sets can just as easily be done by `-o` options in the `GIT_SSH_COMMAND` env var, which is what I've done here. Note that I have intentionally not included the disabling of StrictHostKeyChecking which was present in the wrapper script, as I do not think that intentionally reducing someone's security without being asked to do so is a good idea. Similarly I left out setting the timeout, as that isn't related to being asked to use a given identity key. (It may well be that there should be a new `ssh_opts` attribute you can set on a Vcsrepo resource to pass options along, perhaps, but I think that's outside the scope of what I'm trying to fix here.)

I consider this far clearer with the return, but the linter doesn't like it.

carabasdaniel · 2021-02-22T10:20:14Z

Hi @bigpresh,
I've added a commit last week to fix the failing acceptance test and rebased the PR on latest main now.

I think I might need to do a bit of a work-around for the cloning test for some permission issues, but it should be closer to getting it merged.

Thank you for your patience on getting this fix in.

bigpresh · 2021-02-22T11:46:16Z

Many thanks for the update @carabasdaniel, appreciate it!

I suspect the test is going to need to add the server's host key to ~/.ssh/known_hosts before the test clone to avoid:

       Error: Execution of 'git clone root@localhost:/tmp/vcsrepo/testrepo.git /tmp/vcsrepo/testrepo_user_ssh_id' returned 128: Cloning into '/tmp/vcsrepo/testrepo_user_ssh_id'...
       Host key verification failed.
       fatal: Could not read from remote repository.

Maybe something like:

run_shell("ssh-keyscan localhost > #{homedir}/.ssh/known_hosts")

bigpresh · 2021-02-22T20:24:16Z

spec/acceptance/clone_repo_spec.rb

@@ -479,7 +479,7 @@
      run_shell("mkdir -p #{homedir}/.ssh")
      run_shell("ssh-keygen -q -t rsa -f #{homedir}/.ssh/id_rsa -N ''")

-      run_shell('ssh-keygen -R localhost', expect_failures: true)
+      run_shell('rm ${homedir}/.ssh/known_hosts', expect_failures: true)
      run_shell("ssh-keyscan localhost >> #{homedir}/.ssh/known_hosts")


Firstly - could avoid the rm above by just blatting the file with > instead of >>.

Secondly, #{homedir} here - will that be the homedir of the "target" user whose repo we're trying to check out, or that of the user running the Puppet code (root?)? It wants to be the second, not the first.

(EDIT: if it is the latter, then that's my mistake earlier - sorry!)

I think this remaining test failure is a confusion over which user it's SSHing as vs which user had the host keys written to, etc. I think the run_shell() commands which write out a .ssh/config with StrictHostKeyChecking can be removed from the tests here too, since the host key *should* be right, but let's see first if this gets the test to pass before looking at that. (If this is wrong, feel free to revert!)

Repeat the setup of the testssh-user here for this test too. Copying and pasting this feels very wrong; it should probably be refactored out into a method that is called by both of the "check out over SSH" tests, or the two tests turned into subtests in the same context?

Actually run the clone as testuser-ssh after preparing their SSH known hosts etc.

bigpresh · 2021-02-22T22:50:35Z

That should be all the tests passing, although I'm not very happy with the copy & paste cargo-cult way I did it, I'm sure the repetition could be refactored out!

CLAassistant · 2021-02-24T16:59:25Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ Daniel Carabas
❌ bigpreshkt

Daniel Carabas seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

david22swan

LGTM

david22swan · 2021-03-08T14:19:53Z

@bigpresh Thank you for your contribution, this look's good to me so Ive gone ahead and merged.
Thank's for putting in the effort and we're looking forward to seeing more from you in the future.

bastelfreak · 2021-05-27T14:00:11Z

hey ho. Is is possible that this is a backwards-incompatible change? I didn't look in detail but there's some discussion in https://puppetcommunity.slack.com/archives/C11LCKKQ9/p1622029307033000 and it looks like this change broke a user installation with the vcsrepo upgrade 4.0.0->4.0.1

bigpresh · 2021-05-28T22:24:52Z

hey ho. Is is possible that this is a backwards-incompatible change? I didn't look in detail but there's some discussion in https://puppetcommunity.slack.com/archives/C11LCKKQ9/p1622029307033000 and it looks like this change broke a user installation with the vcsrepo upgrade 4.0.0->4.0.1

Can't see the discussion as I'm not part of the Puppet Slack thingy, but this PR does stop StrictHostKeyChecking being disabled without you asking/wanting that - it's possible that they were connecting somewhere that succeeded only because setting an identity to use had the unexpected and unrequested side effect of turning off StrictHostKeyChecking.

EDIT: commit 98cb96f is the change I refer to there - the code used to write out a shell script wrapper to call SSH, which would turn off StrictHostKeyChecking even though you never asked for that. It was replaced with a cleaner direct call to SSH, and the unrequested lowering of the user's security removed.

bigpresh requested a review from a team as a code owner November 18, 2020 14:44

bigpresh force-pushed the bigpresh/git_as_specified_uid_even_with_identity branch from 9f8e9d9 to 5e07b2a Compare November 18, 2020 14:48

carabasdaniel added the bugfix label Nov 23, 2020

pmcmaw self-assigned this Nov 30, 2020

pmcmaw removed their assignment Dec 9, 2020

bigpresh force-pushed the bigpresh/git_as_specified_uid_even_with_identity branch from 41ac50a to b057de4 Compare February 8, 2021 20:39

bigpresh force-pushed the bigpresh/git_as_specified_uid_even_with_identity branch from b057de4 to c32b3da Compare February 8, 2021 20:54

bigpreshkt and others added 5 commits February 22, 2021 12:16

Fix unit tests

eaad8d3

Remove return to satisfy linter

7c7961f

I consider this far clearer with the return, but the linter doesn't like it.

Fix git clone repo acceptance tests

7577b70

carabasdaniel force-pushed the bigpresh/git_as_specified_uid_even_with_identity branch from 545e229 to 7577b70 Compare February 22, 2021 10:16

Add IgnoreUnknown for IdentityAgent ssh configuration in git

b6fc0aa

carabasdaniel force-pushed the bigpresh/git_as_specified_uid_even_with_identity branch from be53731 to 0779fe3 Compare February 22, 2021 14:18

Add localhost to known_hosts

12aa1dc

carabasdaniel force-pushed the bigpresh/git_as_specified_uid_even_with_identity branch from 0779fe3 to 12aa1dc Compare February 22, 2021 15:11

Remove existing known_hosts configuration in acceptance test

f34fa8f

bigpresh commented Feb 22, 2021

View reviewed changes

bigpreshkt added 4 commits February 22, 2021 20:31

Run test with identity as testuser-ssh

f3cdeb0

Actually run the clone as testuser-ssh after preparing their SSH known hosts etc.

Remove unused var

e4454f9

david22swan approved these changes Mar 8, 2021

View reviewed changes

david22swan merged commit e19bc76 into puppetlabs:main Mar 8, 2021

david22swan added backwards-incompatible and removed bugfix labels Jun 2, 2021

pcworld mentioned this pull request Jun 3, 2021

Release prep v5.0.0 #516

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always run as given user, even if identity set #473

Always run as given user, even if identity set #473

bigpresh commented Nov 18, 2020

bigpreshkt commented Nov 18, 2020 •

edited

Loading

carabasdaniel commented Nov 23, 2020

bigpresh commented Nov 23, 2020

codecov-io commented Nov 23, 2020 •

edited

Loading

bigpresh commented Nov 23, 2020

sanfrancrisko commented Dec 7, 2020

carabasdaniel commented Feb 8, 2021 •

edited

Loading

bigpresh commented Feb 8, 2021

bigpreshkt commented Feb 8, 2021

bigpresh commented Feb 8, 2021

bigpresh commented Feb 8, 2021

carabasdaniel commented Feb 22, 2021

bigpresh commented Feb 22, 2021

bigpresh Feb 22, 2021 •

edited

Loading

bigpresh commented Feb 22, 2021

CLAassistant commented Feb 24, 2021 •

edited

Loading

david22swan left a comment

david22swan commented Mar 8, 2021

bastelfreak commented May 27, 2021

bigpresh commented May 28, 2021 •

edited

Loading

Always run as given user, even if identity set #473

Always run as given user, even if identity set #473

Conversation

bigpresh commented Nov 18, 2020

bigpreshkt commented Nov 18, 2020 • edited Loading

carabasdaniel commented Nov 23, 2020

bigpresh commented Nov 23, 2020

codecov-io commented Nov 23, 2020 • edited Loading

Codecov Report

bigpresh commented Nov 23, 2020

sanfrancrisko commented Dec 7, 2020

carabasdaniel commented Feb 8, 2021 • edited Loading

bigpresh commented Feb 8, 2021

bigpreshkt commented Feb 8, 2021

bigpresh commented Feb 8, 2021

bigpresh commented Feb 8, 2021

carabasdaniel commented Feb 22, 2021

bigpresh commented Feb 22, 2021

bigpresh Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

bigpresh commented Feb 22, 2021

CLAassistant commented Feb 24, 2021 • edited Loading

david22swan left a comment

Choose a reason for hiding this comment

david22swan commented Mar 8, 2021

bastelfreak commented May 27, 2021

bigpresh commented May 28, 2021 • edited Loading

bigpreshkt commented Nov 18, 2020 •

edited

Loading

codecov-io commented Nov 23, 2020 •

edited

Loading

carabasdaniel commented Feb 8, 2021 •

edited

Loading

bigpresh Feb 22, 2021 •

edited

Loading

CLAassistant commented Feb 24, 2021 •

edited

Loading

bigpresh commented May 28, 2021 •

edited

Loading