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

network plugin does not allow `localhost` #2203

Open
Piankero opened this Issue Aug 22, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@Piankero
Contributor

Piankero commented Aug 22, 2018

Steps to Reproduce the Problem

localhost is not accepted as valid ip despite it should

I can confirm that I can ping localhost.

kdb mount test.conf /network dump network
kdb setmeta spec/network/addr check/ipaddr ""
kdb set /network/addr localhost
#>Using name system/network/addr
#>The command kdb set failed while accessing the key database with the info:
#>Sorry, the error (#51) occurred ;(
#>Description: value of key is not a valid IP Address
#>Reason: name: system/network/addr value: localhost message: Name or service not known
#>Ingroup: plugin
#>Module: network
#>At: /media/wespe/extended/repository/piankero-libelektra/src/plugins/network/network.c:90
#>Mountpoint: system/network
#>Configfile: /etc/kdb/test.conf.31841:1534931163.130183.tmp

kdb set /network/addr 127.0.0.1                                                        
#> Using name system/network/addr
#> Set string to "127.0.0.1"

Expected Result

localhost is a valid address for the network plugin

Actual Result

localhost gets rejected

System Information

KDB_VERSION: 0.8.24
SO_VERSION: 4
Operating System: Linux Mint 18.2

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Aug 22, 2018

Contributor

Thank you for reporting the issue!

The problem is, that we have hints.ai_flags = AI_NUMERICHOST in line 37 hardcoded. I would suggest that we add an extension where the user can choose if non-numeric hosts are allowed.

The patch should be trivial but we need to design how the metadata should look like. It should be consistent with check/port as proposed in #2169 where we have the same problem (some applications might allow non-numeric ports, others might not).

Maybe the best way is that we always allow non-numeric names (for both IP and ports) but let the plugin transform to a numeric name. This way applications would always work, irrelevant if they themselves pass AI_NUMERICHOST.

Contributor

markus2330 commented Aug 22, 2018

Thank you for reporting the issue!

The problem is, that we have hints.ai_flags = AI_NUMERICHOST in line 37 hardcoded. I would suggest that we add an extension where the user can choose if non-numeric hosts are allowed.

The patch should be trivial but we need to design how the metadata should look like. It should be consistent with check/port as proposed in #2169 where we have the same problem (some applications might allow non-numeric ports, others might not).

Maybe the best way is that we always allow non-numeric names (for both IP and ports) but let the plugin transform to a numeric name. This way applications would always work, irrelevant if they themselves pass AI_NUMERICHOST.

@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Aug 22, 2018

Contributor

Isn't it possible to let the plugin check if a name or number was given and set the relevant flag for it? Adding even more metadata just to check if it numeric or not seems very unintuitive

Contributor

Piankero commented Aug 22, 2018

Isn't it possible to let the plugin check if a name or number was given and set the relevant flag for it? Adding even more metadata just to check if it numeric or not seems very unintuitive

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Aug 22, 2018

Contributor

Who would look at the flag?

I think the transformation would be better. In the error message we would need to make sure that also the original setting is mentioned, so something like:

Cannot bind to IP "127.0.0.1" as resolved from configuration value "localhost".

Contributor

markus2330 commented Aug 22, 2018

Who would look at the flag?

I think the transformation would be better. In the error message we would need to make sure that also the original setting is mentioned, so something like:

Cannot bind to IP "127.0.0.1" as resolved from configuration value "localhost".

@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Sep 1, 2018

Contributor

Let me put it into other words (or better pseudocode):

String ip;
if (key starts with char) {
      ip = gethostbyname(key)
} else if (key starts with number) {
     ip = key
} else {
   //error handling;
}

The code basically checks if it starts with a string [A-Za-z] and assumes a non-numeric name and makes
a DNS request. If not it assumes a numeric host.

http://man7.org/linux/man-pages/man3/gethostbyname.3.html does the dns request.

Isn't this possible?

Contributor

Piankero commented Sep 1, 2018

Let me put it into other words (or better pseudocode):

String ip;
if (key starts with char) {
      ip = gethostbyname(key)
} else if (key starts with number) {
     ip = key
} else {
   //error handling;
}

The code basically checks if it starts with a string [A-Za-z] and assumes a non-numeric name and makes
a DNS request. If not it assumes a numeric host.

http://man7.org/linux/man-pages/man3/gethostbyname.3.html does the dns request.

Isn't this possible?

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Sep 3, 2018

Contributor

hints.ai_flags already implements something like that. So afaik it is only about setting the correct flag.

Contributor

markus2330 commented Sep 3, 2018

hints.ai_flags already implements something like that. So afaik it is only about setting the correct flag.

@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Sep 11, 2018

Contributor

Actually when leaving hints.ai_flags empty it accepts both numeric and non-numeric hosts.
I gave it a try and made PR #2233 to fix this :)

Contributor

Piankero commented Sep 11, 2018

Actually when leaving hints.ai_flags empty it accepts both numeric and non-numeric hosts.
I gave it a try and made PR #2233 to fix this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment