Implement the “external” connector (for Apache2 mod_auth etc.) #1100

stapelberg · 2017-10-21T14:59:51Z

In Debian, we’re currently investigating whether we can use dex as part of https://sso.debian.org/ (see also https://wiki.debian.org/DebianSingleSignOn).

In the current setup, users authenticate against Apache2, and the web application behind sso.debian.org issues certificates based on the REMOTE_USER request environment variable.

Hence, to keep (especially user-visible) changes at a minimum, we’d like to keep the setup the same regarding authentication, just replace the certificates part with dex as an OIDC provider (for GitLab and others).

The “external” connector in this pull request accomplishes that; see also the included documentation.

Please consider merging this pull request. Thanks!

srenatus

This is quite interesting. I'm just a curious bystander (not a dex maintainer), but IMHO this looks good. I wonder a little if it would sense to make the header name configurable?

ericchiang · 2017-10-23T02:15:16Z

Yeah does look interesting.

Just trying to understand the use cases. You run a proxy, then if the user ever tries to hit (dex URL)/callback/external you log the user in, setting the X-Remote-User header on the way back?

What's the UX look like for that? How would another provider other than Apache2 mod_auth use this? Do you have examples?

stapelberg · 2017-10-23T05:31:30Z

Just trying to understand the use cases. You run a proxy, then if the user ever tries to hit (dex URL)/callback/external you log the user in, setting the X-Remote-User header on the way back?

This is correct.

What's the UX look like for that?

This depends on the specific authentication method you have configured in Apache2. Common cases include a modal password prompt (e.g. https://user-images.githubusercontent.com/59393/26989841-371d98c2-4d1a-11e7-8182-6901d76081b5.png), a certificate prompt, some sort of smart-card authentication, etc.

How would another provider other than Apache2 mod_auth use this? Do you have examples?

Have a look at https://httpd.apache.org/docs/current/howto/auth.html for an overview of how to configure authentication in apache.

Also note that this connector is not specific to apache, it’s just that we’re using apache in Debian. I could also imagine using this connector to integrate with proprietary web servers / appliances in company intranets.

ericchiang

Did an initial code review. Biggest concerns are:

Lack of a strategy for refresh tokens.
Some examples so we can test this.

I'd like to see the refresh token concern addressed before we move forward.

ericchiang · 2017-10-23T17:48:42Z

server/server.go

@@ -240,6 +241,9 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy)
 	handleFunc("/auth", s.handleAuthorization)
 	handleFunc("/auth/{connector}", s.handleConnectorLogin)
 	handleFunc("/callback", s.handleConnectorCallback)
+	// For easier connector-specific web server configuration, e.g. for the
+	// "external" connector.
+	handleFunc("/callback/{connector}", s.handleConnectorCallback)


This needs to be the ID of the connector not the type, all connectors can be configured multiple times. I can configure two LDAP connectors with IDs activedirectory and freeipa.

It also needs enforce that handleConnectorCallback only allows a session using whatever value {connector} is. If it's /callback/okta that the user actually logging in through okta.

This needs to be the ID of the connector not the type, all connectors can be configured multiple times. I can configure two LDAP connectors with IDs activedirectory and freeipa.

Yes. The /auth/{connector} route assigns connID := mux.Vars(r)["connector"], which is why I chose /callback/{connector} to signify the connector ID. Is that incorrect? Should “connector” be changed to “id”, here and above?

It also needs enforce that handleConnectorCallback only allows a session using whatever value {connector} is. If it's /callback/okta that the user actually logging in through okta.

Done.

ericchiang · 2017-10-23T17:50:07Z

connector/external/external.go

+}
+
+// Refresh updates the identity during a refresh token request.
+func (m *Callback) Refresh(ctx context.Context, s connector.Scopes, identity connector.Identity) (connector.Identity, error) {


Since this can't be implemented, drop the method. We did this in the OpenID Connect connector because refreshes are hard, but regret it. Better to just not support refresh tokens.

ericchiang · 2017-10-23T17:50:18Z

connector/external/external.go

+	if remoteUser == "" {
+		return connector.Identity{}, fmt.Errorf("required HTTP header X-Remote-User is not set")
+	}
+	return connector.Identity{


Groups as well?

How can I derive any groups from X-Remote-User header, which is just an email address?

Kubernetes allows additional headers that represent groups. https://kubernetes.io/docs/admin/authentication/#authenticating-proxy

Fine with not addressing this now, but it limits how useful this will be to other services.

Ah, thanks for the pointer. Added a TODO to the code, so that someone can actually verify it works with Kubernetes when they implement it :).

ericchiang · 2017-10-23T17:51:33Z

connector/external/external.go

+	if err != nil {
+		return "", fmt.Errorf("failed to parse callbackURL %q: %v", callbackURL, err)
+	}
+	u.Path = u.Path + "/external"


nit, this should probably be passed to the config. Feel free to leave a TODO if it's not obvious.

Done. On second thought, though: shouldn’t this be derived from the connector ID? I couldn’t see whether connectors have access to the ID they’re configured as.

ericchiang · 2017-10-23T17:57:28Z

connector/external/external.go

+// Callback is a connector which returns an identity with the HTTP header
+// X-Remote-User as verified email.
+type Callback struct {
+	Logger logrus.FieldLogger


nit: callback and logger should be lowercase so they're unexported. We only export things we explicitly have a reason to share.

ericchiang · 2017-10-23T18:28:23Z

connector/external/external.go

+// Config holds the configuration parameters for a connector which returns an
+// identity with the HTTP header X-Remote-User as verified email.
+type Config struct {
+	PathSuffix string `json:"pathSuffix"`


I mean, this should be passed by the server since it know the URL this connector will operate at. It doesn't need to be configurable by the user.

Maybe let connectors know what their ID is?

func (c *Config) Open(id string, logger logrus.FieldLogger) (connector.Connector, error)

stapelberg

Thanks for the review! Please take another look.

stapelberg · 2017-10-23T18:25:41Z

connector/external/external.go

+// Callback is a connector which returns an identity with the HTTP header
+// X-Remote-User as verified email.
+type Callback struct {
+	Logger logrus.FieldLogger


stapelberg · 2017-10-23T18:26:07Z

connector/external/external.go

+	if err != nil {
+		return "", fmt.Errorf("failed to parse callbackURL %q: %v", callbackURL, err)
+	}
+	u.Path = u.Path + "/external"


Done. On second thought, though: shouldn’t this be derived from the connector ID? I couldn’t see whether connectors have access to the ID they’re configured as.

stapelberg · 2017-10-23T18:26:39Z

connector/external/external.go

+	if remoteUser == "" {
+		return connector.Identity{}, fmt.Errorf("required HTTP header X-Remote-User is not set")
+	}
+	return connector.Identity{


How can I derive any groups from X-Remote-User header, which is just an email address?

stapelberg · 2017-10-23T18:26:42Z

connector/external/external.go

+}
+
+// Refresh updates the identity during a refresh token request.
+func (m *Callback) Refresh(ctx context.Context, s connector.Scopes, identity connector.Identity) (connector.Identity, error) {


stapelberg · 2017-10-23T18:28:13Z

server/server.go

@@ -240,6 +241,9 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy)
 	handleFunc("/auth", s.handleAuthorization)
 	handleFunc("/auth/{connector}", s.handleConnectorLogin)
 	handleFunc("/callback", s.handleConnectorCallback)
+	// For easier connector-specific web server configuration, e.g. for the
+	// "external" connector.
+	handleFunc("/callback/{connector}", s.handleConnectorCallback)


This needs to be the ID of the connector not the type, all connectors can be configured multiple times. I can configure two LDAP connectors with IDs activedirectory and freeipa.

Yes. The /auth/{connector} route assigns connID := mux.Vars(r)["connector"], which is why I chose /callback/{connector} to signify the connector ID. Is that incorrect? Should “connector” be changed to “id”, here and above?

It also needs enforce that handleConnectorCallback only allows a session using whatever value {connector} is. If it's /callback/okta that the user actually logging in through okta.

Done.

stapelberg

Thanks again. Please take another look.

stapelberg · 2017-10-23T18:47:55Z

connector/external/external.go

+// Config holds the configuration parameters for a connector which returns an
+// identity with the HTTP header X-Remote-User as verified email.
+type Config struct {
+	PathSuffix string `json:"pathSuffix"`


stapelberg · 2017-10-23T18:48:18Z

connector/external/external.go

+	if remoteUser == "" {
+		return connector.Identity{}, fmt.Errorf("required HTTP header X-Remote-User is not set")
+	}
+	return connector.Identity{


Ah, thanks for the pointer. Added a TODO to the code, so that someone can actually verify it works with Kubernetes when they implement it :).

ericchiang · 2017-10-25T16:27:19Z

Code looks fine. Few final requests:

"external" doesn't feel like the correct name. It seems like it should be "authenticatingproxy" or something like that. (open to shorter names)
A working example in tree that we can use to test this. See our LDAP setup for a good example https://github.com/coreos/dex/blob/master/Documentation/ldap-connector.md#getting-started
Docs should call out limitations. This mode doesn't support refresh tokens or groups.

stapelberg · 2017-10-25T16:40:52Z

"external" doesn't feel like the correct name. It seems like it should be "authenticatingproxy" or something like that. (open to shorter names)

How about “authproxy” as a shorter version?

A working example in tree that we can use to test this. See our LDAP setup for a good example https://github.com/coreos/dex/blob/master/Documentation/ldap-connector.md#getting-started

The docs already contain an example Apache config. Are you asking me to add the required config files to actually start a full Apache server, or are you asking me to add a unit test?

Docs should call out limitations. This mode doesn't support refresh tokens or groups.

Will do.

ericchiang · 2017-10-25T16:53:16Z

How about “authproxy” as a shorter version?

Works for me

The docs already contain an example Apache config. Are you asking me to add the required config files to actually start a full Apache server, or are you asking me to add a unit test?

A more complete example would be preferable. I've never run an Apache server, for instance. How would I test this? Even links to a basic "getting started" guide would be helpful for future dex maintainers.

stapelberg · 2017-10-25T16:55:58Z

Got it. I’ll update the pull requests in a few hours.

stapelberg · 2017-10-25T19:54:40Z

All done. Please take another look.

ericchiang

lgtm, though I'm going to follow up with some doc improvements if that's okay.

ericchiang · 2017-10-26T16:49:31Z

Documentation/authproxy.md

+        # Requires Apache 2.4.10+
+        RequestHeader set X-Remote-User expr=%{REMOTE_USER}@debian.org
+
+        ProxyPass "http://localhost:5556/dex/callback/myBasicAuth"


One concern I just realized. The user can still hit the http://localhost:5556/dex/callback URL to login against the authproxy connector. Is this protected from the user setting X-Remote-User?

This seems dangerous. I'd like to add the following recommendation:

If this connect is used, it's recommended to use this as a sole connector.

I can't really imagine a safe way to allow multiple connectors while this is set...

cc @stapelberg

Good catch. Let me send a PR which strips X-Remote-User for all dex URLs aside from the correct one.

👍 awesome, thanks

follow-up for dexidp#1100

Implement the “external” connector (for Apache2 mod_auth etc.)

follow-up for dexidp#1100

stapelberg force-pushed the external branch from 7780d02 to d2a302f Compare October 21, 2017 15:04

srenatus approved these changes Oct 22, 2017

View reviewed changes

ericchiang reviewed Oct 23, 2017

View reviewed changes

stapelberg force-pushed the external branch from d2a302f to 214c19d Compare October 23, 2017 18:25

ericchiang reviewed Oct 23, 2017

View reviewed changes

stapelberg commented Oct 23, 2017

View reviewed changes

stapelberg force-pushed the external branch from 214c19d to cebc478 Compare October 23, 2017 18:48

stapelberg commented Oct 23, 2017

View reviewed changes

stapelberg force-pushed the external branch from cebc478 to b1e6fc3 Compare October 23, 2017 18:54

Implement the “authproxy” connector (for Apache2 mod_auth etc.)

a41d93d

stapelberg force-pushed the external branch from b1e6fc3 to a41d93d Compare October 25, 2017 19:54

ericchiang approved these changes Oct 26, 2017

View reviewed changes

ericchiang merged commit 751c565 into dexidp:master Oct 26, 2017

ericchiang mentioned this pull request Oct 26, 2017

connector: implement LinkedIn connector #1101

Merged

ericchiang reviewed Oct 26, 2017

View reviewed changes

stapelberg mentioned this pull request Oct 26, 2017

authproxy.md: strip X-Remote-User #1103

Merged

stapelberg deleted the external branch October 26, 2017 17:01

stapelberg added a commit to stapelberg/dex that referenced this pull request Oct 26, 2017

authproxy.md: strip X-Remote-User

4931f30

follow-up for dexidp#1100

mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019

Merge pull request dexidp#1100 from stapelberg/external

2e646a9

Implement the “external” connector (for Apache2 mod_auth etc.)

mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019

authproxy.md: strip X-Remote-User

78dd85d

follow-up for dexidp#1100

Implement the “external” connector (for Apache2 mod_auth etc.) #1100

Implement the “external” connector (for Apache2 mod_auth etc.) #1100

Conversation

stapelberg commented Oct 21, 2017

srenatus left a comment

Choose a reason for hiding this comment

ericchiang commented Oct 23, 2017

stapelberg commented Oct 23, 2017

ericchiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stapelberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stapelberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang commented Oct 25, 2017

stapelberg commented Oct 25, 2017

ericchiang commented Oct 25, 2017

stapelberg commented Oct 25, 2017

stapelberg commented Oct 25, 2017

ericchiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang Oct 23, 2017 •

edited

Loading