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

feat(iroh-relay): allow to authenticate nodes via a HTTP POST request #3246

Merged
merged 7 commits into from
Apr 7, 2025

Conversation

Frando
Copy link
Member

@Frando Frando commented Mar 26, 2025

Description

This adds a new access config option to the iroh-relay binary: If set to http, a POST request will be performed for each node that attempts to connect to the relay. The URL to request is set in the config, and a X-Iroh-NodeId header will be set to the hex-encoded node id to check access for. If and only if the response has a 200 status code, and a response text of true, the connecting node will be granted access. In all other cases, the node will be denied access.

Config example to use this:

access.http.url = "http://localhost:8000/api/relays/check-auth/frando"

Breaking Changes

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.

@Frando Frando force-pushed the feat/relay-http-auth branch from 3ebafa0 to 6a43546 Compare March 26, 2025 10:53
Copy link

github-actions bot commented Mar 26, 2025

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3246/docs/iroh/

Last updated: 2025-04-07T10:44:57Z

node_id: NodeId,
) -> iroh_relay::server::Access {
use iroh_relay::server::Access;
let url = url_template.replace("{node_id}", &node_id.to_string());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no strong preference, but instead of doing string interpolation we could just send it as a request header or in the body part of the POST request

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, those would work as well. Also no strong preference on my side.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This string interpolation might be a bit brittle? I kind of like the idea of some new headers for custom info. It also means you don't have to change the config if we in the future decide to add things like the IP address or whatever as you can just add more headers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it:

  • no more URL string replacement
  • The node id is passed in an X-Iroh-NodeId header

Copy link

github-actions bot commented Mar 26, 2025

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: d404a84

@n0bot n0bot bot added this to iroh Mar 26, 2025
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh Mar 26, 2025
@Frando Frando force-pushed the feat/relay-http-auth branch from 78b200a to 65c5390 Compare March 27, 2025 10:15
/// The request will have a header `X-Iroh-Node-Id` set to the hex-encoded node id attempting
/// to connect to the relay.
///
/// To grant access, the HTTP endpoint must return a `200` response with `true` as the response text.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation to need a true in the body?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of this as a safeguard, so that the endpoint implementing this would have to be more explicit. Maybe it's not needed, no strong opinion here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't have a strong opinion... I don't know if this will turn out to be really handy, really annoying, or just completely fine 😄

@flub
Copy link
Contributor

flub commented Apr 3, 2025

What happens if the authentication server needs to authenticate the clients connecting to it?

@Frando
Copy link
Member Author

Frando commented Apr 3, 2025

What happens if the authentication server needs to authenticate the clients connecting to it?

The authentication server has the node id, and can do whatever it likes to authenticate it (i.e. check against an ACL or against a capability token which it previously received over another communication channel).

Or did I misunderstand your question?

@flub
Copy link
Contributor

flub commented Apr 3, 2025

What happens if the authentication server needs to authenticate the clients connecting to it?

The authentication server has the node id, and can do whatever it likes to authenticate it (i.e. check against an ACL or against a capability token which it previously received over another communication channel).

Or did I misunderstand your question?

Yeah, sorry. Didn't ask that very well. What I was wondering is whether the authentication service wants to protect itself. You don't want it to become a NodeId oracle but only want it to be used by authorised relay servers, and right now you can only do that by deploying it on a private network. Or maybe you could put a secret in the URL, but that secret would then have to be in the relay server's config file.

So maybe in the future we'll have to add features like this, and how compatible would that be to the current configuration format is what I was wondering. But it's probably fine and we can deal with it when that happens.

@Frando
Copy link
Member Author

Frando commented Apr 4, 2025

What I was wondering is whether the authentication service wants to protect itself. You don't want it to become a NodeId oracle but only want it to be used by authorised relay servers, and right now you can only do that by deploying it on a private network. Or maybe you could put a secret in the URL, but that secret would then have to be in the relay server's config file.

Ah, now I'm understanding. This is a good point. A secret in the URL would work. What other methods would work well? Some ideas:

  1. Allow to set an Authentication header via either a config value, or an environment variable IROH_RELAY_HTTP_AUTH_HEADER or similar
  2. Allow to set arbitrary headers via the config file

I think a first safeguard would be to make the http config a map instead of a single value, so that more options can be added without breaking the config format. Let me add this.

@flub
Copy link
Contributor

flub commented Apr 4, 2025

A secret in the URL would work. What other methods would work well? Some ideas:

1. Allow to set an `Authentication` header via either a config value, or an environment variable `IROH_RELAY_HTTP_AUTH_HEADER` or similar

2. Allow to set arbitrary headers via the config file

I think a first safeguard would be to make the http config a map instead of a single value, so that more options can be added without breaking the config format. Let me add this.

Making http a map is a great future-proofing response indeed. I wouldn't go any further than that for now.

(And 1. above sounds the most appealing to me in the future, but let's see)

@Frando
Copy link
Member Author

Frando commented Apr 4, 2025

I added support for bearer tokens too.

/// The request will have a header `X-Iroh-Node-Id` set to the hex-encoded node id attempting
/// to connect to the relay.
///
/// To grant access, the HTTP endpoint must return a `200` response with `true` as the response text.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't have a strong opinion... I don't know if this will turn out to be really handy, really annoying, or just completely fine 😄

}

#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
struct HttpAccessConfig {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always nervous whether serde manages to de-serialise from the config file format correctly. I've seen cases where serde expressed things that could not be expressed in the target serialisation. Mind adding a de-serialisation test for this from toml? (We don't need serialise support on these anymore, they're historic.)

There are a few similar tests already, so should be easy enough to copy.

@Frando Frando added this pull request to the merge queue Apr 7, 2025
Merged via the queue into main with commit 592c3b5 Apr 7, 2025
27 checks passed
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in iroh Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants