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

If present, give URL protocol precedence over other SSL settings #608

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

mmogilvi
Copy link
Contributor

This is basically a minor adjustment to #482 , updated for the new 3.11 curl-based geturl(). The original PR objections seemed to be focused on lack of time to properly understand the nuances of the old behavior rather than any realistic objection to the only-slightly-different new behavior.

My setup broke for the 3.10 update, with no easy config-only fix (see below). Now Gentoo just updated to 3.11.1, and the 3.10 version of my fix/patch can't be applied, hence this newer patch. This patch doesn't directly restore 3.9, but it is now possible to make a trivial configuration change for situations like this, by just including the protocol as part of each individual broken URL.

Key points:

  • I think it is basic good citizenship to prefer to extract your IP address from your local router, rather than asking a third party service unnecessarily. Especially when querying it on a regular basis.
  • Many (most?) routers probably don't try to support an SSL web interface (https), probably because handling server certificates for a private service like this is not trivial (expiration, private URL/IP address changes, etc). At least mine doesn't.
  • Best to use SSL wherever possible, though. Especially for the actual update to the external ddns service you are using.
  • However, as near as I could tell (at least for 3.10), there was no way to enable SSL for the update without also forcing SSL for the "get IP address". (It was somehow working in 3.9, although I'm not sure if it was using SSL at all. 3.10 forced SSL more widely (by default), and broke by setup.)
  • Related: At least a few other bugs mention difficulties using plain http for getting IP address...
  • The existing separate SSL setting is still used when the "URL" is incomplete and lacks a protocol, so the behavior change is minimal.

FUTURE ideas:

  • Perhaps this should be documented somehow. (Although the documentation isn't great in general.)
  • Are there more code paths where similar "give precedence to URL protocol" should be changed?
  • Maybe some of the commented URL's in the default configuration file should include the protocol (http or https), especially if it is known that only one actually works.
  • Relatively few default config (commented) URLs currently specifies a protocol directly, but maybe "lack of explicit protocol in URL" should be deprecated because it leaves the nuances of configuring things more confusing... (Protocol appears to be included in several embedded URLs now, though, based on a quick grep of the main script.)

@LenardHess
Copy link
Contributor

Looking at the old behavior and this patch, this is what is happening:

  • Existing behavior:
    • If URL has https://, use HTTPS
    • If ssl is set, use HTTPS, except when disabled via ignore_ssl_option
    • Otherwise use HTTP
  • New behavior:
    • If URL has https://, use HTTPS
    • If URL has http://, use HTTP
    • If ssl is set, use HTTPS, except when disabled via ignore_ssl_option
    • Otherwise use HTTP

The only changed behavior is therefore for http:// URLs with ignore_ssl_option=0 and ssl=1. Those would use HTTP instead of HTTPS with this change.

I like this addition, as it makes the path to forcing HTTP more clear in my opinion (use http:// in the URL). This would also allow dropping the ignore_ssl_option parameter in geturl calls by converting the URL passed to a http:// URL.

As mentioned in #597 - I'm inclined to add a "Disable HTTP"/"Force SSL everywhere" option to allow users to opt out of HTTP all together by either force-upgrading to HTTPS or by logging a warning/error on HTTP. The implication of such an option would be that the user then find any HTTP usage and fix it (Move away from HTTP-only services/providers).
But this will most likely have to wait until more urgent stuff is done.

@LenardHess LenardHess merged commit d195bcc into ddclient:master Jan 4, 2024
7 checks passed
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.

None yet

2 participants