feat: Add All or Nothing Bearer token auth support #24666
Conversation
This commit adds basic authorization support to Edge. Up to this point we didn't need have authorization at all and so the server would receieve and accept requests from anyone. This isn't exactly secure or ideal for a deployment and so we add a basic form of authentication. The way this works is that a user passes in a hex encoded sha256 hash of a given token to the '--bearer-token' flag of the serve command. When the server starts with this flag it will now check a header of the form 'Authorization: Bearer <token>' by making sure it is valid in the sense that it is not malformed and that when token is hashed it matches the value passed in on the command line. The request is denied with either a 400 Bad Request if the header is malformed or a 401 Unauthorized if the hash does not match or the header is missing. The user is provided a new subcommand of the form: 'influxdb3 create token <token>' where the output contains the command to run the server with and what the header should look like to make requests. I can see future work including multiple tokens and rotating between them or adding new ones to a live service, but for now this shall suffice. As part of the commit end-to-end tests are included to run the server and make requests against the HTTP API and to make sure that requests are denied for being unauthorized, accepted for having the right header, or denied for being malformed. Also as part of this commit a small fix is included for 'Accept: */*' headers. We were not checking for them and if this header was included we were denying it instead of sending back the default payload return value.
influxdb3/tests/auth.rs
Outdated
| // Test that there is an extra string after the token foo | ||
| match client | ||
| .get("http://127.0.0.1:8181/api/v3/query_sql?db=foo&q=select+*+from+cpu") | ||
| .header("Authorization", "Bearer foo whee") |
There was a problem hiding this comment.
clap might prevent this but could the user not enter a token that has whitespace in it?
There was a problem hiding this comment.
It does not seem to prevent this if you pass the arg "". I will change it to check for the empty string
influxdb3/tests/auth.rs
Outdated
| match client | ||
| .post("http://127.0.0.1:8181/api/v3/write_lp?db=foo") | ||
| .body("cpu,host=a val=1i 123") | ||
| .send() | ||
| .await | ||
| { | ||
| Err(e) => { | ||
| server.kill().unwrap(); | ||
| panic!("request errored out: {e}"); | ||
| } | ||
| Ok(resp) => { | ||
| if resp.status() != StatusCode::UNAUTHORIZED { | ||
| server.kill().unwrap(); | ||
| panic!("status code was not unauthorized: {:#?}", resp); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
At the sacrifice of debuggable output, you could probably condense your assertion code by using the form:
assert_eq!(
401,
client
.post("...")
.body("...")
.send()
.await
.status()
.as_u16(),
"status code was not unauthorized",
);But then, you might also need a wrapper type to hold your server, so that it can kill the std::process::Command on Drop or something like that.
There was a problem hiding this comment.
Yeah. The other problem is that if panic=abort it'll just skip this and it'll cause the program to remain open. I don't think we are and so it could be condensed, but I opted for the more verbose one in this case. I can experiment with a Drop wrapper to see if it works though!
|
@hiltontj I made the changes to error on an empty string and with some code contortion got the code for assertions to be better and in the image below it shows that the server doesn't continue to run even if there is a panic
|
|
Uh so. Hm really weird, but I merged this via well here, BUT it failed or so I thought. It's currently in main -> de102bc and I can pull it from the command line so uh. GitHub had a bad moment here? I'm going to close the PR, but it's actually merged. Love this. Love hitting a bug. |
This commit adds basic authorization support to Edge. Up to this point we didn't need have authorization at all and so the server would receive and accept requests from anyone. This isn't exactly secure or ideal for a deployment and so we add a basic form of authentication. The way this works is that a user passes in a hex encoded sha256 hash of a given token to the '--bearer-token' flag of the serve command. When the server starts with this flag it will now check a header of the form 'Authorization: Bearer <token>' by making sure it is valid in the sense that it is not malformed and that when token is hashed it matches the value passed in on the command line. The request is denied with either a 400 Bad Request if the header is malformed or a 401 Unauthorized if the hash does not match or the header is missing. The user is provided a new subcommand of the form: 'influxdb3 create token <token>' where the output contains the command to run the server with and what the header should look like to make requests. I can see future work including multiple tokens and rotating between them or adding new ones to a live service, but for now this shall suffice. As part of the commit end-to-end tests are included to run the server and make requests against the HTTP API and to make sure that requests are denied for being unauthorized, accepted for having the right header, or denied for being malformed. Also as part of this commit a small fix is included for 'Accept: */*' headers. We were not checking for them and if this header was included we were denying it instead of sending back the default payload return value.
I know we're very early in all of this, but for future work whether it is through documentation or CLI tooling for generating the raw token, we should use the same process for creating tokens as for other InfluxDB 3 products:
By doing this, we ensure we have sufficient randomness (base64 encoded 512 bits is a fine choice and isn't unwieldy), have a consistent user experience across the product line[*] and have a consistent implementation/developer experience across the product line. Perhaps you would choose a different prefix for OSS 3. [*] for historical reasons, InfluxDB 3 Serverless does not use a prefix ( cc @pauldix |
|
@mgattozzi I think it would be good to update our token setup to be consistent with what @jdstrand mentions. So base64 encode it, prepend |
|
Sounds good. I might not be able to get to it this week, but I can open up an issue later to keep this on our radar |
|
Turned this into an issue -> #24704 |
This commit adds basic authorization support to Edge. Up to this point we didn't need have authorization at all and so the server would receieve and accept requests from anyone. This isn't exactly secure or ideal for a deployment and so we add a basic form of authentication.
The way this works is that a user passes in a hex encoded sha256 hash of a given token to the '--bearer-token' flag of the serve command. When the server starts with this flag it will now check a header of the form 'Authorization: Bearer ' by making sure it is valid in the sense that it is not malformed and that when token is hashed it matches the value passed in on the command line. The request is denied with either a 400 Bad Request if the header is malformed or a 401 Unauthorized if the hash does not match or the header is missing.
The user is provided a new subcommand of the form: 'influxdb3 create token ' where the output contains the command to run the server with and what the header should look like to make requests.
I can see future work including multiple tokens and rotating between them or adding new ones to a live service, but for now this shall suffice.
As part of the commit end-to-end tests are included to run the server and make requests against the HTTP API and to make sure that requests are denied for being unauthorized, accepted for having the right header, or denied for being malformed.
Also as part of this commit a small fix is included for 'Accept: /' headers. We were not checking for them and if this header was included we were denying it instead of sending back the default payload return value.
Closes #24646