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.

Sign up for GitHub

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

Add support for differing internal vs. public URLs. #1722

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for differing internal vs. public URLs. #1722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/dex/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ type Config struct {
// querying the storage. Cannot be specified without enabling a passwords
// database.
StaticPasswords []password `json:"staticPasswords"`

// URL base to use for public-facing links and redirects. Defaults to Issuer.
PublicURL string `json:"publicURL"`
}

//Validate the configuration
Expand Down
1 change: 1 addition & 0 deletions cmd/dex/serve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ func serve(cmd *cobra.Command, args []string) error {
Logger: logger,
Now: now,
PrometheusRegistry: prometheusRegistry,
PublicURL: c.PublicURL,
}
if c.Expiry.SigningKeys != "" {
signingKeys, err := time.ParseDuration(c.Expiry.SigningKeys)
Expand Down
16 changes: 8 additions & 8 deletions server/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,10 @@ type discovery struct {
func (s *Server) discoveryHandler() (http.HandlerFunc, error) {
d := discovery{
Issuer: s.issuerURL.String(),
Auth: s.absURL("/auth"),
Token: s.absURL("/token"),
Keys: s.absURL("/keys"),
UserInfo: s.absURL("/userinfo"),
Auth: s.absURL(s.publicURL, "/auth"),
Token: s.absURL(s.issuerURL, "/token"),
Keys: s.absURL(s.issuerURL, "/keys"),
UserInfo: s.absURL(s.issuerURL, "/userinfo"),
Copy link

@heidemn heidemn Dec 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't this quite arbitrary? The userinfo endpoint can be called by public clients as well.

Seems to me that the entire feature would be better handled on client side, and not in Dex.
E.g. in our backend code, we manually rewrite the host of the endpoint, to directly connect to the Dex container hostname instead of the public endpoint.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure what you mean, /auth is the only thing that needs to be "public" because that's what is used when bouncing users to the underlying backend. The rest are using by authentication consumers, which in many cases are entirely internal.

Copy link
Author

Choose a reason for hiding this comment

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

If there is a way to do this client-side without violating the spec, I'm all ears.

Copy link

Choose a reason for hiding this comment

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

E.g. a single-page app (using PKCE or Implicit Flow) may call the userinfo endpoint from the browser.
I was assuming that "public URL" = called from browser, "internal URL" = called from backend.

If there is a way to do this client-side without violating the spec

Not that I'm aware of... Rewriting the URL was just a pragmatic solution that worked for us.

What could also work, if public and internal URLs use the same scheme (HTTP / HTTPS): Add an /etc/hosts entry for the backend that should use the internal endpoint, like <public_dex_hostname> <internal_dex_ip>.

Copy link
Author

@coderanger coderanger Jan 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sure but that SPA would be a client so you wouldn’t enable this mode. This is specifically for the case of all clients are internal but the IdP isn’t (google in my case). This is common in the use case of using Dex for Kubernetes authentication.

Subjects: []string{"public"},
IDTokenAlgs: []string{string(jose.RS256)},
Scopes: []string{"openid", "email", "groups", "profile", "offline_access"},
Expand Down Expand Up @@ -241,7 +241,7 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) {
if authReq.ConnectorID != "" {
for _, c := range connectors {
if c.ID == authReq.ConnectorID {
http.Redirect(w, r, s.absPath("/auth", c.ID)+"?req="+authReq.ID, http.StatusFound)
http.Redirect(w, r, s.absPath(s.publicURL, "/auth", c.ID)+"?req="+authReq.ID, http.StatusFound)
return
}
}
Expand All @@ -253,7 +253,7 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) {
for _, c := range connectors {
// TODO(ericchiang): Make this pass on r.URL.RawQuery and let something latter
// on create the auth request.
http.Redirect(w, r, s.absPath("/auth", c.ID)+"?req="+authReq.ID, http.StatusFound)
http.Redirect(w, r, s.absPath(s.publicURL, "/auth", c.ID)+"?req="+authReq.ID, http.StatusFound)
return
}
}
Expand All @@ -266,7 +266,7 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) {
Type: conn.Type,
// TODO(ericchiang): Make this pass on r.URL.RawQuery and let something latter
// on create the auth request.
URL: s.absPath("/auth", conn.ID) + "?req=" + authReq.ID,
URL: s.absPath(s.publicURL, "/auth", conn.ID) + "?req=" + authReq.ID,
}
}

Expand Down Expand Up @@ -320,7 +320,7 @@ func (s *Server) handleConnectorLogin(w http.ResponseWriter, r *http.Request) {
// Use the auth request ID as the "state" token.
//
// TODO(ericchiang): Is this appropriate or should we also be using a nonce?
callbackURL, err := conn.LoginURL(scopes, s.absURL("/callback"), authReqID)
callbackURL, err := conn.LoginURL(scopes, s.absURL(s.publicURL, "/callback"), authReqID)
if err != nil {
s.logger.Errorf("Connector %q returned error when creating callback: %v", connID, err)
s.renderError(r, w, http.StatusInternalServerError, "Login error.")
Expand Down
25 changes: 25 additions & 0 deletions server/handlers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,28 @@ func TestHandleInvalidSAMLCallbacks(t *testing.T) {
}
}
}

func TestHandleDiscoveryPublic(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

httpServer, server := newTestServer(ctx, t, func(c *Config) {
c.PublicURL = "https://dex.example.com/"
})
defer httpServer.Close()

rr := httptest.NewRecorder()
server.ServeHTTP(rr, httptest.NewRequest("GET", "/.well-known/openid-configuration", nil))
if rr.Code != http.StatusOK {
t.Errorf("expected 200 got %d", rr.Code)
}
config := map[string]interface{}{}
err := json.Unmarshal(rr.Body.Bytes(), &config)
if err != nil {
t.Fatal(err.Error())
}
authURL := config["authorization_endpoint"].(string)
if authURL != "https://dex.example.com/auth" {
t.Errorf("expected https://dex.example.com/auth got %s", authURL)
}
}
25 changes: 19 additions & 6 deletions server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ type Config struct {
Logger log.Logger

PrometheusRegistry *prometheus.Registry

PublicURL string
}

// WebConfig holds the server's frontend templates and asset configuration.
Expand Down Expand Up @@ -130,6 +132,7 @@ func value(val, defaultValue time.Duration) time.Duration {
// Server is the top level object.
type Server struct {
issuerURL url.URL
publicURL url.URL

// mutex for the connectors map.
mu sync.Mutex
Expand Down Expand Up @@ -175,6 +178,16 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy)
return nil, fmt.Errorf("server: can't parse issuer URL")
}

var publicURL *url.URL
if c.PublicURL == "" {
publicURL = issuerURL
} else {
publicURL, err = url.Parse(c.PublicURL)
if err != nil {
return nil, fmt.Errorf("server: can't parse public URL")
}
}

if c.Storage == nil {
return nil, errors.New("server: storage cannot be nil")
}
Expand Down Expand Up @@ -224,6 +237,7 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy)
templates: tmpls,
passwordConnector: c.PasswordConnector,
logger: c.Logger,
publicURL: *publicURL,
}

// Retrieves connector objects in backend storage. This list includes the static connectors
Expand Down Expand Up @@ -330,17 +344,16 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
s.mux.ServeHTTP(w, r)
}

func (s *Server) absPath(pathItems ...string) string {
func (s *Server) absPath(base url.URL, pathItems ...string) string {
paths := make([]string, len(pathItems)+1)
paths[0] = s.issuerURL.Path
paths[0] = base.Path
copy(paths[1:], pathItems)
return path.Join(paths...)
}

func (s *Server) absURL(pathItems ...string) string {
u := s.issuerURL
u.Path = s.absPath(pathItems...)
return u.String()
func (s *Server) absURL(base url.URL, pathItems ...string) string {
base.Path = s.absPath(base, pathItems...)
return base.String()
}

func newPasswordDB(s storage.Storage) interface {
Expand Down