-
Notifications
You must be signed in to change notification settings - Fork 28
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
[WIP] Messaging hostname FQDN validation #230
Conversation
def hostname_fqdn?(host) | ||
require "socket" | ||
|
||
fqdn = Addrinfo.getaddrinfo(Socket.gethostname, nil).first.getnameinfo.first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if its better to use hostname -f
(with back ticks) here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the Addrinfo.getaddrinfo
doing for us? Can you just use what Socket.gethostname
returns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addrinfo.getaddrinfo
provides more info such as the FQDN whereas Socket.gethostname
will provide the hostname but not necessarily the FQDN. I've tested this on an AWS appliance i.e.
irb(main):002:0> Addrinfo.getaddrinfo(Socket.gethostname, nil).first.getnameinfo.first
=> "<host>.us-west-1.compute.internal"
irb(main):003:0> Socket.gethostname
=> "<host>"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we guaranteed to have the addrinfo
we want first
in the array and the nameinfo
we want to also be first
?
Googling around, I found my buddy from the old sql server adapter days, metaskills opened https://github.com/newrelic/newrelic-ruby-agent/issues number 697. The newrelic ruby agent now defaults to trying hostname -f
first, then hostname
, and then heroku dyno name, and finally gethostname
. See: https://github.com/newrelic/newrelic-ruby-agent/pull/1015/files
And the current code in main:
https://github.com/newrelic/newrelic-ruby-agent/blob/0824b224b9c1be974d0b47ae9422f2f996046218/lib/new_relic/agent/hostname.rb#L11-L38
Note, the original issue was that aws lambda doesn't have the hostname
binary so they needed a backup way to get the information. If we're guaranteed to have it, I wonder if we should just use it, if it's more reliable than using something in ruby.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good suggestion. Thoughts on something like this? (first time using awesome_spawn)
fqdn = Addrinfo.getaddrinfo(Socket.gethostname, nil).first.getnameinfo.first | |
begin | |
fqdn = AwesomeSpawn.run!("hostname", :params => ["-f"]) | |
rescue AwesomeSpawn::CommandResultError | |
require "socket" | |
fqdn = Addrinfo.getaddrinfo(Socket.gethostname, nil).first.getnameinfo.first | |
end | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll be able to guarantee if we'll have hostname
binary installed on appliances. I guess my question is can we just assume hostname
or do we need the fallback?
If we need the fallback, do we need to worry about the order of the arrays (assumption on the first element being the correct one). I recall some ruby stdlib for ip addresses would sometimes return loopback first instead of your publicly visible ip address so I wasn't sure if we have similar problems if there are multiple results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we do anything I'd like to understand how we got different values in the messaging yml and the cert.
@nasark where does the requirement that the messaging server's hostname match what the user enters? |
5160543
to
84b0063
Compare
Checked commit nasark@84b0063 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
Marking as WIP. After a discussion with @agrare we think its better to solve the problem at a lower level and add validation to prevent scenarios where the host in messaging.yml does not match the host being used in the certs |
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
Check if messaging hostname entered matches the FQDN
@miq-bot assign @Fryguy
@miq-bot add_reviewer @agrare, @jrafanie
@miq-bot add_label enhancement
Ref: #229