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

Add SSH support for Embedded Ansible repositories #19108

Merged
merged 2 commits into from
Aug 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions app/models/git_repository.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class GitRepository < ApplicationRecord

attr_reader :git_lock

validates :url, :format => URI::regexp(%w(http https file)), :allow_nil => false
validates :url, :format => URI.regexp(%w[http https file ssh]), :allow_nil => false

default_value_for :verify_ssl, OpenSSL::SSL::VERIFY_PEER
validates :verify_ssl, :inclusion => {:in => [OpenSSL::SSL::VERIFY_NONE, OpenSSL::SSL::VERIFY_PEER]}
Expand Down Expand Up @@ -84,7 +84,7 @@ def update_repo
with_worktree do |worktree|
message = "Updating #{url} in #{directory_name}..."
_log.info(message)
worktree.send(:fetch_and_merge)
worktree.send(:pull)
Fryguy marked this conversation as resolved.
Show resolved Hide resolved
_log.info("#{message}...Complete")
end
@updated_repo = true
Expand Down Expand Up @@ -219,7 +219,12 @@ def worktree_params
params[:certificate_check] = method(:self_signed_cert_cb) if verify_ssl == OpenSSL::SSL::VERIFY_NONE
if (auth = authentication || default_authentication)
params[:username] = auth.userid
params[:password] = auth.password
if auth.auth_key
params[:ssh_private_key] = auth.auth_key
params[:password] = auth.auth_key_password.presence
else
params[:password] = auth.password
end
end
params
end
Expand Down
90 changes: 63 additions & 27 deletions lib/git_worktree.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,23 @@ def initialize(options = {})
require 'rugged'

raise ArgumentError, "Must specify path" unless options.key?(:path)
@path = options[:path]
@email = options[:email]
@username = options[:username] || ""
@bare = options[:bare]
@commit_sha = options[:commit_sha]
@password = options[:password] || ""
@fast_forward_merge = options[:ff] || true
@remote_name = 'origin'
@cred = Rugged::Credentials::UserPassword.new(:username => @username,
:password => @password)
@credentials_set = false
@base_name = File.basename(@path)
@path = options[:path]
@email = options[:email]
@username = options[:username]
@bare = options[:bare]
@commit_sha = options[:commit_sha]
@password = options[:password]
@ssh_private_key = options[:ssh_private_key]
@fast_forward_merge = options[:ff] || true
@certificate_check_cb = options[:certificate_check]

@remote_name = 'origin'
@base_name = File.basename(@path)
Copy link
Member

Choose a reason for hiding this comment

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

BUT WHY DIDN'T YOU ALIGN THESE WITH THE REST!!!1! :rage4:

Copy link
Member Author

Choose a reason for hiding this comment

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

That's why I put the blank line :) IMO, there first few are "extracting options" and the rest are "other" so they are logically separate groups.


if @ssh_private_key && !Rugged.features.include?(:ssh)
Copy link
Member Author

Choose a reason for hiding this comment

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

In our appliance builds I've already talked to @simaishi about getting SSH support for rugged and it's relatively simple. If a local developer doesn't compile rugged with ssh support this gives them a nice message.

FWIW, the command I used to build it locally on a Mac is:

brew install libssh2
PKG_CONFIG_PATH="$PKG_CONFIG_PATH:/usr/local/opt/openssl/lib/pkgconfig" gem install rugged -v 0.27.7

On Fedora/CentOS, the presence of libssh2 seems to be enough to get it to build properly.

We will have to document this somewhere in the dev setup. Even so, a "normal" installation of rugged will still work for user/pass over http(s)...just ssh won't work.

Copy link
Member

Choose a reason for hiding this comment

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

Reference to that change in the appliance:

ManageIQ/manageiq-appliance-build#336

raise GitWorktreeException::InvalidCredentialType, "ssh credentials are not enabled for use. Recompile the rugged/libgit2 gem with ssh support to enable it."
end

process_repo(options)
end

Expand Down Expand Up @@ -201,17 +205,47 @@ def checkout(target_directory)
@repo.checkout_tree(tree, :target_directory => target_directory, :strategy => :force)
end

def credentials_cb(url, _username, _types)
if @credentials_set
raise GitWorktreeException::InvalidCredentials, "Please provide username and password for URL #{url}" if @username.blank? || @password.blank?
raise GitWorktreeException::InvalidCredentials, "Invalid credentials for URL #{url}"
def with_credential_options
if @ssh_private_key
@ssh_private_key_file = Tempfile.new
@ssh_private_key_file.write(@ssh_private_key)
@ssh_private_key_file.close
end

options = {:credentials => method(:credentials_cb)}
options[:certificate_check] = @certificate_check_cb if @certificate_check_cb

yield options
ensure
if @ssh_private_key_file
@ssh_private_key_file.unlink
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I have a better way of doing this, but effectively for every action that requires this method, we are going to be creating and deleting the @ssh_private_key_file, correct? Might be a bit of extra trashing of the file system as a result.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but only for remote interactions, so it only happens once per invocation. I thought the same as you that it might thrash but I think we can deal with that later if it's really an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, kinda assumed. I am not going to "resolve this" as I do want it more visible for others in the future, but I think it can stay as is until it becomes a problem.

@ssh_private_key_file = nil
end
@credentials_set = true
@cred
end

private

def credentials_cb(url, username_from_url, _allowed_types)
username = @username || username_from_url

if @ssh_private_key_file
raise GitWorktreeException::InvalidCredentials, "Please provide username for URL #{url}" if username.blank?

Rugged::Credentials::SshKey.new(
:username => username,
:privatekey => @ssh_private_key_file.path,
:passphrase => @password.presence
)
else
raise GitWorktreeException::InvalidCredentials, "Please provide username and password for URL #{url}" if @username.blank? || @password.blank?

Rugged::Credentials::UserPassword.new(
:username => @username,
:password => @password
)
end
end

def current_branch
@repo.head_unborn? ? 'master' : @repo.head.name.sub(/^refs\/heads\//, '')
end
Expand All @@ -231,9 +265,9 @@ def fetch_and_merge
end

def fetch
@credentials_set = false
options = {:credentials => method(:credentials_cb), :certificate_check => @certificate_check_cb}
@repo.fetch(@remote_name, options)
with_credential_options do |cred_options|
@repo.fetch(@remote_name, cred_options)
end
end

def pull
Expand All @@ -246,8 +280,9 @@ def merge_and_push(commit)
@saved_cid = @repo.ref(local_ref).target.oid
merge(commit, rebase)
rebase = true
@credentials_set = false
@repo.push(@remote_name, [local_ref], :credentials => method(:credentials_cb))
with_credential_options do |cred_options|
@repo.push(@remote_name, [local_ref], cred_options)
end
end
end

Expand Down Expand Up @@ -300,9 +335,10 @@ def open_repo
end

def clone(url)
@credentials_set = false
options = {:credentials => method(:credentials_cb), :bare => true, :remote => @remote_name, :certificate_check => @certificate_check_cb}
@repo = Rugged::Repository.clone_at(url, @path, options)
@repo = with_credential_options do |cred_options|
options = cred_options.merge(:bare => true, :remote => @remote_name)
Rugged::Repository.clone_at(url, @path, options)
end
end

def fetch_entry(path)
Expand Down Expand Up @@ -356,7 +392,7 @@ def current_index
end

def create_commit(message, tree, parents)
author = {:email => @email, :name => @username, :time => Time.now}
author = {:email => @email, :name => @username || @email, :time => Time.now}
NickLaMuro marked this conversation as resolved.
Show resolved Hide resolved
# Create the actual commit but dont update the reference
Rugged::Commit.create(@repo, :author => author, :committer => author,
:message => message, :parents => parents,
Expand Down
1 change: 1 addition & 0 deletions lib/git_worktree_exception.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,5 @@ class BranchMissing < RuntimeError; end
class TagMissing < RuntimeError; end
class RefMissing < RuntimeError; end
class InvalidCredentials < RuntimeError; end
class InvalidCredentialType < RuntimeError; end
end
93 changes: 79 additions & 14 deletions spec/lib/git_worktree_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -324,27 +324,92 @@ def open_existing_repo
end
end

shared_examples_for '#credentials_cb' do |error_regex|
describe "#new" do
let(:git_repo_path) { Rails.root.join("spec", "fixtures", "git_repos", "branch_and_tag.git") }
let(:test_repo) { GitWorktree.new(:path => git_repo_path.to_s, :username => username, :password => password) }

it "call the credentials callback" do
expect(test_repo.credentials_cb("url", nil, nil).class).to eq(Rugged::Credentials::UserPassword)
expect { test_repo.credentials_cb("url", nil, nil) }.to raise_error(GitWorktreeException::InvalidCredentials, error_regex)
it "raises an exception if SSH requested, but rugged is not compiled with SSH support" do
require "rugged"
expect(Rugged).to receive(:features).and_return([:threads, :https])

expect {
GitWorktree.new(:path => git_repo_path, :ssh_private_key => "fake key\nfile content")
}.to raise_error(GitWorktreeException::InvalidCredentialType)
end
end

context "no username and password set" do
let(:username) { nil }
let(:password) { nil }
describe "#with_credential_options" do
let(:git_repo_path) { Rails.root.join("spec", "fixtures", "git_repos", "branch_and_tag.git") }

it_behaves_like '#credentials_cb', /provide username and password/
end
subject do
repo.with_credential_options do |cred_options|
cred_options[:credentials].call("url", nil, [])
end
end

describe "via plaintext" do
let(:repo) { described_class.new(:path => git_repo_path.to_s, :username => username, :password => password) }
let(:username) { "fred" }
let(:password) { "pa$$w0rd" }

it "with both username and password" do
expect(subject).to be_a Rugged::Credentials::UserPassword
end

context "with no username" do
let(:username) { nil }

it "raises an exception" do
expect { subject }.to raise_error(GitWorktreeException::InvalidCredentials, /provide username and password for/)
end
end

context "with no password" do
let(:password) { nil }

it "raises an exception" do
expect { subject }.to raise_error(GitWorktreeException::InvalidCredentials, /provide username and password for/)
end
end
end

describe "via SSH" do
let(:repo) { described_class.new(:path => git_repo_path.to_s, :username => username, :ssh_private_key => ssh_private_key, :password => password) }
let(:username) { "git" }
let(:ssh_private_key) { "fake key\nfile content" }
let(:password) { "pa$$w0rd" }

before do
require "rugged"
allow(Rugged).to receive(:features).and_return([:threads, :https, :ssh])
end

it "with username, ssh_private_key, and password" do
expect(subject).to be_a Rugged::Credentials::SshKey
end

context "with no username" do
let(:username) { nil }

context "bad username or password" do
let(:username) { 'fred' }
let(:password) { 'incorrect' }
it "raises an exception" do
expect { subject }.to raise_error(GitWorktreeException::InvalidCredentials, /provide username for/)
end
end

context "with no password" do
let(:password) { nil }

it "creates a password-less ssh key cred" do
expect(subject).to be_a Rugged::Credentials::SshKey
end
end

it_behaves_like '#credentials_cb', /Invalid credentials/
context "with no ssh_private_key" do
let(:ssh_private_key) { nil }

it "treats it like a user/pass" do
expect(subject).to be_a Rugged::Credentials::UserPassword
end
end
end
end
end
18 changes: 9 additions & 9 deletions spec/models/git_repository_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
repo.update_authentication(:default => {:userid => userid, :password => password})
expect(GitWorktree).to receive(:new).with(clone_args).and_return(gwt)
expect(GitWorktree).to receive(:new).with(args).and_return(gwt)
expect(gwt).to receive(:fetch_and_merge).with(no_args)
expect(gwt).to receive(:pull).with(no_args)

repo.refresh
expect(repo.default_authentication.userid).to eq(userid)
Expand All @@ -79,7 +79,7 @@
args[:certificate_check] = repo.method(:self_signed_cert_cb)
expect(GitWorktree).to receive(:new).with(clone_args).and_return(gwt)
expect(GitWorktree).to receive(:new).with(args).and_return(gwt)
expect(gwt).to receive(:fetch_and_merge).with(no_args)
expect(gwt).to receive(:pull).with(no_args)

repo.refresh
end
Expand All @@ -99,7 +99,7 @@

expect(repo).to receive(:clone_repo_if_missing).once.with(no_args).and_call_original
expect(GitWorktree).to receive(:new).with(anything).and_return(gwt)
expect(gwt).to receive(:fetch_and_merge).with(no_args)
expect(gwt).to receive(:pull).with(no_args)

repo.refresh
expect(repo.git_branches.collect(&:name)).to match_array(branch_list)
Expand All @@ -110,7 +110,7 @@
expect(GitWorktree).to receive(:new).twice.with(anything).and_return(gwt)
expect(gwt).to receive(:branches).with(anything).and_return(branch_list)
expect(gwt).to receive(:tags).with(no_args).and_return(tag_list)
expect(gwt).to receive(:fetch_and_merge).with(no_args)
expect(gwt).to receive(:pull).with(no_args)

allow(gwt).to receive(:branch_info) do |name|
branch_info_hash[name]
Expand All @@ -127,7 +127,7 @@
expect(GitWorktree).to receive(:new).twice.with(anything).and_return(gwt)
expect(gwt).to receive(:branches).with(anything).and_return(branch_list)
expect(gwt).to receive(:tags).with(no_args).and_return(tag_list)
expect(gwt).to receive(:fetch_and_merge).with(no_args)
expect(gwt).to receive(:pull).with(no_args)

allow(gwt).to receive(:branch_info) do |name|
branch_info_hash[name]
Expand All @@ -144,7 +144,7 @@
expect(GitWorktree).to receive(:new).twice.with(anything).and_return(gwt)
expect(gwt).to receive(:branches).with(anything).and_return(branch_list)
expect(gwt).to receive(:tags).with(no_args).and_return(tag_list)
expect(gwt).to receive(:fetch_and_merge).with(no_args)
expect(gwt).to receive(:pull).with(no_args)

allow(gwt).to receive(:branch_info) do |name|
branch_info_hash[name]
Expand All @@ -160,7 +160,7 @@
expect(GitWorktree).to receive(:new).twice.with(anything).and_return(gwt)
expect(gwt).to receive(:branches).with(anything).and_return(branch_list)
expect(gwt).to receive(:tags).with(no_args).and_return(tag_list)
expect(gwt).to receive(:fetch_and_merge).with(no_args)
expect(gwt).to receive(:pull).with(no_args)

allow(gwt).to receive(:branch_info) do |name|
branch_info_hash[name]
Expand All @@ -174,7 +174,7 @@

it "#refresh branches deleted" do
expect(GitWorktree).to receive(:new).twice.with(anything).and_return(gwt)
expect(gwt).to receive(:fetch_and_merge).twice.with(no_args)
expect(gwt).to receive(:pull).twice.with(no_args)
expect(gwt).to receive(:branches).twice.with(anything).and_return(branch_list)
expect(gwt).to receive(:tags).twice.with(no_args).and_return(tag_list)

Expand All @@ -194,7 +194,7 @@

it "#refresh tags deleted" do
expect(GitWorktree).to receive(:new).twice.with(anything).and_return(gwt)
expect(gwt).to receive(:fetch_and_merge).twice.with(no_args)
expect(gwt).to receive(:pull).twice.with(no_args)
expect(gwt).to receive(:branches).twice.with(anything).and_return(branch_list)
expect(gwt).to receive(:tags).twice.with(no_args).and_return(tag_list)

Expand Down