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

Go login #2010

Merged
merged 19 commits into from
May 17, 2018
Merged

Go login #2010

merged 19 commits into from
May 17, 2018

Conversation

DylanVolz
Copy link

@DylanVolz DylanVolz commented Mar 14, 2018

This fixes #1247

This PR implements api/1.3/user/login in go. It follows the same login flow as perl (except the token flow):
check the local user password using scrypt
check the local user password using hash1
if ldap is configured check password using ldap

if any match the user is logged in.

@DylanVolz
Copy link
Author

A next step will be persisting a single connection to ldap for reuse with a singleton or similar implementation.

@asfgit
Copy link
Contributor

asfgit commented Mar 14, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1237/
Test PASSed.

@dangogh dangogh added this to the 2.3 milestone Mar 14, 2018
@dewrich
Copy link
Contributor

dewrich commented Mar 14, 2018

Changes need to be made for the following requirements:

  • Deployment automation
  • Upgrade process (postinstall)
  • Startup (systemd)

Copy link
Member

@dangogh dangogh left a comment

Choose a reason for hiding this comment

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

  • 2 files missing the Apache header.

  • also add a note to the CHANGELOG.md

I'll test this out soon -- can't get to it today, tho..

@@ -0,0 +1,101 @@
package auth
Copy link
Member

Choose a reason for hiding this comment

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

Needs the Apache license header

@@ -0,0 +1,111 @@
package auth
Copy link
Member

Choose a reason for hiding this comment

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

Needs the Apache license header

@asfgit
Copy link
Contributor

asfgit commented Mar 21, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1263/
Test PASSed.

@asfgit
Copy link
Contributor

asfgit commented Mar 22, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1264/
Test FAILed.

@asfgit
Copy link
Contributor

asfgit commented Mar 22, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1265/
Test FAILed.

@asfgit
Copy link
Contributor

asfgit commented Mar 22, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1273/
Test PASSed.

@asfgit
Copy link
Contributor

asfgit commented Mar 22, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1275/
Test PASSed.

@asfgit
Copy link
Contributor

asfgit commented Mar 26, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1296/
Test PASSed.

@asfgit
Copy link
Contributor

asfgit commented Mar 26, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1297/
Test FAILed.

@asfgit
Copy link
Contributor

asfgit commented Mar 26, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1298/
Test PASSed.

@asfgit
Copy link
Contributor

asfgit commented Apr 3, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1321/
Test PASSed.

for my $k (@requiredKeys) {
if (! exists $ldapConf{$k} ) {
errorOut("$k is a required key in $fileName");
}
}

delete $ldapConf{setupLdap}

Copy link
Member

Choose a reason for hiding this comment

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

syntax error here -- missing ; at the end of that delete

@asfgit
Copy link
Contributor

asfgit commented Apr 9, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1365/
Test PASSed.

handleErrs(http.StatusBadRequest, err)
return
}
authenticated, err := checkLocalUser(form, db)
Copy link
Member

Choose a reason for hiding this comment

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

this needs to check that the local user exists first without checking the password. If it doesn't exist or user's role is disallowed, immediately error out without checking LDAP.

@asfgit
Copy link
Contributor

asfgit commented Apr 9, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1369/
Test PASSed.

@asfgit
Copy link
Contributor

asfgit commented May 1, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1496/
Test PASSed.

@asfgit
Copy link
Contributor

asfgit commented May 7, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1526/
Test PASSed.

@asfgit
Copy link
Contributor

asfgit commented May 7, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1527/
Test PASSed.

if cfg.LDAPConfPath != "" {
cfg.LDAPEnabled, cfg.ConfigLDAP, err = GetLDAPConfig(cfg.LDAPConfPath)
if err != nil {
cfg.LDAPEnabled = false // probably unnecessary
Copy link
Member

Choose a reason for hiding this comment

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

I'd support removing this comment. All return values should be treated as undefined, if a returned error is not nil.

I'd rather omit the comment, than have someone remove the line because of the comment in the future.

Copy link
Author

Choose a reason for hiding this comment

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

+1

}
err = VerifyPassword(form.Password, hashedPassword)
if err != nil {
if hashedPassword == sha1Hex(form.Password) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be part of VerifyPassword?

Copy link
Author

Choose a reason for hiding this comment

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

I renamed VerifyPassword to VerifySCRYPTPassword since the sha1Hex check is for backwards compatibility.

@asfgit
Copy link
Contributor

asfgit commented May 7, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1530/
Test PASSed.

Copy link
Member

@rob05c rob05c left a comment

Choose a reason for hiding this comment

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

Needs to vendor gopkg.in/asn1-ber.v1

../vendor/gopkg.in/ldap.v2/add.go:16:2: cannot find package "gopkg.in/asn1-ber.v1"

@asfgit
Copy link
Contributor

asfgit commented May 10, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1546/
Test PASSed.

@rob05c
Copy link
Member

rob05c commented May 11, 2018

I think this needs to check for the ldap.conf file in the same way the existing Perl does
https://github.com/apache/incubator-trafficcontrol/blob/master/traffic_ops/app/lib/TrafficOps.pm#L502
https://github.com/apache/incubator-trafficcontrol/blob/master/traffic_ops/app/lib/TrafficOps.pm#L414

Making it configurable is good, but we should support the existing method too, and not require a config change on upgrade.

}

func getLDAPConf(s string) (*ConfigLDAP, error) {
ldapConf := ConfigLDAP{}
Copy link
Member

Choose a reason for hiding this comment

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

The LDAP timeout should have a reasonable default. It's defaulting to 0 now, and my LDAP (which is probably misconfigured) is hanging indefinitely. I'd suggest something like 10 seconds, if it isn't set in the config.

Should be accomplished by changing this to

const DefaultLDAPTimeout = time.Duration(60 * time.Second)
ldapConf := ConfigLDAP{LDAPTimeoutSecs: DefaultLDAPTimeout / time.Second}

@asfgit
Copy link
Contributor

asfgit commented May 15, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1575/
Test PASSed.

@rob05c rob05c dismissed dangogh’s stale review May 17, 2018 22:35

Verified all concerns have been addressed.

@rob05c
Copy link
Member

rob05c commented May 17, 2018

Looks good!

Tested, everything works

  • loads ldap config from existing config location
  • using non-ldap password works
  • using ldap password works
  • ldap users without TO users are not allowed to log in
  • TO user without corresponding ldap user login works
  • verified all of Dan's comments have been fixed

@rob05c rob05c merged commit 9d85130 into apache:master May 17, 2018
@zrhoffman zrhoffman added the authentication Relating to login, registration, passwords, tokens, etc. label Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authentication Relating to login, registration, passwords, tokens, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Login APIs
6 participants