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

Setting just servername to :authority pseudo header in client when using TLS. #2518

Merged
merged 1 commit into from Dec 11, 2021

Conversation

everpeace
Copy link
Contributor

@everpeace everpeace commented Dec 10, 2021

fixes #2510

google.golang.org/grpc@v1.42.0 (updated recently in #2481) introduced better handling for :authority pseudo-header implemented in grpc/grpc-go#4817. This requires WithAuthority dial option value must match with the server name of its transport creds.

The current implementation always uses url.Host ("host:port") of --addr as an authority pseudo-header value because HTTP/2(RFC7540) defines so, i.e the authority portion of target URI without userinfo part. This causes grpc-go's authority validation to fail.

This PR fixes the issue so that it uses ServerName in the tls config as :authority header when tls specified, uses normal host:port part otherwise.

…ing tls.

HTTP/2(RFC7540) defines :authority pseudo header includes the authority portion
of target URI but it must not include userinfo part (i.e. url.Host).

However, when TLS certificate specified, grpc-go requires it must match
with its servername specified for certificate validation.

Signed-off-by: Shingo Omura <everpeace@gmail.com>
@everpeace everpeace marked this pull request as ready for review December 10, 2021 12:13
@adriankostrubiak-tomtom

If there's a publicly published docker image, I would be happy to validate this fix resolves the issue I was seeing in #2510. Please just let me know.

Regardless, thank you for looking into this.

@everpeace
Copy link
Contributor Author

everpeace commented Dec 10, 2021

Oh, thank you very much for the offer. I published docker images built on the branch. Please try it.

https://hub.docker.com/r/everpeace/buildkit/tags

  • everpeace/buildkit:fix-authority-header-ccbf7f33
  • everpeace/buildkit:rootless-fix-authority-header-ccbf7f33

docker build/push log:

~/src/github.com/moby/buildkit
❯ git rev-parse --short HEAD                                    
ccbf7f33

❯ make images
...

❯ docker tag moby/buildkit:local everpeace/buildkit:fix-authority-header-$(git rev-parse --short HEAD)
❯ docker tag moby/buildkit:local-rootless everpeace/buildkit:rootless-fix-authority-header-$(git rev-parse --short HEAD)

 ❯ docker push everpeace/buildkit:fix-authority-header-$(git rev-parse --short HEAD)                                      
...
fix-authority-header-ccbf7f33: digest: sha256:e91719f03df6f4d58282b7f3951d6ef99b2acb1d34d881daebb1d1faad66af3d size: 1158

❯ docker push everpeace/buildkit:rootless-fix-authority-header-$(git rev-parse --short HEAD)
...
rootless-fix-authority-header-ccbf7f33: digest: sha256:6d71fcfd8f13decea418882337d37151b7cfeb107c42694433015e13e1e309d9 size: 1996

@adriankostrubiak-tomtom

No, thank you for helping get this fixed!

I can confirm that running the build as described in #2510 with image everpeace/buildkit:fix-authority-header-ccbf7f33 results in success as expected. This is consistent with image moby/buildkit:v0.9.3, and the regression seen in the current moby/buildkit:master is resolved AFAIK.

Thank you!

@tonistiigi
Copy link
Member

tonistiigi commented Dec 10, 2021

Is this fix backward compatible for old(v0.9.3) client to the new daemon and new client to old daemon configurations. If not, then can these cases be covered?

@adriankostrubiak-tomtom
Copy link

adriankostrubiak-tomtom commented Dec 10, 2021

My manual, by hand testing shows:

Client Daemon Success
moby/buildkit:v0.9.3 moby/buildkit:v0.9.3 yes
moby/buildkit:master moby/buildkit:v0.9.3 no
everpeace/buildkit:fix-authority-header-ccbf7f33 moby/buildkit:v0.9.3 yes
moby/buildkit:v0.9.3 everpeace/buildkit:fix-authority-header-ccbf7f33 yes
moby/buildkit:master everpeace/buildkit:fix-authority-header-ccbf7f33 no
everpeace/buildkit:fix-authority-header-ccbf7f33 everpeace/buildkit:fix-authority-header-ccbf7f33 yes

(again, based on running a build as described in #2510 )

@tonistiigi
Copy link
Member

@adriankostrubiak-tomtom Thanks.

@everpeace
Copy link
Contributor Author

@adriankostrubiak-tomtom Thanks for your compatibility check!

@AkihiroSuda AkihiroSuda merged commit 0dfc2aa into moby:master Dec 11, 2021
@AkihiroSuda
Copy link
Member

Thank you!

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.

Possible regression in client credential validation/handling?
4 participants