fix #6617 and #6659

[SamMHD]

closes #6617 and #6659

---

In order to fix #6617 I have changed how dagRoute.DisableAuth is caclulated.

Also, to fix #6659 I thought it is needed to change behavior when we are upgrading a request to HTTPS (when permitInsecure is not set and we are redirecting HTTP to HTTPS). Thereby, when dagRoute.AuthContext/dagRoute.AuthDisabled is configured it will affect redirection as well -- before these changes, redirection won't care about them.

---

Changes:

• use dagRoute's AuthContext and AuthDisabled in HTTPS-Upgrade to fix 6659
• Use globalExtAuth.AuthPolicy.Disabled to calculate dagRoute.AuthDisabled
• Fix Tests

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.03%. Comparing base (db432cc) to head (2827b9c).
Report is 48 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6661      +/-   ##
==========================================
- Coverage   81.76%   81.03%   -0.74%     
==========================================
  Files         133      133              
  Lines       15944    20016    +4072     
==========================================
+ Hits        13037    16220    +3183     
- Misses       2614     3503     +889     
  Partials      293      293              

Files with missing lines	Coverage Δ
internal/dag/httpproxy_processor.go	91.00% <100.00%> (-0.41%)	⬇️
internal/envoy/v3/route.go	81.00% <100.00%> (+0.97%)	⬆️
internal/featuretests/v3/envoy.go	99.13% <100.00%> (-0.16%)	⬇️

... and 124 files with indirect coverage changes

[SamMHD]

@sunjayBhatia can you please add required labels to this one?
cc: @skriss @rajatvig @davinci26

[SamMHD]

Thank you @tsaarni for the label.
When can we proceed to merge?

[tsaarni]

@SamMHD I will try to arrange time for myself to review ASAP today or tomorrow!

[SamMHD]

Thank you so much @tsaarni

[tsaarni]

Looks good!

Regarding testing the fix: Would it be possible to add a new test case in the e2e test suite that would have caught this bug. There are global ext auth tests here but they currently only cover disabled: false.

Also tagging @clayton-gonsalves for your input, as your review would be valuable since you originally implemented this functionality.

[SamMHD]

Thank you for your feedback; I'll add the tests as suggested.

I previously attempted to write tests for this scenario but found the existing test functions a bit incompatible to implement disabled: true for GlobalAuth, which led me to initially omit them. Now that you've requested it, I'll revisit this and include the necessary tests.

[tsaarni]

I previously attempted to write tests for this scenario but found the existing test functions a bit incompatible to implement disabled: true for GlobalAuth, which led me to initially omit them. Now that you've requested it, I'll revisit this and include the necessary tests.

Exactly, the existing test functions make no sense for disabled: true. There could be a new one to test this behaviour specifically.

[SamMHD]

Exactly, the existing test functions make no sense for disabled: true. There could be a new one to test this behaviour specifically.

I think I have implemented a workaround to this by making the test function more flexible.
I'll push the changes now, hope you find time to review this, too.

Btw, Do you think we still need to add this to e2e tests?

[SamMHD]

@tsaarni Do have time to review the changes again?

[SamMHD]

@tsaarni @sunjayBhatia @skriss is there any update on this?

[SamMHD]

Why nobody answer us? :(((
@tsaarni @sunjayBhatia @skriss

[tsaarni]

Sorry @SamMHD I haven’t had a chance to review yet, I've been swamped 😅

[SamMHD]

thank you @tsaarni , when do you think you can give it a look?

[SamMHD]

Hello again, Is there any update on this PR? @tsaarni

[tsaarni]

Hi @SamMHD and apologies once more for the delays! I've now created an environment to test this PR. It works well, however it seems override can only be done at the route level and not at the virtual host level. This example does not work:

apiVersion: projectcontour.io/v1
kind: HTTPProxy
metadata:
  name: echoserver
spec:
  virtualhost:
    fqdn: protected.127-0-0-101.nip.io
    tls:
      secretName: ingress
    authorization:
      authPolicy:
        disabled: false
  routes:
    - services:
        - name: echoserver
          port: 80

It seems that with this scenario we end up in this code branch where it will try use the uninitialized extension name from HTTPProxy.spec.virtualhost.authorization.extensionRef for the virtualhost and fail.

[SamMHD]

Very good point dear @tsaarni , I have visited that code branch and fixed it, other than this one more thing came to my attention, and it was that "If we disable global auth in vhost configuration then we can not enable it back again in a route" so I have fixed it in "else clause" of same code branch.

Can you give it a try or help me setup your test environment for myself? I think it should be fixed now.

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 14d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
• Mark this PR as fresh by commenting or pushing a commit
• Close this PR
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[SamMHD]

dear @tsaarni , do you have any update on this PR?

[izturn]

/lgtm

[SamMHD]

Thank you so much @izturn 🙌
Are we going to merge or we need additional approvals?

[tsaarni]

@SamMHD Looks very good! I re-tested the PR and it worked perfectly now. I still added suggestions for the change log to hopefully highlight the change bit more clearly to the users, please check and see what you think.

[tsaarni]

Suggested change

	Global external authorization or vhost-level authorization is enabled by default unless an AuthPolicy explicitly disables it. By default, `disabled` is set to `GlobalExtAuth.AuthPolicy.Disabled`. This global setting can be overridden by vhost-level AuthPolicy, which can further be overridden by route-specific AuthPolicy. Therefore, the final authorization state is determined by the most specific policy applied at the route level.
	Global external authorization can now be disabled by default and enabled by overriding the vhost and route level auth policies.
	This is achieved by setting the `globalExtAuth.authPolicy.disabled` in the configuration file or `ContourConfiguration` CRD to `true`, and setting the `authPolicy.disabled` to `false` in the vhost and route level auth policies.
	The final authorization state is determined by the most specific policy applied at the route level.

[tsaarni]

Suggested change

	## Disable External Authorization in UpgradeHTTPS
	From now on, Contour will configure Envoy to handle HTTPS Redirection without authorization on routes. (previously if GlobalExtAuth was set, Envoy would check request with ext_auth before redirection which could result in 401 instead of redirection)
	## Disable External Authorization in HTTPS Upgrade
	When external authorization is enabled, no authorization check will be performed for HTTP to HTTPS redirection.
	Previously, external authorization was checked before redirection, which could result in a 401 Unauthorized error instead of a 301 Moved Permanently status code.