plugins/download: GitLab registry header auth

[gitu]

• add auth to oci_downloader for gitlab registries
• authentication has same workflow as public auth but with authenticated token fetch

Why the changes in this PR are needed?

OCI sourced bundles can not properly authenticate when using GitLab Registry.
See also: authorizer.go#L299

What are the changes in this PR?

Adds a not so elegant solution to get the header explicitly to containerd on, while not only relaying on the client to add the header, which doesn't work for gitlab.

Notes to assist PR review:

I'm not really sure if reflection is the way to go here, but it solves the problem quite elegantly and is only called on pulls.

[ashutosh-narkar]

@DerGut given your work on the OCI downloader, it would be great if you could help to review this change.

[netlify]

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	fd0ba9d
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/64ba5a656fafb90008343e8e
😎 Deploy Preview	https://deploy-preview-6045--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[DerGut]

Hi @gitu, sorry for the late reply!

Thanks a lot for the contribution, I'm having a look at it now.

[DerGut]

This looks great! It's a bit unfortunate that we can't just reuse the HTTPAuthPlugin.Prepare for this.

I checked the places in containerd where the token is fetched and it doesn't seem easy to introduce the docker.Authorizer without causing an import cycle and doing a breaking change to their API (assuming they would even let us make that change).
In addition to this, the token auth spec is fairly specific on what types of authentication mechanisms can be expected from the token server (namely OAuth and Basic) so it makes less sense to enable users to pass in a custom implementation (in contrast to the registry auth).

So passing the authHeader looks like the way to go! 👏

My biggest remark is on the relationship between the HTTPHeaderAuthPlugin and HTTPAuthPlugin. I haven't checked it thoroughly but I believe all HTTPAuthPlugin.Prepare implementations ultimately set the Authorization header of the request. As a new reader I would be wondering what the difference between both interfaces and functions was.

I realize that their is no real way around introducing a new function+interface because we can't alter the existing one and don't have access to private functions from the download package. I do think that the naming could emphasize a bit more that the AuthHeader basically provides the same functionality as Prepare with a different signature.

I think calling bearerAuthPlugin.AuthHeader from inside the bearerAuthPlugin.Prepare could also help emphasize this (even though it would make the code a bit more verbose), i.e. getting a header internally from AuthHeader and adding the key values to the request header.

What are your thoughts?

[DerGut]

Could you add a comment explaining what it's for?

[gitu]

tried to do that, can you have another look

[DerGut]

Thank you! Looks good 👍

[gitu]

My biggest remark is on the relationship between the HTTPHeaderAuthPlugin and HTTPAuthPlugin. I haven't checked it thoroughly but I believe all HTTPAuthPlugin.Prepare implementations ultimately set the Authorization header of the request. As a new reader I would be wondering what the difference between both interfaces and functions was.

I realize that their is no real way around introducing a new function+interface because we can't alter the existing one and don't have access to private functions from the download package. I do think that the naming could emphasize a bit more that the AuthHeader basically provides the same functionality as Prepare with a different signature.
I think calling bearerAuthPlugin.AuthHeader from inside the bearerAuthPlugin.Prepare could also help emphasize this (even though it would make the code a bit more verbose), i.e. getting a header internally from AuthHeader and adding the key values to the request header.

I restructured that a bit so now they both call the same function (addAuthorizationHeader) that adds to the header map. I hope that clearly show that both functions (Prepare(req) and AuthHeader) essentially do the same thing.

[DerGut]

Hey @gitu 👋 thanks for the changes!

I think they look great! I also like what you did with the func addAuthorizationHeader 👍

@ashutosh-narkar do you have any further questions?

[ashutosh-narkar]

My biggest remark is on the relationship between the HTTPHeaderAuthPlugin and HTTPAuthPlugin. I haven't checked it thoroughly but I believe all HTTPAuthPlugin.Prepare implementations ultimately set the Authorization header of the request. As a new reader I would be wondering what the difference between both interfaces and functions was.
I realize that their is no real way around introducing a new function+interface because we can't alter the existing one and don't have access to private functions from the download package. I do think that the naming could emphasize a bit more that the AuthHeader basically provides the same functionality as Prepare with a different signature.
I think calling bearerAuthPlugin.AuthHeader from inside the bearerAuthPlugin.Prepare could also help emphasize this (even though it would make the code a bit more verbose), i.e. getting a header internally from AuthHeader and adding the key values to the request header.

I restructured that a bit so now they both call the same function (addAuthorizationHeader) that adds to the header map. I hope that clearly show that both functions (Prepare(req) and AuthHeader) essentially do the same thing.

The changes look good @gitu. As @DerGut mentioned, I was also wondering about the new httpHeaderAuthPlugin interface. The AuthHeader method implemented in the bearer token plugin essentially does what Prepare does. I'm wondering in download/oci_download.go if we created a HTTP request and passed that to plugin.Prepare and then extracted the necessary headers and added them to docker.WithAuthHeader, will this not work? If it does, then we don't need the new interface or any changes in plugins/rest/auth.go. WDYT?

[gitu]

The changes look good @gitu. As @DerGut mentioned, I was also wondering about the new httpHeaderAuthPlugin interface. The AuthHeader method implemented in the bearer token plugin essentially does what Prepare does. I'm wondering in download/oci_download.go if we created a HTTP request and passed that to plugin.Prepare and then extracted the necessary headers and added them to docker.WithAuthHeader, will this not work? If it does, then we don't need the new interface or any changes in plugins/rest/auth.go. WDYT?

@ashutosh-narkar that was my initial implementation but then that would effect all plugins and not only plugins/rest/auth.go. With the proposed solution the plugin itself can decide if it want that header added to the token flow.

[ashutosh-narkar]

but then that would effect all plugins and not only plugins/rest/auth.go

@gitu which plugins are you referring to here? In the current implementation only the bearerAuthPlugin implements the httpHeaderAuthPlugin interface. The OCI downloader is expected to support the other authentication methods as well eg. OAuth2. So how will this work with the current changes?

[gitu]

but then that would effect all plugins and not only plugins/rest/auth.go

@gitu which plugins are you referring to here? In the current implementation only the bearerAuthPlugin implements the httpHeaderAuthPlugin interface. The OCI downloader is expected to support the other authentication methods as well eg. OAuth2. So how will this work with the current changes?

@ashutosh-narkar to the 7 plugind that implement the HTTPAuthPlugin

and I'm just not sure on what side effect I would have introduced that way. I'm also not sure on how the flow would look like with the oauth flow for the oci downloader. So I went with the minimal amount of change on existing flows.
If that would now also be required for the Oauth2 workflow we would have to implement that header auth or maybe use one of the other configurations for the DockerAuthorizer beside the WithClient one. But I was just focused on the problem that I could test against an existing registry. What is your opinion regarding that?

[ashutosh-narkar]

and I'm just not sure on what side effect I would have introduced that way. I'm also not sure on how the flow would look like with the oauth flow for the oci downloader. So I went with the minimal amount of change on existing flows.
If that would now also be required for the Oauth2 workflow we would have to implement that header auth or maybe use one of the other configurations for the DockerAuthorizer beside the WithClient one. But I was just focused on the problem that I could test against an existing registry. What is your opinion regarding that?

@gitu your reasoning seems fair. @DerGut if we were to use Prepare instead of a new interface as described in this comment to get the headers do you see any side effects from this? I would ideally like to avoid creating a new interface and have it currently implemented by only one authentication method when the OCI downloader supports most other authentication methods.

[ashutosh-narkar]

and I'm just not sure on what side effect I would have introduced that way. I'm also not sure on how the flow would look like with the oauth flow for the oci downloader. So I went with the minimal amount of change on existing flows.
If that would now also be required for the Oauth2 workflow we would have to implement that header auth or maybe use one of the other configurations for the DockerAuthorizer beside the WithClient one. But I was just focused on the problem that I could test against an existing registry. What is your opinion regarding that?

@gitu your reasoning seems fair. @DerGut if we were to use Prepare instead of a new interface as described in this comment to get the headers do you see any side effects from this? I would ideally like to avoid creating a new interface and have it currently implemented by only one authentication method when the OCI downloader supports most other authentication methods.

@DerGut any thoughts here?

[DerGut]

The problem is that docker.WithAuthHeader() expects a static header that we need to compute on setup and we would need to make up an *http.Request to pass to Prepare.
We also don't have access to the private rest.HTTPAuthPlugin implementations from the download package. So we can't decide to only run Prepare on the bearerAuthPlugin.

Other plugins make requests to external services during the Prepare call (e.g. to get tokens from a cloud provider IMDS instance). There shouldn't be any side-effects that would break future calls to Prepare but since some plugins expect very specific requests (e.g. the aws authPlugin variants expect a number of http headers), a call to Prepare with a fake request, just to setup the docker.Authorizer is likely to fail. See

var req http.Request
if err := plugin.Prepare(&req); err != nil {
  // We can only assume that a plugin other than bearerAuthPlugin was used
}

opt := docker.WithAuthHeader(req.Header)

We should be able to ignore the errors without problem but it feels odd that this is the only mechanism to decide whether we can use the modified request to call docker.WithAuthHeader or not.

---

But I agree, that this approach could get rid of some additional complexity introduce by another interface. I tried follow a similar approach but only building the docker.Authorizer after we have gathered the necessary data (i.e. a valid request) in this draft: gitu#2

What do you think @gitu @ashutosh-narkar?

[ashutosh-narkar]

@DerGut your approach described here seems like a good path to follow. @gitu WDYT?

[gitu]

@DerGut thanks for your rewrite, that basically did my naive initial solution in a much nicer way.

[ashutosh-narkar]

@gitu @DerGut the changes look good. Can y'all please squash the commits and checkout the changes in plugins/rest/auth.go and we can get this in. So the only files changed in the PR should be download/oci_download.go, download/oci_download_test.go and download/testharness.go. Thanks!

[ashutosh-narkar]

Also can you please fix the DCO check failure https://github.com/open-policy-agent/opa/pull/6045/checks?check_run_id=15183106996.

[ashutosh-narkar]

LGTM. Thanks @gitu and @DerGut!