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

Implement an API for exporting session events #7360

Merged
merged 5 commits into from
Jul 13, 2021
Merged

Conversation

xacrimon
Copy link
Contributor

@xacrimon xacrimon commented Jun 18, 2021

This PR implements a gRPC API available via IAuditLog to stream audit events from a session recording.
Enterprise buddy: https://github.com/gravitational/teleport.e/pull/278

This resolves issue #7305.

There's a couple places in this PR where errors are not relayed correctly due to them being based on cancel contexts. What would be the ideal way to propagate an error upwards when cancelling a context?

@xacrimon xacrimon self-assigned this Jun 18, 2021
@xacrimon xacrimon force-pushed the joel/session-export-api branch 2 times, most recently from 1e0c7db to 59661d8 Compare June 22, 2021 00:13
@russjones
Copy link
Contributor

russjones commented Jun 22, 2021

@xacrimon Regarding streaming the events over, how would we handle print events? For example, an external plugin would get all the print events, but how would the user piece them together (with timing information) to play the session back?

Also, most of the events here, like session.start will already have been exported from the main audit log, so won't that lead to duplicates?

@alex-kovoy @klizhentas Can you review the changes being made here as well? How do we plan to use the session exporter to actually work and how will users use it?

@klizhentas
Copy link
Contributor

@russjones I think the solution that @xacrimon proposed here is good - it is independent on serialization format. I am not concerned about event duplication. The deduplication should happen on the receiving backend. The fluentd-forwarder should be able to reassemble the session to required format or forward parts of the session to the backend.

@xacrimon xacrimon requested a review from klizhentas June 23, 2021 09:54
@xacrimon xacrimon added this to the 6.2.x - "Papercuts" milestone Jun 23, 2021
@xacrimon xacrimon marked this pull request as ready for review June 24, 2021 08:27
Copy link
Contributor

@nklaassen nklaassen left a comment

Choose a reason for hiding this comment

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

I'm not sure about this pattern of returning a context which will be cancelled with an error. Idk if this is better but did you consider returning a chan error instead?

api/utils/errcontext.go Outdated Show resolved Hide resolved
lib/auth/grpcserver.go Outdated Show resolved Hide resolved
lib/auth/grpcserver.go Outdated Show resolved Hide resolved
@xacrimon xacrimon requested a review from nklaassen June 25, 2021 11:50
lib/events/multilog.go Outdated Show resolved Hide resolved
lib/events/discard.go Outdated Show resolved Hide resolved
lib/events/auditlog.go Outdated Show resolved Hide resolved
lib/auth/grpcserver.go Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
@xacrimon xacrimon requested a review from nklaassen June 25, 2021 16:51
lib/events/auditlog.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
integration/integration_test.go Outdated Show resolved Hide resolved
integration/integration_test.go Outdated Show resolved Hide resolved
@xacrimon xacrimon requested a review from nklaassen June 25, 2021 21:59
Copy link
Contributor

@nklaassen nklaassen left a comment

Choose a reason for hiding this comment

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

Okay I found a few places that still have the issue with the buffered error channel but lgtm after those are fixed

lib/events/auditlog.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
lib/auth/auth_with_roles.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
@xacrimon
Copy link
Contributor Author

Okay, there is some strange bug with it not picking up all events sometimes. I'll investigate, in the meantime, no need to review.

@xacrimon
Copy link
Contributor Author

xacrimon commented Jun 30, 2021

Fixed the error, integration tests now pass. It doesn't support playing back the old-style session recordings however. Is that a requirement or are they not in use anymore? If we do have to support them, what's the best way to read events from them.

api/client/client.go Outdated Show resolved Hide resolved
@xacrimon xacrimon requested a review from nklaassen June 30, 2021 19:48
Copy link
Contributor

@nklaassen nklaassen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing all of my comments

build.assets/Makefile Outdated Show resolved Hide resolved
lib/events/api.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

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

Bot.

@xacrimon xacrimon merged commit 98d51c5 into master Jul 13, 2021
@xacrimon xacrimon deleted the joel/session-export-api branch July 13, 2021 23:29
xacrimon added a commit that referenced this pull request Jul 21, 2021
xacrimon added a commit that referenced this pull request Jul 21, 2021
reedloden pushed a commit that referenced this pull request Aug 10, 2022
…5348)

The code attempts to wait on an existing download if one is already
in progress rather than starting a concurrent download of the same
session. If this code path runs, we incorrectly defer a call to a
nil function, triggering a panic.

This bug was introduced in #7360.

In some cases, an auth server can see a response from the uploader
with an empty session ID. If this is the case, skip emitting the
session end and upload events, as there's no action to take.

This scenario can happen when multiple auth servers are running
upload completers and working on the same uploads simultaneously.
fheinecke pushed a commit that referenced this pull request Aug 13, 2022
…on (#15360)

The code attempts to wait on an existing download if one is already
in progress rather than starting a concurrent download of the same
session. If this code path runs, we incorrectly defer a call to a
nil function, triggering a panic.

This bug was introduced in #7360.
reedloden pushed a commit that referenced this pull request Aug 15, 2022
#15376)

The code attempts to wait on an existing download if one is already
in progress rather than starting a concurrent download of the same
session. If this code path runs, we incorrectly defer a call to a
nil function, triggering a panic.

This bug was introduced in #7360.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants