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

Allow configuration of returned auth proxy header #1839

Merged
merged 4 commits into from
Jan 7, 2021

Conversation

seuf
Copy link
Contributor

@seuf seuf commented Oct 20, 2020

This is a small PR to allow configuration of the header returned by the auth proxy.
We are using traefik as reverse proxy + traefik-forward-auth plugged on google to access our argocd interface.
But in traefik-forward-auth, the returned header after authentication is X-Forwarded-User.
With this feature, we can now login to argocd using the dex config :

connectors:
      - type: authproxy
        id: traefik
        name: traefik
        config:
          headerName: X-Forwarded-User

dex will fetch the header set by traefik forward auth and authenticate the user with the correct user email.

@seuf seuf force-pushed the authproxy-header-configuration branch 2 times, most recently from e86961d to 8955390 Compare October 20, 2020 09:21
@seuf
Copy link
Contributor Author

seuf commented Nov 23, 2020

Hi, Is it possible to review this ? we are using this branch in production for a few weeks now, and it works great.

type Config struct{}
type Config struct {
HeaderName string `json:"headerName"`
Groups []string `json:"groups"`
Copy link
Member

Choose a reason for hiding this comment

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

Can you please remove this change from this PR? It's not described anywhere and has nothing to do with headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I can split in 2 PR to add also the groups configuration.

Copy link
Member

Choose a reason for hiding this comment

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

You can submit it in a separate PR, but we are working on #1841 which will add this feature generally to all connectors, so I'm not sure we'll be accepting any more config changes like this.

@@ -14,16 +14,24 @@ import (

// 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{}
type Config struct {
HeaderName string `json:"headerName"`
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be called something like UserHeaderName or just UserHeader. Another common header name is X-Forwarded-Groups for group information.


// Open returns an authentication strategy which requires no user interaction.
func (c *Config) Open(id string, logger log.Logger) (connector.Connector, error) {
return &callback{logger: logger, pathSuffix: "/" + id}, nil
if c.HeaderName == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Note that this actually modifies the config. I think it would be better to create a local variable for the header and fall back to the default if it's empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I've used a default value in the callback directly without modifying the config.

connector/authproxy/authproxy.go Outdated Show resolved Hide resolved
connector/authproxy/authproxy.go Outdated Show resolved Hide resolved
connector/authproxy/authproxy.go Outdated Show resolved Hide resolved
connector/authproxy/authproxy.go Outdated Show resolved Hide resolved
connector/authproxy/authproxy.go Outdated Show resolved Hide resolved
seuf and others added 4 commits December 17, 2020 16:50
Signed-off-by: seuf <seuf76@gmail.com>
Signed-off-by: seuf <seuf76@gmail.com>
Signed-off-by: seuf <seuf76@gmail.com>
Co-authored-by: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com>
Signed-off-by: seuf <seuf76@gmail.com>
@seuf seuf force-pushed the authproxy-header-configuration branch from fcc3164 to e164bb3 Compare December 17, 2020 15:50
@sagikazarmark sagikazarmark added this to the v2.28.0 milestone Jan 7, 2021
Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Thanks @seuf

@sagikazarmark sagikazarmark merged commit 4f32639 into dexidp:master Jan 7, 2021
xtremerui pushed a commit to concourse/dex that referenced this pull request Mar 16, 2021
The official docker release for this release can be pulled from

```
ghcr.io/dexidp/dex:v2.28.0
```

**Features:**

- Add c_hash to id_token, issued on /auth endpoint, when in hybrid flow (dexidp#1773, @HEllRZA)
- Allow configuration of returned auth proxy header (dexidp#1839, @seuf)
- Allow to disable os.ExpandEnv for storage + connector configs by env variable DEX_EXPAND_ENV = false (dexidp#1902, @heidemn-faro)
- Added the possibility to activate lowercase for UPN-Strings (dexidp#1888, @VF-mbrauer)
- Add "Cache-control: no-store" and "Pragma: no-cache" headers to token responses (dexidp#1948, @nabokihms)
- Add gomplate to the docker image (dexidp#1893, @nabokihms)
- Graceful shutdown (dexidp#1963, @nabokihms)
- Allow public clients created with API to have no client_secret (dexidp#1871, @spohner)

**Bugfixes:**

- Fix the etcd PKCE AuthCode deserialization (dexidp#1908, @bnu0)
- Fix garbage collection logging of device codes and device request (dexidp#1918, @nabokihms)
- Discovery endpoint contains updated claims and auth methods (dexidp#1951, @nabokihms)
- Return invalid_grant error if auth code is invalid or expired (dexidp#1952, @nabokihms)
- Return an error to auth requests with the "request" parameter (dexidp#1956, @nabokihms)

**Minor changes:**

- Change default themes to light/dark (dexidp#1858, @nabokihms)
- Various developer experience improvements
- Dependency upgrades
- Tons of small fixes and changes
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.

None yet

2 participants