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

extend --profile cmd line option to allow interface to be specified #838

Merged
merged 1 commit into from
Sep 9, 2017

Conversation

jpz
Copy link
Contributor

@jpz jpz commented Sep 5, 2017

Closes #691

@dajohi
Copy link
Member

dajohi commented Sep 5, 2017

I like this. I am just wondering if the default should be to listen on localhost only...
any opinions?

@davecgh
Copy link
Member

davecgh commented Sep 5, 2017

I agree with @dajohi. Just like the RPC server, this should should only listen on localhost by default. That was a big reason it only allowed a port to begin with back in the early days.

I agree with being able to specify other interfaces though as that is quite reasonable for certain setups, but it shouldn't expose your process to the world by default.

@jpz
Copy link
Contributor Author

jpz commented Sep 6, 2017 via email

@jpz jpz force-pushed the profile_with_interface_option branch from 92420b3 to cb739fd Compare September 6, 2017 03:16
@davecgh
Copy link
Member

davecgh commented Sep 6, 2017

Yes, 127.0.0.1 is preferred.

@dajohi
Copy link
Member

dajohi commented Sep 6, 2017

localhost is preferred :)

@jpz
Copy link
Contributor Author

jpz commented Sep 6, 2017

Can you elaborate - what your rationale for saying localhost is preferred?

After reading through the requisite Stackoverflow reading :) the feedback seemed to be that it makes no difference unless we were opening a Unix domain socket, but this is a TCP connection.

@dajohi
Copy link
Member

dajohi commented Sep 6, 2017

localhost will listen on ::1 as well. Using localhost will try ::1 first, then 127.0.0.1 on systems that prefer ipv6.

@jpz
Copy link
Contributor Author

jpz commented Sep 6, 2017

I prefer to leave it as it is, I think connecting to 127.0.0.1 will always work, and the arguments for hardening against localhost spoofing still stand, even though it's an unlikely attack vector. Still, it's hard to foresee security issues across multiple versions of multiple operating systems.

I can just remember working in environments (old Windows NT versions) where the HOSTS file was modifiable.

I would only see a need to resolve to an IPv6 address from localhost, if I were running on a platform that doesn't support IPv4 - which is hard to imagine.

I can change it if you still think that I should however.

Copy link
Member

@dajohi dajohi left a comment

Choose a reason for hiding this comment

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

OK

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. In addition to the inline comments, please update sampleconfig/sampleconfig.go and doc.go accordingly as well.

config.go Outdated

// check the Profile is a valid address
_, portStr, err := net.SplitHostPort(cfg.Profile)

Copy link
Member

Choose a reason for hiding this comment

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

Extra blank line.

dcrd.go Outdated
dcrdLog.Infof("Creating profiling server "+
"listening on %s", listenAddr)
"listening on %s", cfg.Profile)
Copy link
Member

Choose a reason for hiding this comment

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

Please use listenAddr that was just defined as to avoid a source change that doesn't actually change anything.

@jpz jpz force-pushed the profile_with_interface_option branch from c0b7539 to 0139cc6 Compare September 9, 2017 05:13
@jpz jpz force-pushed the profile_with_interface_option branch from 0139cc6 to 509eed3 Compare September 9, 2017 05:27
@jpz
Copy link
Contributor Author

jpz commented Sep 9, 2017

done. thanks for the review - squashed and rebased

@davecgh davecgh merged commit 509eed3 into decred:master Sep 9, 2017
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.

3 participants