core: improve anon/auth token logic #148
Conversation
the method is intended to request authenticated token, per pydocs, but was passing an headers which was always missing Authorization. Signed-off-by: tarilabs <matteo.mortari@gmail.com>
if a token was saved in auth, it shall be used in subsequent requests. This avoid a situation where: to upload a blob, first is done anonymously, then retry with token then upload a manifest, avoid the attempt to upload anonymously if a token was present in the previous flow Signed-off-by: tarilabs <matteo.mortari@gmail.com>
in the first flow using auth backend for token: 1. try do_request with no auths at all 2. the attempt to gain an anon token is success, but then the request fails with 401 3. at this point, in the third attempt, give chance to the flow to request a token but avoid any anon tokens. Please note: this happens effectively only on the first run of the flow. Subsequent do_request flow invocations should just succeed now on the 1st request by re-using the token --simplified behaviour introduced with this proposal Signed-off-by: tarilabs <matteo.mortari@gmail.com>
Signed-off-by: tarilabs <matteo.mortari@gmail.com>
Signed-off-by: tarilabs <matteo.mortari@gmail.com>
from: oras-project#148 (comment) > And if the basic auth is there, skip over asking for an anon token as it stands, in case the basic auth are present, these are exchanged for the request token. Signed-off-by: tarilabs <matteo.mortari@gmail.com>
tarilabs
force-pushed
the
tarilabs-20240706
branch
from
102381c
to
e164a49
Compare
This reverts commit 102381c. Signed-off-by: tarilabs <matteo.mortari@gmail.com>
This reverts commit 1e891d2. Signed-off-by: tarilabs <matteo.mortari@gmail.com>
This reverts commit 6e22667. Signed-off-by: tarilabs <matteo.mortari@gmail.com>
…ect#153 this was taken care in oras-project#153 This reverts commit 10e010b. Signed-off-by: tarilabs <matteo.mortari@gmail.com>
from: oras-project#148 (comment) > And if the basic auth is there, skip over asking for an anon token as it stands, in case the basic auth are present, these are exchanged for the request token. Signed-off-by: tarilabs <matteo.mortari@gmail.com>
tarilabs
force-pushed
the
tarilabs-20240706
branch
from
e164a49
to
bb4c2ec
Compare
|
up to bb4c2ec (next 9c3f003 is just linting) been using this external project to test it works as expected exchanging for the request token, examples:
I think this incorporates needed changes to avoid anon tokens when a request token should be used instead, required to my knowledge and exemplified above. In the longer term, I'm thinking to contribute some automated tests, as suggested #147 (comment) |
Signed-off-by: tarilabs <matteo.mortari@gmail.com>
|
kindly let me know if these overall changes looks good @vsoch , thank you so much for your patience on this one 🙏 hope these changes helps |
| @@ -152,7 +151,7 @@ def request_anonymous_token(self, h: auth_utils.authHeader) -> bool: | |||
| if h.scope: | |||
| params["scope"] = h.scope | |||
|
|
|||
| logger.debug(f"Final params are {params}") | |||
| logger.debug(f"Requesting anon token with params: {params}") | |||
There was a problem hiding this comment.
Just a sanity check - could this have any parameters we wouldn't want to be printed? I would probably leave this line out entirely.
There was a problem hiding this comment.
only service and scope:
Lines 147 to 151 in 625c6ff
which to my understanding are not sensitive info and done analogously for the auth token.
Just to be clear, I've just changed the hardcoded text to make it significant it's about an anon token when observing the logs, and the params was there before :)
I thought aligning log statements in:
- https://github.com/tarilabs/oras-py/blob/7d891e96e24f16f690be4c3953a53cbd819dfe6f/oras/auth/token.py#L127-L128
- https://github.com/tarilabs/oras-py/blob/7d891e96e24f16f690be4c3953a53cbd819dfe6f/oras/auth/token.py#L154-L155
to your comment of:
... put this debug message in the function that does that, and put a bit more metadata to make the comment more useful ...
(source)
but let me know if you prefer me to change those log debug statement to something else!
There was a problem hiding this comment.
only service and scope:
I just wanted to double check this - I think this should be ok to print.
|
thank you so much @vsoch 🙏 🚀 🪐 |
while testing for #147 with ghcr.io and quay.io (so I could test the Token auth backend), I noticed the following and I believe these modification improve on the token backend logic.
I might have missed on something however 😅 , so kindly be patience and let me know if I'm not considering any gap!
Example with ghcr.io: https://github.com/users/tarilabs/packages/container/package/demo20240629-oras147
Example with quay.io: https://quay.io/repository/mmortari/demo20240629-oras147?tab=tags
To make the review hopefully easier, I've left the changes in separate commits, with richer description.
wdyt?
(note: c243eb2 introduced later as in #147 notebooks I was only testing auth backends for push and pull)