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

Basic Auth userdata no longer working in validator client as of v1.10.0 #5881

Closed
jshufro opened this issue Aug 13, 2023 · 11 comments · Fixed by #5884
Closed

Basic Auth userdata no longer working in validator client as of v1.10.0 #5881

jshufro opened this issue Aug 13, 2023 · 11 comments · Fixed by #5884
Labels
meta-bug Issues that identify a bug and require a fix.

Comments

@jshufro
Copy link
Contributor

jshufro commented Aug 13, 2023

Describe the bug

Using a url of the form https://user:password@bn.com no longer works with lodestar, presenting this error:

info: Waiting for genesis message=Request cannot be constructed from a URL that includes credentials: https://user:password@bn.com

Expected behavior

Lodestar should parse the URI per spec and set the Authorization header as it did in previous versions

Steps to reproduce

No response

Additional context

No response

Operating system

Linux

Lodestar version or commit hash

v1.10.0

@jshufro jshufro added the meta-bug Issues that identify a bug and require a fix. label Aug 13, 2023
@nflaig
Copy link
Member

nflaig commented Aug 13, 2023

Looks like this just worked before because the version of the library we used was not compliant to fetch spec, as per spec this is expected behavior.

Lodestar should parse the URI per spec and set the Authorization header as it did in previous versions

Are you sure this was the previous behavior, I checked the code of the library we were using and it looks like it just passed the URL as is which is pretty bad in terms of security.

I think the easiest solution would be to just parse the URL, remove the basic auth credentials and add a Authorization header as suggested here node-fetch/node-fetch#1304 (comment).

Does that work for your use case?

@jshufro
Copy link
Contributor Author

jshufro commented Aug 13, 2023

Thanks for the quick reply.

I'm certain previous versions worked- I run Rocket Rescue Node for the Rocket Pool folks and we've been using basic auth with an hmac credential for a while now. I can see connections from lodestar clients still (presumably they just haven't updated yet):

image

As for fetch, I think their assumption is that they're running in a browser, and browsers have started disallowing username:password creds in the URI... which is especially frustrating given that fetch comes standard in Node 18.

That said, while the RFC specifically mentions deprecating username:password, userinfo is still a valid part of the authority section of the URL.

Any non-browser-focused http library worth its salt should parse the userinfo element of the URI and encode it in the Authorization header before any http(s) requests are made- golang's standard http client does this automatically, for instance.

Is it possible for Lodestar to parse the URI and populate the Authorization header when calling fetch? That would certainly solve our issue. If not, we can work with a cli flag that adds Authorization to requests, though I think this is more operational state to maintain- prysm has such a cli flag for grpc headers, and they occasionally forget to include them on requests.

I can confirm that a small wrapper around fetch succeeds with the behavior correct:

import fetch from 'node-fetch';

let u = new URL("https://foo:bar@fbi.gov")

function fetch_internal(url) {
  let stripped = new URL(url)
  stripped.username=''
  stripped.password=''
  
  let opts = {}
  if (url.username) {
    opts.headers = { 
      Authorization: `Basic ${Buffer.from(`${url.username}:${url.password}`).toString('base64')}`
    }
  }
  
  fetch(stripped, opts)
} 

fetch_internal(u)

@jshufro
Copy link
Contributor Author

jshufro commented Aug 13, 2023

(As an after thought, the fact that the native URL type in nodejs doesn't have a toString() variant that strips the userinfo is somewhat criminal...)

@nflaig
Copy link
Member

nflaig commented Aug 13, 2023

I'm certain previous versions worked

Yes it worked, it just kept the URL as is, it did not add it as Authorization header, most HTTP servers allow to use either approach.

As for fetch, I think their assumption is that they're running in a browser

The Lodestar HTTP client has to work in both browser and node, we used cross-fetch for a while which used polyfill implementations and the server-side was not spec compliant in many other aspects so we had different behavior depending on the environment.

The main reason we switched to native fetch was due to regressions in the polyfill implementation when upgrading to node 20.

I also think whether or not the request is done from a browser or server doesn't really matter, credentials should ideally not be part of the URL. The previous library allowing this and not populating the header was rather a bug and has been addressed by node-fetch/node-fetch#1268 but they also just throw an error now as per spec.

Any non-browser-focused http library worth its salt should parse the userinfo element of the URI and encode it in the Authorization header before any http(s) requests are made

Is it possible for Lodestar to parse the URI and populate the Authorization header when calling fetch?

That makes sense to me and we can add that to our HTTP client. Populating the header form the URL credentials would also restore previous behavior.

@jshufro
Copy link
Contributor Author

jshufro commented Aug 13, 2023

Ah, their logic is more sound than I gave them credit for.

Still:

  1. ...also throw an error when constructing the Request with a url that includes credentials (think this is for the best)
  2. or if we should auto "converted to an Authorization value" as mention in the spec also and strip away the usr/psw from the url
  3. or just simply allow it

Obviously I'm biased, but I would have gone with option 2, and wouldn't have merged a PR deprecating a valid use-case until the new behavior is in place...

Anyway, thanks for everything.

@nflaig
Copy link
Member

nflaig commented Aug 13, 2023

There seems to be a option isAuthenticationFetch which would built the Authorization header but this is disabled in all major browsers and Node fetch has not yet implemented it.

See nodejs/undici/lib/fetch/index.js#L1558-L1560

  // 17. If isAuthenticationFetch is true, then create an authentication entry
  if (isAuthenticationFetch) {
    // TODO
  }

We already have our own http client implementation which uses fetch, best to implement it there

const res = await this.fetch(url, {

Will take a look at this tomorrow, should be quite easy to add.

@jshufro
Copy link
Contributor Author

jshufro commented Aug 13, 2023

We dug into our stack a bit and discovered that converting raw https://(userinfo)@foo.org URIs appears to be an undocumented feature of haproxy.

I tested by grabbing node-fetch@v2 and sending off some requests to one of our haproxy instances and observing the result in capture request header authorization logs.

Thanks again

@nflaig
Copy link
Member

nflaig commented Aug 14, 2023

Thanks for digging into this.

I found where the magic happens nodejs/node/lib/_http_client.js#L309-L312

    if (options.auth && !this.getHeader('Authorization')) {
      this.setHeader('Authorization', 'Basic ' +
                     Buffer.from(options.auth).toString('base64'));
    }

node-fetch@v2 just parses the URL and add auth to options passed to http client

{
// ...
auth: 'user:password'
// ...
}

node-fetch@v3 and native fetch (undici) do not work that way.

@nflaig
Copy link
Member

nflaig commented Aug 14, 2023

@jshufro I'm curious, do you only use basic authentication for beacon nodes or also execution clients? Since ELs have their own auth mechanism using jwt tokens it might not be required there.

@jshufro
Copy link
Contributor Author

jshufro commented Aug 14, 2023

@jshufro I'm curious, do you only use basic authentication for beacon nodes or also execution clients? Since ELs have their own auth mechanism using jwt tokens it might not be required there.

Our architecture only exposes the BNs externally.

Basically, working inwards from edge, we have:

  1. cloudflare "orange cloud" proxy
  2. haproxy
  3. a custom reverse proxy based on this library to protect our users from mev theft: rescue-proxy. Our HMAC credentials are validated at this layer.
  4. The BN
  5. Finally, the EC

Given that validator clients perform all their duties against the beacon node, we didn't see a need to expose our ECs, but we locked down the BNs thoroughly. It's a lot of hops but we've found that performance is quite good anyway

@nflaig
Copy link
Member

nflaig commented Aug 14, 2023

Thanks for the detailed info @jshufro.

I think we don't have to support it in our JsonRpcHttpClient used to communicate with execution client as I doubt anybody is using it there. Generally, execution clients are either not protected (public RPC endpoints) or expose the execution engine which has its own authentication with JWTs, we should not advocate for using basic authentication there.

Given that validator clients perform all their duties against the beacon node, we didn't see a need to expose our ECs, but we locked down the BNs thoroughly.

Your use case makes sense, using basic authentication should work with all clients as it can be specified in the URL and does not require other options like extra headers. Using an API key as query parameter as beaconcha.in does for their monitoring is strictly worse as it will always be part of the URL and more prone to leak credentials.

It would be good if there was a more standardized approach to protecting beacon node APIs, similar to execution clients and the keymanager API.

It's a lot of hops but we've found that performance is quite good anyway

Yeah it really depends on where those proxies are deployed and what there are doing. I had previously worked a lot with kubernetes and service meshs, and some requests there had 10+ hops, the overhead was minimal even though the traffic was encrypted/decrypted on every hop (I guess envoy is just really performant).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta-bug Issues that identify a bug and require a fix.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants