Skip to content

Make sure (*Struct).Fields map is not nil before assigment#52627

Closed
kirecek wants to merge 1 commit intogravitational:masterfrom
kirecek:ej/nil-map
Closed

Make sure (*Struct).Fields map is not nil before assigment#52627
kirecek wants to merge 1 commit intogravitational:masterfrom
kirecek:ej/nil-map

Conversation

@kirecek
Copy link
Copy Markdown
Contributor

@kirecek kirecek commented Feb 28, 2025

During the following operation:

				out.Fields[trimmedKey] = &types.Value{
					Kind: &types.Value_StringValue{
						StringValue: trimmedVal,
					},
				}

fields map is not initialized and panics, i.e:

panic: assignment to entry in nil map

goroutine 2747 [running]:
github.com/gravitational/teleport/api/types/events.(*Struct).trimToMaxSize(0xc00370f5c0, 0x2dc4)
    github.com/gravitational/teleport/api@v0.0.0/types/events/events.go:195 +0x5d0
github.com/gravitational/teleport/api/types/events.(*AppSessionDynamoDBRequest).TrimToMaxSize(0xc002b3c848, 0x10000)
    github.com/gravitational/teleport/api@v0.0.0/types/events/events.go:811 +0x4ee
github.com/gravitational/teleport/lib/events.(*ProtoStream).RecordEvent(0xc00307bd40, {0x55740285e470, 0xc0032bfc70}, {0x55740275f7c0?, 0xc003908970?})
    github.com/gravitational/teleport/lib/events/stream.go:395 +0x9e
github.com/gravitational/teleport/lib/events.(*SessionWriter).processEvents(0xc00307be60)
    github.com/gravitational/teleport/lib/events/session_writer.go:435 +0x336
github.com/gravitational/teleport/lib/events.NewSessionWriter.func1()
    github.com/gravitational/teleport/lib/events/session_writer.go:61 +0x1c
created by github.com/gravitational/teleport/lib/events.NewSessionWriter in goroutine 2744
    github.com/gravitational/teleport/lib/events/session_writer.go:60 +0x33b

This occurs for example in DynamoDB queries.

I added very simple test case that covers trimToMaxSize for given type.

changelog: fix panic when trimming audit log entries

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Feb 28, 2025

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link
Copy Markdown
Contributor


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

Copy link
Copy Markdown
Contributor Author

@kirecek kirecek left a comment

Choose a reason for hiding this comment

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

I have read the CLA Document and I hereby sign the CLA

@milos-teleport milos-teleport added the c-spb Internal Customer Reference label Mar 3, 2025
@capnspacehook
Copy link
Copy Markdown
Contributor

recheck

@capnspacehook
Copy link
Copy Markdown
Contributor

Thanks for the PR! I thought this was covered already in lib/events.TestTrimToMaxSize but I guess Struct objects were not properly populated so the test never trimmed them. Will add that in a followup PR.

@capnspacehook capnspacehook enabled auto-merge March 13, 2025 02:24
@kirecek
Copy link
Copy Markdown
Contributor Author

kirecek commented Mar 13, 2025

Yes, Struct mostly does not implement AuditEvent interface covered by the TestTrimToMaxSize, therefore I took easier/faster path and implemented test only for this particular struct 🙈

All checks in gh workflows failed tho, rebased to master again.

auto-merge was automatically disabled March 13, 2025 12:52

Head branch was pushed to by a user without write access

@capnspacehook
Copy link
Copy Markdown
Contributor

@kirecek it looks like some of our workflows won't run when the PR is created by an external contributer, so I created a PR here that has your changes: #53035. I made some small modifications to clean up the Struct trimming logic. Thanks for the PR!

@kirecek
Copy link
Copy Markdown
Contributor Author

kirecek commented Mar 13, 2025

cool, thank you! Closing this then :)

@kirecek kirecek closed this Mar 13, 2025
@kirecek kirecek deleted the ej/nil-map branch June 24, 2025 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c-spb Internal Customer Reference size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants