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

internal/getproviders: Add URL to error message for clarity #30810

Merged
merged 2 commits into from Apr 14, 2022

Conversation

radeksimko
Copy link
Member

Occasionally terraform init on some providers may return the following error message:

Error while installing citrix/citrixadc v1.13.0: could not query provider
registry for registry.terraform.io/citrix/citrixadc: failed to retrieve
authentication checksums for provider: 403 Forbidden

That 403 is most often returned from GitHub (rather than Registry API) and this change makes it more obvious.

Similar to #29298

@apparentlymart
Copy link
Member

Thanks for looking into this, @radeksimko!

I like this idea in principle but I think we need to be careful because the current design of our provider registry protocol encourages private registry implementations to use package URLs containing enough information for the server to authenticate the request. Our intent was for implementers to use short-lived signatures for that, but I worry that some may have just encoded a long-term shared secret or bearer token into the URL instead, which this would then return into the UI.

As a compromise, what do you think about showing just the hostname from the URL, leading to an error message something like this:

failed to retrieve authentication checksums: github.com returned 403 Forbidden

I would concede that it's possible that someone could encode a secret as part of the hostname rather than as part of the path or query, but that at least seems less likely to disclose something problematic than if we show the entire URL.

(I guess this feedback retroactively applies to #29298 too, which I didn't catch at the time. Maybe the fact that we've not heard any feedback about that other change is a signal that this isn't as big a problem as I fear it is, although it could equally be that there just haven't been many private provider registries yet in order to encounter the problem.)


We also have #29349 open to give a way to explicitly declare that the server which delivers the packages should be sent the same credentials as the registry was, to simplify the case where a registry is hosting its own files under the same credentials as the registry itself. If we can design a resolution to that then I'd say it's probably okay to return the full URL in the situation where that feature is being used, as long as the design of whatever protocol addition we add allows the registry to declare explicit intent that the URL itself is not being used for authentication purposes.

@radeksimko
Copy link
Member Author

That's a totally valid concern I didn't consider. :nod:

As a compromise, what do you think about showing just the hostname from the URL

That sounds good to me. I'll modify the PR to just print the hostname & update the other two places to do the same - that is in fact something I proposed initially as part of #29298 😄

Occasionally `terraform init` on some providers may return the following error message:

Error while installing citrix/citrixadc v1.13.0: could not query provider
registry for registry.terraform.io/citrix/citrixadc: failed to retrieve
authentication checksums for provider: 403 Forbidden

The 403 is most often returned from GitHub (rather than Registry API)
and this change makes it more obvious.
@radeksimko
Copy link
Member Author

radeksimko commented Apr 14, 2022

I have updated the PR. I think the host(name) is still an improvement.

I am however hoping though we can reach a point where auth is more of a decoupled concept so we can expose full URLs, or at least be sure that the auth mechanism allows us to use the Redacted() version of the URL. The URL makes it much easier to understand the root cause (e.g. typo in filename).

Copy link
Member

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

Thanks! This seems like a good compromise for now.

@radeksimko radeksimko merged commit 746af01 into main Apr 14, 2022
@radeksimko radeksimko deleted the clarify-getfile-err branch April 14, 2022 15:14
@github-actions
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants