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

Auth support in frontends for k8s and file #3460

Merged

Conversation

Zatte
Copy link
Contributor

@Zatte Zatte commented Jun 8, 2018

What does this PR do?

Adds functionality to have (all) authentication methods configurable at the front-end layer (instead of the entrypoint level). Support available in "File" and "Kubernetes" backends.

Motivation

Need to have fine grained "service level"/"frontend" access control, most common request is to use "Forwarder" authentication on a per frontend basis; Full discussion in #2162

Related to #2116, #2162 , #2734, #1465

More

  • Added/updated tests
  • Added/updated documentation

I updated docs for the providers "File" and "Kubernetes" since these are my primary use cases. Updating the remaining providers should be easy but not something i can not allocate time for.

Additional Notes

Not sure how to best handle the existing BasicAuth logic. For now i left BasicAuth as is since any changes would break configuration contracts currently in place. This means that Basic Auth can be configured in 2 places (adding confusion for users).

For Kubernetes I moved BasicAuth to use the new (generic) auth method as it didn't change the configuration contract (k8 annotations)

I'm feeling hesitant going further with this PR without maintainer feedback.

I have updated but not rebuilt the docs.

@Zatte Zatte requested a review from a team as a code owner June 8, 2018 14:22
@Zatte Zatte changed the title Auth support in frontends (Generic) Auth support in frontends Jun 8, 2018
@Zatte
Copy link
Contributor Author

Zatte commented Jun 9, 2018

Please note that the integration tests run (at least in my environment) but the test-server seem to have had issues:
https://semaphoreci.com/containous/traefik/branches/pull-request-3460/builds/1
if [ -n "$SHOULD_TEST" ]; then ci_retry make pull-images; fi
=>

Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
make: *** [pull-images] Error 123
make pull-images failed, attempt 3/3```

@ldez
Copy link
Member

ldez commented Jun 9, 2018

@Zatte

---> Making bundle: validate-golint (in .)
Errors from golint:
provider/kubernetes/builder_configuration_test.go:224:25: func parameter forwardUrl should be forwardURL
testhelpers/config.go:147:1: comment on exported function WithFrontEndAuth should be of the form "WithFrontEndAuth ..."

Please fix the above errors. You can test via "golint" and commit the result.

make: *** [validate] Error 1
make validate failed, attempt 3/3

@Zatte
Copy link
Contributor Author

Zatte commented Jun 10, 2018

Thanks @ldez , I fixed those issues and other issues appeared, they do however seem unrelated to the changes in this PR.

----------------------------------------------------------------------
FAIL: acme_test.go:160: AcmeSuite.TestOnDemandRetrieveAcmeCertificateWithWildcard

acme_test.go:167:
    s.retrieveAcmeCertificate(c, testCase)
acme_test.go:286:
    c.Assert(err, checker.IsNil)
... value *errors.errorString = &errors.errorString{s:"try operation failed: domain traefik.acme.wtf found instead of *.acme.wtf"} ("try operation failed: domain traefik.acme.wtf found instead of *.acme.wtf")
----------------------------------------------------------------------

I can dive into this if it is not an already known issue. The acme code seem to be somewhat volatile, is there a preferred branch i can rebase on?

@ldez
Copy link
Member

ldez commented Jun 18, 2018

@Zatte Could you rebase on master?

You also have to add the feature for all providers:

  • File
  • k8s
  • Docker
  • Rancher
  • Consul Catalog
  • Marathon
  • Mesos
  • ECS
  • KV

@Zatte
Copy link
Contributor Author

Zatte commented Jun 18, 2018

Then i assume the current approach is acceptable, and no major architecture changes are planned.

I'll start looking into adding more providers but have limited time to allocate on this PR atm. Contributions welcome.

Todo (I will keep this updated as I progress):

  • Rebase
  • File
  • k8s
  • KV
  • Docker
  • Rancher
  • Consul Catalog
  • Marathon
  • Mesos
  • ECS

Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.

Here is some k8s feedback!

@@ -9,6 +9,10 @@ const (
annotationKubernetesAuthRealm = "ingress.kubernetes.io/auth-realm"
annotationKubernetesAuthType = "ingress.kubernetes.io/auth-type"
annotationKubernetesAuthSecret = "ingress.kubernetes.io/auth-secret"
annotationKubernetesForwardAuthURL = "ingress.kubernetes.io/auth-forward-url"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In keeping in line with other ingress controllers, perhaps auth-url should be used instead: (https://github.com/kubernetes/ingress-nginx/blob/master/docs/user-guide/nginx-configuration/annotations.md)

annotationKubernetesForwardAuthURL = "ingress.kubernetes.io/auth-forward-url"
annotationKubernetesForwardAuthTrustHeaders = "ingress.kubernetes.io/auth-forward-trust-headers"
annotationKubernetesForwardAuthTLSCert = "ingress.kubernetes.io/auth-forward-tls-cert"
annotationKubernetesForwardAuthTLSKey = "ingress.kubernetes.io/auth-forward-tls-key"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Private keys should never be used in annotations.

These should be in secrets, and referenced via a auth-tls-secret annotation, just like with basic auth :)

func handleForwardAuthConfig(i *extensionsv1beta1.Ingress, k8sClient Client) (*types.Auth, error) {
ForwardAuth := &types.Forward{}

ForwarURL := getStringValue(i.Annotations, annotationKubernetesForwardAuthURL, "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ForwarURL and not ForwardURL?

@ldez ldez force-pushed the forward_auth_in_frontends branch 4 times, most recently from 1338c69 to 54438e9 Compare June 29, 2018 09:44
@ldez ldez added this to the 1.7 milestone Jun 29, 2018
@ldez ldez changed the title (Generic) Auth support in frontends Auth support in frontends Jun 29, 2018
@ldez
Copy link
Member

ldez commented Jun 30, 2018

Support for other providers will be in another PR.

@mmatur mmatur force-pushed the forward_auth_in_frontends branch from da1c2b3 to a4912df Compare July 2, 2018 09:21
Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@traefiker traefiker merged commit bb14ec7 into traefik:master Jul 2, 2018
@khuey
Copy link

khuey commented Jul 2, 2018

Is there code in flight for docker anywhere? I'll do it myself if not but I don't want to duplicate someone's work if so.

@ldez
Copy link
Member

ldez commented Jul 2, 2018

@khuey we are working to add the support for all other providers.
A PR will coming soon.

@jbdoumenjou jbdoumenjou mentioned this pull request Jul 4, 2018
11 tasks
@Cas-pian

This comment has been minimized.

@mannharleen

This comment has been minimized.

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

Successfully merging this pull request may close these issues.

None yet

9 participants