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.

Sign up for GitHub

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

Jump to bottom

Fix basic auth not generating a random password #800

Closed
wants to merge 3 commits into from
Closed

Fix basic auth not generating a random password #800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a028d4a
testcase for issue #772
Tieske Dec 12, 2015
4ed8d4a
if no password given, generate one and return it in an extra field. f…
Tieske Dec 13, 2015
f5c7ec7
fix regression
Tieske Dec 13, 2015
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
17 changes: 17 additions & 0 deletions kong/plugins/basic-auth/daos.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
local BaseDao = require "kong.dao.cassandra.base_dao"
local crypto = require "kong.plugins.basic-auth.crypto"
local utils = require "kong.tools.utils"

local function encrypt_password(password, credential)
credential.password = crypto.encrypt(credential)
Expand All @@ -26,4 +27,20 @@ function BasicAuthCredentials:new(properties)
BasicAuthCredentials.super.new(self, properties)
end

function BasicAuthCredentials:insert(params)
if params.password then
return BasicAuthCredentials.super.insert(self, params)
else
-- No password was provided, so we insert a random generated password
local newpwd = utils.random_string()
params.password = newpwd
local data, err = BasicAuthCredentials.super.insert(self, params)
-- inserting the data has encrypted the password field by now,
-- so add a new field with the generated plain text password
-- which will be returned to the requester
if data then data.plain_password = newpwd end
return data, err
end
end
Copy link
Member

Choose a reason for hiding this comment

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

The idea is to rather implement this at the schema level, to avoid having to do the same operations for all future DAO implementations. Kong's "Model" layer is the schema definition of an entity. It is where most of the logic should be implemented so that it is abstracted as much as possible from DAOs (and the Admin API).

Granted, the schema and the DAO are in the same file as of now, but ideally such logic should be implemented above (see the encrypt_password method), and leverage the schema default and func, or again on_insert functions to generate a random password. Things might be a bit tricky because the schema validation will not like an unknown field (plain_password), but I believe this is where the changes must be done and improvements to the schema_validation must be done too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not following you. In this case we need code executing before the update (generating password), and after (adding it in plaintext). func only runs the 'before' code.

Moving the code out of insert in a function above, would mean splitting it up in before and after functions. Calling both, and pass the password around in the arguments. The code would become worse imo. Is this what you meant or didn't I understand it correctly?

Looking at the schema definition I think func could be renamed to before_update and an after_update could be added. That would make it more clear imo, and easier to implement this type of updates.
Performance wise you might want to drop them and add the same two on the schema-level instead of the field-level (fewer check and function calls). Maybe could replace the marshall functions on the DAO also, though I haven't really looked in to those.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I'll explain in more details, as I don't know how much of this you already know, hang on. Sorry if I am repeating things you already know. Also, all of the following points are is still open to discussion, they are just the current implementation following the needs we faced with Kong (and the little time we had).

One of Kong's requirements was to be database agnostic. In order to do that, we initially used to have a Model, represented with "Lua classes" for each entity, and a DAO, which was the actual implementation of a given database's operations. The application code would use the Model (with insert/update... methods), which in turn would call the DAO. This lead to a lot of duplicated code, and the addition of new entities was much more verbose, which we did not want because we wanted anybody to simply be able to define a new entity with as little effort as possible.

To do so, we decided to ditch out the Model layer. But since a common (and valid) design pattern wants one to implement validation logic at the Model layer, we came up with those stateless schemas, which define rules on an entity. Having the validation being performed on the Model allows it to be run when the entity is manipulated form anywhere (Admin API, CLI, Proxy logic...). The schema_validations and schemas work together to apply various rules on entities and properties of those entities. The schema validation is then called by the DAO before any DB operations.

In the long term, we would like several things:

  • Kong to really be DB agnostic. That means an entity can be defined once, and any DAO implementation will follow the same validation rules. For now, the schema validation is called by the DAO itself, which is not optimal yet. When we implement the PostgreSQL DAO, it would have to also call the schema validation for example. This subject is still open to discussion.
  • A developer could create an entity just with a schema definition. Currently this is not the case, a developer has to define the schema, and implement a DAO that simply inherits from the base_dao (since it implements basic CRUD operations and schema validations). implementing the DAO is most of the time just a formality, and the resulting DAO is simply inherited from base_dao without anything added. Like the keyauth-credentials DAO. But sometimes, one can add new methods (for example the APIs's DAO's find_all(), which are implemented differently depending on the database. PostgreSQL and Cassandra won't be executing the same code to retrieve all APIs, as you might have already guessed.

As you can see, a Kong entity is currently made of a schema, and a DAO. This design pattern is simpler than an ORM one (like we previously has with the Model classes) and, as we learned over time, more adapted for Lua and ngx_lua. Entities are simple tables, but rules apply on this table's values.

Hence, the real "Model" layer of Kong are the schemas. Now, I did not say the API to define and validate schema was complete. The schema_validation is where all the logic happens (it suffers from its own complexity, for example I prefer the validation I implemented for the config validation in next, which is more modular).

Back to the point of my initial comment: if you implement such logic in the DAO, this logic only applies for the Cassandra DAO, and not for any future DAO. It also prevents one from running, say, a dry-run of the entity validation. Which, we never know, might be useful for some validations in the Admin API or CLI. My idea was to follow the current design pattern of Kong: implement this logic in the key-credential's schema. Yes, that would probably mean improvements are to be made on the schema API (like you said, before_* and after_* methods) (iirc, a on_insert method exists). The day we abstract DAO creation for entities, nobody will have to worry about re-implementing that logic in all existing DAO implementations (for each datastore I mean). It would also probably improve the schema validation API for other contributors and users (for example, #771 mentioned a way to "hide" fields, that has also been suggested in #385).

I hope I was clear enough. Maybe I will think of other things to add. The whole point is: schemas are the current Model layer of Kong. Validation should happen in the Model (so, schema), and that the actual schema definition API is not yet finished and can certainly be cleaned-up/improved.


return {basicauth_credentials = BasicAuthCredentials}
29 changes: 28 additions & 1 deletion spec/plugins/basic-auth/api_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,33 @@ describe("Basic Auth Credentials API", function()
assert.equal(consumer.id, credential.consumer_id)
end)

it("[SUCCESS] should create a basicauth credential with password omitted", function()
local response, status = http_client.post(BASE_URL, {username = "bob2"})
assert.equal(201, status)
credential = json.decode(response)
assert.equal(consumer.id, credential.consumer_id)
end)

it("[SUCCESS] should create a basicauth credential with random password", function()
credential = {}
local response, status = http_client.post(BASE_URL, {username = "bob3"})
assert.equal(201, status)
credential = json.decode(response)
local first_password = credential.password
-- delete credential created
local dao = spec_helper.get_env().dao_factory
local ok, err = dao.basicauth_credentials:delete(credential)
assert.True(ok)
assert.falsy(err)
-- recreate exact same
local response, status = http_client.post(BASE_URL, {username = "bob3"})
assert.equal(201, status)
credential = json.decode(response)
local second_password = credential.password
assert.not_equal(first_password, second_password)
end)


it("[SUCCESS] should encrypt a password", function()
local base_url = spec_helper.API_URL.."/consumers/alice/basic-auth/"
local response, status = http_client.post(base_url, {username = "alice", password = "1234"})
Expand Down Expand Up @@ -86,7 +113,7 @@ describe("Basic Auth Credentials API", function()
local response, status = http_client.get(BASE_URL)
assert.equal(200, status)
local body = json.decode(response)
assert.equal(2, #(body.data))
assert.equal(4, #(body.data))
end)

end)
Expand Down