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

Make the mobileconfig API parameterized and more robust #2358

Closed
ainar-g opened this issue Nov 24, 2020 · 4 comments
Closed

Make the mobileconfig API parameterized and more robust #2358

ainar-g opened this issue Nov 24, 2020 · 4 comments

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Nov 24, 2020

We've already had quite a few bugs about the mobileconfig HTTP API and its autoselection of hostnames. Instead of churning out more temporary solutions, let's solve the issue in a more systematic way in v0.105.0:

(Updated 2020-11-27 after @ameshkov's comments.)

  1. The mobileconfig API MUST NOT respond with naked IP addresses or hosts with ports.
  2. The client MUST provide the host that is expected in the output XML, for example in a GET query parameter. This allows us to use the API in situations where the server supports several hosts, as well as with client IDs. But if they don't, use tls.server_name, and if it's not set, return a 500 error.
  3. The UI MUST NOT show the links to the mobileconfig API unless tls.server_name is set. Perhaps show a form for getting a link with the desired host instead.

Things to think about and figure out:

  1. Q: Should we make the tls.server_name parameter required? What about servers that can accept connections targeting multiple hosts?
    A: It is required, but only when clients don't provide a host of their own.
  2. Q: Should there be a default host when the client doesn't provide one, or should we be strict and always require one?
    A: The default is tls.server_name.
  3. Q: Should we ban GETting the mobileconfig API over plain HTTP?
    A: No, since clients provide the host now.
  4. Q: What about wildcard certificates?
    A: If tls.server_name is set, it does not matter what certificate is used. If not, handle this in the UI and ask users to enter the required hostname.

@ameshkov, can you please validate my thoughts here?

@ameshkov
Copy link
Member

The mobileconfig API MUST NOT respond with naked IP addresses or hosts with ports.
Should we make the tls.server_name parameter required? What about servers that can accept connections targeting multiple hosts?
Should we ban GETting the mobileconfig API over plain HTTP?

Since it now uses the hostname provided by the client, it does not matter how it was requested.

Should there be a default host when the client doesn't provide one, or should we be strict and always require one?

Yep, if the hostname parameter is empty, we can use config.tls.server_name.
If it's not set either, we should return error 500.

What about wildcard certificates?

If config.tls.server_name is configured, it does not matter what certificate is used.
If it is not configured, we should handle this in the UI and ask the user to enter the desired hostname -- add it to the checklist here: #2075

adguard pushed a commit that referenced this issue Nov 25, 2020
Merge in DNS/adguard-home from 2358-mobileconfig to master

Updates #2358.

Squashed commit of the following:

commit ab3c7a7
Merge: fa002e4 b4a35fa
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Nov 25 16:11:06 2020 +0300

    Merge branch 'master' into 2358-mobileconfig

commit fa002e4
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Nov 25 15:19:00 2020 +0300

    home: improve mobileconfig http api
@ainar-g
Copy link
Contributor Author

ainar-g commented Nov 25, 2020

The backend and the documentation are mostly done. @ArtemBaskal, could you please add the frontend logic?

adguard pushed a commit that referenced this issue Dec 1, 2020
…and more robust

Merge in DNS/adguard-home from feature/2358 to master

Updates #2358.

Squashed commit of the following:

commit b2b91ee
Author: Artem Baskal <a.baskal@adguard.com>
Date:   Tue Dec 1 14:54:35 2020 +0300

    + client: 2358 Make the mobileconfig API parameterized and more robust
@ainar-g
Copy link
Contributor Author

ainar-g commented Dec 1, 2020

I feel like this is done for now. @ameshkov, should we keep this issue open for future improvements, for example multiple hosts support, or should we close this issue for now?

@ameshkov
Copy link
Member

ameshkov commented Dec 1, 2020

@ainar-g nope, let's close it

@ainar-g ainar-g closed this as completed Dec 1, 2020
adguard pushed a commit that referenced this issue Dec 10, 2020
Merge in DNS/adguard-home from 2358-mobileconfig to master

Updates #2358.

Squashed commit of the following:

commit ab3c7a7
Merge: fa002e4 b4a35fa
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Nov 25 16:11:06 2020 +0300

    Merge branch 'master' into 2358-mobileconfig

commit fa002e4
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Nov 25 15:19:00 2020 +0300

    home: improve mobileconfig http api
adguard pushed a commit that referenced this issue Dec 10, 2020
…and more robust

Merge in DNS/adguard-home from feature/2358 to master

Updates #2358.

Squashed commit of the following:

commit b2b91ee
Author: Artem Baskal <a.baskal@adguard.com>
Date:   Tue Dec 1 14:54:35 2020 +0300

    + client: 2358 Make the mobileconfig API parameterized and more robust
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants