Prevent nil panic from derefeence nil context in authorization middleware #3829
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
troydai
requested review from
evan2645,
amartinezfayo,
azdagron,
MarcosDY and
rturner3
as code owners
troydai
force-pushed
the
fix-nil-context
branch
from
2c5a37b to
7b506fb
Compare
…leware Nil panics happens authorization middleware's Preprocess function when a nil context is passed down from a failed database operation while fetching entries in function WithCallerEntries. This change addresses this risk by dereference the context and fetch the logger earlier before the ctx variable is mutated. Extended explanation: WithCallerEntries returns error when a database operation failure happens. The error itself is not fatal, however, it returns a nil context. Returns zero value when errors is correct behavior. However, the reconcileResult function and opaAuth function bubble up the nil context as if it is populated. In the authorization middleware's Preprocess function, it attempts to retrieve a logger from the context thus cased a nil panic. Fix: The fix is simple: since the dereference happens for the purpose of fetching a logger. The logger is prefetched before the context is mutated. This is simplest fix of all the forms. Other alternative fix include introduce a separate variable to hold mutated context and I believe it overcomplicates the code. Other changes: This change also include a fix in the authorization_opa.go to explicitly returns nil context in when functions errored out. It doesn't change the logic because it is already returning nil. Signed-off-by: Troy Dai <[email protected]>
troydai
force-pushed
the
fix-nil-context
branch
from
7b506fb to
32678db
Compare
Signed-off-by: Troy Dai <[email protected]>
azdagron
reviewed
Feb 7, 2023
| @@ -55,12 +55,14 @@ func (m *authorizationMiddleware) Preprocess(ctx context.Context, methodName str | |||
| ctx = rpccontext.WithLogger(ctx, rpccontext.Logger(ctx).WithFields(fields)) | |||
| } | |||
|
|
|||
| logger := rpccontext.Logger(ctx) | |||
|
|
|||
| var deniedDetails *types.PermissionDeniedDetails | |||
| ctx, allow, err := m.opaAuth(ctx, req, methodName) | |||
There was a problem hiding this comment.
Can we also not alias the ctx variable? I think that would be a little more robust towards future refactors.
authedCtx, allow, err := m.opaAuth(ctx, req, methodName)
....
if allow {
return authedCtx, nil
}
There was a problem hiding this comment.
Done. With the new variable name, it is not necessary to fetch the logger early because the ctx is now safe to dereference. This reduce the changes in this PR.
1. Use a different variable to hold the return context to avoid mutate the ctx variable; 2. Revert the previous change of fetching logger earilier. With the new variable, it is now safe to fetch logger from the ctx. Signed-off-by: Troy Dai <[email protected]>
Signed-off-by: Troy Dai <[email protected]>
|
Thank you. Updated the PR description, will merge once all the checks pass. |
Only maintainers have permissions to merge. We'll take care of it :) |
|
Got it. There is a race condition test failed. Glance at it, it doesn't seem to be related to this change. Shall I dig into it or there will be a retry? |
|
It's a racy test. I'll retrigger it after the rest have finished. |
stevend-uber
pushed a commit
to stevend-uber/spire
that referenced
this pull request
Oct 16, 2023
…ware (spiffe#3829) * Prevent nil panic from derefeence a nil context in authorization middleware Nil panics happens authorization middleware's Preprocess function when a nil context is passed down from a failed database operation while fetching entries in function WithCallerEntries. This change addresses this risk by dereference the context and fetch the logger earlier before the ctx variable is mutated. Extended explanation: WithCallerEntries returns error when a database operation failure happens. The error itself is not fatal, however, it returns a nil context. Returns zero value when errors is correct behavior. However, the reconcileResult function and opaAuth function bubble up the nil context as if it is populated. In the authorization middleware's Preprocess function, it attempts to retrieve a logger from the context thus cased a nil panic. Fix: The fix is simple: since the dereference happens for the purpose of fetching a logger. The logger is prefetched before the context is mutated. This is simplest fix of all the forms. Other alternative fix include introduce a separate variable to hold mutated context and I believe it overcomplicates the code. Other changes: This change also include a fix in the authorization_opa.go to explicitly returns nil context in when functions errored out. It doesn't change the logic because it is already returning nil. Signed-off-by: Troy Dai <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request check list
Affected functionality
Prevent a nil panic when a database operation failure happens during retrieving entries.
Description of change
Nil panic happens in authorization middleware's Preprocess function when a nil context is passed down from a failed database operation while fetching entries in function WithCallerEntries. This change addresses this risk by introduce a new variable to hold the returned context so as to avoid dereferencing a potential nil ctx.
Extended explanation:
WithCallerEntries returns error when a database operation failure happens. The error itself is not fatal, however, it returns a nil context. Returns zero value when errors is correct behavior. However, the reconcileResult function and opaAuth function bubble up the nil context as if it is populated. In the authorization middleware's Preprocess function, it attempts to retrieve a logger from the context thus cased a nil panic.
Fix:
The fix is simple: Since the dereference happens for the purpose of fetching a logger, a new variable is introduced to store returned context and deferencing will happen on the original contexty. This is simplest fix of all the forms. Other alternative fix include introduce a separate variable to hold mutated context and I believe it overcomplicates the code.
Other changes:
This change also include a fix in the
authorization_opa.goto explicitly returns nil context in when functions errored out. It doesn't change the logic because it is already returning nil.Which issue this PR fixes