Fix #2902: ‘autoRedirect’ hardcode ‘https’ scheme

[icefed]

[GordonTheTurtle]

Please sign your commits following these rules: https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work The easiest way to do this is to amend the last commit:

$ git clone -b "master" [email protected]:icefed/distribution.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

[codecov[bot]]

Codecov Report

Merging #2903 into master will decrease coverage by 0.12%. The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2903      +/-   ##
==========================================
- Coverage   60.45%   60.33%   -0.13%     
==========================================
  Files         102      102              
  Lines        8001     8017      +16     
==========================================
  Hits         4837     4837              
- Misses       2517     2533      +16     
  Partials      647      647

Flag	Coverage Δ
#linux	60.33% <0%> (-0.13%)	:arrow_down:

Impacted Files	Coverage Δ
registry/auth/token/accesscontroller.go	59.06% <0%> (-7.11%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3226863...660e55b. Read the comment docs.

[bitfehler]

I was about to create almost the same PR. This would be really useful, so that a single distribution/docker-auth setup could be used both without TLS on an internal network and with TLS (via a reverse proxy) from the outside.

If someone could maybe make a comment as to what it would take to get this merged? I'd be happy to help get this in shape.

[icefed]

@bitfehler I have rebased the code and added unit tests, someone needs to review it.

[Jamstah]

This seems like a sensible change from a functionality perspective, but I haven't worked on this area of the code before, so can you help me understand the need?

Are you running the registry and an auth server behind the same proxy, with the auth server at /auth/token?

Is there a reason you aren't using TLS internally, which I would have considered to be good practice?

Note the OAuth2 spec calls out sending secrets in the plain: https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2.1

I do wonder if forcing a manually set http realm is better than allowing an http realm by default.

[bitfehler]

This seems like a sensible change from a functionality perspective, but I haven't worked on this area of the code before, so can you help me understand the need?

Are you running the registry and an auth server behind the same proxy, with the auth server at /auth/token?

Sort of, except I am re-writing /auth/token to /auth, because that's what docker-auth wants, and I am now actually much more concerned about the /auth/token path hardcoded in the autoredirect code path than the TLS part. But, hey... :slightly_smiling_face:

Is there a reason you aren't using TLS internally, which I would have considered to be good practice?

I am actually using TLS everywhere now, but I still think having the choice would be nice. Making it more explicit does sound reasonable, though. Just as an idea, i think my favorite solution would be to be able to set realm to e.g. https://{host}/auth, and then the {host} gets replaced with the value of the host header. That way one would have very explicit control over everything, but with the flexibility of autoredirect (or, in fact, even more).

[Jamstah]

I suppose you could template it with {scheme} and {port} as well if you need to.

I'm still a little unsure that it would get many users - what's the need for accessing the same repo from two different hosts?

[bitfehler]

I'm still a little unsure that it would get many users

That I cannot judge, but the feature is already present with autoredirect. I would say the suggested approach is just fixing a shortcoming in that feature.

what's the need for accessing the same repo from two different hosts?

For me, right now, resiliency: having an internal domain for internal services (where the routing involves much fewer moving parts). And, though I am not doing this right now, I was entertaining the idea of routing the /auth endpoint for the external domain to a separate auth instance, to be able to apply different rules (e.g. internal one allows anonymous access, external doesn't).

[Jamstah]

This code hasn't been touched for a long time really, so picking some recent maintainers to request a review.

[icefed]

@Jamstah

I guess its not enabled by default, I can see some use cases, and it is an improvement over what's there today.

Sorry, I doesn't get it, are you mean we keep the default behavior to return https and the admin can configure whether or not to enable my modifications?

I would never recommend going without TLS in production, even internally, but for poc/dev environments its not so bad.

Yes, when I submitted the PR, it was intended for internal deployment and development purposes within the company.

[icefed]

Hi @bitfehler @Jamstah , I have added too config parameters, hopes that will filling your requirements.

// default /auth/token
autoredirectpath
// default false (seems too long...)
autoredirectforcetlsdisabled

[bitfehler]

I am not the authority here, but in my opinion the configuration is becoming way too complicated (from a user perspective).

I don't know if and how the release cycle management restricts the modification/removal of options, but my favorite solution would be this:

• realm gains supports for inserting template parameter {host}, and maybe also {scheme} and {port}
  • This change would be backwards-compatible
  • Currently autoredirect includes the port (i.e. it fills in the entire authority, not just the host, so depending on desired functionality, it should support either {authority}, or, more flexible, {host} and {port}
  • If {scheme} is to be supported, it's fine to just say "looks at incoming request and X-Forwarded-For", folks should know what they are doing when they use this, and the vast majority will just use https://{host}/... anyways
• autoredirect gets removed or at least deprecated with a warning that users should move to templated realm

Just my 2 cents...

[icefed]

I am not the authority here, but in my opinion the configuration is becoming way too complicated (from a user perspective).

I don't know if and how the release cycle management restricts the modification/removal of options, but my favorite solution would be this:

* `realm` gains supports for inserting template parameter `{host}`, and maybe also `{scheme}` and `{port}`
  
  * This change would be backwards-compatible
  * Currently `autoredirect` includes the port (i.e. it fills in the entire authority, not just the host, so depending on desired functionality, it should support either `{authority}`, or, more flexible, `{host}` and `{port}`
  * If `{scheme}` is to be supported, it's fine to just say "looks at incoming request and X-Forwarded-For", folks should know what they are doing when they use this, and the vast majority will just use `https://{host}/...` anyways

* `autoredirect` gets removed or at least deprecated with a warning that users should move to templated `realm`

Just my 2 cents...

@bitfehler Remove autoredirect is a breaking change, it will affect existing users, I don't think it's a good idea.

configuration is becoming way too complicated, remove autoredirectforcetlsdisabled may be simpler, then we let admin being aware of X-Forwarded-For header, otherwise redirect to https.

If you want custom redirect path, you can set autoredirectpath to /auth.