636514 - Create a new mozilla_sreg backend

Status: VERIFIED FIXED

(Cloud Services :: Server: Core, defect)

Description

[Tarek Ziadé (:tarek)]

That uses the server described here: https://wiki.mozilla.org/Services/Sync/SRegAPI

Comment 2

[Tarek Ziadé (:tarek)]

Attachment: Adds the mozilla_sreg backend

Implements the client-side for sreg.

Comment 3

[Toby Elliott [:telliott]]

Comment on attachment 526229 [details] [diff] [review]
Adds the mozilla_sreg backend

Hmm, I thought I was supposed to write this half. Anyway... 

A few things concern me:

1) The __init__.py preload approach doesn't seem optimal, and it means people can't just drop in additional auths without having to worry about an hg update messing them up. Should we consider something more akin to the account-portal dynamic-load? (This isn't a blocker, just noting it)

2) I don't like the idea that this inherits from MozillaAuth. It should be replacing MozillaAuth and inheriting straight from LDAPSql. The only stuff MozillaAuth brings to the party is the proxy functions, and those can just move over here. Once everything is switched over to this, we should expect mozilla.py to be deleted.

3) We either need to rethink how we're doing the proxy, or we need to change a little bit of the spec. There are a few "error" conditions that aren't really errors. For example, if the user requests a reset, but doesn't have a valid email on file, this class will get back a 400, which it turns into a backend error. That's a really bad user experience. We can either make that a 200 with a non-success code, or have _proxy throw a specific error. Other non-200 functions we'll need to trap: writing to an existing username, non-valid password reset code.

4) Related to that, we aren't currently verifying that the user exists on many of these functions. Are we just assuming that that has already been determined to be the case and thus we should assume a 404 is a backend problem? That's a reasonable assumption, but I want to make sure we're clear on it. I suppose 404 always has the same meaning, so we could track it separately in proxy.

Comment 4

[Tarek Ziadé (:tarek)]

(In reply to comment #3)
> Comment on attachment 526229 [details] [diff] [review]
> Adds the mozilla_sreg backend
> 
> Hmm, I thought I was supposed to write this half. Anyway... 

oops sorry -- there's still the reg half though

> 
> A few things concern me:
> 
> 1) The __init__.py preload approach doesn't seem optimal, and it means people
> can't just drop in additional auths without having to worry about an hg update
> messing them up. Should we consider something more akin to the account-portal
> dynamic-load? (This isn't a blocker, just noting it)

there's also a dynamic load. you just need to use the fully qualified name in the config file:

[auth]
backend = package.module.here

> 
> 2) I don't like the idea that this inherits from MozillaAuth. It should be
> replacing MozillaAuth and inheriting straight from LDAPSql. The only stuff
> MozillaAuth brings to the party is the proxy functions, and those can just move
> over here. Once everything is switched over to this, we should expect
> mozilla.py to be deleted.

Good point. I'll change this

> 
> 3) We either need to rethink how we're doing the proxy, or we need to change a
> little bit of the spec. There are a few "error" conditions that aren't really
> errors. For example, if the user requests a reset, but doesn't have a valid
> email on file, this class will get back a 400, which it turns into a backend
> error. That's a really bad user experience. We can either make that a 200 with
> a non-success code, or have _proxy throw a specific error. Other non-200
> functions we'll need to trap: writing to an existing username, non-valid
> password reset code.

So, some of these used to be 200 + a body with the error code, and I changed it because the spec was change that way. I am fine to do 200s + error code.

> 
> 4) Related to that, we aren't currently verifying that the user exists on many
> of these functions. 
> Are we just assuming that that has already been determined
> to be the case and thus we should assume a 404 is a backend problem? That's a
> reasonable assumption, but I want to make sure we're clear on it. I suppose 404
> always has the same meaning, so we could track it separately in proxy.

We do. This is called for every method except the user creation: https://hg.mozilla.org/services/server-sreg/file/c879701bb46c/syncsreg/controller.py#l62

if the user does not exists it returns a 404 immediatly. also, maybe a specific code in the body + a 400 would be more explicit

Comment 5

[Toby Elliott [:telliott]]

(In reply to comment #4)
> (In reply to comment #3)

> > 
> > 3) We either need to rethink how we're doing the proxy, or we need to change a
> > little bit of the spec. There are a few "error" conditions that aren't really
> > errors. For example, if the user requests a reset, but doesn't have a valid
> > email on file, this class will get back a 400, which it turns into a backend
> > error. That's a really bad user experience. We can either make that a 200 with
> > a non-success code, or have _proxy throw a specific error. Other non-200
> > functions we'll need to trap: writing to an existing username, non-valid
> > password reset code.
> 
> So, some of these used to be 200 + a body with the error code, and I changed it
> because the spec was change that way. I am fine to do 200s + error code.
> 

I'm fine either way. I think the 400 is more correct, but if the 200 is easier, that's good too. The point is simply that we need to be accounting for those three in some fashion that is delivered back up to the user, especially in account-portal.




> > 
> > 4) Related to that, we aren't currently verifying that the user exists on many
> > of these functions. 
> > Are we just assuming that that has already been determined
> > to be the case and thus we should assume a 404 is a backend problem? That's a
> > reasonable assumption, but I want to make sure we're clear on it. I suppose 404
> > always has the same meaning, so we could track it separately in proxy.
> 
> We do. This is called for every method except the user creation:
> https://hg.mozilla.org/services/server-sreg/file/c879701bb46c/syncsreg/controller.py#l62
> 
> if the user does not exists it returns a 404 immediatly. also, maybe a specific
> code in the body + a 400 would be more explicit

OK, so we are making the assumption. That's all I wanted to verify here.

Comment 6

[Toby Elliott [:telliott]]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> 
> > > 
> > > 3) We either need to rethink how we're doing the proxy, or we need to change a
> > > little bit of the spec. There are a few "error" conditions that aren't really
> > > errors. For example, if the user requests a reset, but doesn't have a valid
> > > email on file, this class will get back a 400, which it turns into a backend
> > > error. That's a really bad user experience. We can either make that a 200 with
> > > a non-success code, or have _proxy throw a specific error. Other non-200
> > > functions we'll need to trap: writing to an existing username, non-valid
> > > password reset code.
> > 
> > So, some of these used to be 200 + a body with the error code, and I changed it
> > because the spec was change that way. I am fine to do 200s + error code.
> > 
> 
> I'm fine either way. I think the 400 is more correct, but if the 200 is easier,
> that's good too. The point is simply that we need to be accounting for those
> three in some fashion that is delivered back up to the user, especially in
> account-portal.

Thinking about these some more, I suggest adding two new errors to services.auth - NoEmailError and InvalidCodeError. We'll use those to distinguish those problems where relevant.

I think it's reasonable to 503 on trying to put an existing username. We should be checking that before submitting, and the race condition seems like a reasonable time to error out and have them retry.

Comment 7

[Toby Elliott [:telliott]]

Attachment: How we'd trap the errors in account portal

Comment 8

[Tarek Ziadé (:tarek)]

Comment on attachment 526838 [details] [diff] [review]
How we'd trap the errors in account portal

Review of attachment 526838 [details] [diff] [review]:

looks good -- also we pbly need an extra test

Comment 9

[Tarek Ziadé (:tarek)]

Attachment: Adds the mozilla_sreg backend

Comment 10

[Tarek Ziadé (:tarek)]

Comment on attachment 528631 [details] [diff] [review]
Adds the mozilla_sreg backend

Review of attachment 528631 [details] [diff] [review]:

.

Comment 11

[Tarek Ziadé (:tarek)]

Attachment: Adds the mozilla_sreg backend

small fix

Comment 12

[Tarek Ziadé (:tarek)]

Attachment: Adds the mozilla_sreg backend

the previous was a wrong patch

Comment 13

[Toby Elliott [:telliott]]

Comment on attachment 528638 [details] [diff] [review]
Adds the mozilla_sreg backend

Review of attachment 528638 [details] [diff] [review]:

Looks good.

Comment 14

[Tarek Ziadé (:tarek)]

Pushed at https://hg.mozilla.org/services/server-core/rev/331b12ec8c47

Comment 15

[Michael Coates [:mcoates] (acct no longer active)]

Comment on attachment 528638 [details] [diff] [review]
Adds the mozilla_sreg backend

Review of attachment 528638 [details] [diff] [review]:

::: services/auth/mozilla_sreg.py
@@ +181,5 @@
+    def update_password(self, user_id, new_password,
+                        old_password=None, key=None):
+        """Change the user password.
+
+        Uses the admin bind or the user bind if the old password is provided.

I don't like the design of overloading update_password with both admin and user functionality. This makes it too easy to make a small mistake in reg (not sending the user's submitted old password) and it translates into an admin call in sreg.

I'd like us to break this out into update_password (for users that provide old_password) and admin_update_password (for the case where the old password is not provided)

Comment 16

[Tarek Ziadé (:tarek)]

(In reply to comment #15)
> Comment on attachment 528638 [details] [diff] [review] [review]
> Adds the mozilla_sreg backend
> 
> Review of attachment 528638 [details] [diff] [review] [review]:
> 
> ::: services/auth/mozilla_sreg.py
> @@ +181,5 @@
> +    def update_password(self, user_id, new_password,
> +                        old_password=None, key=None):
> +        """Change the user password.
> +
> +        Uses the admin bind or the user bind if the old password is
> provided.
> 
> I don't like the design of overloading update_password with both admin and
> user functionality. This makes it too easy to make a small mistake in reg
> (not sending the user's submitted old password) and it translates into an
> admin call in sreg.
> 
> I'd like us to break this out into update_password (for users that provide
> old_password) and admin_update_password (for the case where the old password
> is not provided)

Sure that sounds more explicit, will work on a patch

Comment 17

[Tarek Ziadé (:tarek)]

Created a new bug for this: see #655281.

Michael, is this a blocker for this backend or can this specific change be postponed to the next push ? If it's ok to do it afterwards, please close the bug. o/w let's make 655281 a blocker of this one.

Comment 18

[Michael Coates [:mcoates] (acct no longer active)]

Yea, this is a blocker. 

If we had a mistake in the reg area that worked with this code we could introduce a scenario where a user could update another user's password.

So I don't want to introduce this code into prod and risk that possibility at all.

Comment 19

[Tarek Ziadé (:tarek)]

(In reply to comment #18)
> Yea, this is a blocker. 
> 
> If we had a mistake in the reg area that worked with this code we could
> introduce a scenario where a user could update another user's password.
> 
> So I don't want to introduce this code into prod and risk that possibility
> at all.

I fail to see how you could have a scenario where the change asked will make our system weaker or stronger wrt hacking a user account.

The only possible attack is for user A to make the system think he's user B by stealing his password or his password reset key: the password change process is asking for a authentication or a password reset key.

In all cases, even with the change you're asking, there will still be a way for reg to call sreg to change a user password without the original password since we have password reset keys for this.

In case user A sends a call with a wrong password --the use case you're describing if I understand correctly--, it's rejected before it reaches this code because we authenticate him first, so the backend don't have a chance to do an admin call there,

Comment 20

[Michael Coates [:mcoates] (acct no longer active)]

The concern is that a code change (or existing error) introduces a weakness into the reg code that calls update_password. For example, the reg code incorrectly handles the error case where the user doesn't send the old_password form value at all (not blank, totally deleted) and modifies the username to another user account (this is likely not possible, I'm assuming we don't pass the username and grab it from the session instead). In this case the proposed update_password code would fail into the "admin" mode and allow the user to update another user's password.

Is this currently possible, not sure.  But I don't want to put this vulnerable code into prod and then have to wait an unknown number of weeks for the patch to split it into update_password and admin_update_password.  In that interim we could have other patches to reg, which seem harmless in themselves, but actually introduce a vulnerability that could be compounded by this issue.

Comment 21

[Tarek Ziadé (:tarek)]

closing as the patch is done in bug 655281

Comment 22

[Tracy Walker [:tracy]]

account registration and node assignment are working fine on staging.