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

Implement RFC 9540 #47

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

nothingmuch
Copy link

Note: This is a breaking change, and requires directory support for RFC 9540 as well.

This change modifies the relay to assume that both the the OHTTP gateway
and the ohttp keys reside at /.well-known/ohttp-gateway, with POST
requests implementing the OHTTP gateway functionality and GET requests
fetching the OHTTP keys.

Extends #46, but does not strictly depend on it.

@nothingmuch
Copy link
Author

Opened as draft because of the breaking nature of this change, and in case #46 is rejected in which case this should be rebased to not depend on it.

When requests to the relay include a path & query, previously this would
be copied to the request sent to the gateway.

This behavior did not match the specified behavior in RFC 9458, where
the relay and gateway resource are related by a fixed 1:1 mapping,
whereas copying the path is a family of 1:1 mappings between relay
resources and gateway resources.

Less pedantic behavior could arguably simply ignore any extraneous path
or query elements in the request URI, but based on the logic that any
deployment relying on the existing behavior would be incompatible with
the existing directory implementation, this more conservative path was
chosen.

This was only tested manually by temporarily modifying the ohttp_req
test helper to ensure it fails (for both the request matching and the
explicit treatment of req.uri().path_and_query()), in order to avoid
modifying it with additional long lived complexity to allow overriding
the path. The rationale is that in the near term protocol opt-in by
directories will give meaning to path components of directory requests,
and the modified behavior fails closed.
Copy link
Collaborator

@DanGould DanGould left a comment

Choose a reason for hiding this comment

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

Do I understand correctly that if #46 is merged as well as this PR, the changes will not break our practical use of this relay implementation in the wild that we're aware of?

Or is your preference not to support both /ohttp-keys AND this change?

@nothingmuch
Copy link
Author

Do I understand correctly that if #46 is merged as well as this PR, the changes will not break our practical use of this relay implementation in the wild that we're aware of?

#46 should be fine to merge for the stated reason.

However, this PR will break unless the payjoin-directory is also updated (2nd commit of payjoin/rust-payjoin#530 which I will shortly make into its own separate PR).

Or is your preference not to support both /ohttp-keys AND this change?

Actually I think we should support both at least for some transition period as it's just 2 lines in the code, and because the ohttp-keys endpoint is depended on not just by the ohttp-relay but also payjoin receivers. I did remove support for it in payjoin/rust-payjoin#530, but only for testing purposes.

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.

2 participants