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

Fix incomplete access logs #1273

Merged
merged 7 commits into from
Apr 8, 2019
Merged

Fix incomplete access logs #1273

merged 7 commits into from
Apr 8, 2019

Conversation

scottinet
Copy link
Contributor

@scottinet scottinet commented Mar 26, 2019

Description

Context: access logs are invoked every time an API route is invoked, to keep a trace about Kuzzle's activity.

There are two problems with how access logs are currently handled:

  1. If a connection drops before the end of an API request, then the warning No connection retrieved for connection id: <connection id> is logged. This is a problem, because, even if the connection had since dropped, the query was executed. This means that our access logs are incomplete
  2. Whenever we log messages such as the warning previously described, or the ones printed when an authentication token cannot be decoded, this invokes pluginsManager.trigger which creates a promise in the event loop. With enough activity, this can quickly clogs up the event loop and endanger Kuzzle's stability

This PR solves the 1st problem by simply using the provided Request object, which holds basic connection and token informations, when the raw network connection object is not available (meaning that the connection no longer exists).
The second problem is solved by simply removing the usages of the log:xxx events, as those didn't provide valuable information at all.

⚠️ access logging is a sensitive feature of kuzzle, please be extra careful when reviewing this PR

Other changes

When printing an access log, we now print (anonymous) instead of -1 when a query is executed by an unauthenticated user.

How to test

  1. Activate access logs by setting server.logs.transports.silent = false in the kuzzlerc configuration file
  2. Run a script that stresses Kuzzle to the point that it starts buffering requests to prevent overload
  3. Shut down the script suddenly

Before this PR: many No connection retrieved for connection id... warnings are printed on the console. With this PR: access logs are properly printed even on dead connections.

Moreover, Kuzzle takes far less time to recover with this PR than without, because less promises are put on the event loop.

Script that you can use to test this PR:

const WebSocket = require('uws');

const
  nbjobs = 10000,
  ws = new WebSocket('ws://172.18.0.5:7512', {perMessageDeflate: false}),
  payload = JSON.stringify({controller:'server', action:'info'});

ws.on('open', () => {
  let done = 0;
  const start = Date.now();

  ws.on('message', data => {
    done++;

    if (done === nbjobs) {
      clearInterval(interval);
      const elapsed = (Date.now() - start) / 1000;
      console.log(`Elapsed: ${elapsed}s, Throughput: ${Math.round(nbjobs/elapsed)} requests/s`);
      ws.close();
    }
  });

  for (let i = 0; i < nbjobs; i++) {
    ws.send(payload);
  }
});

@@ -104,7 +104,7 @@ describe('lib/core/api/core/entrypoints/embedded/index', () => {

entrypoint = new EntryPoint(kuzzle);

Object.defineProperty(entrypoint, 'log', {
Object.defineProperty(entrypoint, 'logger', {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unit tests were broken because they checked that the logger was never called... but sinon's spies were put on the wrong entrypoint property name, so the checks always passed 😑

@codecov-io
Copy link

codecov-io commented Mar 26, 2019

Codecov Report

Merging #1273 into 1-dev will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##            1-dev    #1273      +/-   ##
==========================================
+ Coverage   93.75%   93.77%   +0.02%     
==========================================
  Files          98       98              
  Lines        6793     6800       +7     
==========================================
+ Hits         6369     6377       +8     
+ Misses        424      423       -1
Impacted Files Coverage Δ
lib/api/core/models/repositories/userRepository.js 95% <100%> (ø) ⬆️
lib/api/core/entrypoints/embedded/index.js 97.79% <100%> (+0.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5135ad...a7943d4. Read the comment docs.

user = '-';
}

this.logger.info(ip +
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you use string literals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because that would be impractical and hard to read in this very specific case, given the amount of variables used to create an access log message.

try {
const
b64Payload = request.input.jwt.split('.')[1],
payload = Buffer.from(b64Payload, 'base64').toString('utf8');
user = JSON.parse(payload)._id;
}
catch (err) {
this.kuzzle.pluginsManager.trigger('log:warn', `Unable to extract user from jwt token: ${request.input.jwt}`);
// do nothing: we don't know anything about the token, it may be
// invalid on-purpose for all we know
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this comment : how and in wich case the token would be invalid on-purpose ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgery.

We're dealing with data coming from the outside, so nothing can be trusted.

verb = extra.method;
url = extra.url;

// try to get plain user name, 1st form jwt token if present, then from basic auth
if (connection.headers.authorization) {
// try to get plain user name, 1st form jwt token if present, then from

Choose a reason for hiding this comment

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

very-nitpicking: s/1st form jwt token/1st from jwt token/ ;)

@scottinet scottinet merged commit 3cf23ce into 1-dev Apr 8, 2019
@scottinet scottinet deleted the fix-incomplete-access-logs branch April 8, 2019 15:28
@kuzzle kuzzle mentioned this pull request Apr 29, 2019
Aschen pushed a commit that referenced this pull request Apr 29, 2019
# [1.7.3](https://github.com/kuzzleio/kuzzle/releases/tag/1.7.3) (2019-04-29)


#### Bug fixes

- [ [#1288](#1288) ] [bulk] fix an error when trying a non-partial bulk update   ([scottinet](https://github.com/scottinet))
- [ [#1286](#1286) ] [bugfix] allows bulk inserts on aliases   ([benoitvidis](https://github.com/benoitvidis))
- [ [#1282](#1282) ] [bugfix] scan keys on redis cluster   ([benoitvidis](https://github.com/benoitvidis))
- [ [#1279](#1279) ] Users must be authenticated to use auth:logout   ([scottinet](https://github.com/scottinet))

#### Enhancements

- [ [#1292](#1292) ] KZL 1032 - Throw an error when the realtime controller is invoked by plugin developers   ([benoitvidis](https://github.com/benoitvidis))
- [ [#1257](#1257) ] Add ability to define mapping policy for new fields   ([Aschen](https://github.com/Aschen))
- [ [#1291](#1291) ] [Kuzzle CLI] Fix --help on subcommands   ([Yoann-Abbes](https://github.com/Yoann-Abbes))
- [ [#1289](#1289) ] [WebSocket] Handle ping/pong packets   ([scottinet](https://github.com/scottinet))
- [ [#1273](#1273) ] Fix incomplete access logs   ([scottinet](https://github.com/scottinet))
---
@Aschen Aschen mentioned this pull request Jun 14, 2019
Aschen added a commit that referenced this pull request Jun 14, 2019
Release 1.8.0

Bug fixes

    [ #1311 ] Fix promise leaks (scottinet)
    [ #1298 ] Fix disabled protocol initialization (Aschen)
    [ #1297 ] Fix timeouts on plugin action returing the request (benoitvidis)
    [ #1288 ] Fix an error when trying a non-partial bulk update (scottinet)
    [ #1286 ] Allows bulk inserts on aliases (benoitvidis)
    [ #1282 ] Scan keys on redis cluster (benoitvidis)
    [ #1279 ] Users must be authenticated to use auth:logout (scottinet)

New features

    [ #1315 ] Add the new Vault module to handle encrypted application secrets (Aschen)
    [ #1302 ] Add write and mWrite (Aschen)
    [ #1305 ] Add pipes & hooks wildcard event (thomasarbona)

Enhancements

    [ #1318 ] Add a maximum ttl to auth:login (benoitvidis)
    [ #1301 ] Upgrade the WebSocket libraries (scottinet)
    [ #1308 ] Events triggering refactor (scottinet)
    [ #1300 ] Collection specifications methods cloisoned to a collection (thomasarbona)
    [ #1295 ] Improve validation error messages (benoitvidis)
    [ #1292 ] Throw an error when the realtime controller is invoked by plugin developers (benoitvidis)
    [ #1257 ] Add ability to define mapping policy for new fields (Aschen)
    [ #1291 ] Fix --help on subcommands (Yoann-Abbes)
    [ #1289 ] Handle ping/pong packets (scottinet)
    [ #1273 ] Fix incomplete access logs (scottinet)

Others

    [ #1317 ] Add ps dependency to plugin-dev Docker image for pm2 (benoitvidis)
    [ #1312 ] Check that .kuzzlerc.sample is well-formed (scottinet)
    [ #1299 ] Add Kuzzle Nightly & Redis 3 and 4 test (alexandrebouthinon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants