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

Feature request: ssl-client-cert header optional when using client cert authentication #1714

Closed
sander-su opened this issue Nov 16, 2017 · 6 comments · Fixed by #1722 or #1734
Closed

Comments

@sander-su
Copy link

sander-su commented Nov 16, 2017

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/.):

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.):


Is this a BUG REPORT or FEATURE REQUEST? FEATURE REQUEST:

NGINX Ingress controller version:
0.9.0-beta.17

Kubernetes version (use kubectl version):
1.7.10

Environment:
not relevant

What happened:
When using TLS client cert auth several headers are added.
One of these headers is: ssl-client-cert which contains $ssl_client_raw_cert.

Our application seems not being able to cope with the multiline cert header (I think, or header size or something). When the header is not send is works fine.
So I would like the option of not sending the raw cert header when using client cert auth.
Now the DN is also sent it is not always needed to send the raw cert. It only add to the size of the request which is also not desirable.

As a workaround I tried overwriting the header with a set-header configmap but this does not seem to work. Currently my workaround will be a custom nginx.conf.tmpl but this is something I do not like for obvious reasons.

What you expected to happen:

How to reproduce it (as minimally and precisely as possible):
Use a client cert for authentication and use the echoservice to check if the headers are send to the backend.

Anything else we need to know:

@sander-su sander-su changed the title Feature request: ssl-client-cert optional when using client cert authentication Feature request: ssl-client-cert header optional when using client cert authentication Nov 16, 2017
@LeonMelis
Copy link

I am experiencing the same issue.

When enabling the ingress.kubernetes.io/auth-tls-secret, nginx-ingress-controller sends the entire SSL certificate to the webserver in the header. In our case this is Apache, which will return a 400 because it considers it an invalid header:

Output of nginx-ingress-controller with --v=5:

"GET / HTTP/1.1
Host: [removed]
ssl-client-cert: -----BEGIN CERTIFICATE-----
[removed]
-----END CERTIFICATE-----

ssl-client-verify: SUCCESS
ssl-client-dn: [removed]
Connection: close
X-Real-IP: 10.132.0.3
X-Forwarded-For: 10.132.0.3
X-Forwarded-Host: [removed]
X-Forwarded-Port: 443
X-Forwarded-Proto: https
X-Original-URI: /
X-Scheme: https
user-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0
accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
accept-language: en-US,en;q=0.5
accept-encoding: gzip, deflate, br
upgrade-insecure-requests: 1
pragma: no-cache
cache-control: no-cache
cookie: [removed]

Verbose error log of Apache

[Fri Nov 17 17:12:06.538255 2017] [core:debug] [pid 357] protocol.c(959): (22)Invalid argument: [client 10.0.1.82:57108] Failed to read request header line ssl-client-cert: -----BEGIN CERTIFICATE-----
[Fri Nov 17 17:12:06.538344 2017] [core:debug] [pid 357] protocol.c(1312): [client 10.0.1.82:57108] AH00567: request failed: error reading the headers
10.0.1.82 - - [17/Nov/2017:17:12:06 +0000] "GET / HTTP/1.1" 400 0 "-" "-"

When I remove the ingress.kubernetes.io/auth-tls-secret annotation, the service works as expected, but of course without client certificate authentication

@ppknap
Copy link

ppknap commented Nov 20, 2017

I had the same issue. Looks more like a bug introduced in #1572 when ssl_client_escaped_cert was changed to ssl_client_raw_cert. The raw PEM format inside the HTTP header causes many server parsers to break(in my case Go server failed with invalid mime-type error).

Is there a reason why ssl_client_escaped_cert was replaced (other than additional boilerplate putting the cert to correct PEM format on the server side)?

@aledbf
Copy link
Member

aledbf commented Nov 20, 2017

@rikatz ping

@rikatz
Copy link
Contributor

rikatz commented Nov 20, 2017

@aledbf @ppknap I'm checking here. The behaviour must be the same as in Apache with RequestHeader set SSL_CLIENT_CERT "%{SSL_CLIENT_CERT}s".

I'll make some tests here to return you the right directive :)

@rikatz
Copy link
Contributor

rikatz commented Nov 20, 2017

So, the behaviour between Apache and NGINX are different.

According to NGINX Doc, the previously used variable (ssl_client_cert) is going to be deprecated, and they have replaced that to ssl_client_escaped_cert and ssl_client_raw_cert.

ssl_client_raw_cert doesn't work as a valid header for none of the tested environments here (made some tests with python and golang), while ssl_client_escaped_cert replaces some 'invalid' characters with URL Encoded characters (like %20 instead of spaces).

The thing here is that, any of the changes is going to cause impacts in working applications. The right way now is to return to ssl_client_escaped_cert (as the currently configuration is not working anyway), but we need to figure out with NGINX if ssl_client_cert is really going to be deprecated, and if there's going to be another alternative to this.

The ticket that changed this in NGINX code was this: http://hg.nginx.org/nginx/rev/82f0b8dcca27

My opinion is that we should have kept ssl_client_cert in nginx until they announce the final deprecation of this variable, and then replace to nginx_client_escaped_cert, but as this has been made yet, I'm going to open a PR this afternoon to revert nginx.tmpl to use nginx_client_escaped_cert.

@sander-su
Copy link
Author

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants