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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Ability to modify query parameters in the URL rewriting policy [PR #724](https://github.com/3scale/apicast/pull/724)
- 3scale referrer policy [PR #728](https://github.com/3scale/apicast/pull/728)
- Liquid templating support in the rate-limit policy [PR #719](https://github.com/3scale/apicast/pull/719)
- Default credentials policy [PR #741](https://github.com/3scale/apicast/pull/741), [THREESCALE-586](https://issues.jboss.org/browse/THREESCALE-586)

### Changed

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
{
"$schema": "http://apicast.io/policy-v1/schema#manifest#",
"name": "Default credentials",
"summary": "Provides default credentials for unauthenticated requests",
"description":
["This policy allows to expose a service without authentication. ",
"It can be useful, for example, for legacy apps that cannot be adapted to ",
"send the auth params. ",
"When the credentials are not provided in the request, this policy ",
"provides the default ones configured. ",
"An app_id + app_key or a user_key should be configured."],
"version": "builtin",
"configuration": {
"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"
]
}
]
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
--- Default credentials policy

local tostring = tostring

local policy = require('apicast.policy')
local _M = policy.new('Default credentials policy')

local new = _M.new

function _M.new(config)
local self = new(config)

if config then
self.default_credentials = {
user_key = config.user_key,
app_id = config.app_id,
app_key = config.app_key
}
else
self.default_credentials = {}
end

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, ...

local service_creds = service:extract_credentials()
if not service_creds then return true end

local backend_version = tostring(service.backend_version)

if backend_version == '1' then
return service_creds.user_key == nil
elseif backend_version == '2' then
return service_creds.app_id == nil and service_creds.app_key == nil
end
end

local function provide_creds_for_version_1(service, default_creds)
if default_creds.user_key then
-- follows same format as Service.extract_credentials()
service.extracted_credentials = {
default_creds.user_key,
user_key = default_creds.user_key
}

ngx.log(ngx.DEBUG, 'Provided default creds for request')
else
ngx.log(ngx.WARN, 'No default user key configured')
end
end

local function provide_creds_for_version_2(service, default_creds)
if default_creds.app_id and default_creds.app_key then
-- follows same format as Service.extract_credentials()
service.extracted_credentials = {
default_creds.app_id,
default_creds.app_key,
app_id = default_creds.app_id,
app_key = default_creds.app_key
}

ngx.log(ngx.DEBUG, 'Provided default creds for request')
else
ngx.log(ngx.WARN, 'No default app_id + app_key configured')
end
end

local creds_provider = {
["1"] = provide_creds_for_version_1,
["2"] = provide_creds_for_version_2
}

local function backend_version_is_supported(backend_version)
return creds_provider[backend_version] ~= nil
end

local function provide_creds(service, default_creds)
local backend_version = tostring(service.backend_version)
creds_provider[backend_version](service, default_creds)
end

function _M:rewrite(context)
local service = context.service

if not service then
ngx.log(ngx.ERR, 'No service in the context')
return
end

local backend_version = tostring(service.backend_version)
if not backend_version_is_supported(backend_version) then
ngx.log(ngx.ERR, 'Incompatible backend version: ', backend_version)
return
end

if creds_missing(service) then
provide_creds(service, self.default_credentials)
end
end

return _M
1 change: 1 addition & 0 deletions gateway/src/apicast/policy/default_credentials/init.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
return require('default_credentials')
8 changes: 7 additions & 1 deletion gateway/src/apicast/proxy.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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 err
if not credentials then
credentials, err = service:extract_credentials()
end

if not credentials then
ngx.log(ngx.WARN, "cannot get credentials: ", err or 'unknown error')
Expand Down
Loading