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.

Sign up for GitHub

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

rsc: Add route to check for valid api key #1549

Merged
merged 5 commits into from
May 2, 2024
Merged

rsc: Add route to check for valid api key #1549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c47c431
rsc: Add route to check for valid api key
V-FEXrt Apr 24, 2024
2332be1
typo
V-FEXrt Apr 24, 2024
1a841e4
check auth when creating api
V-FEXrt Apr 24, 2024
f6e2cc3
use rmap over rmapPass
V-FEXrt Apr 25, 2024
7c50eda
cleanup
V-FEXrt May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions rust/rsc/src/rsc/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,13 @@ fn create_router(
move |req, next| api_key_check::api_key_check_middleware(req, next, conn.clone())
})),
)
.route(
"/auth/check",
post(axum::http::StatusCode::OK).layer(axum::middleware::from_fn({
let conn = conn.clone();
move |req, next| api_key_check::api_key_check_middleware(req, next, conn.clone())
})),
)
// Unauthorized Routes
.route(
"/job/matching",
Expand Down Expand Up @@ -508,6 +515,36 @@ mod tests {

assert_eq!(res.status(), StatusCode::BAD_REQUEST);

// Authorization check with invalid auth should 401
Copy link
Collaborator

Choose a reason for hiding this comment

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

MDN suggests this often means "unauthenticated" in practice rather than a literal "unauthorized", and while there's definitely no reason not to follow the literal meaning since we're using a very specific client software, I might have gone with a 403 Forbidden as an unambiguous response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I agree that 401 means "unauthenticated" which imo is correct here since the point is to check if the provided auth key grants authentication :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense -- I was reading it as the auth key basically being the credentials themselves, in which case you've got valid authentication and just not authorization (i.e. this would be the gate for a restricted-push store), but sounds like my model of what's going on was slightly off.

let res = router
.call(
Request::builder()
.uri("/auth/check")
.method(http::Method::POST)
.header("Authorization", "badauth")
.body(Body::empty())
.unwrap(),
)
.await
.unwrap();

assert_eq!(res.status(), StatusCode::UNAUTHORIZED);

// Authorization check with valid auth should 200
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one is probably the best code. There's potential in 204 No Content, but I'm not sure the connotations there exactly match up.

let res = router
.call(
Request::builder()
.uri("/auth/check")
.method(http::Method::POST)
.header("Authorization", api_key.clone())
.body(Body::empty())
.unwrap(),
)
.await
.unwrap();

assert_eq!(res.status(), StatusCode::OK);

// Correctly inserting a job should 200
let res = router
.call(
Expand Down
34 changes: 32 additions & 2 deletions share/wake/lib/system/remote_cache_api.wake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,9 +199,18 @@ export def makeRemoteCacheApi (config: String): Result RemoteCacheApi Error =
else failWithError "Remote cache config was set with non-integer port. Saw: {portStr}"

def auth = if authStr ==* "" then None else Some authStr
def api = RemoteCacheApi domain port auth

RemoteCacheApi domain port auth
| Pass
# TODO: check for compatable wake version
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this version check would be on the server side? What could we check for here?

Copy link
Contributor Author

@V-FEXrt V-FEXrt Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

see followup PR :)

but tl;dr we send our version to the server and it tells us if it thinks we are compatible


# If auth is not set we are done. Just return the api
require Some _ = auth
else Pass api

# Auth was set so it must be validated.
api
| rscApiCheckAuthorization
| rmap (\_ api)

# rscApiPostStringBlob: Posts a named string as a blob to the remote server defined by *api*
# then returns the id associated to the blob. Requires authorization.
Expand Down Expand Up @@ -325,6 +334,27 @@ export def rscApiFindMatchingJob (req: CacheSearchRequest) (api: RemoteCacheApi)

mkCacheSearchResponse json

# rscApiCheckAuthorization: Checks if the provided authorization key is valid.
#
# ```
# api | rscApiCheckAuthorization = Pass Unit
# ```
export def rscApiCheckAuthorization (api: RemoteCacheApi): Result Unit Error =
require Some auth = api.getRemoteCacheApiAuthorization
else failWithError "rsc: Checking authorization requires authorization but none was provided"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know what sort of logic chokepoints you have closer to the surface and with this being export it might indeed be the best way to handle it,1 but don't forget that if everything ultimately funnels through a small number of narrow necks, you can collect all the rsc: prefixes through addErrorContext and save a bit of typing/typo risk.

Footnotes

  1. Actually, why is it exported? Doesn't seem like anything which would be helpful for general audiences, but I don't know what sort of package organization you're planning. ↩

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why is it exported

I consider any HTTP web route as a public part of the RemoteCacheApi interface. Anyone with an instance of the apii should be able to call any function on the remote server.

Since POST auth/check is a function made available on the server, anyone should be able to call it.

If everything ultimately funnels through a small number of narrow necks, you can collect all the rsc: prefixes

I'll need you to give an example of what you mean, but maybe given the above comment this doesn't make sense anymore? The important thing to keep in mind is that this library is just a wrapper around what is available as the public interface on the server. What the consumer does with it (in this case remote_cache_runner) is entirely up to them

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, part 1 makes sense and with that clarification, part 2 is indeed invalid. I basically just meant that if you had a lot of potential errors which were not in directly-exported functions but instead were only accessible by going through a small number of entry points, you wouldn't have to add the rsc: prefix to each of the errors at the place you throw them, and could instead call addErrorContext "rsc" at those chokepoints, for what would likely be fewer times than you have errors.


require Pass response =
Copy link
Contributor

Choose a reason for hiding this comment

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

How frequently is this function called?
The db should cache the HTTPRequest job but I'm curious if the wake function should be a target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How frequently is this function called?

Two answers here

  1. As much as the consuming client calls it
  2. In the case of RemoteCacheRunner/default wake implementation, only once per wake invocation so a target doesn't change anything here

Aside:

The db should cache the HTTPRequest job

This isn't the case. Web requests are never safe to cache as the result can change at any time. Because of that web request jobs are never stored in the db

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM.

makeRoute api "auth/check"
| buildHttpRequest
| setMethod HttpMethodPost
| addAuthorizationHeader auth
| makeRequest

match response.getHttpResponseStatusCode
Some 200 -> Pass Unit
Some x -> failWithError "Invalid auth key. Status code: {str x}"
None -> failWithError "Invalid auth key. Unable to determine status code"

# rscApiGetStringBlob: Downloads a blob and returns the contents as a string
#
# ```
Expand Down
Loading