Use http_max_redirects opt to replace monkeypatch
http_max_redirects was introduced in 4.2.2, so upgrade kubeclient. The monkey-patch was global so we will have to check that all instances of Kubeclient::Client are handled. Spec all methods of KubeClient This should provide better confidence that we are indeed disallowing redirection in all cases
This commit is contained in:
parent
e4dc22e330
commit
f234aef994
2
Gemfile
2
Gemfile
|
@ -224,7 +224,7 @@ gem 'asana', '~> 0.8.1'
|
||||||
gem 'ruby-fogbugz', '~> 0.2.1'
|
gem 'ruby-fogbugz', '~> 0.2.1'
|
||||||
|
|
||||||
# Kubernetes integration
|
# Kubernetes integration
|
||||||
gem 'kubeclient', '~> 4.0.0'
|
gem 'kubeclient', '~> 4.2.2'
|
||||||
|
|
||||||
# Sanitize user input
|
# Sanitize user input
|
||||||
gem 'sanitize', '~> 4.6'
|
gem 'sanitize', '~> 4.6'
|
||||||
|
|
|
@ -425,7 +425,7 @@ GEM
|
||||||
kgio (2.10.0)
|
kgio (2.10.0)
|
||||||
knapsack (1.17.0)
|
knapsack (1.17.0)
|
||||||
rake
|
rake
|
||||||
kubeclient (4.0.0)
|
kubeclient (4.2.2)
|
||||||
http (~> 3.0)
|
http (~> 3.0)
|
||||||
recursive-open-struct (~> 1.0, >= 1.0.4)
|
recursive-open-struct (~> 1.0, >= 1.0.4)
|
||||||
rest-client (~> 2.0)
|
rest-client (~> 2.0)
|
||||||
|
@ -1053,7 +1053,7 @@ DEPENDENCIES
|
||||||
jwt (~> 2.1.0)
|
jwt (~> 2.1.0)
|
||||||
kaminari (~> 1.0)
|
kaminari (~> 1.0)
|
||||||
knapsack (~> 1.17)
|
knapsack (~> 1.17)
|
||||||
kubeclient (~> 4.0.0)
|
kubeclient (~> 4.2.2)
|
||||||
letter_opener_web (~> 1.3.0)
|
letter_opener_web (~> 1.3.0)
|
||||||
license_finder (~> 5.4)
|
license_finder (~> 5.4)
|
||||||
licensee (~> 8.9)
|
licensee (~> 8.9)
|
||||||
|
|
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: Upgrade kubeclient to 4.2.2 and swap out monkey-patch to disallow redirects
|
||||||
|
merge_request: 24284
|
||||||
|
author:
|
||||||
|
type: other
|
|
@ -1,22 +0,0 @@
|
||||||
class Kubeclient::Client
|
|
||||||
# Monkey patch to set `max_redirects: 0`, so that kubeclient
|
|
||||||
# does not follow redirects and expose internal services.
|
|
||||||
# See https://gitlab.com/gitlab-org/gitlab-ce/issues/53158
|
|
||||||
def create_rest_client(path = nil)
|
|
||||||
path ||= @api_endpoint.path
|
|
||||||
options = {
|
|
||||||
ssl_ca_file: @ssl_options[:ca_file],
|
|
||||||
ssl_cert_store: @ssl_options[:cert_store],
|
|
||||||
verify_ssl: @ssl_options[:verify_ssl],
|
|
||||||
ssl_client_cert: @ssl_options[:client_cert],
|
|
||||||
ssl_client_key: @ssl_options[:client_key],
|
|
||||||
proxy: @http_proxy_uri,
|
|
||||||
user: @auth_options[:username],
|
|
||||||
password: @auth_options[:password],
|
|
||||||
open_timeout: @timeouts[:open],
|
|
||||||
read_timeout: @timeouts[:read],
|
|
||||||
max_redirects: 0
|
|
||||||
}
|
|
||||||
RestClient::Resource.new(@api_endpoint.merge(path).to_s, options)
|
|
||||||
end
|
|
||||||
end
|
|
|
@ -76,9 +76,12 @@ module Gitlab
|
||||||
|
|
||||||
attr_reader :api_prefix, :kubeclient_options
|
attr_reader :api_prefix, :kubeclient_options
|
||||||
|
|
||||||
|
# We disable redirects through 'http_max_redirects: 0',
|
||||||
|
# so that KubeClient does not follow redirects and
|
||||||
|
# expose internal services.
|
||||||
def initialize(api_prefix, **kubeclient_options)
|
def initialize(api_prefix, **kubeclient_options)
|
||||||
@api_prefix = api_prefix
|
@api_prefix = api_prefix
|
||||||
@kubeclient_options = kubeclient_options
|
@kubeclient_options = kubeclient_options.merge(http_max_redirects: 0)
|
||||||
end
|
end
|
||||||
|
|
||||||
def create_or_update_cluster_role_binding(resource)
|
def create_or_update_cluster_role_binding(resource)
|
||||||
|
|
|
@ -24,6 +24,32 @@ describe Gitlab::Kubernetes::KubeClient do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
shared_examples 'redirection not allowed' do |method_name|
|
||||||
|
before do
|
||||||
|
redirect_url = 'https://not-under-our-control.example.com/api/v1/pods'
|
||||||
|
|
||||||
|
stub_request(:get, %r{\A#{api_url}/})
|
||||||
|
.to_return(status: 302, headers: { location: redirect_url })
|
||||||
|
|
||||||
|
stub_request(:get, redirect_url)
|
||||||
|
.to_return(status: 200, body: '{}')
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'does not follow redirects' do
|
||||||
|
method_call = -> do
|
||||||
|
case method_name
|
||||||
|
when /\A(get_|delete_)/
|
||||||
|
client.public_send(method_name)
|
||||||
|
when /\A(create_|update_)/
|
||||||
|
client.public_send(method_name, {})
|
||||||
|
else
|
||||||
|
raise "Unknown method name #{method_name}"
|
||||||
|
end
|
||||||
|
end
|
||||||
|
expect { method_call.call }.to raise_error(Kubeclient::HttpError)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
describe '#core_client' do
|
describe '#core_client' do
|
||||||
subject { client.core_client }
|
subject { client.core_client }
|
||||||
|
|
||||||
|
@ -103,6 +129,8 @@ describe Gitlab::Kubernetes::KubeClient do
|
||||||
:update_service_account
|
:update_service_account
|
||||||
].each do |method|
|
].each do |method|
|
||||||
describe "##{method}" do
|
describe "##{method}" do
|
||||||
|
include_examples 'redirection not allowed', method
|
||||||
|
|
||||||
it 'delegates to the core client' do
|
it 'delegates to the core client' do
|
||||||
expect(client).to delegate_method(method).to(:core_client)
|
expect(client).to delegate_method(method).to(:core_client)
|
||||||
end
|
end
|
||||||
|
@ -123,6 +151,8 @@ describe Gitlab::Kubernetes::KubeClient do
|
||||||
:update_cluster_role_binding
|
:update_cluster_role_binding
|
||||||
].each do |method|
|
].each do |method|
|
||||||
describe "##{method}" do
|
describe "##{method}" do
|
||||||
|
include_examples 'redirection not allowed', method
|
||||||
|
|
||||||
it 'delegates to the rbac client' do
|
it 'delegates to the rbac client' do
|
||||||
expect(client).to delegate_method(method).to(:rbac_client)
|
expect(client).to delegate_method(method).to(:rbac_client)
|
||||||
end
|
end
|
||||||
|
@ -139,6 +169,8 @@ describe Gitlab::Kubernetes::KubeClient do
|
||||||
let(:extensions_client) { client.extensions_client }
|
let(:extensions_client) { client.extensions_client }
|
||||||
|
|
||||||
describe '#get_deployments' do
|
describe '#get_deployments' do
|
||||||
|
include_examples 'redirection not allowed', 'get_deployments'
|
||||||
|
|
||||||
it 'delegates to the extensions client' do
|
it 'delegates to the extensions client' do
|
||||||
expect(client).to delegate_method(:get_deployments).to(:extensions_client)
|
expect(client).to delegate_method(:get_deployments).to(:extensions_client)
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in New Issue