-
Notifications
You must be signed in to change notification settings - Fork 303
Fixes #38649 - Long container push uploads result in authentication error #11485
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
base: master
Are you sure you want to change the base?
Conversation
Reviewer's GuideAdds logic to detect long-running container push operations, extend expired registry tokens by one minute, and retry authorization to prevent mid-push authentication failures. Sequence diagram for extended token expiry during container pushsequenceDiagram
participant Client
participant RegistryProxiesController
participant PersonalAccessToken
participant User
Client->>RegistryProxiesController: Push large container image (upload_blob)
RegistryProxiesController->>PersonalAccessToken: Check token validity
alt Token expired but valid for registry
RegistryProxiesController->>PersonalAccessToken: Extend expires_at by 1 minute
PersonalAccessToken-->>RegistryProxiesController: Save updated token
RegistryProxiesController->>User: Set User.current
User-->>RegistryProxiesController: User.current set
RegistryProxiesController-->>Client: Continue upload
else Token not valid or not registry
RegistryProxiesController-->>Client: Render unauthorized error
end
Class diagram for updated PersonalAccessToken handlingclassDiagram
class RegistryProxiesController {
+blob_upload_operation?()
+token_expired_but_valid?()
+extend_expired_token()
}
class PersonalAccessToken {
+expired?()
+name
+expires_at
+user_id
+find_by_token(token)
+save!()
}
class User {
+current
+unscoped
+find(user_id)
}
RegistryProxiesController --> PersonalAccessToken : uses
RegistryProxiesController --> User : sets current user
PersonalAccessToken --> User : belongs to
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (link)
General comments:
- Extract the repeated header parsing and token lookup in token_expired_but_valid? and extend_expired_token into a shared helper to avoid duplication.
- Consider moving the token expiry extension logic into the PersonalAccessToken model (e.g. a
refresh_expiry!
method) to keep the controller thin and adhere to single responsibility. - Avoid hardcoding the new expiry duration (1 minute) in the controller; make it configurable or derive it from the original token settings for flexibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the repeated header parsing and token lookup in token_expired_but_valid? and extend_expired_token into a shared helper to avoid duplication.
- Consider moving the token expiry extension logic into the PersonalAccessToken model (e.g. a `refresh_expiry!` method) to keep the controller thin and adhere to single responsibility.
- Avoid hardcoding the new expiry duration (1 minute) in the controller; make it configurable or derive it from the original token settings for flexibility.
## Security Issues
### Issue 1
<location> `app/controllers/katello/api/registry/registry_proxies_controller.rb:147` </location>
<issue_to_address>
**security (generic-api-key):** Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
*Source: gitleaks*
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
What are the changes introduced in this pull request?
If token expires during a container push, extend the token expiry to be able to finish the upload.
What are the testing steps for this pull request?
Reproducing is a little bit of a hassle.
First, update the token expiration from
expires_at: 6.minutes.from_now
toexpires_at: 1.minutes.from_now
in the controller in token method.Next, I used a box with a large disk to create a large container image of size 10G with:
Build the image with
podman build -t large-image .
Push the image like
podman push $IMAGE_SHA docker://hostname/organization_label/product_label/large_image:latest
If you're not able to reproduce the authentication expiration, you can also try adding network delays.
I did it using:
sudo tc qdisc add dev enp3s0 root netem delay 100ms
where enp3s0 is the interface name fromip a
Summary by Sourcery
Bug Fixes: