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
Conversation
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:
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. |
That's a totally valid concern I didn't consider. :nod:
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.
1375b9c
to
8869206
Compare
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 |
There was a problem hiding this 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.
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
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. |
Occasionally
terraform init
on some providers may return the following error message:That 403 is most often returned from GitHub (rather than Registry API) and this change makes it more obvious.
Similar to #29298