Update Zypper auth check #1270

jesusbv · 2025-01-09T16:49:41Z

Description

When a hybrid system runs zypper command, the check
needs to take care of the subscription verification

Currently, the check lacks paid extension ID, resulting in
an unsuccessful verification

This change fixes that

If system is hybrid, and the path being accessed belongs to a paid extension,
we check that the paid extension subscription is active

if the repository path belongs to a free repository or no matches with paid extensions,
no need to check the subscription

if ZypperAuth handle_scc_subscription method gets called without a product id,
the method will check all non free products suscriptions to be active, if any

This fixes bsc#1230157

Related Issue / Ticket / Trello card: https://bugzilla.suse.com/show_bug.cgi?id=1230157

How to test

Start an Azure instance, register LTSS, wait > 20 min for the cache to get expired
run zypper ref should work

Change Type

Please select the correct option.

Bug Fix (a non-breaking change which fixes an issue)
New Feature (a non-breaking change which adds new functionality)
Documentation Update (a change which only updates documentation)

Checklist

Please check off each item if the requirement is met.

I have reviewed my own code and believe that it's ready for an external review.
I have provided comments for any hard-to-understand code.
I have documented the MANUAL.md file with any changes to the user experience.
If my changes are non-trivial, I have added a changelog entry to notify users at package/obs/rmt-server.changes.

Review

Please check out our review guidelines
and get in touch with the author to get a shared understanding of the change.

When a hybrid system runs zypper command, the check needs to take care of the subscription verification Currently, the check lacks paid extension ID, resulting in an unsuccessful verification This change fixes that If system is hybrid, and the path being accessed belongs to a paid extension, we check that the paid extension subscription is active if the repository path belongs to a free repository or no matches with paid extensions, no need to check the subscription if ZypperAuth handle_scc_subscription method gets called without a product id, the method will check _all_ non free products suscriptions to be active, if any This fixes bsc#1230157

jesusbv · 2025-01-10T10:22:15Z

engines/zypper_auth/lib/zypper_auth/engine.rb

+              repos_paths.each do |repo_path|
+                if headers.fetch('X-Original-URI', '').include? repo_path
+                  logger.info "verifying paid extension #{paid_extension.identifier}"
+                  return ZypperAuth.verify_instance(request, logger, @system, paid_extension.id)


We could call handle_scc_subscription from path_allowed? or all_allowed_paths in the same file

I think adjusting the logic for ZypperAuth's path_allowed? (as in this PR) split the responsibilities clearer

This creates a loop ZypperAuth.verify_instance calls handle_scc_subscription in 2 places. In one of those places we have if is_valid && system.hybrid? && !handle_scc_subscription(request, system, verification_provider, params_product_id) So if the instance verification passed and the system is hybrid we end up in handle_scc_subscription and then here, since the system is a hybrid system we turn around and call verify_instance again. I do not understand how this is going to end well.

and then here, since the system is a hybrid system we turn around and call verify_instance again

I do not know what you mean, if you mean that we call ZypperAuth.verify_instance twice,

one in line 166 (which we return the returned value of that call)

one in line 173

the call in line 173 happens if the system is not hybrid (and I would add byos) or if it is hybrid/byos and the path trying to be accessed was not found in that system products

is that what you meant, @rjschwei ?

my comment of moving the handle_scc_subscription (in ZypperAuth engine) call to path_allowed? (in strict_authentication engine) is more of "whose responsibility is it ?"

For example, if zypper is trying to access path A, it makes sense that the method checking if it is allowed
path_allowed? checks that the path is a non free repo and then checks that the subscription is valid/not expired

while verify_instance maybe should verify that the instance metadata is valid

It was just an open question than a suggestion

@rjschwei can you explain what you mean with

and then here, since the system is a hybrid system we turn around and call verify_instance again

?

jesusbv · 2025-01-10T10:24:35Z

I will add the tests and coverage but it can be reviewed in the meantime

digitaltom · 2025-01-10T12:56:18Z

engines/zypper_auth/lib/zypper_auth/engine.rb

@@ -51,9 +51,22 @@ def verify_instance(request, logger, system, params_product_id = nil)
    end

    def handle_scc_subscription(request, system, verification_provider, params_product_id = nil)


Would such name describe that method intent better?

Suggested change

def handle_scc_subscription(request, system, verification_provider, params_product_id = nil)

def has_active_subscription?(request, system, verification_provider, params_product_id = nil)

engines/zypper_auth/lib/zypper_auth/engine.rb

digitaltom · 2025-01-10T13:04:33Z

engines/zypper_auth/lib/zypper_auth/engine.rb

-        def path_allowed?(path)
-          return false unless original_path_allowed?(path)
+        def path_allowed?(headers)
+          return false unless original_path_allowed?(headers)


What's going on here, original_path_allowed? is an alias for path_allowed?, and then we call original_path_allowed? from inside path_allowed??

rjschwei · 2025-01-10T14:21:24Z

engines/zypper_auth/lib/zypper_auth/engine.rb

+        def path_allowed?(headers)
+          return false unless original_path_allowed?(headers)
+
+          if @system.hybrid?


But shouldn't this apply to proxy systems as well? A BYOS system that uses RMT as a proxy to SCC should have the same verification right? OR do we not hit this code path for proxy clients?

we do, and I think byos? system should be included, unless there are BYOS subscriptions to register the system that include LTSS, meaning one regcode including system + LTSS instead of two regcodes (something I wanted to check here before including it)

I think the workflow is two reg codes

jesusbv self-assigned this Jan 9, 2025

jesusbv requested review from digitaltom and rjschwei January 9, 2025 16:49

jesusbv added the 2.21 label Jan 9, 2025

rjschwei approved these changes Jan 9, 2025

View reviewed changes

jesusbv marked this pull request as draft January 9, 2025 20:00

jesusbv force-pushed the fix-auth-repo branch from 1b158b0 to ac56301 Compare January 10, 2025 10:17

jesusbv changed the title ~~Fix the check for allowing repositories~~ Update Zypper auth check Jan 10, 2025

jesusbv commented Jan 10, 2025

View reviewed changes

jesusbv marked this pull request as ready for review January 10, 2025 10:24

Merge branch 'master' into fix-auth-repo

9b38c38

jesusbv requested a review from rjschwei January 10, 2025 10:33

jesusbv added 2 commits January 10, 2025 10:40

Update changelog

881f475

Add pubcloud section to changelog

82a34d0

digitaltom reviewed Jan 10, 2025

View reviewed changes

engines/zypper_auth/lib/zypper_auth/engine.rb Show resolved Hide resolved

digitaltom reviewed Jan 10, 2025

View reviewed changes

engines/zypper_auth/lib/zypper_auth/engine.rb Show resolved Hide resolved

digitaltom reviewed Jan 10, 2025

View reviewed changes

rjschwei reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Zypper auth check #1270

Update Zypper auth check #1270

jesusbv commented Jan 9, 2025 •

edited

Loading

jesusbv Jan 10, 2025

rjschwei Jan 10, 2025

jesusbv Jan 10, 2025

jesusbv Jan 10, 2025 •

edited

Loading

jesusbv Jan 13, 2025 •

edited

Loading

jesusbv commented Jan 10, 2025

digitaltom Jan 10, 2025 •

edited

Loading

digitaltom Jan 10, 2025

jesusbv Jan 10, 2025

rjschwei Jan 10, 2025

jesusbv Jan 10, 2025 •

edited

Loading

		@@ -51,9 +51,22 @@ def verify_instance(request, logger, system, params_product_id = nil)
		end

		def handle_scc_subscription(request, system, verification_provider, params_product_id = nil)

	def handle_scc_subscription(request, system, verification_provider, params_product_id = nil)
	def has_active_subscription?(request, system, verification_provider, params_product_id = nil)

Update Zypper auth check #1270

Are you sure you want to change the base?

Update Zypper auth check #1270

Conversation

jesusbv commented Jan 9, 2025 • edited Loading

Description

How to test

Change Type

Checklist

Review

jesusbv Jan 10, 2025

Choose a reason for hiding this comment

rjschwei Jan 10, 2025

Choose a reason for hiding this comment

jesusbv Jan 10, 2025

Choose a reason for hiding this comment

jesusbv Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

jesusbv Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

jesusbv commented Jan 10, 2025

digitaltom Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

digitaltom Jan 10, 2025

Choose a reason for hiding this comment

jesusbv Jan 10, 2025

Choose a reason for hiding this comment

rjschwei Jan 10, 2025

Choose a reason for hiding this comment

jesusbv Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

jesusbv commented Jan 9, 2025 •

edited

Loading

jesusbv Jan 10, 2025 •

edited

Loading

jesusbv Jan 13, 2025 •

edited

Loading

digitaltom Jan 10, 2025 •

edited

Loading

jesusbv Jan 10, 2025 •

edited

Loading