Skip to content

Conversation

@evanebb
Copy link
Contributor

@evanebb evanebb commented Mar 1, 2025

What type of PR is this?
feature

Which issue does this PR fix:
#2587

What does this PR do / Why do we need it:
Currently the github.com/chartmuseum/auth package is used for token authentication, which only supports RSA keys. It also doesn't seem to be updated anymore, and uses a pretty old version of the golang-jwt package which could use a bump.
I have ripped out the important bits and re-implemented the token handling directly using golang-jwt, which allows it to support EC/ED25519 keys as well as RSA keys.

Testing done on this change:
In addition to the added automated tests, I have done some manual testing by spinning up a local Zot instance + authorization server. I tested using an RSA (RS256), ECDSA (ES256) and ED25519 key/certificate, did some API calls with tokens signed using these keys and some logins/pushes/pulls using the Docker CLI. This all seemed to work just fine.

Automation added to e2e:

Will this break upgrades or downgrades?
It shouldn't, I've tried to keep the behavior as close to what it was before as possible, but I can't guarantee it (there are some more improvements I could think of, but I don't want to break backward compatibility completely).

Does this PR introduce any user-facing change?:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@evanebb evanebb force-pushed the token-auth-improvements branch from 021eda7 to 24acd0f Compare March 1, 2025 21:49
@codecov
Copy link

codecov bot commented Mar 1, 2025

Codecov Report

Attention: Patch coverage is 92.59259% with 12 lines in your changes missing coverage. Please review.

Project coverage is 91.00%. Comparing base (e7fb9c5) to head (199148c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/api/authn.go 86.36% 4 Missing and 2 partials ⚠️
pkg/api/bearer.go 94.82% 2 Missing and 1 partial ⚠️
pkg/test/auth/bearer.go 95.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2998      +/-   ##
==========================================
+ Coverage   90.99%   91.00%   +0.01%     
==========================================
  Files         175      176       +1     
  Lines       32798    32900     +102     
==========================================
+ Hits        29843    29940      +97     
- Misses       2232     2234       +2     
- Partials      723      726       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rchincha
Copy link
Contributor

rchincha commented Mar 2, 2025

@evanebb thanks for this PR.

Pls address the CI failures.

Copy link
Contributor

@andaaron andaaron left a comment

Choose a reason for hiding this comment

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

The code is much cleaner compared to what we had before.
Please make sure to add relevant tests and reach the coverage threshold.

@rchincha
Copy link
Contributor

rchincha commented Mar 5, 2025

@evanebb when you are ready, move it out of draft and mark it ready for review.

@evanebb evanebb marked this pull request as ready for review March 5, 2025 18:31
@evanebb
Copy link
Contributor Author

evanebb commented Mar 5, 2025

@rchincha done, I wanted to do a bit more manual testing before doing so.

Something that could cause problems is that the old code would also accept a file containing a public key for the http.auth.bearer.cert directive (right here, which calls this function under the hood), whereas my changes always assume that it is a certificate (which makes parsing it a bit simpler).
However, this is undocumented behavior as far as I can see. The documentation only mentions passing a certificate file through this parameter.

(also, I noticed that the CI will intermittently fail with a bind: address already in use error. I did mess around a bit in the tests that spin up a test HTTP server, but I'm going to assume that this just happens from time-to-time and a re-run 'fixes' it?)

@evanebb evanebb force-pushed the token-auth-improvements branch from 468d7dd to 199148c Compare March 5, 2025 19:49
@rchincha
Copy link
Contributor

rchincha commented Mar 5, 2025

(also, I noticed that the CI will intermittently fail with a bind: address already in use error. I did mess around a bit in the tests that spin up a test HTTP server, but I'm going to assume that this just happens from time-to-time and a re-run 'fixes' it?)

Some flaky tests, we just have to arbitrate free ports.

@rchincha rchincha merged commit d465690 into project-zot:main Mar 6, 2025
41 checks passed
@rchincha rchincha linked an issue Mar 7, 2025 that may be closed by this pull request
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.

[Feat]: Support EC public key

3 participants