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

Unix Socket Handling #2280

Merged
merged 2 commits into from Mar 16, 2020
Merged

Unix Socket Handling #2280

merged 2 commits into from Mar 16, 2020

Conversation

ryanjyoder
Copy link
Contributor

The current handling of Unix sockets is not correct.

  1. The service should not delete the socket before binding to it. If the socket exists, that implies another service is already listening on the socket.
  2. The socket should instead be cleaned up at shutdown.
  3. The permissions should NOT be world writable by default.

One of the main reasons to use sockets is for security. Using sockets allows you to lock access down to a specific unix group/user.However, due to the various use cases for unix sockets, it may be a good idea to accept a net.Listener interface, so that the socket can be configured before the server is started.

@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

Merging #2280 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2280      +/-   ##
==========================================
+ Coverage   98.38%   98.47%   +0.08%     
==========================================
  Files          41       41              
  Lines        2296     2293       -3     
==========================================
- Hits         2259     2258       -1     
+ Misses         21       20       -1     
+ Partials       16       15       -1
Impacted Files Coverage Δ
gin.go 99.13% <100%> (+0.84%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a71af9c...5253b8f. Read the comment docs.

gin.go Outdated
listener, err := net.Listen("unix", file)
if err != nil {
return
}
defer listener.Close()
err = os.Chmod(file, 0777)
defer os.Remove(file)

if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

remove this block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch 👍

@thinkerou thinkerou added this to the 1.6 milestone Mar 16, 2020
@thinkerou thinkerou added the bug label Mar 16, 2020
Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

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

LGTM

@appleboy appleboy merged commit 67008be into gin-gonic:master Mar 16, 2020
pull bot pushed a commit to scope-demo/gin that referenced this pull request Mar 16, 2020
* do not set unix socket permissions. Cleanup unix socket.

* removed useless error checking
byebyebruce pushed a commit to byebyebruce/gin that referenced this pull request Mar 25, 2020
* do not set unix socket permissions. Cleanup unix socket.

* removed useless error checking
@ePirat
Copy link

ePirat commented Mar 8, 2021

How can I set the socket file permissions to set it group-writable so that my go program and webserver reading/writing the socket do not need to share the same user?

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

Successfully merging this pull request may close these issues.

None yet

4 participants