Commit afeaa2e7 authored by Nick Thomas's avatar Nick Thomas

Don't take fingerprints for the internal authorized_keys API

gitlab-shell-authorized-keys does a GET request against the internal
API to learn whether a given SSH key is known to GitLab and, if so,
what user it is attached to. This is used to validate SSH sessions.

For a very long time, gitlab-shell has sent only the full base64
encoded public key, from which we derive a fingerprint for looking up
the key on the GitLab side. However, the API does have legacy support
for gitlab-shell sending just the key fingerprint. It has not been used
in gitlab-shell since March 2016.

We want to update GitLab so it derives the SHA256 fingerprint for the
search, rather than the MD5 fingerpring. Doing this is made easier if
we remove the `fingerprint` parameter and only accept `key` in the
API endpoint.

This is safe to do, because `fingerprint` is unused, and does not break
any compatibility guarantees since the internal API is not bound by the
v4 REST API compatibility promise.

The documentation does not refer to the `fingerprint` parameter at all,
and doing this in 14.0 should means that we don't have to think about
zero-downtime compatibility either, although that should also be fine.

Changelog: removed
parent 95667afb
...@@ -167,18 +167,15 @@ module API ...@@ -167,18 +167,15 @@ module API
end end
# #
# Get a ssh key using the fingerprint # Check whether an SSH key is known to GitLab
# #
# rubocop: disable CodeReuse/ActiveRecord
get '/authorized_keys', feature_category: :source_code_management do get '/authorized_keys', feature_category: :source_code_management do
fingerprint = params.fetch(:fingerprint) do fingerprint = Gitlab::InsecureKeyFingerprint.new(params.fetch(:key)).fingerprint
Gitlab::InsecureKeyFingerprint.new(params.fetch(:key)).fingerprint
end key = Key.find_by_fingerprint(fingerprint)
key = Key.find_by(fingerprint: fingerprint)
not_found!('Key') if key.nil? not_found!('Key') if key.nil?
present key, with: Entities::SSHKey present key, with: Entities::SSHKey
end end
# rubocop: enable CodeReuse/ActiveRecord
# #
# Discover user by ssh key, user id or username # Discover user by ssh key, user id or username
......
...@@ -341,40 +341,6 @@ RSpec.describe API::Internal::Base do ...@@ -341,40 +341,6 @@ RSpec.describe API::Internal::Base do
end end
describe "GET /internal/authorized_keys" do describe "GET /internal/authorized_keys" do
context "using an existing key's fingerprint" do
it "finds the key" do
get(api('/internal/authorized_keys'), params: { fingerprint: key.fingerprint, secret_token: secret_token })
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['id']).to eq(key.id)
expect(json_response['key'].split[1]).to eq(key.key.split[1])
end
it 'exposes the comment of the key as a simple identifier of username + hostname' do
get(api('/internal/authorized_keys'), params: { fingerprint: key.fingerprint, secret_token: secret_token })
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['key']).to include("#{key.user_name} (#{Gitlab.config.gitlab.host})")
end
end
context "non existing key's fingerprint" do
it "returns 404" do
get(api('/internal/authorized_keys'), params: { fingerprint: "no:t-:va:li:d0", secret_token: secret_token })
expect(response).to have_gitlab_http_status(:not_found)
end
end
context "using a partial fingerprint" do
it "returns 404" do
get(api('/internal/authorized_keys'), params: { fingerprint: "#{key.fingerprint[0..5]}%", secret_token: secret_token })
expect(response).to have_gitlab_http_status(:not_found)
end
end
context "sending the key" do
context "using an existing key" do context "using an existing key" do
it "finds the key" do it "finds the key" do
get(api('/internal/authorized_keys'), params: { key: key.key.split[1], secret_token: secret_token }) get(api('/internal/authorized_keys'), params: { key: key.key.split[1], secret_token: secret_token })
...@@ -385,7 +351,7 @@ RSpec.describe API::Internal::Base do ...@@ -385,7 +351,7 @@ RSpec.describe API::Internal::Base do
end end
it 'exposes the comment of the key as a simple identifier of username + hostname' do it 'exposes the comment of the key as a simple identifier of username + hostname' do
get(api('/internal/authorized_keys'), params: { fingerprint: key.fingerprint, secret_token: secret_token }) get(api('/internal/authorized_keys'), params: { key: key.key.split[1], secret_token: secret_token })
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
expect(json_response['key']).to include("#{key.user_name} (#{Gitlab.config.gitlab.host})") expect(json_response['key']).to include("#{key.user_name} (#{Gitlab.config.gitlab.host})")
...@@ -404,7 +370,6 @@ RSpec.describe API::Internal::Base do ...@@ -404,7 +370,6 @@ RSpec.describe API::Internal::Base do
expect(response).to have_gitlab_http_status(:not_found) expect(response).to have_gitlab_http_status(:not_found)
end end
end end
end
describe "POST /internal/allowed", :clean_gitlab_redis_shared_state do describe "POST /internal/allowed", :clean_gitlab_redis_shared_state do
context "access granted" do context "access granted" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment