Add username in accesslog

[bastiaanb]

Description

This PR fixes issue #2060: logging of authenticated username in accesslog. Works for both Basic and Digest authentication.

[bastiaanb]

@ldez could we still get this into 1.4? We need this feature to ditch nginx as http router.....

[ldez]

@bastiaanb we are ok to put this in 1.4, but you need to add tests (https://github.com/containous/traefik/blob/master/integration/access_log_test.go)

[bastiaanb]

@ldez I have adjusted the access_log_test to correctly reflect how and when the req.URL.User gets set (by the authenticator code).
Is that sufficient? My guess is that the scenario where the URL.User is already set before invocation of the logger does not happen at all.

[ldez]

@bastiaanb could you remove the merge of the master branch, and change the base PR branch to 1.4 (and rebase). Ask me if you need help.

We need to support the 2 cases:

• user from Traefik auth
• user from external

Consequently we need 2 tests.

Could you revert your changes in the test and add a new test?

[m3co-code]

If possible would it be maybe better to not modify the request object itself, but to extend the access log code to understand both sources? @bastiaanb WDYT?

IMHO it's not a good practice to modify the request object in such a way as it creates an implicit dependency between our auth and logging components. Think about what happens when we would replace the auth middleware with a 3rd party one that does it for us? Then the logging would again be broken.

[bastiaanb]

@ldez oh, exeternal auth is actually used? Is a usage scenario available somewhere? I presume 'external auth' typically would be TLS client certificates?
I am a go-lang novice, so it will take a bit of time to write the proper test code....

[bastiaanb]

@marco-jantke what would be the proper way to relay the username from the auth component to the access logger?

[m3co-code]

I am not an expert with the HTTP authentication methods, but it seems in both cases (Digest + Basic Auth) the username is transferred in plain text and we can simply read it from the request. For the basic auth this was working anyway already before, for Digest Auth we would need some parsing in order to get the username. See https://en.wikipedia.org/wiki/Digest_access_authentication#Example_with_explanation for the digest authentication process. Can you try if this is a feasible approach?

I would also like to hear the opinion of other Maintainers and whether others see the same benefits in making the logging middleware smarter to keep coupling of the components low. @containous/traefik

[bastiaanb]

Hi Marco, By duplicating the code that identifies (not authenticates) the user in the access log code, we would violate separation of concerns a lot more than by setting the username in the req.URL.User. It would need to be aware of all authentication methods that may be supported in the future (e.g. token, cookie, oauth2, JWT). Also the identity should show up in the access log only in case of successfull authentication. So it seems to me that the proper way would be to define what data authenticators should add to the request context, e.g. user id, othe user attributes, authentication method, identity provider, etc. That way downstream http.HandlerFuncs can add these to the backend HTTP request headers for example (instead of having the auth code do this) or implement more sophisticated authorization than just auth-required. But I don't really know yet how the accesslog code would be able to access this. Maybe via the same pattern as the SaveBackend handler?

…

________________________________ Van: Marco Jantke <[email protected]> Verzonden: dinsdag 19 september 2017 13:25 Aan: containous/traefik CC: Bastiaan Bakker; Mention Onderwerp: Re: [containous/traefik] Add username in accesslog (#2111) I am not an expert with the HTTP authentication methods, but it seems in both cases (Digest + Basic Auth) the username is transferred in plain text and we can simply read it from the request. For the basic auth this was working anyway already before, for Digest Auth we would need some parsing in order to get the username. See https://en.wikipedia.org/wiki/Digest_access_authentication#Example_with_explanation for the digest authentication process. Can you try if this is a feasible approach? Digest access authentication - Wikipedia<https://en.wikipedia.org/wiki/Digest_access_authentication#Example_with_explanation> en.wikipedia.org Digest access authentication is one of the agreed-upon methods a web server can use to negotiate credentials, such as username or password, with a user's web browser. I would also like to hear the opinion of other Maintainers and whether others see the same benefits in making the logging middleware smarter to keep coupling of the components low. @containous/traefik<https://github.com/orgs/containous/teams/traefik> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#2111 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AD5zLBAoIPhNTmo8AHKsjzSuk1KDndcwks5sj6SpgaJpZM4PUoC->.

[m3co-code]

Thank you for the reasoning @bastiaanb!

Also the identity should show up in the access log only in case of successful authentication.

This is a very good argument to go with your approach. About duplicating the code to extract the username, I slightly disagree, because you could, of course, extract the code and reuse it at both places. Anyway I think I am convinced to go with your approach. Going into code review now :)

[m3co-code]

Can you add a simple unit test to authenticator_test.go that proofs that r.URL.User is written for both, basic and digest auth? It should not be too complicated, given we have some tests there already.

Additionally, can you add a comment here that the handler is responsible to set the request.URL.User in case of a successful authentication, in order to add the username to the access logs? I don't know where to document it better.

[ldez]

@bastiaanb any news?

[bastiaanb]

@ldez sorry, I haven't been able to work on this issue in the last weeks at all. I hope to pick this up in two weeks after a (laptopless) holiday.

[ldez]

@bastiaanb any news?

[juliens]

LGTM

[m3co-code]

Only a suggestion to also cover digest auth in the test case left. Thanks for bringing this forwards @mmatur 👍

[m3co-code]

Can we do another test request that uses digest authentication to have both cases covered? Would be fine for me to just do it in this test case.

[mmatur]

Done @marco-jantke

[m3co-code]

LGTM, thanks 👍

[ldez]

LGTM