Skip to content

Add support for listening on unix domain sockets#488

Merged
bnfinet merged 4 commits intovouch:masterfrom
squalus:socket
Jan 21, 2023
Merged

Add support for listening on unix domain sockets#488
bnfinet merged 4 commits intovouch:masterfrom
squalus:socket

Conversation

@squalus
Copy link
Contributor

@squalus squalus commented Jul 31, 2022

Added unix domain socket support for the listen setting.

Added socket_mode setting to change socket permissions.

Example configuration:

listen: /run/vouch-proxy/socket
socket_mode: 0600

@bnfinet
Copy link
Member

bnfinet commented Jul 31, 2022

@squalus thanks for your contributions. Both of these PRs are of interest.

That said, could you please read the README. I see some items worthy of discussion here.

@squalus squalus force-pushed the socket branch 2 times, most recently from d8ba628 to 52cdede Compare July 31, 2022 04:12
@squalus
Copy link
Contributor Author

squalus commented Jul 31, 2022

Added an issue, changelog entry, and unit test

main_test.go Outdated
config := &cfg.Config{
Listen: fmt.Sprintf("unix:%s", socketFile),
}
lis, cleanupFn, err := listen(config)
Copy link
Member

Choose a reason for hiding this comment

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

instead of contructing a config object, please create and consume a config file in config/testing and then use a setup function similar to...
https://github.com/vouch/vouch-proxy/blob/master/pkg/cfg/cfg_test.go#L23-L26

please also add config examples in config/config.yml_example and config/config.yml_example_listen_unix_socket

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Updated test to use testing config files instead of constructing a config object
  • Added config examples to config/config.yml_example. I didn't add another standalone one, because it would just be an exact duplicate of another one of the other files with one extra line.

main.go Outdated

}

func listen(config *cfg.Config) (lis net.Listener, cleanupFn func(), err error) {
Copy link
Member

Choose a reason for hiding this comment

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

for consistency, instead of passing config *cfg.Config please just use cfg.Cfg.___ and leave the signature as listen(). Config isn't meant to be passed around, just plucked from the ether as a global.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the listen function to use the global configuration object

@bnfinet
Copy link
Member

bnfinet commented Aug 12, 2022

@squalus thanks for the fine addition to VP

I've left a few comments inline to the code.

Please do also ensure that the unix socket can be configured via environmental variables and adjust this test accordingly..
https://github.com/vouch/vouch-proxy/blob/master/pkg/cfg/cfg_test.go#L129

Cheers!

@bnfinet
Copy link
Member

bnfinet commented Aug 12, 2022

One other good addition would be to add a check to the basicTest() to make sure that if cfg.Listen starts with unix: then socket_mode is required and set properly.
https://github.com/vouch/vouch-proxy/blob/master/pkg/cfg/cfg.go#L398

Perhaps SocketMode should default to 0600?

In general, VP tries to catch configuration errors and offer clear logging in hopes of helping fellow admins find their way quickly, and avoid support tickets showing up here.

@squalus
Copy link
Contributor Author

squalus commented Aug 13, 2022

One other good addition would be to add a check to the basicTest() to make sure that if cfg.Listen starts with unix: then socket_mode is required and set properly. https://github.com/vouch/vouch-proxy/blob/master/pkg/cfg/cfg.go#L398

Perhaps SocketMode should default to 0600?

In general, VP tries to catch configuration errors and offer clear logging in hopes of helping fellow admins find their way quickly, and avoid support tickets showing up here.

Updated listen() to default SocketMode to 0777. (I'm copying PostgreSQL's defaults)

@squalus
Copy link
Contributor Author

squalus commented Aug 13, 2022

@bnfinet
Copy link
Member

bnfinet commented Jan 21, 2023

@squalus sorry for the delay in reviewing this PR

I've set a default of socket perms to 0660 somewhat based off of what nginx is doing

https://serverfault.com/questions/437077/what-should-be-proper-permission-of-unix-socket

Thanks again for the fine PR, I'm going to merge and ship

@bnfinet bnfinet merged commit c1925e2 into vouch:master Jan 21, 2023
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