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

[meta]: authority issues #4717

Closed
3 of 5 tasks
easwars opened this issue Aug 30, 2021 · 9 comments
Closed
3 of 5 tasks

[meta]: authority issues #4717

easwars opened this issue Aug 30, 2021 · 9 comments
Assignees

Comments

@easwars
Copy link
Contributor

easwars commented Aug 30, 2021

This is a meta-issue to track all the things that we need to fix (or add new functionality) with respect to how :authority is used in gRPC-Go.

  • Determining :authority from the target URI must be the responsibility of the name resolver. See: HTTP2 authority header is not set to resolver.Address.ServerName (?) #4516

    • Add an optional interface, to be implemented by name resolvers or their builders, that contains a single method to return the channel’s authority.
      • This method must not perform any I/O, be non-blocking and the return value must not change over time.
      • This value will be used to populate the :authority pseudo-header, and serverName for TLS handshake by default.
      • gRPC will provide an implementation which conforms to RFC 3986 and returns the path portion of the target, after stripping query parameters. This will be used if the scheme’s name resolver does not provide a custom one.
      • This parsed target will be passed to load balancers in their BuildOptions, with the correct authority set.
  • Add a call option to override the :authority header on a per-RPC basis. client: Add CallOption for setting authority; allow even without WithInsecure #3444

    • An optional interface will be added, to be implemented by AuthInfo implementations, to validate this override.
      • Implementations will need to perform a hostname validation check on the peer certificate, received during the handshake, to validate this override.
      • RPCs will fail if this call option is set, but:
        • AuthInfo for the subChannel does not implement this interface
        • validation by AuthInfo fails
      • Existing TLS based credentials implementations will be enhanced to support this.
      • Insecure credentials will allow for any authority value.
  • Continue to support the per-address serverName override in the addresses returned by the name resolver.

    • This field represents a security risk, since an attacker can specify both the addresses and the serverName to be used and can direct traffic to balancers/hosts controlled by them. This is addressed by the use of the WithAuthority dial option.
    • This override should affect both the :authority header sent in the HEADERS frame and the serverName used for the TLS handshake.
  • Make the WithAuthority dial option work for secure credentials as well.

    • Currently this is supported only for insecure credentials.
    • This needs to influence both, the :authority header and the serverName.
    • This override also disables any per-address overrides specified by the name resolver above.
  • Deprecate the OverrideServerName() method on credentials.TransportCredentials interface.

#RouteLookupService

@serathius
Copy link

To override authority in etcd client we used scheme://authority/endpoint format of target. Compared to WithAuthority this works also with secure credentials.
etcd-io/etcd#13192

@easwars
Copy link
Contributor Author

easwars commented Sep 29, 2021

To override authority in etcd client we used scheme://authority/endpoint format of target. Compared to WithAuthority this works also with secure credentials. etcd-io/etcd#13192

The authority in scheme://authority/endpoint is not supposed to be the ClientConn's authority. It is the authority as used by the name resolver. For example, in dns scheme, this authority represents the DNS server name. And other name resolvers can use this authority in other ways.

The ClientConn's authority is the value that gets sent out as the :authority pseudo-header during stream creation. It is also the value that is used as the ServerName during the authentication handshake.

@serathius
Copy link

If I understood correctly, authority in scheme://authority/endpoint is not supposed to be the ClientConn's, so this behavior is not officially supported and might be dropped in future. However, this is currently the only reliable way to override authority that works for all the schemes and TLS config.

For etcd v3.5 I plan to depend on this behavior to implement fix for authority header. Please let me know if there is better alternative.

@easwars
Copy link
Contributor Author

easwars commented Sep 29, 2021

As part of #4817, we are adding support for WithAuthority for all transport credentials.

The Authority field of resolver.Target passed to the name resolver will continue to contain authority from scheme://authority/endpoint. In a follow up PR, I will be changing the Authority field of resolver.Target to contain the ClientConn's authority when it is passed to balancer implementation as part of balancer.BuildOptions.

Where/how do you use this authority? Do you have a custom name resolver or a custom load balancer?

@serathius
Copy link

We use custom resolver https://github.com/etcd-io/etcd/blob/3df272774672366beb02c5447782805ab5fec957/client/v3/internal/resolver/resolver.go#L30

We use it to do a live update of endpoints used by client. I wasn't the one to implement it so I'm not sure if this is best way to implement it, though.

@easwars
Copy link
Contributor Author

easwars commented Sep 29, 2021

And how do you use the authority field here? It is not clear from the file you linked.

@serathius
Copy link

Authority is generated by the original target generated here https://github.com/etcd-io/etcd/blob/3df272774672366beb02c5447782805ab5fec957/client/v3/client.go#L302

In etcd v3.5.0 target uses pattern etcd-endpoints://{unique id}/#initially[{endpoints}], this resulted in grpc setting authority to #initially[{endpoints}], which is not even valid authority value as # is forbidden.

I have proposed a fix etcd-io/etcd@c929a91#diff-2bbaa3c83234888775aabf7697076e8035451045c479938e03ebb5c929132679L300-L310 that uses scheme://authority/endpoint as I didn't find any alternative reliable way to set identity when using our custom resolver.

@easwars
Copy link
Contributor Author

easwars commented Sep 30, 2021

Please take a look at #4817 if you are interested. The changes will hopefully affect you positively.

@easwars
Copy link
Contributor Author

easwars commented May 16, 2022

Filed #5360 and #5361 to track tasks which were not completed as part of this issue.

@easwars easwars closed this as completed May 16, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants