Separate incoming and outgoing metadata in context #1157
Conversation
|
I would appreciate some advice for how/where to test this change. |
There was a problem hiding this comment.
How about implementing a "proxy gRPC server" that forwards the requests and responses.
The proxy server will reuse the context to start the forwarding RPC, and the test can check that metadata is not carried over, but other things (like timeout) are carried over.
metadata/metadata.go
Outdated
|
|
||
| // NewContext creates a new context with md attached. | ||
| // NewContext creates a new context with outgoing md attached. |
There was a problem hiding this comment.
Should we also have a NewOutgoingContext?
So we can make NewContext call NewOutgoingContext,
and probably guide new users to use the new function instead?
This will prevent the incoming RPCs' metadata from appearing in outgoing RPCs
unless it is explicitly copied, e.g.:
incomingMD, ok := metadata.FromContext(ctx)
if ok {
ctx = metadata.NewContext(ctx, incomingMD)
}
Fixes grpc#1148
dfawley
force-pushed
the
metadata_dont_forward
branch
from
c7321b3
to
1e7cb38
Compare
|
PTAL, thanks! |
|
This change broke backward compatibility, I filed #1178 to track the issue. |
menghanl
added
the
Type: API Change
|
Wow, this changes broke backward compatibility, especially our trace system. Every request has a trace-id set to ctx. For example, service A call B, B call C and then B resp to A. We do not need handle ctx in v1.2.1. |
|
This is considered a fix for a fairly serious security bug. Automatically forwarding information between services was never intentional. Even so, I am sorry this change caused problems for you, and I apologize for not announcing it more broadly before it was merged. We are working to improve our communication, and we will use this as an example of things we can do better. |
|
I can confirm this is very breaking, I had to hunt down this PR using git blame unfortunately :( |
|
This change also broke my code. Although, I understand it was done because of security concerns. |
grpc/grpc-go#1157 changed the APIs for getting metadata from contexts and making contexts from metadata. This PR updates common to rely on the new library. Means that anyone who vendors common will need a recent grpc-go too.
grpc/grpc-go#1157 changed the APIs for getting metadata from contexts and making contexts from metadata. This PR updates common to rely on the new library. Means that anyone who vendors common will need a recent grpc-go too.
* Add API authentication mechanism * Decode HMAC string correctly * Make server decide when to auth * Fix call to removed grpc method (see grpc/grpc-go#1157) * Fix api_client references * Add login token to controller spec * Stop naming our error variables * Add comment about static auth token * Fix merge error
This will prevent the incoming RPCs' metadata from appearing in outgoing RPCs
unless it is explicitly copied, e.g.:
incomingMD, ok := metadata.FromContext(ctx)
if ok {
ctx = metadata.NewContext(ctx, incomingMD)
}
Fixes #1148