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

gRPC: header specified in auth-response-headers is not rewritten #3914

Closed
kd7lxl opened this issue Mar 21, 2019 · 10 comments · Fixed by #7331
Closed

gRPC: header specified in auth-response-headers is not rewritten #3914

kd7lxl opened this issue Mar 21, 2019 · 10 comments · Fixed by #7331
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@kd7lxl
Copy link
Contributor

kd7lxl commented Mar 21, 2019

Is this a request for help? (If yes, you should use our troubleshooting guide and community support channels, see https://kubernetes.io/docs/tasks/debug-application-cluster/troubleshooting/.): no

What keywords did you search in NGINX Ingress controller issues before filing this one? (If you have found any duplicates, you should instead reply there.): grpc


Is this a BUG REPORT or FEATURE REQUEST? (choose one): BUG REPORT

NGINX Ingress controller version: 0.21

What happened:
When using a gRPC backend, external auth, and auth-response-headers, the specified header is not rewritten with the one from the auth response. The original request header is passed through unmodified.

What you expected to happen:
Headers defined in auth-response-headers should be rewritten from the auth response.

How to reproduce it (as minimally and precisely as possible):

kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/auth-response-headers: Authorization
    nginx.ingress.kubernetes.io/auth-url: http://authn.authn.svc/verify
    nginx.ingress.kubernetes.io/backend-protocol: GRPC
...

Anything else we need to know:
This is caused because the auth-response-header template uses proxy_set_header instead of grpc_set_header.

Here is an example from the generated nginx conf:

            proxy_set_header 'Authorization' $authHeader0;
            
            client_max_body_size                    1m;
            
            grpc_set_header Host                   $best_http_host;
            
            # Pass the extracted client certificate to the backend
            
            # Allow websocket connections
            grpc_set_header                        Upgrade           $http_upgrade;
            grpc_set_header                        Connection        $connection_upgrade;

We can see that Authorization uses proxy_set_header while the other headers correctly use grpc_set_header.

@aledbf
Copy link
Member

aledbf commented Mar 21, 2019

When using a gRPC backend, external auth, and auth-response-headers, the specified header is not rewritten with the one from the auth response.

There is no support for gRPC auth, the annotation only works for HTTP

@aledbf aledbf added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 21, 2019
@kd7lxl
Copy link
Contributor Author

kd7lxl commented Mar 25, 2019

A workaround for this bug is to manually add the missing line with a configuration snippet annotation:

    nginx.ingress.kubernetes.io/configuration-snippet: |
      grpc_set_header 'Authorization' $authHeader0;

@kd7lxl
Copy link
Contributor Author

kd7lxl commented Apr 15, 2019

Why is this kind/feature? The docs don't mention anything about this combination being mutually exclusive, and the code attempts to configure external auth with gRPC backends, it just does it wrong. Since the implementation exists, but is broken, it should be classified as a bug.

@aledbf aledbf added kind/bug Categorizes issue or PR as related to a bug. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Apr 15, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 15, 2019
@kd7lxl
Copy link
Contributor Author

kd7lxl commented Jul 15, 2019

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 15, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 13, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 12, 2019
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@derekperkins
Copy link
Contributor

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
5 participants