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

Optional signature middleware #21

Merged
merged 6 commits into from
Jan 7, 2025

Conversation

masongup-mdsol
Copy link
Member

@masongup-mdsol masongup-mdsol commented Jan 2, 2025

This PR redoes the middleware system. The previous code had one middleware which always rejected the request entirely if it could not find or validate a MAuth signature, not allowing it to reach the lower layers at all. This may be desirable in some cases, and is possible to use reasonably flexibly with the way most server routers support dynamically attaching layers to only some of the routes. After some work with it though, I decided that it is not sufficiently flexible for many reasonable uses. So here, I renamed the old layer and service to be RequiredMAuthValidationLayer and created a new one named OptionalMAuthValidationLayer. I also decided that a configuration option for this was insufficiently clear as to the intent of the user. This is a breaking change for any users of the old layer, but I don't think there are any right now, aside from the demo service I deployed. Making this not a breaking change would also require a default setting for the original layer, which in my opinion is insufficiently clear to users and readers of code using it regarding which mode the layer is operating in.

The Optional layer will attempt to check for a mauth signature, attach an extension to the request if it finds a valid one. However, it will always forward the request down the layers, whether or not it finds a valid signature. If this is used, it is the responsibility of the handler to specify that the extension is required, or use some other algorithm to validate requests.

To make this smoother, the ValidatedRequestDetails now also impls Axum's FromRequestParts, so you can include it in a handler function, and it will validate that it is present, make it available to the function if desired, and also return a 401 Unauthorized response if it is not present. The return code and body of the default Axum Extension extractor is not very good for this use case.

Also updated to use Yohei's updated mauth-core.

@masongup-mdsol
Copy link
Member Author

masongup-mdsol commented Jan 3, 2025

Updated a few things:

  • Discovered the newer versions of Axum provide a handy OptionalFromRequestParts trait, so I impl'ed that for ValidatedRequestDetails too, which makes it simple to check the MAuth validation, but handle lack of validation in the handler instead of auto-401ing
  • Refactored away from the async_trait macro, which is apparently no longer needed
  • Refactored validate_request_optionally to be infallible. It was pretty awkward to preserve and do something with the only possible failure mode of not being able to retrieve the full body, when that seems very unlikely to ever happen anyways. Implemented instead the behavior of logging an error via tracing and continuing with an empty body if we fail to retrieve the body, just to avoid any possibility of panicking at runtime.
  • This in turn allows OptionalMAuthValidationService to use the below layer's error type, which considerably simplifies things
  • Also refactor RequiredMAuthValidationService to return a 401 error directly instead of the MAuth error, and log the MAuth error instead. This required adding the requirements of making the return type explicitly axum::Response<Body> and a generic constraint on the below layer service return type to impl Into<Response<Body>>, and also simplified the return error type by letting that copy the below layer service's error type. This results in the layer being slightly less generic but much simpler to use in web servers - it is no longer required to create an extra layer to handle the error and decide what to return.
  • Added docs with doc tests to provide examples of how to use all of these possible variations on which middleware to use and what effect it has on the routes specified.

pub(crate) async fn validate_request_optionally(&self, req: Request) -> Request {
let (mut parts, body) = req.into_parts();
if parts.headers.contains_key(MAUTH_V2_SIGNATURE_HEADER)
|| parts.headers.contains_key(MAUTH_V1_SIGNATURE_HEADER)

Choose a reason for hiding this comment

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

is this check case-insensitive?

https://www.rfc-editor.org/rfc/rfc7230#section-3.2

Each header field consists of a case-insensitive field name

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the headers field here is a http::header::HeaderMap, not a standard HashMap. It already fully accounts for the case-insensitive nature of HTTP headers.

Choose a reason for hiding this comment

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

nice!!

@masongup-mdsol masongup-mdsol merged commit 5df1848 into master Jan 7, 2025
4 checks passed
@masongup-mdsol masongup-mdsol deleted the optional-signature-middleware branch January 7, 2025 18:36
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