Skip to content
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

Fixes #36827 - Tokens for container registries now return OAuth2-compatible headers #10768

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

qcjames53
Copy link
Contributor

@qcjames53 qcjames53 commented Oct 13, 2023

What are the changes introduced in this pull request?

When podman commands request tokens from Katello, the tokens now return the correct specification. In particular:

  • Token response 'expires_at' field is now an 'expires_in' field, representing the number of seconds until the token expires. This is consistent with docker specs shown here.
  • Token response 'issued_at' returns an RFC3339-compatible datetime.

Considerations taken when implementing this change?

n/a

What are the testing steps for this pull request?

  1. Create a lifecycle environment containing a valid docker repository.
  2. Go to the LE's 'details' tab and set the 'Unauthenticated pull' field to 'Yes'.
  3. Open a console in the Katello box. Run podman search <server>/<docker repo name> to get the repo address.
  4. Run podman pull <repo address> to ensure everything is working properly. There should be no pull errors. If your repo doesn't have a 'latest' version, you may need to append :<version> to the command.
  5. Open app/controllers/katello/api/registry/registry_proxies_controller.rb and enter require 'byebug'; byebug on line 152 (before response.headers).
  6. Relaunch foreman/rails.
  7. Run podman pull <repo address>. Head to the rails console. Byebug should have stopped execution.
  8. In the byebug console, quickly enter expiration_seconds to view the return value of the expires_in feld. This should be around 60 seconds (could be 59ish).
  9. Quickly enter 'issue_time' to view the return value of the issued_at field. This should be an RFC3339-formatted datetime.
  10. Once these two fields are verified, enter 'c' to continue execution.
  11. Comment out the byebug line and restart rails/foreman with ctrl+c.
  12. In the web UI, disable unauthenticated pull for the lifecycle environment.
  13. Try running podman pull <repo address> and ensure that this fails due to not being authenticated.
  14. Run podman login <server> to login.
  15. Again run podman pull <repo address> to ensure everything is working properly.
  16. Uncomment the byebug line and restart rails/foreman.
  17. Run podman pull <repo address>. In the rails console, verify the same two fields on the token. expiration_seconds should now be around 6 minutes, but you can still expect an RFC3339 datetime from issue_time.
  18. When done testing Katello, remove the byebug line and restart rails/foreman.
  19. Open a smart proxy linked to this Katello instance (katello-proxy-devel if using forklift is great).
  20. Add the LE to the smart proxy, set 'Unauthenticated pull' to yes, and start a complete sync to the proxy.
  21. On the smart proxy, run podman search <server>/<docker repo name> to get the repo address.
  22. Run podman pull <repo address>. Follow this up with a podman run command to test the container if you wish.
  23. In the web UI, disable unauthenticated pull for the lifecycle environment. Re-sync the smart proxy.
  24. Try running podman pull <repo address> on the smart proxy and ensure that this fails due to not being authenticated.
  25. Run podman login <proxy> to login.
  26. Again run podman pull <repo address> to ensure everything is working properly.

@qcjames53
Copy link
Contributor Author

Note: This shouldn't be merged until the complimentary capsule changes are approved (SAT-20757, PR to follow).

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to test it still, but I have a couple comments for now:

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great from a test perspective for auth'd users:

{"token":"$...GUS","expires_in":359,"issued_at":"2023-10-27T18:37:19+00:00"}

Once those comments are resolved this should be good to go.

@qcjames53 qcjames53 force-pushed the 36827 branch 2 times, most recently from 551529e to 9c2ea54 Compare October 30, 2023 16:22
ianballou
ianballou previously approved these changes Oct 31, 2023
Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me! Let's just wait to merge until Katello/smart_proxy_container_gateway#27 is ready.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider n-1 support where, at least in downstream, we support running Foreman version n with a Foreman Proxy version n-1. Because of release cycles that comes down to n-2 in upstream.

When I see Katello/smart_proxy_container_gateway#27 then I think removing the expires_at field here means you break n-1 compatibility proxies with smart_proxy_container_gateway.

I think the relevant spec (which you helpfully linked, greatly appreciated!) part is https://distribution.github.io/distribution/spec/auth/token/#requesting-a-token and I suspect (though it's not explicitly mentioned) that additional fields are silently ignored. My recommendation would be to keep expires_at for now with a comment that it's outside the spec, but kept for backwards compatibility.

@ianballou
Copy link
Member

@ekohl that's a great point, we need to make sure n-1 smart proxy support is still supported. That means a smart proxy running the older version of container gateway should be able to work with this new code.

I agree that the best way to deal with this would be to keep the expires_at field around.

@ianballou ianballou dismissed their stale review November 1, 2023 14:13

Need to test n-1 smart proxy syncing

@qcjames53
Copy link
Contributor Author

I just pushed a new version which is confirmed working with n-1 smart proxies. Will update testing steps momentarily. Thanks for all the good feedback!

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your commit message is very long. I'd recommend reading https://cbea.ms/git-commit/#limit-50 (actually the whole post is full of good ideas)

@qcjames53
Copy link
Contributor Author

qcjames53 commented Nov 1, 2023

Your commit message is very long. I'd recommend reading https://cbea.ms/git-commit/#limit-50 (actually the whole post is full of good ideas)

I can fix this up on the next rebase + squash. Is the standard practice to continually touch up the original commit message instead of concatenating messages for new commits?

…atible headers

- 'expires_at' has been depricated and is slated for removal in 4.13. We're keeping it for now to maintain smart proxy upgrade compatibility.
- 'expires_at' is now 'expires_in'. This represents second count after issued_at time when the token expires.
- All token times are now rendered in rfc3339 format.
@qcjames53 qcjames53 changed the title Fixes #36827 - Container registries on Katello now return the correct… Fixes #36827 - Tokens for container registries now return OAuth2-compatible headers Nov 1, 2023
@ekohl
Copy link
Member

ekohl commented Nov 1, 2023

Is the standard practice to continually touch up the original commit message instead of concatenating messages for new commits?

Yes it is. The commit message is what shows up in git log and tools like git blame. It should accurately describe the changes you made. I've been thankful of my past self that I could understand the context of a commit. Likewise, I've also had situations where I questioned why a few years ago I made certain changes.

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me (again) !

{"token":"$2a$10$1b6453892473a467d0737uawQSD8Z6HKUQo.p3cKq3H5eWXFf2GUS","expires_in":532332,"issued_at":"2023-10-27T18:35:11+00:00","expires_at":"2023-11-02T22:27:24+00:00"}

@qcjames53 qcjames53 merged commit ad42463 into Katello:master Nov 6, 2023
4 of 5 checks passed
@qcjames53 qcjames53 deleted the 36827 branch November 6, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants