Audit Logging

[CathalOConnorRH]

Add audit logging to grpc requests using grpc-middleware and zap logger. Addresses #246

Some example request logs are below :

grpc_cli ls
{"grpc.start_time":"2022-12-06T16:15:45Z","system":"grpc","span.kind":"server","grpc.service":"grpc.reflection.v1alpha.ServerReflection","grpc.method":"ServerReflectionInfo","peer.address":"127.0.0.1:40388","grpc.user":"unauthenticated_request","grpc.code":"OK","grpc.time_duration_in_ns":520866}

grpc_cli call
{"grpc.start_time":"2022-12-06T16:16:28Z","system":"grpc","span.kind":"server","grpc.service":"tekton.results.v1alpha2.Results","grpc.method":"ListResults","peer.address":"127.0.0.1:33534","grpc.user":"system:serviceaccount:tekton-pipelines:tekton-results-debug","grpc.issuer":"kubernetes/serviceaccount","grpc.code":"OK","grpc.time_duration_in_ns":6033652} {"grpc.start_time":"2022-12-06T16:16:28Z","system":"grpc","span.kind":"server","grpc.service":"grpc.reflection.v1alpha.ServerReflection","grpc.method":"ServerReflectionInfo","peer.address":"127.0.0.1:33534","grpc.user":"system:serviceaccount:tekton-pipelines:tekton-results-debug","grpc.issuer":"kubernetes/serviceaccount","grpc.code":"OK","grpc.time_duration_in_ns":9241731}

[tekton-robot]

Hi @CathalOConnorRH. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[khrm]

/ok-to-test

[adambkaplan]

/hold

#268 should merge first (both configure zap, thus are in conflict).

[adambkaplan]

Two items:

• Error is a potential return here. Should we raise an error, treat it as an unauthenticated request, or something else?
• Please comment why we are using ParseUnverified. The godoc has an explict warning ("do not use unless you know what you are doing")

[CathalOConnorRH]

1. I will update the code to handle the error returned and log it but I think we should return the error to the calling method as it should stop the rest of the code from processing, WDYT ?
3. We are using ParseUnverifed to just take the name of the sub from the token. we don't verify it. that's still left up to the existing code to verify. Do you think we should verify it here also ?

[adambkaplan]

I don't think we need to verify it - we indirectly verify through the TokenAccessReview. I think we just need to comment that explains "this code is just extracting values, it is not doing any form of verification".

[adambkaplan]

I will update the code to handle the error returned and log it but I think we should return the error to the calling method as it should stop the rest of the code from processing, WDYT ?

I do have a concern that since this is an interceptor, an error causes the request to fail. Perhaps we set grpc.user to unknown in the context?

[CathalOConnorRH]

Updated to include those points.

[adambkaplan]

/approve

Looks good to me. If there is an additional +1 or lgtm, I will lift the hold.

[alan-ghelardi]

Eventually it'd be interesting to customize Zap settings through the config-logging ConfigMap. This would make the log output as well as the process of tweaking it homogeneous with other Tekton components, including the Watcher.

[tekton-robot]

@alan-ghelardi: changing LGTM is restricted to collaborators

In response to this:

Eventually it'd be interesting to customize Zap settings through the config-logging ConfigMap. This would make the log output as well as the process of tweaking it homogeneous with other Tekton components, including the Watcher.
/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dibyom]

/lgtm

[adambkaplan]

/hold cancel

Once this PR is rebased, I can merge.

[adambkaplan]

/lgtm

Re-adding LGTM due to rebase.

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [adambkaplan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[khrm]

/kind feature
We need this label for release as discussed in the last WG call.