From 9296833aa32e954c5fc1eaa323d238b0fb5bd76f Mon Sep 17 00:00:00 2001 From: Marius Kittler Date: Wed, 4 Dec 2024 16:56:09 +0100 Subject: [PATCH] Add config option to restrict asset downloads to logged-in users * Disallow unauthenticated access to assets in web application routes * Does NOT cover access served via Apache/NGINX * Does NOT cover adjusting the cache service and additional tooling like the `openqa-clone-job` script * See https://progress.opensuse.org/issues/170380 --- etc/openqa/openqa.ini | 2 ++ lib/OpenQA/Setup.pm | 1 + lib/OpenQA/WebAPI.pm | 22 ++++++++++++++-------- t/03-auth.t | 23 +++++++++++++++++++++-- t/config.t | 1 + 5 files changed, 39 insertions(+), 10 deletions(-) diff --git a/etc/openqa/openqa.ini b/etc/openqa/openqa.ini index 1485f74bd9a..b55057e41bd 100644 --- a/etc/openqa/openqa.ini +++ b/etc/openqa/openqa.ini @@ -125,6 +125,8 @@ ## Authentication method to use for user management [auth] #method = Fake|OpenID|OAuth2 +## whether authentication is required to access assets; does NOT affect assets made available directly via e.g. Apache or NGINX +#require_for_assets = 0 ## for GitHub, salsa.debian.org and providers listed on https://metacpan.org/pod/Mojolicious::Plugin::OAuth2#Configuration ## one can use: diff --git a/lib/OpenQA/Setup.pm b/lib/OpenQA/Setup.pm index 2a3c738cd24..e90b6646dd6 100644 --- a/lib/OpenQA/Setup.pm +++ b/lib/OpenQA/Setup.pm @@ -61,6 +61,7 @@ sub read_config ($app) { }, auth => { method => 'OpenID', + require_for_assets => 0, }, 'scm git' => { update_remote => '', diff --git a/lib/OpenQA/WebAPI.pm b/lib/OpenQA/WebAPI.pm index 3bc8419fd25..de4ec9fe70f 100644 --- a/lib/OpenQA/WebAPI.pm +++ b/lib/OpenQA/WebAPI.pm @@ -141,11 +141,15 @@ sub startup ($self) { $r->get('/tests/list_scheduled_ajax')->name('tests_ajax')->to('test#list_scheduled_ajax'); # only provide a URL helper - this is overtaken by apache - $r->get('/assets/*assetpath')->name('download_asset')->to('file#download_asset'); + my $config = $app->config; + my $require_auth_for_assets = $config->{auth}->{require_for_assets}; + my $assets_r = $require_auth_for_assets ? $logged_in : $r; + $assets_r->get('/assets/*assetpath')->name('download_asset')->to('file#download_asset'); my $test_r = $r->any('/tests/'); $test_r = $test_r->under('/')->to('test#referer_check'); my $test_auth = $auth->any('/tests/' => {format => 0}); + my $test_asset_r = $require_auth_for_assets ? $test_auth : $test_r; $test_r->get('/')->name('test')->to('test#show'); $test_r->get('/ajax')->name('job_next_previous_ajax')->to('test#job_next_previous_ajax'); $test_r->get('/modules/:moduleid/fails')->name('test_module_fails')->to('test#module_fails'); @@ -171,9 +175,9 @@ sub startup ($self) { $test_r->get('/video' => sub ($c) { $c->render_testfile('test/video') })->name('video'); $test_r->get('/logfile' => sub ($c) { $c->render_testfile('test/logfile') })->name('logfile'); # adding assetid => qr/\d+/ doesn't work here. wtf? - $test_r->get('/asset/')->name('test_asset_id')->to('file#test_asset'); - $test_r->get('/asset/#assettype/#assetname')->name('test_asset_name')->to('file#test_asset'); - $test_r->get('/asset/#assettype/#assetname/*subpath')->name('test_asset_name_path')->to('file#test_asset'); + $test_asset_r->get('/asset/')->name('test_asset_id')->to('file#test_asset'); + $test_asset_r->get('/asset/#assettype/#assetname')->name('test_asset_name')->to('file#test_asset'); + $test_asset_r->get('/asset/#assettype/#assetname/*subpath')->name('test_asset_name_path')->to('file#test_asset'); my $developer_auth = $test_r->under('/developer')->to('session#ensure_admin'); $developer_auth->get('/ws-console')->name('developer_ws_console')->to('developer#ws_console'); @@ -413,11 +417,13 @@ sub startup ($self) { $api_ro->post('/webhooks/product')->name('apiv1_evaluate_webhook_product')->to('webhook#product'); # api/v1/assets + + my $api_assets_readonly = $require_auth_for_assets ? $api_ru : $api_public_r; $api_ro->post('/assets')->name('apiv1_post_asset')->to('asset#register'); - $api_public_r->get('/assets')->name('apiv1_get_asset')->to('asset#list'); + $api_assets_readonly->get('/assets')->name('apiv1_get_asset')->to('asset#list'); $api_ra->post('/assets/cleanup')->name('apiv1_trigger_asset_cleanup')->to('asset#trigger_cleanup'); - $api_public_r->get('/assets/')->name('apiv1_get_asset_id')->to('asset#get'); - $api_public_r->get('/assets/#type/#name')->name('apiv1_get_asset_name')->to('asset#get'); + $api_assets_readonly->get('/assets/')->name('apiv1_get_asset_id')->to('asset#get'); + $api_assets_readonly->get('/assets/#type/#name')->name('apiv1_get_asset_name')->to('asset#get'); $api_ra->delete('/assets/')->name('apiv1_delete_asset')->to('asset#delete'); $api_ra->delete('/assets/#type/#name')->name('apiv1_delete_asset_name')->to('asset#delete'); @@ -529,7 +535,7 @@ sub startup ($self) { }); # allow configuring Cross-Origin Resource Sharing (CORS) - if (my $access_control_allow_origin = $app->config->{global}->{access_control_allow_origin_header}) { + if (my $access_control_allow_origin = $config->{global}->{access_control_allow_origin_header}) { my %allowed_origins = map { trim($_) => 1 } split ',', $access_control_allow_origin; my $fallback_origin = delete $allowed_origins{'*'} ? '*' : undef; $app->hook( diff --git a/t/03-auth.t b/t/03-auth.t index bfa3cd45410..3e49beee6d9 100644 --- a/t/03-auth.t +++ b/t/03-auth.t @@ -19,13 +19,17 @@ use Mojo::URL; use MIME::Base64 qw(encode_base64url decode_base64url); use Mojolicious; +my $file_api_mock = Test::MockModule->new('OpenQA::WebAPI::Controller::File'); +$file_api_mock->redefine(download_asset => sub ($self) { $self->render(text => 'asset-ok') }); +$file_api_mock->redefine(test_asset => sub ($self) { $self->render(text => 'test-asset-ok') }); + my $tempdir = tempdir("/tmp/$FindBin::Script-XXXX")->make_path; $ENV{OPENQA_CONFIG} = $tempdir; OpenQA::Test::Database->new->create; sub test_auth_method_startup ($auth, @options) { - my @conf = ("[auth]\n", "method = \t $auth \t\n", "[openid]\n", "httpsonly = 0\n"); - $tempdir->child('openqa.ini')->spew(join('', @conf, @options)); + my @conf = ("[auth]\n", "method = \t $auth \t\n"); + $tempdir->child('openqa.ini')->spew(join('', @conf, @options, "[openid]\n", "httpsonly = 0\n")); my $t = Test::Mojo->new('OpenQA::WebAPI'); is $t->app->config->{auth}->{method}, $auth, "started successfully with auth $auth"; $t->get_ok('/login' => {Referer => 'http://open.qa/tests/42'}); @@ -36,6 +40,21 @@ sub mojo_has_request_debug { $Mojolicious::VERSION <= 9.21 } combined_like { test_auth_method_startup('Fake')->status_is(302) } mojo_has_request_debug ? qr/302 Found/ : qr//, 'Plugin loaded'; +subtest 'restricted asset downloads with setting `[auth] require_for_assets = 1`' => sub { + my $t = test_auth_method_startup('Fake', "require_for_assets = 1\n"); + my $expected_redirect = '/login?return_page=%2Fassets%2Fiso%2Ftest.iso'; + $t->get_ok('/assets/iso/test.iso')->status_is(200)->content_is('asset-ok', 'can access asset when logged in'); + $t->get_ok('/tests/42/asset/iso/test.iso')->status_is(200); + $t->content_is('test-asset-ok', 'can access test asset when logged in'); + $t->get_ok('/logout')->status_is(302)->get_ok('/assets/iso/test.iso')->status_is(302); + $t->content_unlike(qr/asset-ok/, 'asset not accessible when logged out'); + $t->header_is('Location', $expected_redirect, 'redirect to login when accessing asset'); + $t->get_ok('/logout')->status_is(302); + $t->get_ok('/tests/42/asset/iso/test.iso')->status_is(302); + $t->header_like('Location', qr|/login\?return_page=.*test.iso|, 'redirect to login when accessing test asset'); + $t->content_unlike(qr/asset-ok/, 'test asset not accessible when logged out'); +}; + subtest OpenID => sub { # OpenID relies on external server which we mock to not rely on external dependencies my $openid_mock = Test::MockModule->new('Net::OpenID::Consumer'); diff --git a/t/config.t b/t/config.t index 8b7db2865b0..e12350db832 100644 --- a/t/config.t +++ b/t/config.t @@ -59,6 +59,7 @@ subtest 'Test configuration default modes' => sub { }, auth => { method => 'Fake', + require_for_assets => 0, }, 'scm git' => { update_remote => '',