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

Support OKTA authentication #17236

Merged
merged 2 commits into from Jan 9, 2019
Merged

Support OKTA authentication #17236

merged 2 commits into from Jan 9, 2019

Conversation

@fyery-chen
Copy link
Contributor

@fyery-chen fyery-chen commented Dec 26, 2018

Problem:
Some customers need to authenticate by using OKTA platform.

Solved:
Added another SAML2.0 authentication method OKTA.

Issue:
#15574

Related PR of types:
rancher/types#663

@@ -139,7 +139,7 @@ func InitializeSamlServiceProvider(configToSet *v3.SamlConfig, name string) erro
sp.IDPMetadata.EntityID = idm.EntityID
sp.IDPMetadata.SPSSODescriptors = idm.SPSSODescriptors
sp.IDPMetadata.IDPSSODescriptors = idm.IDPSSODescriptors
if name == ADFSName {
if name == ADFSName || name == OkTAName {
Copy link
Contributor

@mrajashree mrajashree Jan 3, 2019

Choose a reason for hiding this comment

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

Is this done because we tested integration between an Okta Identity Provider and Rancher's ADFS provider?

Copy link
Contributor Author

@fyery-chen fyery-chen Jan 3, 2019

Choose a reason for hiding this comment

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

Year!

@@ -29,6 +29,7 @@ import (
const PingName = "ping"
const ADFSName = "adfs"
const KeyCloakName = "keycloak"
const OkTAName = "okta"
Copy link
Contributor

@mrajashree mrajashree Jan 3, 2019

Choose a reason for hiding this comment

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

nit: can we make the 'k' capital as well? OKTAName

Copy link
Contributor Author

@fyery-chen fyery-chen Jan 3, 2019

Choose a reason for hiding this comment

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

Year, of course, maybe I missed this problem, thanks!

@@ -31,7 +31,8 @@ func (s *Provider) ServeHTTP(w http.ResponseWriter, r *http.Request) {
log.Debugf("RESPONSE: ===\n%s\n===\nNOW: %s\nERROR: %s",
parseErr.Response, parseErr.Now, parseErr.PrivateErr)
}
http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden)
redirectURL := r.URL.Host + "/login"
Copy link
Contributor

@orangedeng orangedeng Jan 3, 2019

Choose a reason for hiding this comment

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

Why redirect?

Copy link
Contributor Author

@fyery-chen fyery-chen Jan 3, 2019

Choose a reason for hiding this comment

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

If do not redirect, it will return an invalid page if we direct access a route path of authentication, I do not think this is a good UX, because we can access Rancher server from other(okta) platform.

Copy link
Contributor

@mrajashree mrajashree Jan 3, 2019

Choose a reason for hiding this comment

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

The redirect URL can also include the error code as a query parameter, because UI has logic to handle certain error codes. So can we append the error code here?, like in line
https://github.com/rancher/rancher/blob/master/pkg/auth/providers/saml/saml_client.go#L279
http.Redirect(w, r, redirectURL+"/login?errorCode=403", http.StatusFound)

Copy link
Contributor Author

@fyery-chen fyery-chen Jan 3, 2019

Choose a reason for hiding this comment

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

Awesome, good idea, thank you!

Copy link
Contributor Author

@fyery-chen fyery-chen Jan 3, 2019

Choose a reason for hiding this comment

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

@mrajashree I've updated as you suggested, please review again.

Copy link
Contributor

@mrajashree mrajashree Jan 3, 2019

Choose a reason for hiding this comment

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

Thanks!

@fyery-chen
Copy link
Contributor Author

@fyery-chen fyery-chen commented Jan 3, 2019

@mrajashree I've updated, please review again, thank you!

@mrajashree
Copy link
Contributor

@mrajashree mrajashree commented Jan 3, 2019

LGTM

@orangedeng
Copy link
Contributor

@orangedeng orangedeng commented Jan 4, 2019

LGTM, should merge after types PR merged. @fyery-chen You might need to rebase types and rancher PR too.

@fyery-chen
Copy link
Contributor Author

@fyery-chen fyery-chen commented Jan 4, 2019

@orangedeng I've rebased, please check.

fyery-chen added 2 commits Jan 9, 2019
To support OKTA authentication

Added another SAML2.0 authentication method OKTA

Issue: rancher#15574
@alena1108 alena1108 merged commit 7fab64d into rancher:master Jan 9, 2019
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants