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
Comments
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.
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? |
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): As for That said, while the RFC specifically mentions deprecating Any non-browser-focused http library worth its salt should parse the Is it possible for Lodestar to parse the URI and populate the 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) |
(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...) |
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.
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.
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. |
Ah, their logic is more sound than I gave them credit for. Still:
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. |
There seems to be a option 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
Will take a look at this tomorrow, should be quite easy to add. |
We dug into our stack a bit and discovered that converting raw I tested by grabbing node-fetch@v2 and sending off some requests to one of our haproxy instances and observing the result in Thanks again |
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: 'user:password'
// ...
} node-fetch@v3 and native fetch (undici) do not work that way. |
@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:
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 |
Thanks for the detailed info @jshufro. I think we don't have to support it in our
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.
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). |
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 versionsSteps to reproduce
No response
Additional context
No response
Operating system
Linux
Lodestar version or commit hash
v1.10.0
The text was updated successfully, but these errors were encountered: