[2.0] InspIRCd should not unload all modules if it cannot locate a remote include on rehash #30

Closed
Justasic opened this Issue Apr 6, 2012 · 9 comments

Comments

Projects
None yet
6 participants
@Justasic
Contributor

Justasic commented Apr 6, 2012

I am tired of seeing this: http://pastie.org/3740805 because I rehash and something in my DNS changed (dynamic DNS) and no longer resolves, thus causing inspircd to unload ALL core modules thinking that part of the config is now gone. Instead I think it should abort the rehash and maybe allow some kind of conformation override.

I also believe that when a DNS hostname is placed in the link:ipaddr block that the hostname be resolved at every rehash instead of being cached, it makes Dynamic DNS very annoying to work with when your old ip is cached.

@Shawn-Smith

This comment has been minimized.

Show comment
Hide comment
@Shawn-Smith

Shawn-Smith Apr 6, 2012

Contributor

Maybe it could cache the config file each time and then on future rehashes if it can't find the remote one it can resort to the cache.

Contributor

Shawn-Smith commented Apr 6, 2012

Maybe it could cache the config file each time and then on future rehashes if it can't find the remote one it can resort to the cache.

@SaberUK

This comment has been minimized.

Show comment
Hide comment
@SaberUK

SaberUK Apr 6, 2012

Contributor

Not being able to access a remote include is an error and should be treated as such. In my opinion the correct behavior should be to abort the rehash and inform the rehashing user of the error. Caching the file as suggested by @Shawn-Smith could introduce unintended configuration errors of potentially disastrous significance.

Contributor

SaberUK commented Apr 6, 2012

Not being able to access a remote include is an error and should be treated as such. In my opinion the correct behavior should be to abort the rehash and inform the rehashing user of the error. Caching the file as suggested by @Shawn-Smith could introduce unintended configuration errors of potentially disastrous significance.

@blitmap

This comment has been minimized.

Show comment
Hide comment
@blitmap

blitmap Apr 10, 2012

Contributor

I imagine the rehash is being done as it goes down the config, so if it can't locate a remote include, it's already applied a section of the "new config". It would be a lot more work to first undo what has been applied, and then abort the rehash....

I would abort doing anything else if the remote include cannot be gotten to. So a partial application, not a more clever abort of a rehash :>

Two things to implement:

  1. Stop at the point where a remote include fails when rehashing. Abort doing "the rest of it".
  2. Don't cache the IP of the host given in the remote include, resolve with each rehash to avoid screwing up dynamic DNS setups. (perhaps someone should look for other non-user IPs being cached elsewhere? -- dynamic DNS setups are becoming more common)

Later :o

I'm blabbering, just thought I'd add that... (for anyone who wants to try implementing this)

Contributor

blitmap commented Apr 10, 2012

I imagine the rehash is being done as it goes down the config, so if it can't locate a remote include, it's already applied a section of the "new config". It would be a lot more work to first undo what has been applied, and then abort the rehash....

I would abort doing anything else if the remote include cannot be gotten to. So a partial application, not a more clever abort of a rehash :>

Two things to implement:

  1. Stop at the point where a remote include fails when rehashing. Abort doing "the rest of it".
  2. Don't cache the IP of the host given in the remote include, resolve with each rehash to avoid screwing up dynamic DNS setups. (perhaps someone should look for other non-user IPs being cached elsewhere? -- dynamic DNS setups are becoming more common)

Later :o

I'm blabbering, just thought I'd add that... (for anyone who wants to try implementing this)

@Justasic

This comment has been minimized.

Show comment
Hide comment
@Justasic

Justasic Apr 11, 2012

Contributor

well I didn't think about dynamic dns on the resolution of remote includes but that would be an issue as well, I was saying that link blocks need to resolve the address everytime a /connect is given to the sever or your dyndns is going to get cached and end up causing more issues than should happen. I know this happens even after /cleardns, it results in me manually adding the ip address into the link block. Just some thoughts I guess, I do admit it will be quite a feat to get the config parser to not follow this behavior.

Contributor

Justasic commented Apr 11, 2012

well I didn't think about dynamic dns on the resolution of remote includes but that would be an issue as well, I was saying that link blocks need to resolve the address everytime a /connect is given to the sever or your dyndns is going to get cached and end up causing more issues than should happen. I know this happens even after /cleardns, it results in me manually adding the ip address into the link block. Just some thoughts I guess, I do admit it will be quite a feat to get the config parser to not follow this behavior.

@Justasic

This comment has been minimized.

Show comment
Hide comment
@Justasic

Justasic May 22, 2012

Contributor

Also as an added note, I think there should be a override ability to allow the daemon to continue starting even if there are modules that cannot be found

Contributor

Justasic commented May 22, 2012

Also as an added note, I think there should be a override ability to allow the daemon to continue starting even if there are modules that cannot be found

@kaniini

This comment has been minimized.

Show comment
Hide comment
@kaniini

kaniini May 23, 2012

Contributor

@justastic dynamic DNS + remote includes combined is a major security flaw, are you sure your configuration is actually valid?

Contributor

kaniini commented May 23, 2012

@justastic dynamic DNS + remote includes combined is a major security flaw, are you sure your configuration is actually valid?

@SaberUK

This comment has been minimized.

Show comment
Hide comment
@SaberUK

SaberUK May 23, 2012

Contributor

I think the best way to fix this would be to fail the rehash if an executable include returns a value other than EXIT_SUCCESS in ParseStack::ParseExec().

Contributor

SaberUK commented May 23, 2012

I think the best way to fix this would be to fail the rehash if an executable include returns a value other than EXIT_SUCCESS in ParseStack::ParseExec().

@Justasic

This comment has been minimized.

Show comment
Hide comment
@Justasic

Justasic May 23, 2012

Contributor

@nenolod yes, I understand that dynamic DNS is a security flaw but hosting under 18 means I have to accept it and do my best to mitigate it. Yes the configuration is valid, it's just a bit annoying that inspircd caches the ip from the resolved hostname to save time (what? 3 ms?). The reason why I asked to allow the daemon to continue starting even if there's no module found is because when I make a testnet I don't want to redo my configs, I would rather use my network's configs while ignoring a few extra modules, if that makes sense.

@SaberUK Yeah, that's what I was thinking.

Contributor

Justasic commented May 23, 2012

@nenolod yes, I understand that dynamic DNS is a security flaw but hosting under 18 means I have to accept it and do my best to mitigate it. Yes the configuration is valid, it's just a bit annoying that inspircd caches the ip from the resolved hostname to save time (what? 3 ms?). The reason why I asked to allow the daemon to continue starting even if there's no module found is because when I make a testnet I don't want to redo my configs, I would rather use my network's configs while ignoring a few extra modules, if that makes sense.

@SaberUK Yeah, that's what I was thinking.

@attilamolnar

This comment has been minimized.

Show comment
Hide comment
Member

attilamolnar commented Mar 12, 2013

solution implemented in https://github.com/attilamolnar/inspircd/compare/insp20%2Bmandatorytag, comments are welcome

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