Fix inconsistencies in app key validations #3639

mayorova · 2023-11-27T14:12:16Z

What this PR does / why we need it:

There is a number of issues with the current validation rules for User Keys, Application IDs and Application Keys:

what's stated in the UI in the hints is just not correct for App Ids and Keys
some characters that are accepted by System, can not be synchronized with Backend (for example, keys containing /) - so the user will think the key is correct, but it will never work for API auth.
some characters that are supposed to be accepted (specifically .) are actually rejected by System (because it's not accepted in Rails path by the router by default).
user_key field allows empty value, but this just doesn't work. Trying to open such an app when a service uses API Key auth method returns a 500 Internal Server Error. Also, obviously such a key can't be synchronized with backend, so is not functional.

Which issue(s) this PR fixes

https://issues.redhat.com/browse/THREESCALE-7570

https://issues.redhat.com/browse/THREESCALE-10002

Verification steps

Try to set a user key, app ID or app key with a / or a space and confirm that the operation does not succeed, and the appropriate error is shown.
Try to create and delete an application key with a dot . in it, and confirm that it works as expected.
Try to update an application and set an empty user_key, it should fail.

Special notes for your reviewer:

The issue with slashes (failing on the backend) is due to path traversing protection in sinatra that apisonator uses.
In this PR we're just forbidding / in user keys, app IDs and keys.

But alternative (maybe a better) approach could be to fix it on apisonator side by setting the following for the internal API:

set :protection, except: :path_traversal

But I'm not 100% sure that it is safe and doesn't have any negative consequences, so I thought we could be more conservative and just disallow /. This character doesn't work at this moment anyway, so we're not removing any existing functionality. Also, it has never been documented anywhere that it was a supported character.

There might be an issue for some customers that might potentially use 3scale together with their 3rd party OpenID / OAuth providers that generate client ID and secrets automatically, and might include /. But if requested by anyone, we can revisit.

…p ids/keys

app/models/cinstance.rb

jlledom · 2023-11-28T08:22:49Z

app/models/cinstance.rb


  validates :user_key, format: { with: /\A#{USER_KEY_FORMAT}\Z/ }, length: { maximum: 256 },
-            allow_nil: true, allow_blank: true


How could a user key be nil before?

It still can be, because allow_blank includes nil as well...

I will check if there are such entries in the DB. In theory those applications that are on App Id / App Key or OpenID modes don't need user_key, but I think technically they would still be auto-generated.

Actually, I'm thinking... Maybe we need to stop allowing blank values, I think it doesn't make any sense. And actually, there are not so many blank user keys in the current DB, about 9 on the accounts that are still active, and among these there are at least 3 that belong to internal test accounts...

So, I think we can remove allow_blank as well.

jlledom · 2023-11-28T08:47:01Z

app/models/cinstance.rb

+  # The following characters are accepted:
+  # A-Z a-z 0-9 ! " # $ % & ' ( ) * + , - . : ; < = > ? @ [ \ ] ^ _ ` { | } ~
+  # Spaces and / are not allowed
+  validates :application_id, format: { with: /\A[\x21-\x2E\x30-\x7E]+\Z/ }, length: { in: 4..255 }


This could prevent updating existing applications as well. I've been looking for any touch among Cininstance associations that could try to update the application as a result of some other action. The only thing I found is this:

porta/app/models/application_key.rb

Line 78 in 18c529f

proxy_association.owner.touch if succeeded

I'm not sure what that proxy_associtation.owner means, could it be the Cinsntance? If that's the case Oauth applications won't be able to regenerate keys on if they have spaces or slashes in their ids.

Good catch, I'll check that.

There are just 2 apps with invalid app ID: one is on test account, and another one is not in use since 2020.

test/unit/application_keys_test.rb

jlledom · 2023-11-28T08:55:29Z

I tried locally and the UI works fine for me.

I found an issue when trying to send queries to apicast. These are the keys for my app:

Using the key abc.abc works fine:

$ curl "http://api-2.staging.apicast.dev:8080/?app_id=d0945643&app_key=abc.abc"
{
  "method": "GET",
  "path": "/",
  "args": "app_id=d0945643&app_key=abc.abc",
...

But I couldn't use the [abc] key:

$ curl "http://api-2.staging.apicast.dev:8080/?app_id=d0945643&app_key=%5Babc%5D"
Authentication failed

Am I doing something wrong?

Co-authored-by: Joan Lledó <jlledo@redhat.com>

mayorova · 2023-11-29T13:26:02Z

It's great that you have checked this @jlledom !

I've tried this also, and it turns out that when you encode in the curl command, backend receives the key kind of double-encoded (% is changed to %25): app_id=d0945643&app_key=%255Babc%255D

[] have a special meaning in curls, but they can be escaped:

$ curl "http://api-2.staging.apicast.dev:8080/?app_id=d0945643&app_key=\[abc\]"

or alternatively:

$ curl --globoff "http://api-2.staging.apicast.dev:8080/?app_id=d0945643&app_key=[abc]"

See "globbing letters" in https://curl.se/docs/url-syntax.html

Note that there is another outstanding issue that is not solved here: https://issues.redhat.com/browse/THREESCALE-9033
It will be solved separately, as it requires a change to pisoni gem.

jlledom

If don't feel like doing the refactor that's fine.

jlledom · 2023-12-01T11:29:10Z

app/lib/backend/model_extensions/service.rb

+      # TODO: Refactor and remove me
      def backend_id
        prefix_key
      end


At first glance, it seems this could be replaced by alias :backend_id :id. I would prevent adding #TODOs amd #FIXMEs if possible.

#3647

It would probably be more correct to have:

def backend_id id.to_s end

because this would be a full equivalent of the current situation.

However, using an integer instead of string should not have a negative effect.

jlledom · 2023-12-01T11:35:31Z

app/models/service.rb

+  # TODO: Refactor and remove me
  def prefix_key(key = id)
    key.to_s
  end


This could be just removed and prevent adding more #TODOs. You would need to make some changes in the places where this is called from. If after the changes it passes the tests, I'd say it's safe enough.

Yeah, I'd prefer to do it in another PR, because some of the changes are unrelated. I promise to fix it ASAP! 😛

Here you go: #3647

mayorova · 2023-12-01T11:42:34Z

config/locales/en.yml

-        user_key: "Only alphanumeric characters [0-9, a-z, A-Z], hyphen-minus (-) and underscore, no spaces and up to 256 characters."
-        key: "Only alphanumeric characters [0-9, a-z, A-Z], hyphen-minus (-) and underscore, no spaces and between 5 and 255 characters."
+        user_key: "Allowed characters: [A-Z a-z 0-9 - _ .], or Base64 format without forward slash (/), no spaces and up to 256 characters."
+        key: "Allowed characters: [A-Z a-z 0-9 ! \" # $ % & ' ( ) * + , - . : ; < = > ? @ [ \ ] ^ _ ` { | } ~], no spaces and between 5 and 255 characters."


@josemigallas what do you think, are those messages OK?

mayorova added 5 commits November 27, 2023 11:50

Fix dots in user keys and application keys

9627ef6

Do not allow slash in user_key value

854de50

Not allow slash in app keys and ids

18c529f

Fix RuboCop warning

02d84a9

Update UI hints and API spec with the correct validation rules for ap…

9fbcace

…p ids/keys

jlledom reviewed Nov 28, 2023

View reviewed changes

Update test/unit/application_keys_test.rb

4972ca5

Co-authored-by: Joan Lledó <jlledo@redhat.com>

mayorova force-pushed the fix-inconsistencies-in-app-key-validations branch from 0061ffb to 4972ca5 Compare November 28, 2023 09:32

mayorova marked this pull request as ready for review November 28, 2023 13:41

mayorova added 2 commits November 29, 2023 12:23

Do not allow empty user_key

839d510

Remove obsolete and unused prefix in user key generation

98917e6

jlledom approved these changes Dec 1, 2023

View reviewed changes

mayorova commented Dec 1, 2023

View reviewed changes

mayorova merged commit 6847767 into master Dec 4, 2023
17 of 21 checks passed

mayorova deleted the fix-inconsistencies-in-app-key-validations branch December 4, 2023 13:54

mayorova mentioned this pull request Feb 22, 2024

Fix utilization URL for special characters 3scale/pisoni#33

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inconsistencies in app key validations #3639

Fix inconsistencies in app key validations #3639

mayorova commented Nov 27, 2023 •

edited

jlledom Nov 28, 2023

mayorova Nov 28, 2023

mayorova Nov 28, 2023

jlledom Nov 28, 2023

mayorova Nov 28, 2023

mayorova Nov 29, 2023

jlledom commented Nov 28, 2023 •

edited

mayorova commented Nov 29, 2023 •

edited

jlledom left a comment

jlledom Dec 1, 2023

mayorova Dec 1, 2023

jlledom Dec 1, 2023

mayorova Dec 1, 2023

mayorova Dec 1, 2023

mayorova Dec 1, 2023


		validates :user_key, format: { with: /\A#{USER_KEY_FORMAT}\Z/ }, length: { maximum: 256 },
		allow_nil: true, allow_blank: true

Fix inconsistencies in app key validations #3639

Fix inconsistencies in app key validations #3639

Conversation

mayorova commented Nov 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlledom commented Nov 28, 2023 • edited

mayorova commented Nov 29, 2023 • edited

jlledom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova commented Nov 27, 2023 •

edited

jlledom commented Nov 28, 2023 •

edited

mayorova commented Nov 29, 2023 •

edited