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

fix: rewrite Host header in dex reverse proxy #6183

Merged
merged 2 commits into from
May 12, 2021

Conversation

okhaliavka
Copy link
Contributor

@okhaliavka okhaliavka commented May 7, 2021

Fixes #3975

In Istio, HTTP traffic is routed to a cluster based on Host header. Dex reverse proxy does not rewrite Host header, so traffic does not get routed to argocd-dex-server cluster and no cluster-level configuration (e.g. mTLS) is applied. Because of this, request to dex-server fails in environments where strict mTLS is enabled or where outbound traffic policy is set to REGISTRY_ONLY.

Signed-off-by: Alexey Khalyavka [email protected]

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

Alexey Khalyavka added 2 commits May 7, 2021 03:03
Signed-off-by: Alexey Khalyavka <[email protected]>
Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @akhalyavka !

@jannfis jannfis merged commit 9bf83b4 into argoproj:master May 12, 2021
@okhaliavka okhaliavka deleted the dex-proxy-host-header branch May 12, 2021 20:43
@jannfis jannfis self-assigned this Jun 25, 2021
@jannfis jannfis added the needs-verification PR requires pre-release verification label Jun 25, 2021
@alexef
Copy link
Member

alexef commented Sep 10, 2022

I'm still seeing the issue in ArgoCD 2.4.11. Was this fix released?

@thuandt
Copy link

thuandt commented Oct 12, 2022

@alexef me too. I'm still seeing this issue on ArgoCD v2.4.14

@hSATAC
Copy link

hSATAC commented Oct 14, 2022

+1 to this.
According to the commit tags, this was released since v2.1.0
But we're still having this on v2.3.7

@rajshivage
Copy link

v2.1.0

I'm still seeing this issue in v2.4.12

@ankitcharolia
Copy link
Contributor

I still face the issue in version 2.5.7
WORKAROUND: renaming svc/argocd-dex-server port name to TCP. It works fine.

@NadgobKhan
Copy link

@ankitcharolia Hello, I have the same issue (I actually have an Nginx load balancer that listens on port 80 and proxies the connection to https://localhost:8080, where argocd-server resides), but I don't know what exactly to rename. All three port of argocd-dex-server are TCP already. Could you help me please?

@AhmedMItman
Copy link

AhmedMItman commented Feb 25, 2023

@NadgobKhan If you still waiting for the reply,
What @ankitcharolia means is,

You need to change the port name name from

 - name: http
    port: 5556
    protocol: TCP
    targetPort: http

to

 - name: tcp
    port: 5556
    protocol: TCP
    targetPort: http

This is in svc/argocd-dex-server

FYI: It is a workaround,

@AhmedMItman
Copy link

@akhalyavka I Think this ticket needs to be opened again, the issue still exists, please check the previous messages

Thanks in advance.

@mindovermiles262
Copy link

Seeing this issue still. Renaming the port to tcp worked but I think this still needs a fix
ArgoCD v2.6.7+5bcd846

@pre
Copy link

pre commented Apr 13, 2023

With STRICT Istio mTLS, the issue still persists and the workaround is to rename the dex service port to either https or tcp: https://github.com/argoproj/argo-cd/blob/v2.6.7/manifests/ha/install.yaml#L16450

Related:
#3975 (comment)

@okhaliavka
Copy link
Contributor Author

Sorry for coming back to this so late. I must have screwed up my testing, it seems like there is at least one more place in the codebase where the header needs to be rewritten. I'll try to fix it next week.

@arnzchen
Copy link

Still seeing this issue in v2.8.0 after this #13500 was merged in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-verification PR requires pre-release verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to query provider "https://argocd-host/api/dex": 502 Bad Gateway: