Project

Profile

Help

Issue #1728

Please relax input validation on --login for 'pulp-admin user create'

Added by kfiresmith almost 5 years ago. Updated almost 2 years ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
High
Category:
-
Sprint/Milestone:
-
Start date:
Due date:
Estimated time:
Severity:
2. Medium
Version:
2.7.1
Platform Release:
2.9.2
OS:
Triaged:
Yes
Groomed:
Yes
Sprint Candidate:
Yes
Tags:
Easy Fix, Pulp 2
Sprint:
Sprint 5
Quarter:

Description

While working out the process for setting up AD-integration for Pulp to allow SSO logins in pulp-admin, I discovered that '@' is not an allowed character of a login:

$pulp-admin auth user create --login
Validation failed for argument [--login]: value must contain only letters,
numbers, underscores, periods and hyphens

This unfortunately is a show-stopper issue for Pulp at our organization. Please relax the input validation so that we can continue working out how to get mod_auth_kerb set up for pulp.

Thanks!

Associated revisions

Revision bd85660d View on GitHub
Added by Austin Macdonald over 4 years ago

Allow at sign in user ids

Adjust the regex to allow usernames to contain the at sign so that email addresses can be valid usernames.

closes #1728

Revision bd85660d View on GitHub
Added by Austin Macdonald over 4 years ago

Allow at sign in user ids

Adjust the regex to allow usernames to contain the at sign so that email addresses can be valid usernames.

closes #1728

History

#1 Updated by semyers almost 5 years ago

Here's a diff that appears to work, but I have no idea if it's a good idea:

diff --git a/client_lib/pulp/client/validators.py b/client_lib/pulp/client/validators.py
index 4eb1d87..c941358 100644
--- a/client_lib/pulp/client/validators.py
+++ b/client_lib/pulp/client/validators.py
@@ -11,7 +11,7 @@ from pulp.common import dateutils
 from pulp.common.plugins import importer_constants


-ID_REGEX_ALLOW_DOTS = re.compile(r'^[.\-_A-Za-z0-9]+$')
+ID_REGEX_ALLOW_DOTS = re.compile(r'^[.@\-_A-Za-z0-9]+$')
 ID_REGEX = re.compile(r'^[\-_A-Za-z0-9]+$')


diff --git a/client_lib/test/unit/test_validators.py b/client_lib/test/unit/test_validators.py
index cb2ac34..3e35d89 100644
--- a/client_lib/test/unit/test_validators.py
+++ b/client_lib/test/unit/test_validators.py
@@ -131,7 +131,6 @@ class TestIdAllowDots(unittest.TestCase):
         # Single input
         self.assertRaises(ValueError, validators.id_validator_allow_dots, '**invalid**')
         self.assertRaises(ValueError, validators.id_validator_allow_dots, '**inval.id**')
-        self.assertRaises(ValueError, validators.id_validator_allow_dots, 'invalid-@')
         self.assertRaises(ValueError, validators.id_validator_allow_dots, '-_-_- ')

         # Multiple input
diff --git a/server/pulp/server/db/model/__init__.py b/server/pulp/server/db/model/__init__.py
index 29fea05..2b18b55 100644
--- a/server/pulp/server/db/model/__init__.py
+++ b/server/pulp/server/db/model/__init__.py
@@ -1014,7 +1014,7 @@ class User(AutoRetryDocument):
     :type _ns: mongoengine.StringField
     """

-    login = StringField(required=True, regex=r'^[.\-_A-Za-z0-9]+$')
+    login = StringField(required=True, regex=r'^[.@\-_A-Za-z0-9]+$')
     name = StringField()
     password = StringField()
     roles = ListField(StringField())

Something to note is that we have a specific test to make sure the @ sign is caught as an invalid character. I don't know, and the test doesn't make clear, why the @ sign is disallowed. It could maybe be a terrible thing to change the validation this way, and it could also be totally fine with the added benefit of (maybe?) getting kerberos users working.

#2 Updated by amacdona@redhat.com almost 5 years ago

To determine if this will work, one thing we need to check on is that "@" is properly escaped when hitting the user API endpoints.

#3 Updated by kfiresmith almost 5 years ago

Hrm - having a hard time locating the line in the last portion of the diff - in fact I can't find it in any of the pulp/server/db/... init.py files:

[root@trenton ksf_mgr]# grep login /usr/lib/python2.7/site-packages/pulp/server/db/model/__init__.py
[root@trenton ksf_mgr]# echo $?
1
[root@trenton ksf_mgr]# vim /usr/lib/python2.7/site-packages/pulp/server/db/model/__init__.py
[root@trenton ksf_mgr]# locate '.py' | grep pulp | grep server | grep init | grep db | egrep .py$
/usr/lib/python2.7/site-packages/pulp/server/db/__init__.py
/usr/lib/python2.7/site-packages/pulp/server/db/migrate/__init__.py
/usr/lib/python2.7/site-packages/pulp/server/db/migrations/__init__.py
/usr/lib/python2.7/site-packages/pulp/server/db/model/__init__.py
[root@trenton ksf_mgr]# for i in $(locate '.py' | grep pulp | grep server | grep init | grep db | egrep .py$); do grep login $i; done

Running pulp-server-2.7.1-1.el7.

Any advice?

#4 Updated by mhrivnak almost 5 years ago

I'm not aware of any problems this might cause.

#5 Updated by mhrivnak almost 5 years ago

  • Priority changed from Normal to High
  • Severity changed from 3. High to 2. Medium
  • Triaged changed from No to Yes
  • Tags Easy Fix added

#6 Updated by semyers almost 5 years ago

kfiresmith wrote:

Running pulp-server-2.7.1-1.el7.

^ Ah, I completely missed this when I looked at this issue before.

Any advice?

The diff I posted was made against a recent 2.8 beta. Earlier versions of pulp will certainly be missing pulp.server.db.model. Tests aren't packaged up, so the change to validators.py is the only one you can make. It's possible that patching validators.py as seen in the diff will get the validaiton loosened up for you, but I think it's unlikely. The @ was presumably invalid for a reason in earlier versions of pulp (including 2.7), but changes in 2.8 appear to have made it so that @ doesn't break mongo or our API. So, my advice would be to test this patch with a 2.8 beta install and let us know if everything is ruined (or not!). :)

#7 Updated by kfiresmith almost 5 years ago

semyers wrote:

The @ was presumably invalid for a reason in earlier versions of pulp (including 2.7), but changes in 2.8 appear to have made it so that @ doesn't break mongo or our API. So, my advice would be to test this patch with a 2.8 beta install and let us know if everything is ruined (or not!). :)

Ah - well that's no big problem. Since this isn't quite in production yet, I'll perform an upgrade to 2.8b as soon as I get back from Death Valley. If this ticket isn't updated by me by March 20th or so, I have expired in the desert and some hapless replacement will have to come in and figure this out again on their own.

#8 Updated by kfiresmith almost 5 years ago

Using @SeMeyers' changes appears to work insofar as i've been able to create the account!

  1. pulp-admin auth user create --login
    Enter password for user [] :
    Re-enter password for user []:
    User [] successfully created

Disregard the below message - I'd forgotten to restart the necessary services.
-Alright - I've gotten back and gotten upgraded to 2.8.0 and updated the files that exist on the local pulp server system. No joy:

  1. pulp-admin auth user create --login
    Enter password for user [] :
    Re-enter password for user []:
    Invalid properties: ['login']
    -

#9 Updated by kfiresmith almost 5 years ago

This input validation relaxation to allow '@' hasn't seemed to cause us any problems running Pulp 2.8. We've tested these AD accounts with mod_auth_kerb.so and mod_auth_gssapi.so and everything is working for AD/Kerberos integration with the code changes given by semyers in comment #1.

We would love to have these changes integrated into Pulp's next release.

#10 Updated by rbarlow almost 5 years ago

I propose that we put this patch on master for the 2.9 release. Any objections?

#11 Updated by bmbouter almost 5 years ago

Going into the 2.9 release sounds fine, but FYI at this moment master is still 2.8.3. master will become 2.9.0 when we branch off the 2.8-dev branch from master.

#12 Updated by rbarlow almost 5 years ago

  • Platform Release set to 2.9.0
  • Triaged changed from Yes to No

Let's retriage it, since it has a patch and we have community testing from kfiresmith. I propose 2.9.0.

#13 Updated by kfiresmith almost 5 years ago

Please ignore last comment. Something reverted the local customizations to the python files listed in smeyers' patch. I re-added '@' to the regex and everything works again. D'oh!

#14 Updated by mhrivnak almost 5 years ago

  • Triaged changed from No to Yes

#15 Updated by semyers over 4 years ago

  • Platform Release deleted (2.9.0)

#16 Updated by bmbouter over 4 years ago

  • Sprint Candidate changed from No to Yes

#17 Updated by amacdona@redhat.com over 4 years ago

  • Groomed changed from No to Yes

#18 Updated by mhrivnak over 4 years ago

  • Sprint/Milestone set to 23

#19 Updated by amacdona@redhat.com over 4 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to amacdona@redhat.com

#20 Updated by amacdona@redhat.com over 4 years ago

  • Status changed from ASSIGNED to POST

#21 Updated by Anonymous over 4 years ago

  • Status changed from POST to MODIFIED
  • % Done changed from 0 to 100

#22 Updated by amacdona@redhat.com over 4 years ago

  • Platform Release set to 2.9.2

#23 Updated by semyers over 4 years ago

  • Status changed from MODIFIED to 5

#24 Updated by pthomas@redhat.com over 4 years ago

  • Status changed from 5 to 6

verified

[root@tigger ~]# pulp-admin auth user create --login jim_bob@AD.COLLEGE.EDU
Enter password for user [jim_bob@AD.COLLEGE.EDU] : 
Re-enter password for user [jim_bob@AD.COLLEGE.EDU]: 
User [jim_bob@AD.COLLEGE.EDU] successfully created

[root@tigger ~]# rpm -qa pulp-server
pulp-server-2.9.2-0.2.beta.el7.noarch
[root@tigger ~]# 

#25 Updated by semyers over 4 years ago

  • Status changed from 6 to CLOSED - CURRENTRELEASE

#27 Updated by bmbouter almost 3 years ago

  • Sprint set to Sprint 5

#28 Updated by bmbouter almost 3 years ago

  • Sprint/Milestone deleted (23)

#29 Updated by bmbouter almost 2 years ago

  • Tags Pulp 2 added

Please register to edit this issue

Also available in: Atom PDF