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

[THREESCALE-586] Default credentials policy #741

Merged
merged 5 commits into from
Jun 4, 2018

Conversation

davidor
Copy link
Contributor

@davidor davidor commented Jun 1, 2018

Closes #734

To avoid repeating work.
Other policies could have already extracted the credentials. For
example, the default_credentials one.
@davidor davidor force-pushed the default-credentials-policy branch from b18cebd to f40f1de Compare June 1, 2018 15:07
@davidor davidor added this to the 3.3 milestone Jun 1, 2018
@davidor davidor force-pushed the default-credentials-policy branch 4 times, most recently from 54cf5dc to 1a012e4 Compare June 1, 2018 16:09
@davidor davidor changed the title [WIP] Default credentials policy Default credentials policy Jun 1, 2018
@davidor davidor requested a review from mikz June 1, 2018 16:11
return self
end

local function creds_missing(service)
Copy link
Contributor Author

@davidor davidor Jun 1, 2018

Choose a reason for hiding this comment

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

This should be moved to the Service module. It will simplify the unit tests a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that in order to do this, we'd need to extract a Credentials class. Credentials are used in many parts of the code (service, backend_client, proxy, etc.), so this would not be an easy refactor. That's why I'm leaning towards leaving it for a future PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should try to avoid overloading the Service module. Just because it is coupled to 3scale data model and in the future we should work without it.

So I'd try to rather try to make very small independent components and Credentials class could work pretty well I think. But it should not really depend on "service".
I agree it is much bigger effort, because it should be designed in a way it will support various authentication methods like TLS, JWT, ...

@davidor davidor changed the title Default credentials policy [WIP] Default credentials policy Jun 1, 2018
@davidor davidor removed the request for review from mikz June 1, 2018 16:27
@davidor davidor changed the title [WIP] Default credentials policy Default credentials policy Jun 4, 2018
@davidor davidor requested a review from mikz June 4, 2018 08:37
"configuration": {
"type": "object",
"properties": {
"user_key": { "type": "string" },
Copy link
Contributor

Choose a reason for hiding this comment

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

I was hoping this actually could be "oneOf" and user would chose user_key/app_id. But that probably can't happen because of the limitation of the react library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find a way to do that.
Something like this does not work:

{  
   "type":"object",
   "properties":{  
      "credentials":{  
         "type":"object",
         "oneOf":[  
            {  
               "type":"object",
               "properties":{  
                  "user_key":{  
                     "type":"string"
                  }
               }
            },
            {  
               "type":"object",
               "properties":{  
                  "app_key":{  
                     "type":"string"
                  },
                  "app_id":{  
                     "type":"string"
                  }
               }
            }
         ]
      }
   }
}

Copy link
Contributor

@mikz mikz Jun 4, 2018

Choose a reason for hiding this comment

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

Thats exactly what I had in mind.

What about:

{
  "type": "object",
  "properties": {
    "credentials": {
      "type": "array",
      "items": {
        "type": "object",
        "properties": {
          "name": {
            "type": "string",
            "enum": ["app_id", "app_key", "user_key"]
          },
          "value": {
            "type": "string"
          }
        }
      }
    }
  }
}

My goal with this schema would be to avoid the backend version detection and just trust the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That can be loaded in the react lib, but it has other problems.

With that solution, an array with app_id, app_key and user_key is valid. Also, an array with user_key and app_key for example, or an array with only app_key.

Also, with that solution including, for example, 2 different user_keys is valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we keep the backend version detection, then it works exactly the same as current solution.
It has exactly the same problems as it is now: you can fill any combination of them.

We can make one field required if other field is filled in: https://github.com/mozilla-services/react-jsonschema-form#property-dependencies (but that would not work with this array).

I think with https://github.com/mozilla-services/react-jsonschema-form#dynamic we could do a selector to chose a backend version and then let them fill app_id+app_key / user_key.

I'm not advocating for actually doing it, but I'd like to evaluate our options.
Also I think we don't really have to strongly validate everything. If they want to fill in just app_key, let them.

BTW what if we want to expand it to support JWT/OAuth/TLS in the future? Would one structure be better over another? Without oneOf not really, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dependencies would fail to validate things like incluing an app_id with a user_key.
The dynamic schema would work. You can try it in https://mozilla-services.github.io/react-jsonschema-form/ to see how it looks:

{
    "type":"object",
    "properties":{
      "auth_type":{
        "type":"string",
        "enum":[
          "user_key",
          "app_id_and_app_key"
        ],
        "default":"user_key"
      }
    },
    "required":[
      "auth_type"
    ],
    "dependencies":{
      "auth_type":{
        "oneOf":[
          {
            "properties":{
              "auth_type":{
                "enum":[
                  "user_key"
                ]
              },
              "user_key":{
                "type":"string"
              }
            },
            "required":[
              "user_key"
            ]
          },
          {
            "properties":{
              "auth_type":{
                "enum":[
                  "app_id_and_app_key"
                ]
              },
              "app_id":{
                "type":"string"
              },
              "app_key":{
                "type":"string"
              }
            },
            "required":[
              "app_id",
              "app_key"
            ]
          }
        ]
      }
    }
  }

With that schema, the config would be something like:

{
    "auth_type": "user_key",
    "user_key": "some_user_key"
}

or

{
  "auth_type": "app_id_and_app_key",
  "app_id": "some_app_id",
  "app_key": "some_app_key"
}

It looks good to me. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks good to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed 👍

@@ -225,7 +225,13 @@ function _M:rewrite(service, context)

ngx.var.secret_token = service.secret_token

local credentials, err = service:extract_credentials()
-- Another policy might have already extracted the creds.
local credentials = service.extracted_credentials
Copy link
Contributor

Choose a reason for hiding this comment

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

Why .extracted_credentials and not just .credentials ? The default credentials are not really "extracted".

Copy link
Contributor Author

@davidor davidor Jun 4, 2018

Choose a reason for hiding this comment

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

Because .credentials already exists in service and it has a different kind of information. It contains the location for credentials (query, headers, etc.) and also the name used to look for the different types of keys:

https://github.com/3scale/apicast/blob/6a48e85580e6273d3c66d590a3831c5787b4b097/gateway/src/apicast/configuration.lua#L106

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh boy. This would definitely would benefit from some cleanup.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about storing this incontext.credentials ? Or better we do that when we have proper Credentials class/object ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would wait until we extract the Credentials class.


local function provide_creds(service, default_creds)
local backend_version = tostring(service.backend_version)
creds_provider[backend_version](service, default_creds)
Copy link
Contributor

@mikz mikz Jun 4, 2018

Choose a reason for hiding this comment

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

~~This will crash on invalid version. ~~

edit: It won't because the check is somewhere else. But still I think it would be nicer to unify that.

end

local backend_version = tostring(service.backend_version)
if backend_version ~= '1' and backend_version ~= '2' then
Copy link
Contributor

Choose a reason for hiding this comment

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

This duplicates the creds_provider. Imo it should check if there is creds provider and then run it else return an error instead of duplicating the checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the method that gets called before provide_creds (missing_creds), also needs to check if the backend_version is supported by the policy.

Doing the check where it's done now allows us to assume in the rest of the code (missing_creds and provide_creds) that the backend version is supported. If we don't do that, we'll need to add checks and ngx.log(ngx.ERR, ...) in those 2 methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it still can be changed to if not creds_provider[backend_version], right?

My point is that if you remove/add element to creds_provider this line would also have to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I rewrote this in a way that adding/removing supported backend_versions would imply changes only in one part of the code.

local policy_with_default_app_id
local policy_with_default_app_key

local service_with_user_key_in_req
Copy link
Contributor

@mikz mikz Jun 4, 2018

Choose a reason for hiding this comment

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

Wouldn't it be better for this to be a function? Then you could drop the before_each and just use local variables inside the test block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Also, the before_each seems to imply that everything there is needed for every test, which is not true.

Changed 👍

@davidor davidor force-pushed the default-credentials-policy branch from 1a012e4 to 407e7de Compare June 4, 2018 13:06
@davidor davidor requested a review from mikz June 4, 2018 13:09
Copy link
Contributor

@mikz mikz left a comment

Choose a reason for hiding this comment

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

👍 🥇

@davidor davidor changed the title Default credentials policy [THREESCALE-586] Default credentials policy Jun 4, 2018
@davidor davidor merged commit bc19191 into master Jun 4, 2018
@davidor davidor deleted the default-credentials-policy branch June 4, 2018 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants