-
Notifications
You must be signed in to change notification settings - Fork 0
New connector 'keystone federation' for sso using shibboleth #44
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
Changes from all commits
c3d27cc
a2b550a
a5c9630
f6ac6dd
b941a2c
414d08c
f594c2a
7f434c3
77b4b95
bb617b7
4274d8e
1459edc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,293 @@ | ||||||
| package keystone | ||||||
|
|
||||||
| import ( | ||||||
| "context" | ||||||
| "encoding/json" | ||||||
| "fmt" | ||||||
| "net/http" | ||||||
| "net/url" | ||||||
| "time" | ||||||
|
|
||||||
| "github.com/dexidp/dex/connector" | ||||||
| "github.com/dexidp/dex/pkg/log" | ||||||
| ) | ||||||
|
|
||||||
| var ( | ||||||
| _ connector.CallbackConnector = &FederationConnector{} | ||||||
| _ connector.RefreshConnector = &FederationConnector{} | ||||||
| ) | ||||||
|
|
||||||
| // FederationConnector implements the connector interface for Keystone federation authentication | ||||||
| type FederationConnector struct { | ||||||
| cfg FederationConfig | ||||||
| client *http.Client | ||||||
| logger log.Logger | ||||||
|
|
||||||
| // Stores callback information for the federation flow | ||||||
| callbackURL string | ||||||
| state string | ||||||
| } | ||||||
|
|
||||||
| // Validate returns error if config is invalid. | ||||||
| func (c *FederationConfig) Validate() error { | ||||||
| if c.Domain == "" { | ||||||
| return fmt.Errorf("domain field is required in config") | ||||||
| } | ||||||
| if c.Host == "" { | ||||||
| return fmt.Errorf("host field is required in config") | ||||||
| } | ||||||
| if c.AdminUsername == "" { | ||||||
| return fmt.Errorf("keystoneUsername field is required in config") | ||||||
| } | ||||||
| if c.AdminPassword == "" { | ||||||
| return fmt.Errorf("keystonePassword field is required in config") | ||||||
| } | ||||||
| if c.CustomerName == "" { | ||||||
| return fmt.Errorf("customerName field is required in config") | ||||||
| } | ||||||
| if c.ShibbolethLoginPath == "" { | ||||||
| return fmt.Errorf("shibbolethLoginPath field is required in config") | ||||||
| } | ||||||
| if c.FederationAuthPath == "" { | ||||||
| return fmt.Errorf("federationAuthPath field is required in config") | ||||||
| } | ||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| // Open returns a connector using the federation configuration | ||||||
| func (c *FederationConfig) Open(id string, logger log.Logger) (connector.Connector, error) { | ||||||
| return NewFederationConnector(*c, logger) | ||||||
| } | ||||||
|
|
||||||
| func NewFederationConnector(cfg FederationConfig, logger log.Logger) (*FederationConnector, error) { | ||||||
| if err := cfg.Validate(); err != nil { | ||||||
| return nil, err | ||||||
| } | ||||||
| return &FederationConnector{ | ||||||
| cfg: cfg, | ||||||
| client: &http.Client{ | ||||||
| Timeout: time.Duration(30) * time.Second, | ||||||
srm6867 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Unnecessary conversion to time.Duration. |
||||||
| }, | ||||||
| logger: logger, | ||||||
| }, nil | ||||||
| } | ||||||
|
|
||||||
| func (c *FederationConnector) LoginURL(scopes connector.Scopes, callbackURL, state string) (string, error) { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add unit tests for this. |
||||||
| ksBase := normalizeKeystoneURL(c.cfg.Host) | ||||||
|
|
||||||
| // Store the callback URL and state in the connector for use during callback handling | ||||||
| c.callbackURL = callbackURL | ||||||
| c.state = state | ||||||
|
Comment on lines
+78
to
+80
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this even used anywhere? I couldn't find any other references to this. |
||||||
|
|
||||||
| // Use Shibboleth SSO login path for federation | ||||||
| ssoLoginPath := c.cfg.ShibbolethLoginPath | ||||||
|
|
||||||
| // Construct the Shibboleth login URL | ||||||
| u, err := url.Parse(fmt.Sprintf("%s%s", ksBase, ssoLoginPath)) | ||||||
| if err != nil { | ||||||
| return "", fmt.Errorf("parsing SSO login URL: %w", err) | ||||||
| } | ||||||
|
|
||||||
| // The target will be passed through the entire federation flow. | ||||||
| // target is nothing but the redirect url that will be used by shibboleth to redirect back to Dex. | ||||||
| target := fmt.Sprintf("%s?state=%s", callbackURL, state) | ||||||
| q := u.Query() | ||||||
| q.Set("target", target) | ||||||
| u.RawQuery = q.Encode() | ||||||
| c.logger.Debugf("Shibboleth login URL with dex callback=%s", u.String()) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this leak secrets in the logs? |
||||||
| return u.String(), nil | ||||||
| } | ||||||
|
|
||||||
| func (c *FederationConnector) HandleCallback(scopes connector.Scopes, r *http.Request) (connector.Identity, error) { | ||||||
| c.logger.Debugf("Dex Callback received: URL=%s, Method=%s", r.URL.String(), r.Method) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. Does the URL contain any secrets? |
||||||
|
|
||||||
| var ksToken string | ||||||
| var err error | ||||||
| var tokenInfo *tokenInfo | ||||||
| identity := connector.Identity{} | ||||||
|
|
||||||
| // Get state from query parameters | ||||||
| state := r.URL.Query().Get("state") | ||||||
| if state == "" { | ||||||
| c.logger.Error("Missing state in request") | ||||||
| return connector.Identity{}, fmt.Errorf("missing state") | ||||||
|
Comment on lines
+112
to
+113
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't log and return an error at the same time. Like Mridul mentioned, it will end up logging the same error twice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, add some other helpful information into the error being returned. Perhaps a unique ID or something else that is not a secret? |
||||||
| } | ||||||
|
|
||||||
| // Log state information | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant comment. Comments should add context on the "why". The what is usually evident. Same in other places. |
||||||
| c.logger.Debugf("Processing callback for state=%s", state) | ||||||
|
|
||||||
| // Extract federation cookies and use them to get a keystone token | ||||||
| ksToken, err = c.getKeystoneTokenFromFederation(r) | ||||||
| if err != nil { | ||||||
| c.logger.Errorf("Error getting token from federation cookies: %v", err) | ||||||
| return connector.Identity{}, fmt.Errorf("getting token from federation cookies: %w", err) | ||||||
srm6867 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| } | ||||||
| c.logger.Infof("Successfully obtained token from federation cookies") | ||||||
|
|
||||||
| ksBase := normalizeKeystoneURL(c.cfg.Host) | ||||||
| c.logger.Debugf("Retrieving user info with") | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing information in log? |
||||||
| tokenInfo, err = getTokenInfo(r.Context(), c.client, ksBase, ksToken, c.logger) | ||||||
| if err != nil { | ||||||
| return connector.Identity{}, err | ||||||
| } | ||||||
| if scopes.Groups { | ||||||
| c.logger.Infof("groups scope requested, fetching groups") | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add some context about this specific request. Otherwise the log is going to be filled with |
||||||
| var err error | ||||||
| adminToken, err := getAdminTokenUnscoped(r.Context(), c.client, ksBase, c.cfg.AdminUsername, c.cfg.AdminPassword) | ||||||
| if err != nil { | ||||||
| return identity, fmt.Errorf("keystone: failed to obtain admin token: %v", err) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not create a new error type. Instead wrap the error. Either with By creating a new error, we're losing context on the error that the internal API call might be returning. |
||||||
| } | ||||||
| identity.Groups, err = getAllGroupsForUser(r.Context(), c.client, ksBase, adminToken, c.cfg.CustomerName, c.cfg.Domain, tokenInfo, c.logger) | ||||||
| if err != nil { | ||||||
| return connector.Identity{}, err | ||||||
| } | ||||||
| } | ||||||
| identity.Username = tokenInfo.User.Name | ||||||
| identity.UserID = tokenInfo.User.ID | ||||||
|
|
||||||
| user, err := getUser(r.Context(), c.client, ksBase, tokenInfo.User.ID, ksToken) | ||||||
| if err != nil { | ||||||
| return identity, err | ||||||
| } | ||||||
| if user.User.Email != "" { | ||||||
| identity.Email = user.User.Email | ||||||
| identity.EmailVerified = true | ||||||
| } | ||||||
|
|
||||||
| data := connectorData{Token: ksToken} | ||||||
| connData, err := json.Marshal(data) | ||||||
| if err != nil { | ||||||
| return identity, fmt.Errorf("marshal connector data: %v", err) | ||||||
| } | ||||||
| identity.ConnectorData = connData | ||||||
|
|
||||||
| return identity, nil | ||||||
| } | ||||||
|
|
||||||
| // getKeystoneTokenFromFederation gets a Keystone token using an existing federation session. | ||||||
| // This method extracts federation cookies from the request and uses them to authenticate | ||||||
| // with Keystone's federation endpoint. | ||||||
| func (c *FederationConnector) getKeystoneTokenFromFederation(r *http.Request) (string, error) { | ||||||
| c.logger.Debugf("Getting Keystone token from federation cookies") | ||||||
| ksBase := normalizeKeystoneURL(c.cfg.Host) | ||||||
|
|
||||||
| // Prepare the federation auth request | ||||||
| federationAuthURL := fmt.Sprintf("%s%s", ksBase, c.cfg.FederationAuthPath) | ||||||
| c.logger.Infof("Requesting Keystone token from federation auth endpoint: %s", federationAuthURL) | ||||||
|
|
||||||
| req, err := http.NewRequest("GET", federationAuthURL, nil) | ||||||
| if err != nil { | ||||||
| c.logger.Errorf("Error creating federation auth request: %v", err) | ||||||
| return "", fmt.Errorf("creating federation auth request: %w", err) | ||||||
| } | ||||||
|
|
||||||
| // Copy all cookies from the original request to maintain the federation session | ||||||
| for _, cookie := range r.Cookies() { | ||||||
| req.AddCookie(cookie) | ||||||
| } | ||||||
|
Comment on lines
+184
to
+187
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need to copy all the cookies here? Or just the related ones? Is there a security risk by copying other cookies that's not needed? |
||||||
|
|
||||||
| // Copy relevant headers that might be needed for federation | ||||||
| if userAgent := r.Header.Get("User-Agent"); userAgent != "" { | ||||||
| req.Header.Set("User-Agent", userAgent) | ||||||
| } | ||||||
| if referer := r.Header.Get("Referer"); referer != "" { | ||||||
| req.Header.Set("Referer", referer) | ||||||
| } | ||||||
|
|
||||||
| c.logger.Debugf("Federation auth request headers: %v", req.Header) | ||||||
|
|
||||||
| // Use a client that doesn't automatically follow redirects | ||||||
| clientNoRedirect := &http.Client{ | ||||||
| Timeout: c.client.Timeout, | ||||||
| CheckRedirect: func(req *http.Request, via []*http.Request) error { | ||||||
| return http.ErrUseLastResponse | ||||||
| }, | ||||||
| } | ||||||
|
|
||||||
| resp, err := clientNoRedirect.Do(req) | ||||||
| if err != nil { | ||||||
| c.logger.Errorf("Error executing federation auth request: %v", err) | ||||||
| return "", fmt.Errorf("executing federation auth request: %w", err) | ||||||
| } | ||||||
| defer resp.Body.Close() | ||||||
|
|
||||||
| c.logger.Debugf("Federation auth response status: %s", resp.Status) | ||||||
| c.logger.Debugf("Federation auth response headers: %v", resp.Header) | ||||||
|
|
||||||
| // Extract the token from the X-Subject-Token header | ||||||
| token := resp.Header.Get("X-Subject-Token") | ||||||
| if token == "" { | ||||||
| c.logger.Error("No X-Subject-Token found in federation auth response") | ||||||
| return "", fmt.Errorf("no X-Subject-Token found in federation auth response") | ||||||
| } | ||||||
|
|
||||||
| c.logger.Debugf("Successfully obtained Keystone token from federation") | ||||||
| return token, nil | ||||||
| } | ||||||
|
|
||||||
| // Close does nothing since HTTP connections are closed automatically. | ||||||
| func (c *FederationConnector) Close() error { | ||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| // Refresh is used to refresh identity during token refresh. | ||||||
| // It checks if the user still exists and refreshes their group membership. | ||||||
| func (c *FederationConnector) Refresh( | ||||||
| ctx context.Context, scopes connector.Scopes, identity connector.Identity, | ||||||
| ) (connector.Identity, error) { | ||||||
| c.logger.Infof("Refresh called for user %s", identity.UserID) | ||||||
| ksBase := normalizeKeystoneURL(c.cfg.Host) | ||||||
|
|
||||||
| // Get admin token to perform operations | ||||||
| adminToken, err := getAdminTokenUnscoped(ctx, c.client, ksBase, c.cfg.AdminUsername, c.cfg.AdminPassword) | ||||||
| if err != nil { | ||||||
| return identity, fmt.Errorf("keystone federation: failed to obtain admin token: %v", err) | ||||||
| } | ||||||
|
|
||||||
| // Check if the user still exists | ||||||
| user, err := getUser(ctx, c.client, ksBase, identity.UserID, adminToken) | ||||||
| if err != nil { | ||||||
| return identity, fmt.Errorf("keystone federation: failed to get user: %v", err) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. Wrap the error instead of creating a new one. |
||||||
| } | ||||||
| if user == nil { | ||||||
| return identity, fmt.Errorf("keystone federation: user %q does not exist", identity.UserID) | ||||||
| } | ||||||
|
|
||||||
| // Create a token info object with basic user info | ||||||
| tokenInfo := &tokenInfo{ | ||||||
| User: userKeystone{ | ||||||
| Name: identity.Username, | ||||||
| ID: identity.UserID, | ||||||
| }, | ||||||
| } | ||||||
|
|
||||||
| // If there is a token associated with this refresh token, use that to get more info | ||||||
| var data connectorData | ||||||
| if err := json.Unmarshal(identity.ConnectorData, &data); err != nil { | ||||||
| return identity, fmt.Errorf("keystone federation: unmarshal connector data: %v", err) | ||||||
| } | ||||||
|
|
||||||
| // If we have a stored token, try to use it to get token info | ||||||
| if len(data.Token) > 0 { | ||||||
| c.logger.Debugf("Using stored token to get token info") | ||||||
| tokenInfoFromStored, err := getTokenInfo(ctx, c.client, ksBase, data.Token, c.logger) | ||||||
| if err == nil { | ||||||
| // Only use the stored token info if we could retrieve it successfully | ||||||
| tokenInfo = tokenInfoFromStored | ||||||
| } else { | ||||||
| c.logger.Warnf("Could not get token info from stored token: %v", err) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // If groups scope is requested, refresh the groups | ||||||
| if scopes.Groups { | ||||||
| c.logger.Infof("Refreshing groups for user %s", identity.UserID) | ||||||
| var err error | ||||||
| identity.Groups, err = getAllGroupsForUser(ctx, c.client, ksBase, adminToken, c.cfg.CustomerName, c.cfg.Domain, tokenInfo, c.logger) | ||||||
| if err != nil { | ||||||
| return identity, fmt.Errorf("keystone federation: failed to get groups: %v", err) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return identity, nil | ||||||
| } | ||||||
Uh oh!
There was an error while loading. Please reload this page.