When no Accept Header is set in the original request, 2 Accept headers will be added by the plugin

[pokerjocke70]

If you make a request without the Accept header set the plugin will try to add a '*/*' Accept header to the original request but that seem not to work as expected because we end up with application/jwt being added as well and also there are some garbage at the end of the '*/*' value.

// Accept header set, orignal accept header left untouched after introspection

curl -H 'Accept: application/json' --header 'Content-Type: application/json' --header 'Authorization: Bearer *****'  'https://myhost/httpbin/headers?show_env=1'
{
  "headers": {
    "Accept": "application/json",
    "Content-Type": "application/json", 
    "Host": "myhost", 
    "User-Agent": "curl/7.68.0", 
    "X-Original-Uri": "/httpbin/headers?show_env=1", 
    "X-Scheme": "https"
  }
}

// No accept header, Curity adds 2 Accept headers after introspection

curl -H 'Accept:' -H 'Authorization: Bearer *****'  'https://myhost/httpbin/headers?show_env=1'
{
  "headers": {
    "Accept": "application/jwt,*/*\u0000tok",
    "Content-Type": "application/json", 
    "Host": "myhost", 
    "User-Agent": "curl/7.68.0", 
    "X-Original-Uri": "/httpbin/headers?show_env=1", 
    "X-Scheme": "https"
  }
}

The problem seems to originate from the code https://github.com/curityio/nginx_phantom_token_module/blob/master/phantom_token.c#L271

Looking at the code it seems a bit strange that the plugin should alter the original request.

Versions:

Curity: 1.1.0
Nginx: 1.19.6
K8s ingress-nginx-controller: 0.44.0

[travisspencer]

Thanks, @pokerjocke70 , for the report. This is certainly a bug due to the refactor in 6f379c3.

Can you build and test the changes on the accept-header-issue-48 branch? I think that will fix your problem. Meanwhile, we'll see if we can get the subrequest's Accept header to not add application/jwt into the downstream request's Accept header. Shouldn't hurt anything to be there now (unless your service returns JWTs, which I wouldn't expect it to do).

[pokerjocke70]

ok, thanks. We will try to build it today and test it, however for us to move forward we need to get rid of application/jwt value as well. The preferred way (for us at least ) would be that you leave the original request untouched. Why do you even add an Accept header at all?

[travisspencer]

If your calling a standard-compliant HTTP service, I don't think it'll be an issue. The spec says:

If more than one media range applies to a given type, the most specific reference has precedence.

So, if the request includes:

Accept: application/jwt
Accept: */*

Then, the HTTP service should service whatever it has unless it really can return the requested resource as a JWT (which I highly doubt in all cases).

Why do you even add an Accept header at all?

This reason is, as tried to explain: The subrequest adds application/jwt and that updates the main one that is used when calling downstream. Basically, it's an NGINX thing. I'm trying to figure out how to avoid that, but given the above explanation of how the HTTP service receiving the request should process it, I don't see it as a problem.

[pokerjocke70]

Yes, it will probably work with both the 2 new headers. We are moving from an old api gateway that does not tamper with the request so preferably we would like the new gateway to do the same. And the application/jwt is only leaked into the request when trying to add the '/' and that seems to be done after the response from the Curity call.

What if this code:

if (request->headers_in.accept == NULL)
                {
                    ngx_int_t result;

                    if ((result = set_accept_header_value(request, "*/*") != NGX_OK))
                    {
                        return result;
                    }
                }
                else
                {
                    request->headers_in.accept->value = module_context->original_accept_header;
                }

was changed to:

// Only restore the original header, don't add any new header

if (request->headers_in.accept != NULL)
{
        request->headers_in.accept->value = module_context->original_accept_header;
}

[travisspencer]

it will probably work with both the 2 new headers

I don't disagree that what's being done here is sloppy, but the HTTP spec tells me it's gonna be OK.

We are moving from an old api gateway that does not tamper with the request so preferably we would like the new gateway to do the same

I don't think this matters if the downstream services comply with HTTP and don't product JWTs.

What if this code...was changed to:

Then, the request would look like this:

Accept: applicaiton/jwt

The way NGINX works is that the subrequest's headers are the same as the main one's (the new subrequest's headers point to the same headers as the main request). So, once the subrequest is created and the Accept header is set to application/jwt, the main requests headers are also updated. Because the main request had no Accept header, the downstream call will only be for a JWT, which won't work.

I think this would have gone unnoticed if it weren't for the bug in setting the length of the string in the Accept header with a value of */*. That was tacking on garbage to the end, so the client (after being proxied) was saying to the downstream service that it only accepted application/jwt and */*Foo. The HTTP server didn't have a representation of the resource that matched either of these media types, so it returned a 406.

With the change I've made now, the HTTP server will be told that the client accepts anything, but prefers JWTs (due to the sloppiness). The server doesn't have a JWT representation of any resource, so it will see that the client accepts anything and will return a non-error result.

I'll look a bit more into how to make this subrequest's header changes to not affect the main request, but please test it. I'm 99% it will solve your problem.

[pokerjocke70]

We have tested the fix now and can verify that is solves the problem with the invalid characters. Thanks.

[travisspencer]

Great, @pokerjocke70. Shall we make a release, so you can use that? We can continue to cleanup this sloppiness in the meantime, but it would feel good to get a release that unblocks you. Should we do that?

[pokerjocke70]

Please go ahead with the release.

[anestos]

Solved by #49