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

Disable HTTP Basic Auth on CORS pre-flight OPTIONS request #356

Merged
merged 1 commit into from
Aug 1, 2019

Conversation

peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Jul 15, 2019

port of #354 to master

When CORS is enabled on a k8s ingress definition that also has Basic Auth, the connection from a browser fails because the browser (correctly) does not send the Basic Auth credentials on the pre-flight OPTIONS request.

Example annotations to recreate:

    ingress.kubernetes.io/auth-realm: Authentication Required
    ingress.kubernetes.io/auth-secret: somesecret
    ingress.kubernetes.io/auth-type: basic
    ingress.kubernetes.io/cors-allow-credentials: "true"
    ingress.kubernetes.io/cors-allow-methods: POST,PUT,GET,DELETE
    ingress.kubernetes.io/cors-allow-origin: '*'
    ingress.kubernetes.io/enable-cors: "true"

This PR suggests updating the http-request auth rule to be disabled for OPTIONS requests when CORS is enabled on a backend definition. This would make haproxy consistent with the behaviour in nginx ingresses (we are evaluating a switch from nginx to haproxy to benefit from features only currently available in haproxy).

Questions from PR 354 moved to discussion here:

What kind of features did you find on haproxy you didn't on nginx?

Hitless reload is the key feature driving us to adopt haproxy.

Which issues you have found testing on master?

Swapping the image from the 0.7 branch to one built with this PR, I see this error processing the CORS config:

E0710 22:10:00.594793       8 controller.go:350] unexpected failure restarting the backend: 
template: haproxy-v07.tmpl:727:12: executing "CORS" at <$cors.CorsExportHead...>: can't evaluate field CorsExportHeaders in type *cors.CorsConfig

Related to these lines of the config template:

{{- if $cors.CorsExportHeaders }}
http-response set-header Access-Control-Expose-Headers "{{ $cors.CorsExposeHeaders }}" if {{ $server.ACLLabel }}{{ if $location }}{{ $location.HAMatchTxnPath }}{{ end }}
{{- end }}

@jcmoraisjr
Copy link
Owner

Hi, sorry about the long delay. Couldn't find time to anything else these days during the development of the current snapshot.

The change is fine, there is just a formatting issue on {{- if which would place together if and !METH_OPTIONS without space. I'll release snapshot.3 shortly and will fix this on my own PR.

Hitless reload is the key feature driving us to adopt haproxy.

Great. HAProxy 1.8+ does a nice job here.

can't evaluate field CorsExportHeaders

Sure, I saw the problem here. I didn't use v07 controller anymore so I didn't see this one. snapshot.3 will have this issue fixed.

Regarding rename, git tracks the change and knows that haproxy-v07.tmpl here is haproxy.tmpl there.

Merging now and snapshot.3 is coming soon. I'd like to issue the next v0.7 and v0.6 with haproxy 1.8.21 so I'll wait just a few more days. If we don't have a new release there, I'll tag new releases here anyway.

Thanks for evaluating haproxy-ingress!

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

Successfully merging this pull request may close these issues.

2 participants