Skip to content
This repository has been archived by the owner on Jun 24, 2021. It is now read-only.

IRCv3 STS #350

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

IRCv3 STS #350

wants to merge 7 commits into from

Conversation

kaniini
Copy link
Contributor

@kaniini kaniini commented Jul 9, 2020

Minimal STS implementation. Does not support cap-notify.

@kaniini kaniini requested a review from aaronmdjones July 9, 2020 23:40
@kaniini
Copy link
Contributor Author

kaniini commented Jul 26, 2020

any reason why this hasn't been reviewed yet?

@aaronmdjones
Copy link
Contributor

aaronmdjones commented Jul 26, 2020

I was going to do this tomorrow.


This will not be merged as-is because it is not complete.

Namely, it does not support Server Name Indication. This needs to be extended to take a hostname (ideally as part of the top-level token name, a la connect "irc.server.name" {}), and be allowed to be specified multiple times (again like connect). It should only advertise the capability to the client if the client's SNI-provided hostname matches a configured sts block.

Without this, it would be simply stupid to ever enable it, because STS requires clients to validate the server's certificate, but you can't control which hostnames point at your IRCd.

Also, the enabled key is redundant; simply commenting out the block in the default configuration would be preferred, with the understanding that if you uncomment it, you are enabling it.

@aaronmdjones aaronmdjones self-assigned this Jul 26, 2020
@aaronmdjones
Copy link
Contributor

aaronmdjones commented Jul 26, 2020

Upon further thought, the design I envisage is that whether or not to advertise a port to non-TLS clients should be decided by the listen block (with a listen::stsport key, parsing conditional on listen::sslport being given, and the port given in listen::stsport matching that list of ports), and whether or not to advertise a persistence policy to TLS clients should be, as above, decided by SNI.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants