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

influxdb inlet (auth proxy) #8430

Closed
wants to merge 1 commit into from
Closed

Conversation

polvorin
Copy link
Member

@polvorin polvorin commented Aug 29, 2024

Assuming the identity is enrolled in a project that has influxdb addon enabled, like:

[configure influxdb'  addon]
$ ockam project addon configure  influxdb -e http://some.influxdb.instance:8086 -t $TOKEN -o $ORG_ID --permissions '$PERMISSIONS'

An http authentication inlet is started as:

[create a node, and a http-inlet on it.   it creates a tcp-outlet behind the scenes]
$ ockam node create n1
$ ockam http-inlet create --at /node/n1 --from 8087  --to /project/default/service/forward_to_influxdb/secure/api/service/outlet 

Then sending influxdb' request (writes, queries) to localhost:8087 will cause the request to be sent to upstream influxdb instance (the outlet would be pointing to influxdb), but with the request modified to include an Authentication header with a token obtained from the project' token lease manager. This token is rotated as needed, without clients having to worry about that.

The same can be done through a node' config file as well:

name: n1
http-inlets:
  influxdb:
    from: 0.0.0.0:8087
    to:  /project/default/service/forward_to_influxdb/secure/api/service/outlet

Notes:

  • It requires a secure channel between inlet->outlet, even if both are on the same node (the first case, where we implicitly create the tcp outlet). That's something to maybe improve, right now is mandatory.
  • Right now, it doesn't support serving https, as tcp-inlets doesn't support that yet . So influxdb' clients must be configured to use http (upstream can be http or https).
  • Right now the original Token sent by clients is just discarded (it can be anything). Perhaps we could require a custom string there instead (like "OCKAM-MANAGED" or something. As long as clients don't complaint that that isn't a valid token when reading it from config)

@polvorin polvorin marked this pull request as ready for review August 30, 2024 01:41
@polvorin polvorin requested a review from a team as a code owner August 30, 2024 01:41
@polvorin polvorin changed the title Polvorin/http auth proxy http auth proxy Aug 30, 2024
@polvorin polvorin changed the title http auth proxy http inlet (auth proxy) Aug 30, 2024
Copy link
Member

@davide-baldo davide-baldo left a comment

Choose a reason for hiding this comment

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

The implementing looks good!
There is some room for simplification around the CLI commands and on the endpoint.

Also, this way of creating inlet+interceptor is pretty new (kafka is the only user of interceptor but has many services and multiple inlets).
Ideally, we would like to keep inlet creation independent of interceptors while avoiding the re-creation of CRUD for every inlet type.

implementations/rust/ockam/ockam_api/src/http_auth/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking, but I would appreciate it if you could generalize this code, to intercept HTTPs header, other uses for it are already appearing.

Copy link
Member

Choose a reason for hiding this comment

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

For example, @metaclips is already modifying the Host header on the client side to make services work

}
let out = guard
.state
.process_http_buffer(buffer, &token.unwrap_or_default())?;
Copy link
Member

Choose a reason for hiding this comment

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

In this case, I think it would be safer to close the portal by returning an error.
That should be easier to debug.

Copy link
Member Author

Choose a reason for hiding this comment

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

you mean returning an error for the intercept() function, would that close the portal?.

Right now this case just log a warn/err on the logs.

} = create_inlet;

let prefix_route = if is_http_auth_inlet {
Copy link
Member

Choose a reason for hiding this comment

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

You can avoid modifying the TCP creation by creating an endpoint which handles both the creation of the http_auth_interceptor and the inlet creation (by passing a proper prefix route).
This helps keep the TCP inlet creation code straightforward, regardless of the number of special cases. You can also add any number of custom parameters you need.

Kafka handles the interceptor similarly (but with some extra complexity) using services:

pub(super) async fn start_kafka_inlet_service(

(Post, ["node", "services", DefaultAddress::KAFKA_INLET]) => encode_response(

Copy link
Member Author

Choose a reason for hiding this comment

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

it's significant more boilerplate 🤷 . Agree there should be some better way, but we can think on generalization once there are more than one use of it


/// Create TCP Inlets
#[derive(Clone, Debug, Args)]
pub struct CreateCommand {
Copy link
Member

@davide-baldo davide-baldo Sep 2, 2024

Choose a reason for hiding this comment

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

You should be able to ""inherit"" fields by declaring a field containing the inlet Create command with the #[command(flatten)] attribute. From the clap point of view would be as-if the fields were locally declared.

@davide-baldo
Copy link
Member

Other than InfluxDb, I can't think of other use cases where the Authorization header is dynamically added. Maybe we should name it influxdb-inlet?

@polvorin polvorin changed the title http inlet (auth proxy) influxdb inlet (auth proxy) Sep 10, 2024
add an http inlet (tcp inlet + http interceptor) that modify http
requests passing through it, attaching an Authorization token retrieved
from a token lease manager service. Inteded to be used with
influxdb for now, from that the name.
@polvorin polvorin marked this pull request as draft September 12, 2024 22:28
@polvorin polvorin marked this pull request as draft September 12, 2024 22:28
@polvorin
Copy link
Member Author

closing in favor of #8461

@polvorin polvorin closed this Sep 13, 2024
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.

3 participants