Add IP address checking to services_keyauth authentication method.

Is this secure? I was under the impression that the IP address in the HTTP request is easily spoofed (much like REFERRER and other data in the request.) If that is true I'd rather not offer it at all than give a false sense of security.

My I add another question? Does this really have to do with keyauth? or would IP address access more focused on services, with or without keys being in use?

Mathiaz.sk, I'm trying to find the best place for IP address access control, perhaps it should be done previously by the services module itself instead of an option in the keyauth module.

How did you come to this situation? do you need the keyauth based also in IP address checking or something?

After some research it looks like REMOTE_ADDR is trustworthy, as it comes from Apache and not from the browser. I do agree that maybe this is best as a general services setting rather than a keyauth-specific setting. That way it would be global rather than tied to a specific auth method, although since we can only currently have one auth method per server anyways I'm not sure how much that matters.

Anyways this is a sensible addition and I'll try to review this and do some testing soon.

Went to look at this patch today and it needs a reroll, will try and get back to it soon but I don't consider it a release blocker so someone else might need to.

Marking this as postponed. If somebody wants to get it in for the first stable release please submit a new patch.

Thinking about this, it might be worth working this into Services core rather than into the auth methods themselves. I assume this would take effect AFTER Drupal's existing IP filtering, so it would only be useful in cases where you want a separate filter from the rest of your services.

Anyways this is worth looking into

In my use case, it would be better to restrict the use of a key to a specific IP address. This way a key is associated with an IP, and an attacker could not steal the key and use it from another machine (i.e., another IP address). Since for this IP addresses would be associated with keys, it would need to be done in services_keyauth.

Re #6, IIRC, it is possible to spoof the source address of an IP packet (which is what REMOTE_ADDR comes from), but the attacker would not likely get a response to their call, because return packets would be routed to the wrong address. However, this is a security problem as an attacker could call methods to do things even if they don't see the returns.

I just looked up a post to corroborate my previous paragraph:
http://forums.devshed.com/showpost.php?p=2387654&postcount=2

I'm not aware of any good solution to this spoofability. I do think IP address restriction still has value though, as spoofing an IP is a more sophisticated attack. Services could have a disclaimer that explains this potential vulnerability. Note that Drupal's IP address filtering has this same issue, yet no disclaimer is made.

Accidental double post deleted.

mathiaz.sk: I think I see a problem with your patch from #1:

-    $api_key = db_result(db_query("SELECT kid FROM {services_keys} WHERE domain = '%s'", $domain));
+    $key = db_fetch_object(db_query("SELECT * FROM {services_keys} WHERE domain = '%s'", $domain));
 
     //if (!services_keyauth_validate_key($api_key, $timestamp, $domain, $nonce, $method_name, $hash_parameters, $hash)) {
     if ($hash != services_get_hash($timestamp, $domain, $nonce, $method, $args)) {
       return services_error(t('Invalid API key.'), 401);
     }

If I'm reading that right, the key goes into $key, but $api_key is the variable checked. I suspect that you did a diff versus a file that was not up to date. Could you possibly reroll a patch if this is the case, so I can evaluate it?

Edit: Duh, the line with $api_key is commented out. Nevermind.

I just noticed that you posted this patch half a year ago, so I'll take a look at fixing it myself since you may not be interested in this issue any longer.

I applied mathiaz.sk's changes in 6.x-2.2. Tested them out and it works fine. I made some changes to text and code formatting, but did not change the actual code. (I can enumerate them if desired.) New patch attached.

I think there is good reason for tying the IP addresses to specific API keys (see first paragraph #10). To do this, the changes have to be to services_keyauth, as with this patch. However, I also see the use case for having IP-address restrictions at the Services module level. Perhaps the best solution is to provide both methods. They wouldn't be redundant in my opinion.

Setting to 6.x-2.x-dev temporarily so the patch is tested against the matching code version.

I'll happily review this as I used to work with mathiaz.sk and know how the original patch code used to work on the project it was developed for before it became a formal patch. I'll also if I can get him to have a quick look as well if he has some spare time.

Here's a list of changes I made versus mathiaz.sk's original patch (besides rolling it against different code):
1. Changed "IP Addresses" to "IP addresses" in admin_keys_list().
2. "Write one IP address per line" -> "Enter one IP address per line." ("Write" is colloquial; added period.)
3. Changed error message from "Forbidden IP Address" to "Forbidden IP address." (No reason to capitalize "address"; all other error messages end with period.)
4. Fixed typo "adresses" in comment.
5. Changed comment "// authenticate IP address" to "// Authenticate IP address." (Consistency with other comments.)
6. Broke +80-char code line, since this is done in the rest of the module.
7. Changed Schema API description of ip_addresses column from "The IP addresses of the service key" to "The allowed IP addresses for the service key." (I feel this is more descriptive, but this is a matter of opinion.)

I am wondering why this is going to services core instead of having a separate auth module; maybe IP_Auth?

then the question might become "Can we use IP_Auth together with Key Auth or OAuth" ?

On the other hand, is there a way we can re-use Drupal Access rules? Which you can specify a white list of IP address (Does Services by-pass Drupal Access rules?)

Re skyredwang's questions:

I am wondering why this is going to services core instead of having a separate auth module; maybe IP_Auth?

then the question might become "Can we use IP_Auth together with Key Auth or OAuth" ?

This patches services_keyauth, which is an auth module and not a part of Services core. If it is possible to make this a separate IP_Auth module that enhances services_keyauth, I would be in favor of that. If not possible, these optional IP address restrictions are a pretty lightweight addition to services_keyauth.

On the other hand, is there a way we can re-use Drupal Access rules?

This patch importantly lets you tie specific IP addresses to a specific API key. So, if I give an API key to a remote machine, it would be hard for an attacker to steal that API key and use it to access services--they would have to be coming from the same IP address that was associated with the key. It's not possible to do this with Drupal access rules.

(Does Services by-pass Drupal Access rules?)

Good question. I assumed this patch would provide a second layer of restrictions beyond the access rules, but I have never tested this. Maybe the Drupal access rules aren't used at all by Services?

Rerolled patch. mathiaz.sk, I think the problem with your previous patch might have been that you did not use the -p flag.

Versus my previous roll of the patch, I incorporated the following the changes from mathiaz.sk's patch #3:
1. Added 'default' and 'not null' parameters to ip_addresses in hook_schema().
2. Wrapped long line under if ($is_existing).

So the patch is the same as mathiaz.sk's patch #3 except:
1. Did not remove trailing space from "A random value." line. (Not germane to this issue.)
2. Made change 5 from comment #17 above.

(Does Services by-pass Drupal Access rules?)

I tested this, and no, it does not. So the IP address checking in services_keyauth provides a second layer of IP address restriction that restricts certain API keys to certain addresses. The Drupal access rules alone are not sufficient for restricting access to services: If you have a site that needs to be public, but you need to restrict use of an API key to specific IPs for security, then you need the functionality provided by this patch.

Here's a generic, elaborated use case for this feature:
I have a public site (i.e., world-accessible) that collects some personal data. This personal data needs to be shared with other backend servers. I do not want the personal data to be exposed at all costs. To access the personal data, each backend server must have an API key that allows it to access XML-RPC services of the site. There is a risk that the API key could be stolen or inadvertently exposed (e.g., unencrypted machine backups) and used to obtain personal data. To reduce this risk, the API key can be tied to a specific IP--that way if the API key is compromised, an adversary cannot get personal data unless he or she comes from the specified IP.

As I understand this patch does pairing key codes and specified IP. What about restricting access from specified IP addresses at all (even not using keys)? According to #14 it will be also nice idea to implement.

ygerasimov: I agree that IP-address restrictions on which services can be called regardless of API keys would be useful. We could maybe have two ways of doing this: (1) restricting access to all services by IP, and (2) restricting access to a specific service by IP.

I may not have time to try to implement this in the coming weeks. (Maybe you or someone else?) Do you think this additional IP-address restriction would be necessary before incorporating the patch for API-key-paired IP-address restriction? I am eager to get that included in a release; it's been a long time coming. I don't see any disadvantage to adding some IP-address restriction now and the rest later.

IP-address based authentication is very useful, and because it's so useful, it should become its own auth module for re-usability. and Services should have the architecture to enable multiple auth modules, so people can use Key Auth + IP Auth, or IP Auth + OAuth. or IP Auth + Some-Auth-In-The-Future.

Therefore, I don't want to see you patching the existing Key Auth.

I agree that in the 3.x branch it should be its own auth module, but in 2.x there really isn't any other option but to combine it with key auth. I'm happy to consider the patch above for a 2.3 release should I ever get around to it, but whether that will ever happen I can't say at this point.

I will review and commit in the next couple of days.

@threexk: regarding restricting access for each service separately, I believe that we should start with most simple implementation: restrict to all services at once. After testing this we can expand restriction for each service separately. I will try to implement this functionality and will come back with the results.

Here is patch for creating IP Auth method. I have copied a lot from Key auth to support sessid. I believe we should really change auth system to get possibility to use several restriction methods and not one. Like described in #27.

BTW discussion about splitting of the auth methods can be found in other issue #465546: Decouple api and user authentication

Setting back to 3.x-dev.

Closing as 2.x is feature frozen.