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

Allow the use of alternative net.Listener implementations by downstreams #25855

Merged
merged 7 commits into from
Jul 24, 2023

Conversation

eyedeekay
Copy link
Contributor

This is a simple PR which moves the GetListener function to a DefaultGetListener function, and changes GetListener to be a variable which by default points to the DefaultGetListener function. This allows people who may exist quasi-downstream of Gitea to create alternate "GetListener" functions, with identical signatures, which return different implementations of the net.Listener interface. This approach is expressly intended to be non-invasive and have the least possible impact on the gitea codebase. A previous version of this idea was rejected before: #15544 but because of issues like: #22335 I really think that recommending people configure proxies by hand is exactly the wrong way to do things(This is why there is a Tor Browser.). This tiny change lets me put proper hidden service configuration into single i2p.go file which lives in modules/graceful/ and which never has to be checked in to your codebase or affect your dependencies or bloat your project in any way, it can live on a branch in my fork and I'll fast-forward every release and never the twain shall meet.

The main use-case for this is to listen on Peer-to-Peer networks and Hidden Services directly without error-prone and cumbersome port-forwarding configuration. For instance, I might implement an "I2PGetListener" as follows:

// adapted from i2p.go which is unchecked-in in my modules/graceful/ directory
import "github.com/eyedeekay/onramp"

var garlic = &onramp.Garlic{}

func I2PGetListener(network, address string) (net.Listener, error) {
	// Add a deferral to say that we've tried to grab a listener
	defer GetManager().InformCleanup()
	switch network {
	case "tcp", "tcp4", "tcp6", "i2p", "i2pt":
		return garlic.Listen()
	case "unix", "unixpacket":
// I2P isn't really a replacement for the stuff you use Unix sockets for and it's also not an anonymity risk, so treat them normally
		unixAddr, err := net.ResolveUnixAddr(network, address)
		if err != nil {
			return nil, err
		}
		return GetListenerUnix(network, unixAddr)
	default:
		return nil, net.UnknownNetworkError(network)
	}
}

I could then substitute that GetListener function and be 50% of the way to having a fully-functioning gitea-over-hidden-services instance without any additional configuration(The other 50% doesn't require any code-changes on gitea's part).

There are 2 advantages here, one being convenience, first this turns hidden services into a zero-configuration option for self-hosting gitea, and second safety, these Go libraries are passing around hidden-service-only versions of the net.Addr struct, they're using hidden-service-only versions of the sockets, which are both expressly designed to never require access to any information outside the hidden service network, manipulating the application so it reveals information about the host becomes much more difficult, and some attacks become nearly impossible. It also opens up TLS-over-Hidden Services support which is niche right now, of course, but in a future where gitea instances federate if hidden services want to be part of the federation they're probably going to need TLS certificates. They don't need to be painful to set up.

This doesn't fix an open issue, but it might affect:

I hereby agree to the Code of Conduct published here: https://github.com/go-gitea/gitea/blob/8b89563bf1031089a218e6d05dc61047281b35ee/CODE_OF_CONDUCT.md
I have read and understood the recommendations published here: https://github.com/go-gitea/gitea/blob/8b89563bf1031089a218e6d05dc61047281b35ee/CONTRIBUTING.md

Thank you for your consideration.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 12, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 12, 2023
@@ -150,11 +150,13 @@ func CloseProvidedListeners() error {
return returnableError
}

var GetListener = DefaultGetListener
Copy link
Contributor

Choose a reason for hiding this comment

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

It needs some comments about why it is done so here.

Otherwise it looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Primary reason for it being there is because that is where the GetListener function was before and I was trying to avoid moving anything or otherwise avoid making changes that would be cosmetic or unnecessary. Putting it there kept the whole set of changes to just the 2 lines and makes it very clear that all I did was make a function replaceable. I could move the GetListener variable declaration to the top of the file and it would have the same effect obviously, and if that's preferred I will.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the GetListener to the top of the file with the other vars, commented both GetListener and DefaultGetListener to explain their usage, and added a GetListener/DefaultGetListener pair to net_windows.go

…tListener/DefaultGetListener pair on Windows with same comments/usage
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 13, 2023
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

I meant that the comment could clarify why the GetListener is designed to be a variable, for example, it will be changed by downstream packages?

Otherwise, when future developers maintain this code, they don't see any other "implementation" in code, and don't see any related comment, they might simply remove this variable.

Anyway, the new comment already explains some details, it doesn't block and anything wrong by future developers could be fixed easily.


ps: it could only introduce one var GetListener into "server.go", make "net_{os}.go" only provide DefaultGetListener, then the code would be slightly simplified.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 17, 2023
@lunny
Copy link
Member

lunny commented Jul 20, 2023

I don't think we should have any guarantee for users who using Gitea's internal packages if you mean downstreams that.

@wxiaoguang
Copy link
Contributor

I don't think we should have any guarantee for users who using Gitea's internal packages if you mean downstreams that.

I don't understand your point.

@lunny
Copy link
Member

lunny commented Jul 24, 2023

This allows people who may exist quasi-downstream of Gitea to create alternate "GetListener" functions

If I'm not wrong. They are using Gitea as a library. But this is not a design goal of Gitea. So there is no any gurantee this will not be broken in future changes.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 24, 2023

This allows people who may exist quasi-downstream of Gitea to create alternate "GetListener" functions
If I'm not wrong. They are using Gitea as a library. But this is not a design goal of Gitea. So there is no any gurantee this will not be broken in future changes.

No, in most cases, Gitea is not used as a library. But the downstream developers could add a "my_listener.go" to codebase to use their own listeners.

There is no need to gurantee that this feature won't be broken (downstream developers can handle any breaking), but this change is simple enough and doesn't cause any problem.


Even if we want to refactor the Listener, it is a code-level change and no need to consider about "breaking", this change won't block anything IMO.

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 24, 2023
@lunny lunny added type/refactoring Existing code has been cleaned up. There should be no new functionality. reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Jul 24, 2023
@lunny lunny enabled auto-merge (squash) July 24, 2023 06:56
@lunny lunny merged commit cdd3d4b into go-gitea:main Jul 24, 2023
23 checks passed
@GiteaBot GiteaBot added this to the 1.21.0 milestone Jul 24, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 24, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 25, 2023
* giteaofficial/main: (23 commits)
  Avoid writing config file if not installed (go-gitea#26107)
  Implement auto-cancellation of concurrent jobs if the event is push (go-gitea#25716)
  [skip ci] Updated translations via Crowdin
  doc guide the user to create the appropriate level runner (go-gitea#26091)
  Fix handling of Debian files with trailing slash (go-gitea#26087)
  fix Missing 404 swagger response docs for /admin/users/{username} (go-gitea#26086)
  Allow the use of alternative net.Listener implementations by downstreams (go-gitea#25855)
  Add missing default value for some Bool cli flags (go-gitea#26082)
  Reduce unnecessary DB queries for Actions tasks (go-gitea#25199)
  Use stderr as fallback if the log file can't be opened (go-gitea#26074)
  Make organization redirect warning more clear (go-gitea#26077)
  Replace gogs/cron with go-co-op/gocron (go-gitea#25977)
  Remove `db.DefaultContext` in `routers/` and `cmd/` (go-gitea#26076)
  Categorize admin settings sidebar panel (go-gitea#26030)
  [skip ci] Updated translations via Crowdin
  Fix duplicated url prefix on issue context menu (go-gitea#26066)
  Add context parameter to some database functions (go-gitea#26055)
  Fix branch list auth (go-gitea#26041)
  Fix the truncate and alignment problem for some admin tables (go-gitea#26042)
  Update secrets.en-us.md (go-gitea#26057)
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants