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

Fix generating SSH key fingerprints with OpenSSH 6.8. #9008

Closed
wants to merge 3 commits into
base: master
from

Conversation

6 participants
@sstanovnik
Copy link
Contributor

sstanovnik commented Mar 23, 2015

OpenSSH 6.8 introduces a new feature that changes the default fingerprint format and algorithm used by ssh-keygen. This breaks adding new SSH keys, because GitLab expects the colon-delimited format.

The message the user sees on the Add an SSH Key screen is "Fingerprint cannot be generated", similar to #7413, but the underlying cause is different.

This change checks the OpenSSH version and explicitly specifies the previous format if needed.

@Razer6

This comment has been minimized.

Copy link
Member

Razer6 commented Mar 24, 2015

@jacobvosmaer Can you take a look?

@sstanovnik

This comment has been minimized.

Copy link
Contributor

sstanovnik commented Mar 24, 2015

The new format has "MD5:" prepended which parsed successfully but incorrectly, preventing users from using the SSH key, so I fixed that too.

@jacobvosmaer

This comment has been minimized.

Copy link
Member

jacobvosmaer commented Mar 24, 2015

Thanks for looking into this @sstanovnik

end

if cmd_status.zero?
cmd_output.gsub /(\h{2}:)+\h{2}/ do |match|
self.fingerprint = match

This comment has been minimized.

@jacobvosmaer

jacobvosmaer Mar 24, 2015

Member

This regex code looks a little wonky to me. I wonder if we can just pick out the MD5 fingerprint with something like:

# constant should be defined somewhere at the top of the class
FINGERPRINT_REGEX = Regexp.new('\h{2}:' * 15 + '\h{2}') # 16 hex bytes separated by ':'
self.fingerprint = cmd_output.scan(FINGERPRINT_REGEX).first

This comment has been minimized.

@jacobvosmaer

jacobvosmaer Mar 24, 2015

Member

On the other hand, anchoring against the MD5 prefix seems like a nice defensive idea.

@jacobvosmaer

This comment has been minimized.

Copy link
Member

jacobvosmaer commented Mar 24, 2015

I like the basic idea but I am not 100% sure about the code style. What do you think @vsizov ?

@vsizov

This comment has been minimized.

Copy link
Contributor

vsizov commented Mar 24, 2015

@jacobvosmaer It looks good for me.

@Razer6

This comment has been minimized.

Copy link
Member

Razer6 commented Mar 24, 2015

Already first issue reported related to this https://gitlab.com/gitlab-org/gitlab-ce/issues/1289

@Razer6

This comment has been minimized.

Copy link
Member

Razer6 commented Apr 11, 2015

@vsizov @jacobvosmaer Any updates?

@jacobvosmaer

This comment has been minimized.

Copy link
Member

jacobvosmaer commented Apr 13, 2015

@Razer6: @DouweM will have a look at this.

@DouweM

This comment has been minimized.

Copy link
Member

DouweM commented Apr 14, 2015

I've cleaned the code up somewhat and posted a new MR to gitlab.com: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/519

Thanks for your contribution @sstanovnik, I've credited you in the changelog.

cc @jacobvosmaer

@DouweM DouweM closed this Apr 14, 2015

dzaporozhets added a commit that referenced this pull request Apr 15, 2015

Merge branch 'sstanovnik-openssh_fix' into 'master'
Fix generating SSH key fingerprints with OpenSSH 6.8.

Replaces #9008.

Fixes gitlab-org/gitlab-ce#1289.

cc @jacobvosmaer

See merge request !519
@kristianlm

This comment has been minimized.

Copy link

kristianlm commented Oct 28, 2015

In case my quickfix is useful to anyone else:

    Tempfile.open('gitlab_key_file') do |file|
      file.puts key
      file.rewind
      cmd_output, cmd_status = popen(%W(ssh-keygen -E md5 -lf #{file.path}), '/tmp')
    end

    if cmd_status.zero?
     puts "cmd_status was zero"
      cmd_output.match /MD5:(([\d\h]{2}:)+[\d\h]{2})/ do |match|
        self.fingerprint = match[1]
      end
    end
@jacobvosmaer

This comment has been minimized.

Copy link
Member

jacobvosmaer commented Oct 28, 2015

This got fixed in GitLab 7.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment