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

Add support for Titan protocol #11

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add support for Titan protocol #11

wants to merge 6 commits into from

Conversation

TeddyDD
Copy link

@TeddyDD TeddyDD commented Mar 11, 2022

Hey. I implemented Titan support as middleware. It required few changes in core of gig:

  • passing request reader to Context. This enables the server to read data sent by client after a standard Gemini request. It's important to note that the reader should be used only to implement Gemini protocol extensions. From Gemini spec:

    Clients MUST NOT send anything after the first occurrence of in a request, and servers MUST ignore anything sent after the first occurrence of a <CR><LF>.

  • optional switch in Gig to allow other protocol schemas, so we don't get error on titan:// request
  • stripping path parameters in find method of the router

FakeConn required some minor adjustments to make writing UT possible.

Usage

A basic example is provided in examples/titan. The middleware handles parsing Titan parameters (size, mime and token) and stores them in the Context. Additionally, the titan property is set in the context depending on whenever handled request is done via titan or Gemini scheme. gig.AllowProxying must be enabled to receive Titan requests.

Eye candy

titan

Things to consider

  • NewFakeContext needs new optional argument - a reader. I did not want to refactor all uses of this function in single PR, so I implemented a functional option pattern. If you want to, I can open separate PR to move tlsState argument to option.
  • maybe Titan middleware should be separate package?
  • striping path params is strictly Titan specific. It uconditionally cuts anything matching ;.+$ regex from URL. Not sure if this is an issue, they are rarely used anyway. Go's url library does not support them.

TODO

  • add documentation in README
  • handle path params in router

Closes: #8

@ghost
Copy link

ghost commented Jul 5, 2022

Thanks for adding this. Hopefully you can finish it up so it can be merged. I was considering trying to add this a year ago but never had the time.

@TeddyDD
Copy link
Author

TeddyDD commented Jul 6, 2022

Hi, I'll try to write documentation soon :)

@ghost
Copy link

ghost commented Jul 6, 2022

Thanks. Hopefully @pitr will see this so he can merge it. If not, then maybe there is a way to use your fork directly in golang somehow? Or I could always download the code myself directly, I suppose.

@ghost
Copy link

ghost commented Jul 9, 2022

I assume the client certs work the same way with a titan route as they do with a gemini one, by just using the default gig client cert function (c.Certificate), right? Is this the same with errors too? How to Titan errors (error code 50) work with this?

Two other things that might be useful is to have the logger print whether the request was for gemini or titan (aka: which scheme).

Lastly, what if you made AllowProxying a slice of strings of schemes that are allowed, or something along those lines?

@ghost
Copy link

ghost commented Jul 9, 2022

It looks like your implementation of gig.TitanRedirect will use StatusPermanentFailure for a titan error, so that question is answered :)

@ghost
Copy link

ghost commented Jul 9, 2022

One more thing to consider is some form of timeout (from start of upload, or timeout between received bytes) on upload. I'm not sure if this is already a thing yet.

@TeddyDD
Copy link
Author

TeddyDD commented Jul 9, 2022

It can be done with ReadTimeout property:

g := gig.Default()
g.ReadTimeout = time.Second * 10

I added some basic documentation to README.

@TeddyDD TeddyDD force-pushed the titan branch 2 times, most recently from f55bd19 to 9b0de94 Compare July 9, 2022 11:17
@ghost
Copy link

ghost commented Jul 9, 2022

Cool, thanks!! Btw, I switched to your titan branch for gemini://auragem.space, and looks like it is working well so far. Haven't had any significant problems yet.

@ghost
Copy link

ghost commented Jul 9, 2022

One more thing I wanted to make sure was that if the mime parameter was not provided, then the spec says to default to "text/gemini".

TeddyDD added 4 commits July 9, 2022 23:15
To support Titan router must strip path parameters from last segment
of the path in find method. Otherwise paths like /foo;size=1 won't
match declared path /foo in router.

Path segment parameters are described in RFC 3986, section 3.3.
@TeddyDD
Copy link
Author

TeddyDD commented Jul 9, 2022

You right, fixed.

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.

Basic Titan Support
1 participant