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

Eastwest gateway incorrectly applies DR #38704

Open
nmittler opened this issue May 3, 2022 · 6 comments
Open

Eastwest gateway incorrectly applies DR #38704

nmittler opened this issue May 3, 2022 · 6 comments
Assignees
Labels
area/networking lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed

Comments

@nmittler
Copy link
Contributor

nmittler commented May 3, 2022

Bug Description

If mTLS for a destination is enabled via a DR in a namespace other than istio-system, the eastwest gateway will never get endpoints for the destination service.

This is due to the fact that the eastwest gateway still applies endpoint filters, which use the mtlsChecker. East-west's mtlsChecker will have no DR for the service (since the DR was not applied to the istio-system namespace) and will therefore assume the default non-mTLS setting, which means that the endpoint will be filtered from the east-west gateway.

Judging by the comment in that code, it appears this was intentional. However, I'm not sure that this particular case was considered or tested. Other parts of the logic for eastwest gateway ignore DR, and I suspect this should as well.

Version

head

Additional Information

No response

@nmittler
Copy link
Contributor Author

nmittler commented May 5, 2022

@howardjohn do you know if there is anything that can be done here? I know that this logic was added to deal with a vul.

@howardjohn
Copy link
Member

@stevenctl I think the DR part specifically is actually not for the CVE, but rather to avoid accidentally sending non-mTLS traffic to EW gw (which fails). So to have that same check at the EW gw may not make sense?

Just the DR part - the rest is required.

@nmittler
Copy link
Contributor Author

@stevenctl are you still looking into this? Can I assign to you?

@nmittler
Copy link
Contributor Author

nmittler commented Jul 7, 2022

@stevenctl gentle ping ... are you still looking at this? We had to roll-back the change.

@stevenctl
Copy link
Contributor

#39829 (comment)

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Oct 7, 2022
@stevenctl
Copy link
Contributor

not-stale. Need to verify #40243 fixed this. I don' think it did.

@istio-policy-bot istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Oct 19, 2022
@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jan 18, 2023
@zirain zirain added lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed and removed lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while labels Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed
Projects
None yet
Development

No branches or pull requests

5 participants