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

Wrong error message when login to OCI registry interactively. #2562

Open
rhuss opened this issue Nov 4, 2024 · 9 comments · May be fixed by #2596
Open

Wrong error message when login to OCI registry interactively. #2562

rhuss opened this issue Nov 4, 2024 · 9 comments · May be fixed by #2596
Assignees

Comments

@rhuss
Copy link
Contributor

rhuss commented Nov 4, 2024

When using a default registry with

export FUNC_REGISTRY=docker.io/invaliduser

and then do a func deploy without being logged in you get:

10:10 ➜ kn func deploy
function up-to-date. Force rebuild with --build
Please provide credentials for image registry (index.docker.io).
? Username: rhuss
? Password: ************************************
Incorrect credentials, please try again.
? Username:

although the password is correct (verified with podman login). The reason is probably that for the authentication still invaliduser is used.

I suggest to not asked for the username here, but just reuse the configured one (either via env or via cli option), and just print this out to the user when asking for the password.

@matejvasek
Copy link
Contributor

@rhuss

The reason is probably that for the authentication still invaliduser is used

I do not think so. I believe the correct username is used, but the user is not authorized on the specific namespace; i.e authentication succeeds but authorization does not.

I suggest to not asked for the username here

I am not sure what you mean.
IMO we should ask for username. Imagine you want to push image into some orgnatization.

export FUNC_REGISTRY=docker.io/our-org

You still want to user your personal login (e.g. my-user-name) which belongs to the org, right?

However I agree the error messages could be better.

@rhuss
Copy link
Contributor Author

rhuss commented Nov 4, 2024

You could at least repeat the username or org for which authentication is done. And I don't think it's always about authorization, as the invalid user does not exist at Docker Hub, including much more context around the error. Of course, the user or orga could exist, too, so it's then about authorization. But still, it's then not an invalid credentials error, that clearly indicates an authentication error (it would be no access kind of message for authorization errors).

At least that error took me some hours to figure out that I had a typo in the env's repo name. Yes, there was a warning, but it fades out quickly.

@matejvasek
Copy link
Contributor

And I don't think it's always about authorization, as the invalid user does not exist at Docker Hub

Yes, but I suspect that API would still return authorization error not 404, so I am not sure if we would be able to differentiate. Some APIs return authorization error even for non-existent entries for sake of security or something.

Again I agree the error should be better.

@matejvasek
Copy link
Contributor

@gauron99 would you look into this?

@matejvasek
Copy link
Contributor

Basically CheckAuth() have to return two distinct errors; authentication error and authorization error.

Some time ago the function was checking authentication, but then in #1130 we switched and the function directly checked authorization on given image.

The function should check authentication first, then authorization and return appropriate error if one of these fails.

The user should be then informed about what failed.

@matejvasek
Copy link
Contributor

If you would work on this, @gauron99 , make sure it works against ghcr.io,docker.io,quay.io and gcr.io. Also please do not forget adding tests.

@gauron99
Copy link
Contributor

gauron99 commented Nov 4, 2024

thanks Matej, will take a look

@gauron99 gauron99 self-assigned this Nov 4, 2024
@matejvasek
Copy link
Contributor

This also my required signature change of CredentialsCallback -- it might need additional parameter -- previous error.

@rhuss
Copy link
Contributor Author

rhuss commented Nov 5, 2024

I think the most important part is to add more context to the error message, like the image name for which you want to authenticate. Whether its an authorization, authentication or user-not-known error might be nice to have.

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 a pull request may close this issue.

3 participants