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

Fix KDC and kadmind network setup issues #587

Merged
merged 2 commits into from
Jan 9, 2017

Conversation

greghudson
Copy link
Member

Here are fixes for the two problems discussed in ticket 8530. (I created ticket 8531 to track the IPv4-only startup issue.)

@greghudson greghudson mentioned this pull request Jan 3, 2017
@@ -889,9 +893,15 @@ setup_addresses(struct socksetup *data)
_("Failed setting up a %s socket (for %s)"),
bind_type_names[addr.type],
paddr(ai->ai_addr));
goto cleanup;
if (ret != EAFNOSUPPORT)
Copy link
Contributor

Choose a reason for hiding this comment

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

On IPv4 systems, when ret == EAFNOSUPPORT, won't this result in "Failed setting up" messages being logged? This seems potentially confusing since they may not be fatal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? I'm not sure it's wrong to log that since it points to dubious system behavior. (I think what's going on is that the system decided not to load the IPv6 kernel module because there are no IPv6 kernel interfaces, but it doesn't have a mechanism to suppress returning IPv6 addresses from getaddrinfo(NULL, ...).) The code gets more complicated if we try to log an EAFNOSUPPORT error only if we get it for all getaddrinfo results, but not if we get it for some.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, it would rather complicate things to not log at all. One alternative could be to log a different message for EAFNOTSUPPORT when it is encountered. That said, though, loop_setup_network() takes care of logging that there was a fatal error in setup, so I could live with the code as written.

Copy link

@rbasch rbasch Jan 9, 2017

Choose a reason for hiding this comment

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

This code commit worked for me... (especially since I reported the issue).

Copy link
Member

@tlyu tlyu left a comment

Choose a reason for hiding this comment

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

This looks OK to me. Has Richard tested these patches?

@@ -753,7 +764,7 @@ setup_socket(struct socksetup *data, struct bind_address *ba,
}

/* Try to turn on pktinfo for UDP wildcard sockets. */
if (ba->type == UDP && ba->address == NULL) {
if (ba->type == UDP && is_wildcard(sock_address)) {
krb5_klog_syslog(LOG_DEBUG, _("Setting pktinfo on socket %s"),
Copy link

Choose a reason for hiding this comment

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

Should we simply use pktinfo for all UDP?
Let's take another edge condition - binding to loopback or PtP interfaces with a non /32 mask. The host will receive the connections to anything within that range, even though the interface is defined with a specific address (if you need proof, just set your loopback address to be 127.0.0.1/8 and ping 127.1.1.1). Most likely, you will want to respond using the same address to which you received the packet, so it seems pktinfo is appropriate in all UDP conditions, and if it fails, it should just default to the address of that interface.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't recall the exact reason right now but there was definitely a reason why I didn't use pktinfo for all addresses, just wildcard. I'll have to look into it when I have access to my laptop.

Copy link

Choose a reason for hiding this comment

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

I think the following conditions might have issues: Point-to-Point, Loopback, and Multicast.

I do think if pktinfo fails, the code should use the default socket address (whether or not it does currently is something that should be examined)... I agree pktinfo may not always work but I think it presumptuous to assume "wildcard" is the only condition where it is required. I would be curious what you find.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for testing.

For the purpose of a backported fix, I would like to avoid using pktinfo for non-wildcard sockets, as we have never done that. (Prior to 1.15, we either used pktinfo on a wildcard socket or iterated over the interfaces and bound to each interface address without pktinfo.) We can consider using pktinfo everywhere for 1.16, although my memory agrees with Sarah's that there is a portability issue lurking there.

Copy link

@rbasch rbasch left a comment

Choose a reason for hiding this comment

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

This worked for me (i.e. it fixes the problem I originally reported with 1.15).

getaddrinfo(NULL, ...) may yield an IPv6 wildcard address on IPv4-only
systems, and creating a socket for that address may result in an
EAFNOSUPPORT error.  Tolerate that error as long as we can bind at
least one socket for the address.

ticket: 8531
target_version: 1.15-next
tags: pullup
In net-server.c, use pktinfo on UDP server sockets if they are bound
to wildcard addresses, whether that is explicit or implicit in the
address specification.

ticket: 8530
target_version: 1.15-next
tags: pullup
@tlyu tlyu merged commit d005bea into krb5:master Jan 9, 2017
@greghudson greghudson deleted the net-server-fixes branch January 9, 2017 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants